feat(native-filters): Hide non-numeric columns in numeric range filter (

apache#15385) * feat(native-filters): Hide non-numeric columns in numeric range filter * Return true if type_generic undefined * Code review comments * Replace any with string * fix tests * add missing columns to select Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
CybercentreCanada · Dec 17, 2021 · 03bc14b · 03bc14b
1 parent 1e6cbd1
commit 03bc14b
Show file tree

Hide file tree

Showing 5 changed files with 99 additions and 34 deletions.
diff --git a/.../dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.tsx b/.../dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.tsx
@@ -16,15 +16,16 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React, { useCallback, useState } from 'react';
+import React, { useCallback, useState, useMemo, useEffect } from 'react';
 import { FormInstance } from 'antd/lib/form';
-import { Column, SupersetClient, t } from '@superset-ui/core';
+import { Column, ensureIsArray, SupersetClient, t } from '@superset-ui/core';
 import { useChangeEffect } from 'src/common/hooks/useChangeEffect';
 import { Select } from 'src/common/components';
 import { useToasts } from 'src/messageToasts/enhancers/withToasts';
 import { getClientErrorObject } from 'src/utils/getClientErrorObject';
 import { cacheWrapper } from 'src/utils/cacheWrapper';
 import { NativeFiltersForm } from '../types';
+import { doesColumnMatchFilterType } from './utils';
 
 interface ColumnSelectProps {
   allowClear?: boolean;
@@ -57,13 +58,39 @@ export function ColumnSelect({
   value,
   onChange,
 }: ColumnSelectProps) {
-  const [options, setOptions] = useState();
+  const [columns, setColumns] = useState<Column[]>();
   const { addDangerToast } = useToasts();
   const resetColumnField = useCallback(() => {
     form.setFields([
       { name: ['filters', filterId, formField], touched: false, value: null },
     ]);
-  }, [form, filterId]);
+  }, [form, filterId, formField]);
+
+  const options = useMemo(
+    () =>
+      ensureIsArray(columns)
+        .filter(filterValues)
+        .map((col: Column) => col.column_name)
+        .sort((a: string, b: string) => a.localeCompare(b))
+        .map((column: string) => ({ label: column, value: column })),
+    [columns, filterValues],
+  );
+
+  const currentFilterType = form.getFieldValue('filters')?.[filterId]
+    .filterType;
+  const currentColumn = useMemo(
+    () => columns?.find(column => column.column_name === value),
+    [columns, value],
+  );
+
+  useEffect(() => {
+    if (
+      currentColumn &&
+      !doesColumnMatchFilterType(currentFilterType, currentColumn)
+    ) {
+      resetColumnField();
+    }
+  }, [currentColumn, currentFilterType, resetColumnField]);
 
   useChangeEffect(datasetId, previous => {
     if (previous != null) {
@@ -74,16 +101,14 @@ export function ColumnSelect({
         endpoint: `/api/v1/dataset/${datasetId}`,
       }).then(
         ({ json: { result } }) => {
-          const columns = result.columns
-            .filter(filterValues)
-            .map((col: any) => col.column_name)
-            .sort((a: string, b: string) => a.localeCompare(b));
-          if (!columns.includes(value)) {
+          if (
+            !result.columns.some(
+              (column: Column) => column.column_name === value,
+            )
+          ) {
             resetColumnField();
           }
-          setOptions(
-            columns.map((column: any) => ({ label: column, value: column })),
-          );
+          setColumns(result.columns);
         },
         async badResponse => {
           const { error, message } = await getClientErrorObject(badResponse);
@@ -103,6 +128,7 @@ export function ColumnSelect({
       onChange={onChange}
       options={options}
       placeholder={t('Select a column')}
+      notFoundContent={t('No compatible columns found')}
       showSearch
       allowClear={allowClear}
     />

diff --git a/...board/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx b/...board/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
@@ -21,28 +21,26 @@ import {
   Behavior,
   ChartDataResponseResult,
   Column,
+  GenericDataType,
   getChartMetadataRegistry,
   JsonResponse,
   styled,
   SupersetApiError,
   t,
-  GenericDataType,
-  ensureIsArray,
 } from '@superset-ui/core';
 import {
   ColumnMeta,
-  DatasourceMeta,
   InfoTooltipWithTrigger,
   Metric,
 } from '@superset-ui/chart-controls';
 import { FormInstance } from 'antd/lib/form';
 import React, {
+  forwardRef,
   useCallback,
   useEffect,
-  useState,
-  useMemo,
-  forwardRef,
   useImperativeHandle,
+  useMemo,
+  useState,
 } from 'react';
 import { useSelector } from 'react-redux';
 import { FormItem } from 'src/components/Form';
@@ -74,6 +72,9 @@ import { ColumnSelect } from './ColumnSelect';
 import { NativeFiltersForm } from '../types';
 import {
   datasetToSelectOption,
+  doesColumnMatchFilterType,
+  FILTER_SUPPORTED_TYPES,
+  hasTemporalColumns,
   setNativeFilterFieldValues,
   useForceUpdate,
 } from './utils';
@@ -277,8 +278,6 @@ const FILTERS_WITHOUT_COLUMN = [
 
 const FILTERS_WITH_ADHOC_FILTERS = ['filter_select', 'filter_range'];
 
-const TIME_FILTERS = ['filter_time', 'filter_timegrain', 'filter_timecolumn'];
-
 const BASIC_CONTROL_ITEMS = ['enableEmptyFilter', 'multiSelect'];
 
 // TODO: Rename the filter plugins and remove this mapping
@@ -291,17 +290,6 @@ const FILTER_TYPE_NAME_MAPPING = {
   [t('Group By')]: t('Group by'),
 };
 
-// TODO: add column_types field to DatasourceMeta
-// We return true if column_types is undefined or empty as a precaution against backend failing to return column_types
-const hasTemporalColumns = (
-  dataset: DatasourceMeta & { column_types: GenericDataType[] },
-) => {
-  const columnTypes = ensureIsArray(dataset?.column_types);
-  return (
-    columnTypes.length === 0 || columnTypes.includes(GenericDataType.TEMPORAL)
-  );
-};
-
 /**
  * The configuration form for a specific filter.
  * Assigns field values to `filters[filterId]` in the form.
@@ -681,7 +669,10 @@ const FiltersConfigForm = (
                   ? FILTER_TYPE_NAME_MAPPING[name]
                   : undefined;
                 const isDisabled =
-                  TIME_FILTERS.includes(filterType) &&
+                  FILTER_SUPPORTED_TYPES[filterType].length === 1 &&
+                  FILTER_SUPPORTED_TYPES[filterType].includes(
+                    GenericDataType.TEMPORAL,
+                  ) &&
                   !doLoadedDatasetsHaveTemporalColumns;
                 return {
                   value: filterType,
@@ -756,6 +747,9 @@ const FiltersConfigForm = (
                   form={form}
                   filterId={filterId}
                   datasetId={datasetId}
+                  filterValues={column =>
+                    doesColumnMatchFilterType(formFilter?.filterType, column)
+                  }
                   onChange={() => {
                     // We need reset default value when when column changed
                     setNativeFilterFieldValues(form, filterId, {

diff --git a/...tend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts b/...tend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts
@@ -20,9 +20,22 @@ import { flatMapDeep } from 'lodash';
 import { FormInstance } from 'antd/lib/form';
 import React from 'react';
 import { CustomControlItem, DatasourceMeta } from '@superset-ui/chart-controls';
+import { Column, ensureIsArray, GenericDataType } from '@superset-ui/core';
 
 const FILTERS_FIELD_NAME = 'filters';
 
+export const FILTER_SUPPORTED_TYPES = {
+  filter_time: [GenericDataType.TEMPORAL],
+  filter_timegrain: [GenericDataType.TEMPORAL],
+  filter_timecolumn: [GenericDataType.TEMPORAL],
+  filter_select: [
+    GenericDataType.STRING,
+    GenericDataType.NUMERIC,
+    GenericDataType.TEMPORAL,
+  ],
+  filter_range: [GenericDataType.NUMERIC],
+};
+
 export const useForceUpdate = () => {
   const [, updateState] = React.useState({});
   return React.useCallback(() => updateState({}), []);
@@ -70,3 +83,18 @@ export const datasetToSelectOption = (
   value: item.id,
   label: item.table_name,
 });
+
+// TODO: add column_types field to DatasourceMeta
+// We return true if column_types is undefined or empty as a precaution against backend failing to return column_types
+export const hasTemporalColumns = (
+  dataset: DatasourceMeta & { column_types: GenericDataType[] },
+) => {
+  const columnTypes = ensureIsArray(dataset?.column_types);
+  return (
+    columnTypes.length === 0 || columnTypes.includes(GenericDataType.TEMPORAL)
+  );
+};
+
+export const doesColumnMatchFilterType = (filterType: string, column: Column) =>
+  !column.type_generic ||
+  FILTER_SUPPORTED_TYPES[filterType].includes(column.type_generic);
diff --git a/superset/datasets/api.py b/superset/datasets/api.py
@@ -119,7 +119,7 @@ class DatasetRestApi(BaseSupersetModelRestApi):
         "changed_on_delta_humanized",
         "database.database_name",
     ]
-    show_columns = [
+    show_select_columns = [
         "id",
         "database.database_name",
         "database.id",
@@ -139,12 +139,26 @@ class DatasetRestApi(BaseSupersetModelRestApi):
         "owners.username",
         "owners.first_name",
         "owners.last_name",
-        "columns",
+        "columns.changed_on",
+        "columns.column_name",
+        "columns.created_on",
+        "columns.description",
+        "columns.expression",
+        "columns.filterable",
+        "columns.groupby",
+        "columns.id",
+        "columns.is_active",
+        "columns.is_dttm",
+        "columns.python_date_format",
+        "columns.type",
+        "columns.uuid",
+        "columns.verbose_name",
         "metrics",
         "datasource_type",
         "url",
         "extra",
     ]
+    show_columns = show_select_columns + ["columns.type_generic"]
     add_model_schema = DatasetPostSchema()
     edit_model_schema = DatasetPutSchema()
     add_columns = ["database", "schema", "table_name", "owners"]

diff --git a/tests/datasets/api_tests.py b/tests/datasets/api_tests.py
@@ -633,6 +633,7 @@ def test_update_dataset_create_column(self):
         for column in data["result"]["columns"]:
             column.pop("changed_on", None)
             column.pop("created_on", None)
+            column.pop("type_generic", None)
 
         data["result"]["columns"].append(new_column_data)
         rv = self.client.put(uri, json={"columns": data["result"]["columns"]})
@@ -680,6 +681,7 @@ def test_update_dataset_delete_column(self):
         for column in data["result"]["columns"]:
             column.pop("changed_on", None)
             column.pop("created_on", None)
+            column.pop("type_generic", None)
 
         data["result"]["columns"].append(new_column_data)
         rv = self.client.put(uri, json={"columns": data["result"]["columns"]})
@@ -718,6 +720,7 @@ def test_update_dataset_update_column(self):
         for column in resp_columns:
             column.pop("changed_on", None)
             column.pop("created_on", None)
+            column.pop("type_generic", None)
 
         resp_columns[0]["groupby"] = False
         resp_columns[0]["filterable"] = False