From 183f980b034d6fc440c4cf59e651abb7d2d5f681 Mon Sep 17 00:00:00 2001 From: Grace Date: Mon, 18 May 2020 14:32:38 -0700 Subject: [PATCH] add fix per comments --- .../components/FilterBoxItemControl_spec.jsx | 7 ++- .../util/getFilterConfigsFromFormdata.js | 12 +++-- .../controls/FilterBoxItemControl.jsx | 54 +++++++++++-------- superset-frontend/src/explore/constants.js | 5 ++ .../visualizations/FilterBox/FilterBox.jsx | 18 ++++--- 5 files changed, 65 insertions(+), 31 deletions(-) diff --git a/superset-frontend/spec/javascripts/explore/components/FilterBoxItemControl_spec.jsx b/superset-frontend/spec/javascripts/explore/components/FilterBoxItemControl_spec.jsx index 9b487405d873..c1a38ecbf61f 100644 --- 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 index 5185c148f420..192f7cbc0294 100644 --- 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 index 96c7d7cd729d..b0336ae23bc2 100644 --- 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 { 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={ 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 index de9919935e4b..975a7fe026ac 100644 --- 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 index cd302dc2a9e5..fb84ba61c4ac 100644 --- 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 ( {