fix(FilterBox): treat empty defaultValues as null (#13109)

* fix(FilterBox): treat empty defaultValues as null

* Add a unit test and move constant around

superset-frontend/spec/javascripts/dashboard/util/getFilterConfigsFromFormdata_spec.js

@@ -56,13 +56,25 @@ describe('getFilterConfigsFromFormdata', () => {
     });
   });
 
-  it('should use default value from form_data', () => {
+  it('should use default value and treat empty defaults as null', () => {
     const result = getFilterConfigsFromFormdata({
       ...testFormdata,
       show_sqla_time_column: true,
+      filter_configs: [
+        ...testFormdata.filter_configs,
+        {
+          asc: false,
+          clearable: true,
+          column: 'country',
+          defaultValue: '',
+          key: 'foo',
+          multiple: true,
+        },
+      ],
     });
     expect(result.columns).toMatchObject({
       state: ['CA'],
+      country: null,
     });
   });
 

superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts

@@ -16,9 +16,12 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { getChartIdsInFilterScope } from '../../util/activeDashboardFilters';
-import { TIME_FILTER_MAP } from '../../../visualizations/FilterBox/FilterBox';
-import { NativeFiltersState, NativeFilterState } from '../../reducers/types';
+import { TIME_FILTER_MAP } from 'src/explore/constants';
+import { getChartIdsInFilterScope } from 'src/dashboard/util/activeDashboardFilters';
+import {
+  NativeFiltersState,
+  NativeFilterState,
+} from 'src/dashboard/reducers/types';
 
 export enum IndicatorStatus {
   Unset = 'UNSET',

superset-frontend/src/dashboard/util/getFilterConfigsFromFormdata.js

@@ -17,11 +17,11 @@
  * under the License.
  */
 /* eslint-disable camelcase */
-import { TIME_FILTER_MAP } from '../../visualizations/FilterBox/FilterBox';
 import {
   FILTER_CONFIG_ATTRIBUTES,
   TIME_FILTER_LABELS,
-} from '../../explore/constants';
+  TIME_FILTER_MAP,
+} from 'src/explore/constants';
 
 export default function getFilterConfigsFromFormdata(form_data = {}) {
   const {
@@ -35,14 +35,18 @@ export default function getFilterConfigsFromFormdata(form_data = {}) {
   let configs = filter_configs.reduce(
     ({ columns, labels }, config) => {
       let defaultValues = config[FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE];
+
+      // treat empty string as null (no default value)
+      if (defaultValues === '') {
+        defaultValues = null;
+      }
+
       // defaultValue could be ; separated values,
       // could be null or ''
-      if (
-        config[FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE] &&
-        config[FILTER_CONFIG_ATTRIBUTES.MULTIPLE]
-      ) {
+      if (defaultValues && config[FILTER_CONFIG_ATTRIBUTES.MULTIPLE]) {
         defaultValues = config.defaultValue.split(';');
       }
+
       const updatedColumns = {
         ...columns,
         [config.column]: config.vals || defaultValues,

superset-frontend/src/explore/constants.js

@@ -92,3 +92,14 @@ export const FILTER_CONFIG_ATTRIBUTES = {
 };
 
 export const FILTER_OPTIONS_LIMIT = 1000;
+
+/**
+ * Map control names to their key in extra_filters
+ */
+export const TIME_FILTER_MAP = {
+  time_range: '__time_range',
+  granularity_sqla: '__time_col',
+  time_grain_sqla: '__time_grain',
+  druid_time_origin: '__time_origin',
+  granularity: '__granularity',
+};

superset-frontend/src/visualizations/FilterBox/FilterBox.jsx

@@ -36,19 +36,11 @@ import {
   FILTER_CONFIG_ATTRIBUTES,
   FILTER_OPTIONS_LIMIT,
   TIME_FILTER_LABELS,
+  TIME_FILTER_MAP,
 } from 'src/explore/constants';
 
 import './FilterBox.less';
 
-// maps control names to their key in extra_filters
-export const TIME_FILTER_MAP = {
-  time_range: '__time_range',
-  granularity_sqla: '__time_col',
-  time_grain_sqla: '__time_grain',
-  druid_time_origin: '__time_origin',
-  granularity: '__granularity',
-};
-
 // a shortcut to a map key, used by many components
 export const TIME_RANGE = TIME_FILTER_MAP.time_range;
 
@@ -336,10 +328,12 @@ class FilterBox extends React.PureComponent {
     // Add created options to filtersChoices, even though it doesn't exist,
     // or these options will exist in query sql but invisible to end user.
     Object.keys(selectedValues)
-      .filter(
-        key => selectedValues.hasOwnProperty(key) && key in filtersChoices,
-      )
+      .filter(key => key in filtersChoices)
       .forEach(key => {
+        // empty values are ignored
+        if (!selectedValues[key]) {
+          return;
+        }
         const choices = filtersChoices[key] || (filtersChoices[key] = []);
         const choiceIds = new Set(choices.map(f => f.id));
         const selectedValuesForKey = Array.isArray(selectedValues[key])
@@ -356,21 +350,21 @@ class FilterBox extends React.PureComponent {
             });
           });
       });
-    const { key, label } = filterConfig;
+    const {
+      key,
+      label,
+      [FILTER_CONFIG_ATTRIBUTES.MULTIPLE]: isMultiple,
+      [FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE]: defaultValue,
+      [FILTER_CONFIG_ATTRIBUTES.CLEARABLE]: isClearable,
+      [FILTER_CONFIG_ATTRIBUTES.SEARCH_ALL_OPTIONS]: searchAllOptions,
+    } = filterConfig;
     const data = filtersChoices[key] || [];
     let value = selectedValues[key] || null;
 
     // Assign default value if required
-    if (
-      value === undefined &&
-      filterConfig[FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE]
-    ) {
-      if (filterConfig[FILTER_CONFIG_ATTRIBUTES.MULTIPLE]) {
-        // Support for semicolon-delimited multiple values
-        value = filterConfig[FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE].split(';');
-      } else {
-        value = filterConfig[FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE];
-      }
+    if (value === undefined && defaultValue) {
+      // multiple values are separated by semicolons
+      value = isMultiple ? defaultValue.split(';') : defaultValue;
     }
 
     return (
@@ -380,8 +374,8 @@ class FilterBox extends React.PureComponent {
         defaultOptions={this.transformOptions(data)}
         key={key}
         placeholder={t('Type or Select [%s]', label)}
-        isMulti={filterConfig[FILTER_CONFIG_ATTRIBUTES.MULTIPLE]}
-        isClearable={filterConfig[FILTER_CONFIG_ATTRIBUTES.CLEARABLE]}
+        isMulti={isMultiple}
+        isClearable={isClearable}
         value={value}
         options={this.transformOptions(data)}
         onChange={newValue => {
@@ -396,8 +390,7 @@ class FilterBox extends React.PureComponent {
         onBlur={() => this.onFilterMenuClose(key)}
         onMenuClose={() => this.onFilterMenuClose(key)}
         selectWrap={
-          filterConfig[FILTER_CONFIG_ATTRIBUTES.SEARCH_ALL_OPTIONS] &&
-          data.length >= FILTER_OPTIONS_LIMIT
+          searchAllOptions && data.length >= FILTER_OPTIONS_LIMIT
             ? AsyncCreatableSelect
             : CreatableSelect
         }

superset-frontend/src/visualizations/constants.js

@@ -1,27 +0,0 @@
-/**
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements.  See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License.  You may obtain a copy of the License at
- *
- *   http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied.  See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-export const TIME_CHOICES = [
-  '1 hour ago',
-  '12 hours ago',
-  '1 day ago',
-  '7 days ago',
-  '28 days ago',
-  '90 days ago',
-  '1 year ago',
-];