From d96bb874f2dfa27ff41736982cc0d8027efaaefa Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Mon, 18 May 2020 21:25:10 -0700 Subject: [PATCH] fix: [filter_box] fix 2 issues in single value filter_box (#9829) * fix: [filter_box] fix 2 issues in single value filter_box * add unit test * add fix per comments --- .../components/FilterBoxItemControl_spec.jsx | 49 +++++++++++++++++++ .../util/getFilterConfigsFromFormdata.js | 12 +++-- .../controls/FilterBoxItemControl.jsx | 49 +++++++++++++++++-- superset-frontend/src/explore/constants.js | 5 ++ .../visualizations/FilterBox/FilterBox.jsx | 18 ++++--- 5 files changed, 121 insertions(+), 12 deletions(-) diff --git a/superset-frontend/spec/javascripts/explore/components/FilterBoxItemControl_spec.jsx b/superset-frontend/spec/javascripts/explore/components/FilterBoxItemControl_spec.jsx index 162ab4784..c1a38ecbf 100644 --- a/superset-frontend/spec/javascripts/explore/components/FilterBoxItemControl_spec.jsx +++ b/superset-frontend/spec/javascripts/explore/components/FilterBoxItemControl_spec.jsx @@ -52,4 +52,53 @@ describe('FilterBoxItemControl', () => { const popover = shallow(inst.renderForm()); expect(popover.find(FormRow)).toHaveLength(7); }); + + it('convert type for single value filter_box', () => { + inst = getWrapper({ + datasource: { + columns: [ + { + column_name: 'SP_POP_TOTL', + description: null, + expression: null, + filterable: true, + groupby: true, + id: 312, + is_dttm: false, + type: 'FLOAT', + verbose_name: null, + }, + ], + metrics: [ + { + d3format: null, + description: null, + expression: 'sum("SP_POP_TOTL")', + id: 3, + metric_name: 'sum__SP_POP_TOTL', + verbose_name: null, + warning_text: null, + }, + ], + }, + }).instance(); + inst.setState({ + asc: true, + clearable: true, + column: 'SP_POP_TOTL', + defaultValue: 254454778, + metric: undefined, + multiple: false, + }); + inst.setState = sinon.spy(); + + 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 fb7102cc0..192f7cbc0 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) { + 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 2928dca2b..c23e1fbd1 100644 --- a/superset-frontend/src/explore/components/controls/FilterBoxItemControl.jsx +++ b/superset-frontend/src/explore/components/controls/FilterBoxItemControl.jsx @@ -26,6 +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 = 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, @@ -60,7 +78,28 @@ export default class FilterBoxItemControl extends React.Component { this.props.onChange(this.state); } onControlChange(attr, value) { - this.setState({ [attr]: value }, this.onChange); + let typedValue = value; + const { column: selectedColumnName, multiple } = this.state; + 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.find( + col => col.column_name === selectedColumnName, + ); + + if (selectedColumn && selectedColumn.type) { + const type = selectedColumn.type.toUpperCase(); + 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); + } + } + } + this.setState({ [attr]: typedValue }, this.onChange); } setType() {} textSummary() { @@ -110,7 +149,9 @@ export default class FilterBoxItemControl extends React.Component { this.onControlChange('defaultValue', v)} + onChange={v => + this.onControlChange(FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE, v) + } /> } /> @@ -155,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 de9919935..975a7fe02 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 4f6f63c5e..fb84ba61c 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 && 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 ( {