chore(Dashboard): Simplify scoping logic for cross/native filters (#30719)

superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts

@@ -378,7 +378,7 @@ export function cancelNativeFilterSettings() {
     .should('be.visible')
     .should('have.text', 'There are unsaved changes.');
   cy.get(nativeFilters.modal.footer)
-    .find(nativeFilters.modal.yesCancelButton)
+    .find(nativeFilters.modal.confirmCancelButton)
     .contains('cancel')
     .click({ force: true });
   cy.get(nativeFilters.modal.container).should('not.exist');

superset-frontend/cypress-base/cypress/support/directories.ts

@@ -322,7 +322,9 @@ export const nativeFilters = {
     footer: '.ant-modal-footer',
     saveButton: dataTestLocator('native-filter-modal-save-button'),
     cancelButton: dataTestLocator('native-filter-modal-cancel-button'),
-    yesCancelButton: '[type="button"]',
+    confirmCancelButton: dataTestLocator(
+      'native-filter-modal-confirm-cancel-button',
+    ),
     alertXUnsavedFilters: '.ant-alert-message',
     tabsList: {
       filterItemsContainer: dataTestLocator('filter-title-container'),

superset-frontend/src/dashboard/components/Dashboard.jsx

@@ -212,7 +212,7 @@ class Dashboard extends PureComponent {
 
   applyFilters() {
     const { appliedFilters } = this;
-    const { activeFilters, ownDataCharts, datasources, slices } = this.props;
+    const { activeFilters, ownDataCharts, slices } = this.props;
 
     // refresh charts if a filter was removed, added, or changed
 
@@ -231,22 +231,12 @@ class Dashboard extends PureComponent {
       ) {
         // filterKey is removed?
         affectedChartIds.push(
-          ...getRelatedCharts(
+          ...getRelatedCharts(appliedFilters, activeFilters, slices)[filterKey],
-            appliedFilters,
-            activeFilters,
-            slices,
-            datasources,
-          )[filterKey],
         );
       } else if (!appliedFilterKeys.includes(filterKey)) {
         // filterKey is newly added?
         affectedChartIds.push(
-          ...getRelatedCharts(
+          ...getRelatedCharts(activeFilters, appliedFilters, slices)[filterKey],
-            activeFilters,
-            appliedFilters,
-            slices,
-            datasources,
-          )[filterKey],
         );
       } else {
         // if filterKey changes value,
@@ -261,12 +251,9 @@ class Dashboard extends PureComponent {
           )
         ) {
           affectedChartIds.push(
-            ...getRelatedCharts(
+            ...getRelatedCharts(activeFilters, appliedFilters, slices)[
-              activeFilters,
+              filterKey
-              appliedFilters,
+            ],
-              slices,
-              datasources,
-            )[filterKey],
           );
         }
 

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

@@ -17,13 +17,8 @@
  * under the License.
  */
 
-import { FC, useCallback, useRef, useState } from 'react';
+import { FC, useCallback, useEffect, useMemo, useRef, useState } from 'react';
-import {
+import { NativeFilterScope, styled, t } from '@superset-ui/core';
-  NativeFilterScope,
-  styled,
-  t,
-  useComponentDidUpdate,
-} from '@superset-ui/core';
 import { Radio } from 'src/components/Radio';
 import { AntdForm, Typography } from 'src/components';
 import { ScopingType } from './types';
@@ -32,7 +27,7 @@ import { getDefaultScopeValue, isScopingAll } from './utils';
 
 type FilterScopeProps = {
   pathToFormValue?: string[];
-  updateFormValues: (values: any) => void;
+  updateFormValues: (values: any, triggerFormChange?: boolean) => void;
   formFilterScope?: NativeFilterScope;
   forceUpdate: Function;
   filterScope?: NativeFilterScope;
@@ -64,17 +59,19 @@ const FilterScope: FC<FilterScopeProps> = ({
   chartId,
   initiallyExcludedCharts,
 }) => {
-  const [initialFilterScope] = useState(
+  const initialFilterScope = useMemo(
-    filterScope || getDefaultScopeValue(chartId, initiallyExcludedCharts),
+    () => filterScope || getDefaultScopeValue(chartId, initiallyExcludedCharts),
+    [chartId, filterScope, initiallyExcludedCharts],
   );
   const lastSpecificScope = useRef(initialFilterScope);
-  const [initialScopingType] = useState(
+  const initialScopingType = useMemo(
-    isScopingAll(initialFilterScope, chartId)
+    () =>
-      ? ScopingType.All
+      isScopingAll(initialFilterScope, chartId)
-      : ScopingType.Specific,
+        ? ScopingType.All
+        : ScopingType.Specific,
+    [chartId, initialFilterScope],
   );
-  const [hasScopeBeenModified, setHasScopeBeenModified] =
+  const [hasScopeBeenModified, setHasScopeBeenModified] = useState(false);
-    useState(!!filterScope);
 
   const onUpdateFormValues = useCallback(
     (formValues: any) => {
@@ -87,26 +84,24 @@ const FilterScope: FC<FilterScopeProps> = ({
     [formScopingType, updateFormValues],
   );
 
-  const updateScopes = useCallback(() => {
+  const updateScopes = useCallback(
-    if (filterScope || hasScopeBeenModified) {
+    updatedFormValues => {
-      return;
+      if (hasScopeBeenModified) {
-    }
+        return;
+      }
 
-    const newScope = getDefaultScopeValue(chartId, initiallyExcludedCharts);
+      updateFormValues(updatedFormValues, false);
-    updateFormValues({
+    },
-      scope: newScope,
+    [hasScopeBeenModified, updateFormValues],
-      scoping: isScopingAll(newScope, chartId)
+  );
-        ? ScopingType.All
+
-        : ScopingType.Specific,
+  useEffect(() => {
-    });
+    const updatedFormValues = {
-  }, [
+      scope: initialFilterScope,
-    chartId,
+      scoping: initialScopingType,
-    filterScope,
+    };
-    hasScopeBeenModified,
+    updateScopes(updatedFormValues);
-    initiallyExcludedCharts,
+  }, [initialFilterScope, initialScopingType, updateScopes]);
-    updateFormValues,
-  ]);
-  useComponentDidUpdate(updateScopes);
 
   return (
     <Wrapper>

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

@@ -583,9 +583,9 @@ const FiltersConfigForm = (
     !datasetId || datasetDetails || formFilter?.dataset?.label;
 
   const updateFormValues = useCallback(
-    (values: any) => {
+    (values: any, triggerFormChange = true) => {
       setNativeFilterFieldValues(form, filterId, values);
-      formChanged();
+      if (triggerFormChange) formChanged();
     },
     [filterId, form, formChanged],
   );

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

@@ -297,7 +297,6 @@ function FiltersConfigModal({
     setRemovedFilters,
     setOrderedFilters,
     setSaveAlertVisible,
-    filterChanges,
     filterId => {
       setFilterChanges(prev => ({
         ...prev,
@@ -510,7 +509,8 @@ function FiltersConfigModal({
       unsavedFiltersIds.length > 0 ||
       form.isFieldsTouched() ||
       changed ||
-      didChangeOrder
+      didChangeOrder ||
+      Object.values(removedFilters).some(f => f?.isPending)
     ) {
       setSaveAlertVisible(true);
     } else {

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/Footer/CancelConfirmationAlert.tsx

@@ -61,6 +61,7 @@ export function CancelConfirmationAlert({
             buttonSize="small"
             buttonStyle="primary"
             onClick={onConfirm}
+            data-test="native-filter-modal-confirm-cancel-button"
           >
             {t('Yes, cancel')}
           </Button>

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

@@ -170,7 +170,6 @@ export const createHandleRemoveItem =
       val: string[] | ((prevState: string[]) => string[]),
     ) => void,
     setSaveAlertVisible: Function,
-    filterChanges: FilterChangesType,
     removeFilter: (filterId: string) => void,
   ) =>
   (filterId: string) => {

superset-frontend/src/dashboard/util/getRelatedCharts.test.ts

@@ -19,11 +19,9 @@
 
 import {
   AppliedCrossFilterType,
-  DatasourceType,
   Filter,
   NativeFilterType,
 } from '@superset-ui/core';
-import { DatasourcesState } from '../types';
 import { getRelatedCharts } from './getRelatedCharts';
 
 const slices = {
@@ -32,48 +30,11 @@ const slices = {
   '3': { datasource: 'ds1', slice_id: 3 },
 } as any;
 
-const datasources: DatasourcesState = {
+test('Return all chart ids in global scope with native filters', () => {
-  ds1: {
-    uid: 'ds1',
-    datasource_name: 'ds1',
-    table_name: 'table1',
-    description: '',
-    id: 100,
-    columns: [{ column_name: 'column1' }, { column_name: 'column2' }],
-    column_names: ['column1', 'column2'],
-    column_types: [],
-    type: DatasourceType.Table,
-    metrics: [],
-    column_formats: {},
-    currency_formats: {},
-    verbose_map: {},
-    main_dttm_col: '',
-    filter_select_enabled: true,
-  },
-  ds2: {
-    uid: 'ds2',
-    datasource_name: 'ds2',
-    table_name: 'table2',
-    description: '',
-    id: 200,
-    columns: [{ column_name: 'column3' }, { column_name: 'column4' }],
-    column_names: ['column3', 'column4'],
-    column_types: [],
-    type: DatasourceType.Table,
-    metrics: [],
-    column_formats: {},
-    currency_formats: {},
-    verbose_map: {},
-    main_dttm_col: '',
-    filter_select_enabled: true,
-  },
-};
-
-test('Return chart ids matching the dataset id with native filter', () => {
   const filters = {
     filterKey1: {
       filterType: 'filter_select',
-      chartsInScope: [1, 2],
+      chartsInScope: [1, 2, 3],
       scope: {
         excluded: [],
         rootPath: [],
@@ -88,54 +49,54 @@ test('Return chart ids matching the dataset id with native filter', () => {
     } as unknown as Filter,
   };
 
-  const result = getRelatedCharts(filters, null, slices, datasources);
+  const result = getRelatedCharts(filters, null, slices);
   expect(result).toEqual({
-    filterKey1: [1],
+    filterKey1: [1, 2, 3],
   });
 });
 
-test('Return chart ids matching the dataset id with cross filter', () => {
+test('Return only chart ids in specific scope with native filters', () => {
-  const filters = {
-    '3': {
-      filterType: undefined,
-      scope: [1, 2],
-      targets: [],
-      values: null,
-    } as AppliedCrossFilterType,
-  };
-
-  const result = getRelatedCharts(filters, null, slices, datasources);
-  expect(result).toEqual({
-    '3': [1],
-  });
-});
-
-test('Return chart ids matching the column name with native filter', () => {
   const filters = {
     filterKey1: {
       filterType: 'filter_select',
-      chartsInScope: [1, 2],
+      chartsInScope: [1, 3],
       scope: {
         excluded: [],
         rootPath: [],
       },
       targets: [
         {
-          column: { name: 'column3' },
+          column: { name: 'column1' },
-          datasetId: 999,
+          datasetId: 100,
         },
       ],
       type: NativeFilterType.NativeFilter,
     } as unknown as Filter,
   };
 
-  const result = getRelatedCharts(filters, null, slices, datasources);
+  const result = getRelatedCharts(filters, null, slices);
   expect(result).toEqual({
-    filterKey1: [2],
+    filterKey1: [1, 3],
   });
 });
 
-test('Return chart ids matching the column name with cross filter', () => {
+test('Return all chart ids with cross filter in global scope', () => {
+  const filters = {
+    '3': {
+      filterType: undefined,
+      scope: [1, 2, 3],
+      targets: [],
+      values: null,
+    } as AppliedCrossFilterType,
+  };
+
+  const result = getRelatedCharts(filters, null, slices);
+  expect(result).toEqual({
+    '3': [1, 2],
+  });
+});
+
+test('Return only chart ids in specific scope with cross filter', () => {
   const filters = {
     '1': {
       filterType: undefined,
@@ -147,108 +108,8 @@ test('Return chart ids matching the column name with cross filter', () => {
     } as AppliedCrossFilterType,
   };
 
-  const result = getRelatedCharts(filters, null, slices, datasources);
+  const result = getRelatedCharts(filters, null, slices);
   expect(result).toEqual({
     '1': [2],
   });
 });
-
-test('Return chart ids when column display name matches with native filter', () => {
-  const filters = {
-    filterKey1: {
-      filterType: 'filter_select',
-      chartsInScope: [1, 2],
-      scope: {
-        excluded: [],
-        rootPath: [],
-      },
-      targets: [
-        {
-          column: { name: 'column4', displayName: 'column4' },
-          datasetId: 999,
-        },
-      ],
-      type: NativeFilterType.NativeFilter,
-    } as unknown as Filter,
-  };
-
-  const result = getRelatedCharts(filters, null, slices, datasources);
-  expect(result).toEqual({
-    filterKey1: [2],
-  });
-});
-
-test('Return chart ids when column display name matches with cross filter', () => {
-  const filters = {
-    '1': {
-      filterType: undefined,
-      scope: [1, 2],
-      targets: [],
-      values: {
-        filters: [{ col: 'column4' }],
-      },
-    } as AppliedCrossFilterType,
-  };
-
-  const result = getRelatedCharts(filters, null, slices, datasources);
-  expect(result).toEqual({
-    '1': [2],
-  });
-});
-
-test('Return scope when filterType is not filter_select', () => {
-  const filters = {
-    filterKey1: {
-      filterType: 'filter_time',
-      chartsInScope: [3, 4],
-    } as Filter,
-  };
-
-  const result = getRelatedCharts(filters, null, slices, datasources);
-  expect(result).toEqual({
-    filterKey1: [3, 4],
-  });
-});
-
-test('Return an empty array if no matching charts found with native filter', () => {
-  const filters = {
-    filterKey1: {
-      filterType: 'filter_select',
-      chartsInScope: [1, 2],
-      scope: {
-        excluded: [],
-        rootPath: [],
-      },
-      targets: [
-        {
-          column: { name: 'nonexistent_column' },
-          datasetId: 300,
-        },
-      ],
-      type: NativeFilterType.NativeFilter,
-    } as unknown as Filter,
-  };
-
-  const result = getRelatedCharts(filters, null, slices, datasources);
-  expect(result).toEqual({
-    filterKey1: [],
-  });
-});
-
-test('Return an empty array if no matching charts found with cross filter', () => {
-  const filters = {
-    '1': {
-      filterType: undefined,
-      scope: [1, 2],
-      targets: [],
-      values: {
-        filters: [{ col: 'nonexistent_column' }],
-      },
-    } as AppliedCrossFilterType,
-  };
-
-  const result = getRelatedCharts(filters, null, slices, datasources);
-  expect(result).toEqual({
-    '1': [],
-  });
-});

superset-frontend/src/dashboard/util/getRelatedCharts.ts

@@ -20,56 +20,31 @@
 import {
   AppliedCrossFilterType,
   AppliedNativeFilterType,
-  ensureIsArray,
   Filter,
   isAppliedCrossFilterType,
   isAppliedNativeFilterType,
   isNativeFilter,
 } from '@superset-ui/core';
 import { Slice } from 'src/types/Chart';
-import { DatasourcesState } from '../types';
 
-function checkForExpression(formData?: Record<string, any>) {
+function isGlobalScope(scope: number[], slices: Record<string, Slice>) {
-  const groupby = ensureIsArray(formData?.groupby) ?? [];
+  return scope.length === Object.keys(slices).length;
-  const allColumns = ensureIsArray(formData?.all_columns) ?? [];
-  const checkColumns = groupby.concat(allColumns);
-  return checkColumns.some(
-    (col: string | Record<string, any>) =>
-      typeof col !== 'string' && col.expressionType !== undefined,
-  );
 }
 
 function getRelatedChartsForSelectFilter(
   nativeFilter: AppliedNativeFilterType | Filter,
   slices: Record<string, Slice>,
   chartsInScope: number[],
-  datasources: DatasourcesState,
 ) {
   return Object.values(slices)
     .filter(slice => {
-      const { datasource, slice_id } = slice;
+      const { slice_id } = slice;
-      // not in scope, ignore
+      // all have been selected, always apply
-      if (!chartsInScope.includes(slice_id)) return false;
+      if (isGlobalScope(chartsInScope, slices)) return true;
+      // hand-picked in scope, always apply
+      if (chartsInScope.includes(slice_id)) return true;
 
-      const chartDatasource = datasource
+      return false;
-        ? datasources[datasource]
-        : Object.values(datasources).find(ds => ds.id === slice.datasource_id);
-
-      const { column, datasetId } = nativeFilter.targets?.[0] ?? {};
-      const datasourceColumnNames = chartDatasource?.column_names ?? [];
-
-      // same datasource, always apply
-      if (chartDatasource?.id === datasetId) return true;
-
-      // let backend deal with adhoc columns and jinja
-      const hasSqlExpression = checkForExpression(slice.form_data);
-      if (hasSqlExpression) {
-        return true;
-      }
-
-      return datasourceColumnNames.some(
-        col => col === column?.name || col === column?.displayName,
-      );
     })
     .map(slice => slice.slice_id);
 }
@@ -78,52 +53,23 @@ function getRelatedChartsForCrossFilter(
   crossFilter: AppliedCrossFilterType,
   slices: Record<string, Slice>,
   scope: number[],
-  datasources: DatasourcesState,
 ): number[] {
   const sourceSlice = slices[filterKey];
-  const filters = crossFilter?.values?.filters ?? [];
 
   if (!sourceSlice) return [];
 
-  const sourceDatasource = sourceSlice.datasource
-    ? datasources[sourceSlice.datasource]
-    : Object.values(datasources).find(
-        ds => ds.id === sourceSlice.datasource_id,
-      );
-
   return Object.values(slices)
     .filter(slice => {
       // cross-filter emitter
       if (slice.slice_id === Number(filterKey)) return false;
-      // not in scope, ignore
+      // hand-picked in the scope, always apply
-      if (!scope.includes(slice.slice_id)) return false;
+      const fullScope = [
-
+        ...scope.filter(s => String(s) !== filterKey),
-      const targetDatasource = slice.datasource
+        Number(filterKey),
-        ? datasources[slice.datasource]
+      ];
-        : Object.values(datasources).find(ds => ds.id === slice.datasource_id);
+      if (isGlobalScope(fullScope, slices)) return true;
-
+      // hand-picked in the scope, always apply
-      // same datasource, always apply
+      if (scope.includes(slice.slice_id)) return true;
-      if (targetDatasource === sourceDatasource) return true;
-
-      const targetColumnNames = targetDatasource?.column_names ?? [];
-
-      // let backend deal with adhoc columns and jinja
-      const hasSqlExpression = checkForExpression(slice.form_data);
-      if (hasSqlExpression) {
-        return true;
-      }
-
-      for (const filter of filters) {
-        // let backend deal with adhoc columns
-        if (
-          typeof filter.col !== 'string' &&
-          filter.col.expressionType !== undefined
-        ) {
-          return true;
-        }
-        // shared column names, different datasources, apply filter
-        if (targetColumnNames.includes(filter.col)) return true;
-      }
 
       return false;
     })
@@ -140,7 +86,6 @@ export function getRelatedCharts(
     AppliedNativeFilterType | AppliedCrossFilterType | Filter
   > | null,
   slices: Record<string, Slice>,
-  datasources: DatasourcesState,
 ) {
   const related = Object.entries(filters).reduce((acc, [filterKey, filter]) => {
     const isCrossFilter =
@@ -168,7 +113,6 @@ export function getRelatedCharts(
           actualCrossFilter,
           slices,
           chartsInScope,
-          datasources,
         ),
       };
     }
@@ -186,7 +130,6 @@ export function getRelatedCharts(
           nativeFilter,
           slices,
           chartsInScope,
-          datasources,
         ),
       };
     }

superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts

@@ -51,8 +51,6 @@ const useFilterFocusHighlightStyles = (chartId: number) => {
     dashboardFilters,
   );
 
-  const datasources =
-    useSelector((state: RootState) => state.datasources) || {};
   const slices =
     useSelector((state: RootState) => state.sliceEntities.slices) || {};
 
@@ -60,7 +58,6 @@ const useFilterFocusHighlightStyles = (chartId: number) => {
     nativeFilters.filters as Record<string, Filter>,
     null,
     slices,
-    datasources,
   );
 
   const highlightedFilterId =