feat(native-filters): Auto apply changes in FiltersConfigModal (#14461)

* fix:fix get permission function

* feat: auto apply changes after save filters config modal

* fix: repopulate default values

* test: fix tests

* test: fix tests

* fix: fix CR notes

* fix: fix CR notes

superset-frontend/src/dashboard/actions/nativeFilters.ts

@@ -74,6 +74,7 @@ export const setFilterConfiguration = (
     filterConfig,
   });
   const { id, metadata } = getState().dashboardInfo;
+  const oldFilters = getState().nativeFilters?.filters;
 
   // TODO extract this out when makeApi supports url parameters
   const updateDashboard = makeApi<
@@ -100,7 +101,7 @@ export const setFilterConfiguration = (
       type: SET_FILTER_CONFIG_COMPLETE,
       filterConfig,
     });
-    dispatch(setDataMaskForFilterConfigComplete(filterConfig));
+    dispatch(setDataMaskForFilterConfigComplete(filterConfig, oldFilters));
   } catch (err) {
     dispatch({ type: SET_FILTER_CONFIG_FAIL, filterConfig });
     dispatch({ type: SET_DATA_MASK_FOR_FILTER_CONFIG_FAIL, filterConfig });

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx

@@ -18,7 +18,7 @@
  */
 
 import React from 'react';
-import { render, screen, cleanup } from 'spec/helpers/testing-library';
+import { render, screen } from 'spec/helpers/testing-library';
 import { Provider } from 'react-redux';
 import userEvent from '@testing-library/user-event';
 import {
@@ -71,7 +71,7 @@ const getDateControlTestId = testWithId<string>(
 const FILTER_NAME = 'Time filter 1';
 const FILTER_SET_NAME = 'New filter set';
 
-const addFilterFlow = () => {
+const addFilterFlow = async () => {
   // open filter config modal
   userEvent.click(screen.getByTestId(getTestId('collapsable')));
   userEvent.click(screen.getByTestId(getTestId('create-filter')));
@@ -80,6 +80,7 @@ const addFilterFlow = () => {
   userEvent.click(screen.getByText('Time filter'));
   userEvent.type(screen.getByTestId(getModalTestId('name-input')), FILTER_NAME);
   userEvent.click(screen.getByText('Save'));
+  await screen.findByText('All Filters (1)');
 };
 
 const addFilterSetFlow = async () => {
@@ -175,7 +176,7 @@ describe('FilterBar', () => {
   });
 
   beforeEach(() => {
-    toggleFiltersBar.mockClear();
+    jest.clearAllMocks();
     fetchMock.get(
       'http://localhost/api/v1/time_range/?q=%27Last%20day%27',
       {
@@ -203,11 +204,6 @@ describe('FilterBar', () => {
     mockCore.makeApi = jest.fn(() => mockApi);
   });
 
-  afterEach(() => {
-    cleanup();
-    jest.clearAllMocks();
-  });
-
   const renderWrapper = (props = closedBarProps, state?: object) =>
     render(
       <Provider store={state ? getMockStore(state) : mockStore}>
@@ -306,14 +302,12 @@ describe('FilterBar', () => {
     renderWrapper(openedBarProps, stateWithoutNativeFilters);
     expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
 
-    addFilterFlow();
+    await addFilterFlow();
 
-    await screen.findByText('All Filters (1)');
     expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
   });
 
-  // TODO: fix flakiness and re-enable
+  it('add and apply filter set', async () => {
-  it.skip('add and apply filter set', async () => {
     // @ts-ignore
     global.featureFlags = {
       [FeatureFlag.DASHBOARD_NATIVE_FILTERS]: true,
@@ -321,22 +315,17 @@ describe('FilterBar', () => {
     };
     renderWrapper(openedBarProps, stateWithoutNativeFilters);
 
-    addFilterFlow();
+    await addFilterFlow();
-
-    await screen.findByText('All Filters (1)');
 
     await addFilterSetFlow();
 
     // change filter
     expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
-
+    userEvent.click(await screen.findByText('All Filters (1)'));
     await changeFilterValue();
     await waitFor(() => expect(screen.getAllByText('Last day').length).toBe(2));
 
     // apply new filter value
-    await waitFor(() =>
-      expect(screen.getByTestId(getTestId('apply-button'))).toBeEnabled(),
-    );
     userEvent.click(screen.getByTestId(getTestId('apply-button')));
     await waitFor(() =>
       expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled(),
@@ -352,12 +341,11 @@ describe('FilterBar', () => {
     ).not.toHaveAttribute('data-selected', 'true');
     userEvent.click(screen.getByTestId(getTestId('filter-set-wrapper')));
     expect(await screen.findByText('Last week')).toBeInTheDocument();
-    expect(screen.getByTestId(getTestId('apply-button'))).toBeEnabled();
     userEvent.click(screen.getByTestId(getTestId('apply-button')));
     expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
   });
 
-  it.skip('add and edit filter set', async () => {
+  it('add and edit filter set', async () => {
     // @ts-ignore
     global.featureFlags = {
       [FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET]: true,
@@ -365,9 +353,7 @@ describe('FilterBar', () => {
     };
     renderWrapper(openedBarProps, stateWithoutNativeFilters);
 
-    addFilterFlow();
+    await addFilterFlow();
-
-    await screen.findByText('All Filters (1)');
 
     await addFilterSetFlow();
 
@@ -375,12 +361,13 @@ describe('FilterBar', () => {
     userEvent.click(screen.getByText('Edit'));
 
     await changeFilterValue();
-    await waitFor(() => expect(screen.getAllByText('Last day').length).toBe(1));
 
     // apply new changes and save them
-    expect(
+    await waitFor(() =>
-      screen.getByTestId(getTestId('filter-set-edit-save')),
+      expect(
-    ).toBeDisabled();
+        screen.getByTestId(getTestId('filter-set-edit-save')),
+      ).toBeDisabled(),
+    );
     expect(screen.getByTestId(getTestId('apply-button'))).toBeEnabled();
     userEvent.click(screen.getByTestId(getTestId('apply-button')));
     expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ControlItems.tsx

@@ -23,13 +23,15 @@ import {
 import React, { FC } from 'react';
 import { Checkbox } from 'src/common/components';
 import { FormInstance } from 'antd/lib/form';
-import { getChartControlPanelRegistry } from '@superset-ui/core';
+import { getChartControlPanelRegistry, t } from '@superset-ui/core';
+import { Tooltip } from 'src/components/Tooltip';
 import { getControlItems, setNativeFilterFieldValues } from './utils';
 import { NativeFiltersForm, NativeFiltersFormItem } from '../types';
 import { StyledCheckboxFormItem } from './FiltersConfigForm';
 import { Filter } from '../../types';
 
 type ControlItemsProps = {
+  disabled: boolean;
   filterId: string;
   forceUpdate: Function;
   filterToEdit?: Filter;
@@ -38,6 +40,7 @@ type ControlItemsProps = {
 };
 
 const ControlItems: FC<ControlItemsProps> = ({
+  disabled,
   forceUpdate,
   form,
   filterId,
@@ -59,38 +62,48 @@ const ControlItems: FC<ControlItemsProps> = ({
             controlItem?.config?.renderTrigger,
         )
         .map(controlItem => (
-          <StyledCheckboxFormItem
+          <Tooltip
-            key={controlItem.name}
+            placement="left"
-            name={['filters', filterId, 'controlValues', controlItem.name]}
+            title={
-            initialValue={
+              controlItem.config.affectsDataMask &&
-              filterToEdit?.controlValues?.[controlItem.name] ??
+              disabled &&
-              controlItem?.config?.default
+              t('Populate "Default value" to enable this control')
             }
-            valuePropName="checked"
-            colon={false}
           >
-            <Checkbox
+            <StyledCheckboxFormItem
-              onChange={() => {
+              key={controlItem.name}
-                if (!controlItem.config.resetConfig) {
+              name={['filters', filterId, 'controlValues', controlItem.name]}
-                  forceUpdate();
+              initialValue={
-                  return;
+                filterToEdit?.controlValues?.[controlItem.name] ??
-                }
+                controlItem?.config?.default
-                setNativeFilterFieldValues(form, filterId, {
+              }
-                  defaultDataMask: null,
+              valuePropName="checked"
-                });
+              colon={false}
-                forceUpdate();
-              }}
             >
-              {controlItem.config.label}{' '}
+              <Checkbox
-              {controlItem.config.description && (
+                disabled={controlItem.config.affectsDataMask && disabled}
-                <InfoTooltipWithTrigger
+                onChange={() => {
-                  placement="top"
+                  if (!controlItem.config.resetConfig) {
-                  label={controlItem.config.name}
+                    forceUpdate();
-                  tooltip={controlItem.config.description}
+                    return;
-                />
+                  }
-              )}
+                  setNativeFilterFieldValues(form, filterId, {
-            </Checkbox>
+                    defaultDataMask: null,
-          </StyledCheckboxFormItem>
+                  });
+                  forceUpdate();
+                }}
+              >
+                {controlItem.config.label}{' '}
+                {controlItem.config.description && (
+                  <InfoTooltipWithTrigger
+                    placement="top"
+                    label={controlItem.config.name}
+                    tooltip={controlItem.config.description}
+                  />
+                )}
+              </Checkbox>
+            </StyledCheckboxFormItem>
+          </Tooltip>
         ))}
     </>
   );

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx

@@ -260,6 +260,8 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
     label: filter.title,
   }));
 
+  const showDefaultValue = !hasDataset || (!isDataDirty && hasFilledDataset);
+
   return (
     <>
       <Typography.Title level={5}>{t('Settings')}</Typography.Title>
@@ -434,7 +436,7 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
           data-test="default-input"
           label={<StyledLabel>{t('Default Value')}</StyledLabel>}
         >
-          {(!hasDataset || (!isDataDirty && hasFilledDataset)) && (
+          {showDefaultValue ? (
             <DefaultValue
               setDataMask={dataMask => {
                 setNativeFilterFieldValues(form, filterId, {
@@ -447,6 +449,10 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
               form={form}
               formData={newFormData}
             />
+          ) : hasFilledDataset ? (
+            t('Click "Populate" to get "Default Value" ->')
+          ) : (
+            t('Fill all required fields to enable "Default Value"')
           )}
         </StyledFormItem>
       </StyledContainer>
@@ -461,6 +467,7 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
         </Checkbox>
       </StyledCheckboxFormItem>
       <ControlItems
+        disabled={!showDefaultValue}
         filterToEdit={filterToEdit}
         formFilter={formFilter}
         filterId={filterId}

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts

@@ -42,13 +42,20 @@ export const validateForm = async (
       errors: [error],
     };
     form.setFields([fieldError]);
-    // eslint-disable-next-line no-throw-literal
-    throw { errorFields: [fieldError] };
   };
 
   try {
-    const formValues = (await form.validateFields()) as NativeFiltersForm;
+    let formValues: NativeFiltersForm;
-
+    try {
+      formValues = (await form.validateFields()) as NativeFiltersForm;
+    } catch (error) {
+      // In Jest tests in chain of tests, Ant generate `outOfDate` error so need to catch it here
+      if (!error?.errorFields?.length && error?.outOfDate) {
+        formValues = error.values;
+      } else {
+        throw error;
+      }
+    }
     const validateInstant = (filterId: string) => {
       const isInstant = formValues.filters[filterId]
         ? formValues.filters[filterId].isInstant

superset-frontend/src/dataMask/actions.ts

@@ -19,6 +19,7 @@
 import { DataMask } from '@superset-ui/core';
 import { FilterConfiguration } from '../dashboard/components/nativeFilters/types';
 import { FeatureFlag, isFeatureEnabled } from '../featureFlags';
+import { Filters } from '../dashboard/reducers/types';
 
 export const UPDATE_DATA_MASK = 'UPDATE_DATA_MASK';
 export interface UpdateDataMask {
@@ -33,6 +34,7 @@ export const SET_DATA_MASK_FOR_FILTER_CONFIG_COMPLETE =
 export interface SetDataMaskForFilterConfigComplete {
   type: typeof SET_DATA_MASK_FOR_FILTER_CONFIG_COMPLETE;
   filterConfig: FilterConfiguration;
+  filters?: Filters;
 }
 
 export const SET_DATA_MASK_FOR_FILTER_CONFIG_FAIL =
@@ -44,10 +46,12 @@ export interface SetDataMaskForFilterConfigFail {
 }
 export function setDataMaskForFilterConfigComplete(
   filterConfig: FilterConfiguration,
+  filters?: Filters,
 ): SetDataMaskForFilterConfigComplete {
   return {
     type: SET_DATA_MASK_FOR_FILTER_CONFIG_COMPLETE,
     filterConfig,
+    filters,
   };
 }
 export function updateDataMask(

superset-frontend/src/dataMask/reducer.ts

@@ -34,6 +34,8 @@ import {
   Filter,
   FilterConfiguration,
 } from '../dashboard/components/nativeFilters/types';
+import { areObjectsEqual } from '../reduxUtils';
+import { Filters } from '../dashboard/reducers/types';
 
 export function getInitialDataMask(id?: string): DataMask;
 export function getInitialDataMask(id: string): DataMaskWithId {
@@ -54,21 +56,37 @@ export function getInitialDataMask(id: string): DataMaskWithId {
 }
 
 function fillNativeFilters(
-  data: FilterConfiguration,
+  filterConfig: FilterConfiguration,
-  cleanState: DataMaskStateWithId,
+  mergedDataMask: DataMaskStateWithId,
-  draft: DataMaskStateWithId,
+  draftDataMask: DataMaskStateWithId,
+  currentFilters?: Filters,
 ) {
-  data.forEach((filter: Filter) => {
+  filterConfig.forEach((filter: Filter) => {
-    cleanState[filter.id] = {
+    mergedDataMask[filter.id] = {
       ...getInitialDataMask(filter.id), // take initial data
       ...filter.defaultDataMask, // if something new came from BE - take it
-      ...draft[filter.id], // keep local filter data
+      ...draftDataMask[filter.id], // keep local filter data
     };
+    // if we came from filters config modal and particular filters changed take it's dataMask
+    if (
+      currentFilters &&
+      !areObjectsEqual(
+        filter.defaultDataMask,
+        currentFilters[filter.id]?.defaultDataMask,
+        { ignoreUndefined: true },
+      )
+    ) {
+      mergedDataMask[filter.id] = {
+        ...mergedDataMask[filter.id],
+        ...filter.defaultDataMask,
+      };
+    }
   });
+
   // Get back all other non-native filters
-  Object.values(draft).forEach(filter => {
+  Object.values(draftDataMask).forEach(filter => {
     if (!String(filter?.id).startsWith(NATIVE_FILTER_PREFIX)) {
-      cleanState[filter?.id] = filter;
+      mergedDataMask[filter?.id] = filter;
     }
   });
 }
@@ -106,7 +124,12 @@ const dataMaskReducer = produce(
         );
         return cleanState;
       case SET_DATA_MASK_FOR_FILTER_CONFIG_COMPLETE:
-        fillNativeFilters(action.filterConfig ?? [], cleanState, draft);
+        fillNativeFilters(
+          action.filterConfig ?? [],
+          cleanState,
+          draft,
+          action.filters,
+        );
         return cleanState;
 
       default:

superset-frontend/src/filters/components/GroupBy/controlPanel.ts

@@ -37,6 +37,7 @@ const config: ControlPanelConfig = {
               type: 'CheckboxControl',
               label: t('Multiple select'),
               default: multiSelect,
+              affectsDataMask: true,
               resetConfig: true,
               renderTrigger: true,
               description: t('Allow selecting multiple values'),

superset-frontend/src/filters/components/Select/controlPanel.ts

@@ -61,6 +61,7 @@ const config: ControlPanelConfig = {
               label: t('Multiple select'),
               default: multiSelect,
               resetConfig: true,
+              affectsDataMask: true,
               renderTrigger: true,
               description: t('Allow selecting multiple values'),
             },
@@ -89,6 +90,7 @@ const config: ControlPanelConfig = {
               label: t('Default to first item'),
               default: defaultToFirstItem,
               resetConfig: true,
+              affectsDataMask: true,
               renderTrigger: true,
               description: t('Select first item by default'),
             },
@@ -100,6 +102,7 @@ const config: ControlPanelConfig = {
             config: {
               type: 'CheckboxControl',
               renderTrigger: true,
+              affectsDataMask: true,
               label: t('Inverse selection'),
               default: inverseSelection,
               description: t('Exclude selected values'),