From e5d4765986b90076a4a369423c8b744a756f68df Mon Sep 17 00:00:00 2001
From: Einat Bertenthal <einatbar@users.noreply.github.com>
Date: Thu, 1 Jul 2021 13:28:07 +0300
Subject: [PATCH] feat(dashboard-groupby): group by - add ability to exclude
 columns (#15454)

* feat: group by - add ability to exclude columns

* fix: create column select in a more generic way

* fix: MR comments

* fix: remove description

* fix: multiple value bug in column select

* fix: initial value bug

* fix: lint

* fix: unit tests

* fix: MR comments

* fix: MR comment

Co-authored-by: einatnielsen <einat.bertenthal@nielsen.com>
---
 .../FiltersConfigForm/ColumnSelect.tsx        | 17 ++--
 .../FiltersConfigForm/FiltersConfigForm.tsx   | 58 +++---------
 .../getControlItemsMap.test.tsx               |  6 +-
 .../FiltersConfigForm/getControlItemsMap.tsx  | 92 ++++++++++++++++++-
 .../FiltersConfigForm/utils.ts                |  2 +-
 .../GroupBy/GroupByFilterPlugin.tsx           | 14 ++-
 .../components/GroupBy/controlPanel.ts        | 18 ++++
 .../filters/components/Range/controlPanel.ts  |  3 +-
 .../filters/components/Select/controlPanel.ts | 14 ++-
 .../filters/components/Time/controlPanel.ts   | 17 ++++
 10 files changed, 177 insertions(+), 64 deletions(-)

diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.tsx
index 5ee4095b35..5a05119e54 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.tsx
@@ -34,8 +34,9 @@ interface ColumnSelectProps {
   formField?: string;
   filterId: string;
   datasetId?: number;
-  value?: string;
+  value?: string | string[];
   onChange?: (value: string) => void;
+  mode?: 'multiple' | 'tags';
 }
 
 const localCache = new Map<string, any>();
@@ -57,6 +58,7 @@ export function ColumnSelect({
   datasetId,
   value,
   onChange,
+  mode,
 }: ColumnSelectProps) {
   const [columns, setColumns] = useState<Column[]>();
   const { addDangerToast } = useToasts();
@@ -101,11 +103,11 @@ export function ColumnSelect({
         endpoint: `/api/v1/dataset/${datasetId}`,
       }).then(
         ({ json: { result } }) => {
-          if (
-            !result.columns.some(
-              (column: Column) => column.column_name === value,
-            )
-          ) {
+          const lookupValue = Array.isArray(value) ? value : [value];
+          const valueExists = result.columns.some((column: Column) =>
+            lookupValue?.includes(column.column_name),
+          );
+          if (!valueExists) {
             resetColumnField();
           }
           setColumns(result.columns);
@@ -124,7 +126,8 @@ export function ColumnSelect({
 
   return (
     <Select
-      value={value}
+      mode={mode}
+      value={mode === 'multiple' ? value || [] : value}
       onChange={onChange}
       options={options}
       placeholder={t('Select a column')}
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
index 1ed98814ec..3f45dedf00 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
@@ -72,7 +72,6 @@ import { ColumnSelect } from './ColumnSelect';
 import { NativeFiltersForm } from '../types';
 import {
   datasetToSelectOption,
-  doesColumnMatchFilterType,
   FILTER_SUPPORTED_TYPES,
   hasTemporalColumns,
   setNativeFilterFieldValues,
@@ -269,13 +268,6 @@ export interface FiltersConfigFormProps {
   parentFilters: { id: string; title: string }[];
 }
 
-// TODO: Need to do with it something
-const FILTERS_WITHOUT_COLUMN = [
-  'filter_timegrain',
-  'filter_timecolumn',
-  'filter_groupby',
-];
-
 const FILTERS_WITH_ADHOC_FILTERS = ['filter_select', 'filter_range'];
 
 const BASIC_CONTROL_ITEMS = ['enableEmptyFilter', 'multiSelect'];
@@ -352,15 +344,14 @@ const FiltersConfigForm = (
   // @ts-ignore
   const hasDataset = !!nativeFilterItems[formFilter?.filterType]?.value
     ?.datasourceCount;
-  const hasColumn =
-    hasDataset && !FILTERS_WITHOUT_COLUMN.includes(formFilter?.filterType);
+
   const nativeFilterItem = nativeFilterItems[formFilter?.filterType] ?? {};
   // @ts-ignore
   const enableNoResults = !!nativeFilterItem.value?.enableNoResults;
   const datasetId = formFilter?.dataset?.value;
 
   useEffect(() => {
-    if (datasetId && hasColumn) {
+    if (datasetId && hasDataset) {
       cachedSupersetGet({
         endpoint: `/api/v1/dataset/${datasetId}`,
       })
@@ -376,7 +367,7 @@ const FiltersConfigForm = (
           addDangerToast(response.message);
         });
     }
-  }, [datasetId, hasColumn]);
+  }, [datasetId, hasDataset]);
 
   useImperativeHandle(ref, () => ({
     changeTab(tab: 'configuration' | 'scoping') {
@@ -384,10 +375,10 @@ const FiltersConfigForm = (
     },
   }));
 
-  const hasMetrics = hasColumn && !!metrics.length;
+  const hasMetrics = hasDataset && !!metrics.length;
 
   const hasFilledDataset =
-    !hasDataset || (datasetId && (formFilter?.column || !hasColumn));
+    !hasDataset || (datasetId && (formFilter?.column || !hasDataset));
 
   const hasAdditionalFilters = FILTERS_WITH_ADHOC_FILTERS.includes(
     formFilter?.filterType,
@@ -484,10 +475,9 @@ const FiltersConfigForm = (
     (defaultDatasetSelectOptions.length === 1
       ? defaultDatasetSelectOptions[0].value
       : undefined);
-  const initColumn = filterToEdit?.targets[0]?.column?.name;
   const newFormData = getFormData({
     datasetId,
-    groupby: hasColumn ? formFilter?.column : undefined,
+    groupby: hasDataset ? formFilter?.column : undefined,
     ...formFilter,
   });
 
@@ -546,7 +536,7 @@ const FiltersConfigForm = (
 
   const showDefaultValue = !hasDataset || (!isDataDirty && hasFilledDataset);
 
-  const controlItems = formFilter
+  const { controlItems = {}, mainControlItems = {} } = formFilter
     ? getControlItemsMap({
         disabled: false,
         forceUpdate,
@@ -555,6 +545,7 @@ const FiltersConfigForm = (
         filterType: formFilter.filterType,
         filterToEdit,
         formFilter,
+        removed,
       })
     : {};
 
@@ -731,35 +722,10 @@ const FiltersConfigForm = (
                 }}
               />
             </StyledFormItem>
-            {hasColumn && (
-              <StyledFormItem
-                // don't show the column select unless we have a dataset
-                // style={{ display: datasetId == null ? undefined : 'none' }}
-                name={['filters', filterId, 'column']}
-                initialValue={initColumn}
-                label={<StyledLabel>{t('Column')}</StyledLabel>}
-                rules={[
-                  { required: !removed, message: t('Column is required') },
-                ]}
-                data-test="field-input"
-              >
-                <ColumnSelect
-                  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, {
-                      defaultDataMask: null,
-                    });
-                    forceUpdate();
-                  }}
-                />
-              </StyledFormItem>
-            )}
+            {hasDataset &&
+              Object.keys(mainControlItems).map(
+                key => mainControlItems[key].element,
+              )}
           </StyledRowContainer>
         )}
         <StyledCollapse
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx
index fcff22da0e..b641a58689 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx
@@ -77,6 +77,7 @@ const createControlItems = () => [
   false,
   {},
   { name: 'name_1', config: { renderTrigger: true, resetConfig: true } },
+  { name: 'groupby', config: { multiple: true, required: false } },
 ];
 
 beforeEach(() => {
@@ -87,7 +88,10 @@ function renderControlItems(
   controlItemsMap: ReturnType<typeof getControlItemsMap>,
 ) {
   return render(
-    <>{Object.values(controlItemsMap).map(value => value.element)}</>,
+    // @ts-ignore
+    <>
+      {Object.values(controlItemsMap.controlItems).map(value => value.element)}
+    </>,
   );
 }
 
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
index fd220e458e..45c685de23 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
@@ -26,10 +26,19 @@ import { FormInstance } from 'antd/lib/form';
 import { getChartControlPanelRegistry, styled, t } from '@superset-ui/core';
 import { Tooltip } from 'src/components/Tooltip';
 import { FormItem } from 'src/components/Form';
-import { getControlItems, setNativeFilterFieldValues } from './utils';
+import {
+  doesColumnMatchFilterType,
+  getControlItems,
+  setNativeFilterFieldValues,
+} from './utils';
 import { NativeFiltersForm, NativeFiltersFormItem } from '../types';
-import { StyledRowFormItem } from './FiltersConfigForm';
+import {
+  StyledFormItem,
+  StyledLabel,
+  StyledRowFormItem,
+} from './FiltersConfigForm';
 import { Filter } from '../../types';
+import { ColumnSelect } from './ColumnSelect';
 
 export interface ControlItemsProps {
   disabled: boolean;
@@ -39,6 +48,7 @@ export interface ControlItemsProps {
   filterType: string;
   filterToEdit?: Filter;
   formFilter?: NativeFiltersFormItem;
+  removed?: boolean;
 }
 
 const CleanFormItem = styled(FormItem)`
@@ -53,15 +63,84 @@ export default function getControlItemsMap({
   filterType,
   filterToEdit,
   formFilter,
+  removed,
 }: ControlItemsProps) {
   const controlPanelRegistry = getChartControlPanelRegistry();
   const controlItems =
     getControlItems(controlPanelRegistry.get(filterType)) ?? [];
-  const map: Record<
+  const mapControlItems: Record<
+    string,
+    { element: React.ReactNode; checked: boolean }
+  > = {};
+  const mapMainControlItems: Record<
     string,
     { element: React.ReactNode; checked: boolean }
   > = {};
 
+  controlItems
+    .filter(
+      (mainControlItem: CustomControlItem) =>
+        mainControlItem?.name === 'groupby',
+    )
+    .forEach(mainControlItem => {
+      const initialValue =
+        filterToEdit?.controlValues?.[mainControlItem.name] ??
+        mainControlItem?.config?.default;
+      const initColumn = filterToEdit?.targets[0]?.column?.name;
+      const datasetId = formFilter?.dataset?.value;
+
+      const element = (
+        <>
+          <CleanFormItem
+            name={['filters', filterId, 'requiredFirst', mainControlItem.name]}
+            hidden
+            initialValue={
+              mainControlItem?.config?.requiredFirst &&
+              filterToEdit?.requiredFirst
+            }
+          />
+          <StyledFormItem
+            // don't show the column select unless we have a dataset
+            // style={{ display: datasetId == null ? undefined : 'none' }}
+            name={['filters', filterId, 'column']}
+            initialValue={initColumn}
+            label={
+              <StyledLabel>
+                {t(`${mainControlItem.config?.label}`) || t('Column')}
+              </StyledLabel>
+            }
+            rules={[
+              {
+                required: mainControlItem.config?.required && !removed, // TODO: need to move ColumnSelect settings to controlPanel for all filters
+                message: t('Column is required'),
+              },
+            ]}
+            data-test="field-input"
+          >
+            <ColumnSelect
+              mode={mainControlItem.config?.multiple && 'multiple'}
+              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, {
+                  defaultDataMask: null,
+                });
+                forceUpdate();
+              }}
+            />
+          </StyledFormItem>
+        </>
+      );
+      mapMainControlItems[mainControlItem.name] = {
+        element,
+        checked: initialValue,
+      };
+    });
   controlItems
     .filter(
       (controlItem: CustomControlItem) =>
@@ -129,7 +208,10 @@ export default function getControlItemsMap({
           </Tooltip>
         </>
       );
-      map[controlItem.name] = { element, checked: initialValue };
+      mapControlItems[controlItem.name] = { element, checked: initialValue };
     });
-  return map;
+  return {
+    controlItems: mapControlItems,
+    mainControlItems: mapMainControlItems,
+  };
 }
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts
index 609f7ede6f..d5d97a802b 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts
@@ -98,4 +98,4 @@ export const hasTemporalColumns = (
 export const doesColumnMatchFilterType = (filterType: string, column: Column) =>
   !column.type_generic ||
   !(filterType in FILTER_SUPPORTED_TYPES) ||
-  FILTER_SUPPORTED_TYPES[filterType].includes(column.type_generic);
+  FILTER_SUPPORTED_TYPES[filterType]?.includes(column.type_generic);
diff --git a/superset-frontend/src/filters/components/GroupBy/GroupByFilterPlugin.tsx b/superset-frontend/src/filters/components/GroupBy/GroupByFilterPlugin.tsx
index 6e8a3bfb56..11739c8a12 100644
--- a/superset-frontend/src/filters/components/GroupBy/GroupByFilterPlugin.tsx
+++ b/superset-frontend/src/filters/components/GroupBy/GroupByFilterPlugin.tsx
@@ -68,7 +68,19 @@ export default function PluginFilterGroupBy(props: PluginFilterGroupByProps) {
     // so we can process it like this `JSON.stringify` or start to use `Immer`
   }, [JSON.stringify(defaultValue), multiSelect]);
 
-  const columns = data || [];
+  const groupby = formData?.groupby?.[0]?.length
+    ? formData?.groupby?.[0]
+    : null;
+
+  const withData = groupby
+    ? data.filter(dataItem =>
+        // @ts-ignore
+        groupby.includes(dataItem.column_name),
+      )
+    : data;
+
+  const columns = data ? withData : [];
+
   const placeholderText =
     columns.length === 0
       ? t('No columns')
diff --git a/superset-frontend/src/filters/components/GroupBy/controlPanel.ts b/superset-frontend/src/filters/components/GroupBy/controlPanel.ts
index 970683ec80..e6a6adddc9 100644
--- a/superset-frontend/src/filters/components/GroupBy/controlPanel.ts
+++ b/superset-frontend/src/filters/components/GroupBy/controlPanel.ts
@@ -18,6 +18,7 @@
  */
 import { ControlPanelConfig, sections } from '@superset-ui/chart-controls';
 import { t } from '@superset-ui/core';
+import { sharedControls } from '@superset-ui/chart-controls/lib';
 import { DEFAULT_FORM_DATA } from './types';
 
 const { multiSelect } = DEFAULT_FORM_DATA;
@@ -26,6 +27,23 @@ const config: ControlPanelConfig = {
   controlPanelSections: [
     // @ts-ignore
     sections.legacyRegularTime,
+    {
+      label: t('Query'),
+      expanded: true,
+      controlSetRows: [
+        [
+          {
+            name: 'groupby',
+            config: {
+              ...sharedControls.groupby,
+              label: 'Columns to show',
+              multiple: true,
+              required: false,
+            },
+          },
+        ],
+      ],
+    },
     {
       label: t('UI Configuration'),
       expanded: true,
diff --git a/superset-frontend/src/filters/components/Range/controlPanel.ts b/superset-frontend/src/filters/components/Range/controlPanel.ts
index ad2ecfc4c4..555b019210 100644
--- a/superset-frontend/src/filters/components/Range/controlPanel.ts
+++ b/superset-frontend/src/filters/components/Range/controlPanel.ts
@@ -36,8 +36,7 @@ const config: ControlPanelConfig = {
             config: {
               ...sharedControls.groupby,
               label: 'Column',
-              description:
-                'The numeric column based on which to calculate the range',
+              required: true,
             },
           },
         ],
diff --git a/superset-frontend/src/filters/components/Select/controlPanel.ts b/superset-frontend/src/filters/components/Select/controlPanel.ts
index 80aad2cec1..c06d73c2da 100644
--- a/superset-frontend/src/filters/components/Select/controlPanel.ts
+++ b/superset-frontend/src/filters/components/Select/controlPanel.ts
@@ -18,6 +18,7 @@
  */
 import { t, validateNonEmpty } from '@superset-ui/core';
 import { ControlPanelConfig, sections } from '@superset-ui/chart-controls';
+import { sharedControls } from '@superset-ui/chart-controls/lib';
 import { DEFAULT_FORM_DATA } from './types';
 
 const {
@@ -36,7 +37,18 @@ const config: ControlPanelConfig = {
     {
       label: t('Query'),
       expanded: true,
-      controlSetRows: [['groupby']],
+      controlSetRows: [
+        [
+          {
+            name: 'groupby',
+            config: {
+              ...sharedControls.groupby,
+              label: 'Column',
+              required: true,
+            },
+          },
+        ],
+      ],
     },
     {
       label: t('UI Configuration'),
diff --git a/superset-frontend/src/filters/components/Time/controlPanel.ts b/superset-frontend/src/filters/components/Time/controlPanel.ts
index 466991f79b..9fcbe067f8 100644
--- a/superset-frontend/src/filters/components/Time/controlPanel.ts
+++ b/superset-frontend/src/filters/components/Time/controlPanel.ts
@@ -18,10 +18,27 @@
  */
 import { ControlPanelConfig } from '@superset-ui/chart-controls';
 import { t } from '@superset-ui/core';
+import { sharedControls } from '@superset-ui/chart-controls/lib';
 
 const config: ControlPanelConfig = {
   // For control input types, see: superset-frontend/src/explore/components/controls/index.js
   controlPanelSections: [
+    {
+      label: t('Query'),
+      expanded: true,
+      controlSetRows: [
+        [
+          {
+            name: 'groupby',
+            config: {
+              ...sharedControls.groupby,
+              label: 'Column',
+              required: true,
+            },
+          },
+        ],
+      ],
+    },
     {
       label: t('UI Configuration'),
       expanded: true,