add fix per comments

apache · May 18, 2020 · 183f980 · 183f980
1 parent f0e7416
commit 183f980
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 31 deletions.
diff --git a/superset-frontend/spec/javascripts/explore/components/FilterBoxItemControl_spec.jsx b/superset-frontend/spec/javascripts/explore/components/FilterBoxItemControl_spec.jsx
@@ -91,9 +91,14 @@ describe('FilterBoxItemControl', () => {
       multiple: false,
     });
     inst.setState = sinon.spy();
-    inst.onControlChange('defaultValue', '1');
 
+    inst.onControlChange('defaultValue', '1');
     expect(inst.setState.callCount).toBe(1);
     expect(inst.setState.getCall(0).args[0]).toEqual({ defaultValue: 1 });
+
+    // user input is invalid for number type column
+    inst.onControlChange('defaultValue', 'abc');
+    expect(inst.setState.callCount).toBe(2);
+    expect(inst.setState.getCall(1).args[0]).toEqual({ defaultValue: null });
   });
 });
diff --git a/superset-frontend/src/dashboard/util/getFilterConfigsFromFormdata.js b/superset-frontend/src/dashboard/util/getFilterConfigsFromFormdata.js
@@ -18,7 +18,10 @@
  */
 /* eslint-disable camelcase */
 import { TIME_FILTER_MAP } from '../../visualizations/FilterBox/FilterBox';
-import { TIME_FILTER_LABELS } from '../../explore/constants';
+import {
+  FILTER_CONFIG_ATTRIBUTES,
+  TIME_FILTER_LABELS,
+} from '../../explore/constants';
 
 export default function getFilterConfigsFromFormdata(form_data = {}) {
   const {
@@ -31,10 +34,13 @@ export default function getFilterConfigsFromFormdata(form_data = {}) {
   } = form_data;
   let configs = filter_configs.reduce(
     ({ columns, labels }, config) => {
-      let defaultValues = config.defaultValue;
+      let defaultValues = config[FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE];
       // defaultValue could be ; separated values,
       // could be null or ''
-      if (config.defaultValue && config.multiple) {
+      if (
+        config[FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE] &&
+        config[FILTER_CONFIG_ATTRIBUTES.MULTIPLE]
+      ) {
         defaultValues = config.defaultValue.split(';');
       }
       const updatedColumns = {

diff --git a/superset-frontend/src/explore/components/controls/FilterBoxItemControl.jsx b/superset-frontend/src/explore/components/controls/FilterBoxItemControl.jsx
@@ -26,9 +26,24 @@ import FormRow from '../../../components/FormRow';
 import SelectControl from './SelectControl';
 import CheckboxControl from './CheckboxControl';
 import TextControl from './TextControl';
+import { FILTER_CONFIG_ATTRIBUTES } from '../../constants';
 
-const INTEGRAL_TYPES = ['TINYINT', 'SMALLINT', 'INT', 'INTEGER', 'BIGINT'];
-const DECIMAL_TYPES = ['FLOAT', 'DOUBLE'];
+const INTEGRAL_TYPES = new Set([
+  'TINYINT',
+  'SMALLINT',
+  'INT',
+  'INTEGER',
+  'BIGINT',
+  'LONG',
+]);
+const DECIMAL_TYPES = new Set([
+  'FLOAT',
+  'DOUBLE',
+  'REAL',
+  'NUMERIC',
+  'DECIMAL',
+  'MONEY',
+]);
 
 const propTypes = {
   datasource: PropTypes.object.isRequired,
@@ -65,29 +80,22 @@ export default class FilterBoxItemControl extends React.Component {
   onControlChange(attr, value) {
     let typedValue = value;
     const { column: selectedColumnName, multiple } = this.state;
-    if (value && !multiple && attr === 'defaultValue') {
+    if (value && !multiple && attr === FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE) {
       // if single value filter_box,
       // convert input value string to the column's data type
       const { datasource } = this.props;
-      const selectedColumn = (
-        datasource.columns.filter(
-          col => col.column_name === selectedColumnName,
-        ) || []
-      ).pop();
+      const selectedColumn = datasource.columns.find(
+        col => col.column_name === selectedColumnName,
+      );
 
       if (selectedColumn && selectedColumn.type) {
         const type = selectedColumn.type.toUpperCase();
-        try {
-          if (type === 'BOOLEAN') {
-            typedValue = value === 'true';
-          } else if (INTEGRAL_TYPES.includes(type)) {
-            typedValue = parseInt(value, 10);
-          } else if (DECIMAL_TYPES.includes(type)) {
-            typedValue = parseFloat(value);
-          }
-        } catch (ex) {
-          // eslint-disable-next-line no-console
-          console.warn(`parsing value ${value} to type ${type} error`);
+        if (type === 'BOOLEAN') {
+          typedValue = value === 'true';
+        } else if (INTEGRAL_TYPES.has(type)) {
+          typedValue = isNaN(value) ? null : parseInt(value, 10);
+        } else if (DECIMAL_TYPES.has(type)) {
+          typedValue = isNaN(value) ? null : parseFloat(value);
         }
       }
     }
@@ -141,7 +149,9 @@ export default class FilterBoxItemControl extends React.Component {
             <TextControl
               value={this.state.defaultValue}
               name="defaultValue"
-              onChange={v => this.onControlChange('defaultValue', v)}
+              onChange={v =>
+                this.onControlChange(FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE, v)
+              }
             />
           }
         />
@@ -186,7 +196,9 @@ export default class FilterBoxItemControl extends React.Component {
           control={
             <CheckboxControl
               value={this.state.multiple}
-              onChange={v => this.onControlChange('multiple', v)}
+              onChange={v =>
+                this.onControlChange(FILTER_CONFIG_ATTRIBUTES.MULTIPLE, v)
+              }
             />
           }
         />

diff --git a/superset-frontend/src/explore/constants.js b/superset-frontend/src/explore/constants.js
@@ -78,3 +78,8 @@ export const TIME_FILTER_LABELS = {
   druid_time_origin: t('Origin'),
   granularity: t('Time Granularity'),
 };
+
+export const FILTER_CONFIG_ATTRIBUTES = {
+  DEFAULT_VALUE: 'defaultValue',
+  MULTIPLE: 'multiple',
+};
diff --git a/superset-frontend/src/visualizations/FilterBox/FilterBox.jsx b/superset-frontend/src/visualizations/FilterBox/FilterBox.jsx
@@ -31,7 +31,10 @@ import OnPasteSelect from '../../components/OnPasteSelect';
 import VirtualizedRendererWrap from '../../components/VirtualizedRendererWrap';
 import { getDashboardFilterKey } from '../../dashboard/util/getDashboardFilterKey';
 import { getFilterColorMap } from '../../dashboard/util/dashboardFiltersColorMap';
-import { TIME_FILTER_LABELS } from '../../explore/constants';
+import {
+  FILTER_CONFIG_ATTRIBUTES,
+  TIME_FILTER_LABELS,
+} from '../../explore/constants';
 import FilterBadgeIcon from '../../components/FilterBadgeIcon';
 
 import './FilterBox.less';
@@ -259,19 +262,22 @@ class FilterBox extends React.Component {
     let value = selectedValues[key] || null;
 
     // Assign default value if required
-    if (value === undefined && filterConfig.defaultValue) {
-      if (filterConfig.multiple) {
+    if (
+      value === undefined &&
+      filterConfig[FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE]
+    ) {
+      if (filterConfig[FILTER_CONFIG_ATTRIBUTES.MULTIPLE]) {
         // Support for semicolon-delimited multiple values
-        value = filterConfig.defaultValue.split(';');
+        value = filterConfig[FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE].split(';');
       } else {
-        value = filterConfig.defaultValue;
+        value = filterConfig[FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE];
       }
     }
     return (
       <OnPasteSelect
         placeholder={t('Select [%s]', label)}
         key={key}
-        multi={filterConfig.multiple}
+        multi={filterConfig[FILTER_CONFIG_ATTRIBUTES.MULTIPLE]}
         clearable={filterConfig.clearable}
         value={value}
         options={data.map(opt => {