fix(Filters): Apply native & cross filters on common columns (apache#…

…30438)
preset-io · Oct 15, 2024 · 3629483 · 3629483
1 parent 4cac7fe
commit 3629483
Show file tree

Hide file tree

Showing 21 changed files with 656 additions and 38 deletions.
diff --git a/docs/static/resources/openapi.json b/docs/static/resources/openapi.json
@@ -3053,6 +3053,12 @@
             },
             "type": "array"
           },
+          "column_names": {
+            "items": {
+              "type": "string"
+            },
+            "type": "array"
+          },
           "currency_formats": {
             "type": "object"
           },

diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts
@@ -83,6 +83,7 @@ export interface Dataset {
   owners?: Owner[];
   filter_select?: boolean;
   filter_select_enabled?: boolean;
+  column_names?: string[];
 }
 
 export interface ControlPanelState {

diff --git a/superset-frontend/packages/superset-ui-core/src/query/types/Dashboard.ts b/superset-frontend/packages/superset-ui-core/src/query/types/Dashboard.ts
@@ -80,6 +80,24 @@ export type Filter = {
   description: string;
 };
 
+export type AppliedFilter = {
+  values: {
+    filters: Record<string, any>[];
+  } | null;
+};
+
+export type AppliedCrossFilterType = {
+  filterType: undefined;
+  targets: number[];
+  scope: number[];
+} & AppliedFilter;
+
+export type AppliedNativeFilterType = {
+  filterType: 'filter_select';
+  scope: number[];
+  targets: Partial<NativeFilterTarget>[];
+} & AppliedFilter;
+
 export type FilterWithDataMask = Filter & { dataMask: DataMaskWithId };
 
 export type Divider = Partial<Omit<Filter, 'id' | 'type'>> & {
@@ -89,6 +107,24 @@ export type Divider = Partial<Omit<Filter, 'id' | 'type'>> & {
   type: typeof NativeFilterType.Divider;
 };
 
+export function isAppliedCrossFilterType(
+  filterElement: AppliedCrossFilterType | AppliedNativeFilterType | Filter,
+): filterElement is AppliedCrossFilterType {
+  return (
+    filterElement.filterType === undefined &&
+    filterElement.hasOwnProperty('values')
+  );
+}
+
+export function isAppliedNativeFilterType(
+  filterElement: AppliedCrossFilterType | AppliedNativeFilterType | Filter,
+): filterElement is AppliedNativeFilterType {
+  return (
+    filterElement.filterType === 'filter_select' &&
+    filterElement.hasOwnProperty('values')
+  );
+}
+
 export function isNativeFilter(
   filterElement: Filter | Divider,
 ): filterElement is Filter {

diff --git a/superset-frontend/packages/superset-ui-core/test/query/types/Dashboard.test.ts b/superset-frontend/packages/superset-ui-core/test/query/types/Dashboard.test.ts
@@ -24,6 +24,10 @@ import {
   FilterWithDataMask,
   Divider,
   isNativeFilterWithDataMask,
+  isAppliedCrossFilterType,
+  isAppliedNativeFilterType,
+  AppliedCrossFilterType,
+  AppliedNativeFilterType,
 } from '@superset-ui/core';
 
 const filter: Filter = {
@@ -51,6 +55,20 @@ const filterDivider: Divider = {
   description: 'Divider description.',
 };
 
+const appliedCrossFilter: AppliedCrossFilterType = {
+  filterType: undefined,
+  targets: [1, 2],
+  scope: [1, 2],
+  values: null,
+};
+
+const appliedNativeFilter: AppliedNativeFilterType = {
+  filterType: 'filter_select',
+  scope: [1, 2],
+  targets: [{}],
+  values: null,
+};
+
 test('filter type guard', () => {
   expect(isNativeFilter(filter)).toBeTruthy();
   expect(isNativeFilter(filterWithDataMask)).toBeTruthy();
@@ -68,3 +86,13 @@ test('filter divider type guard', () => {
   expect(isFilterDivider(filterWithDataMask)).toBeFalsy();
   expect(isFilterDivider(filterDivider)).toBeTruthy();
 });
+
+test('applied cross filter type guard', () => {
+  expect(isAppliedCrossFilterType(appliedCrossFilter)).toBeTruthy();
+  expect(isAppliedCrossFilterType(appliedNativeFilter)).toBeFalsy();
+});
+
+test('applied native filter type guard', () => {
+  expect(isAppliedNativeFilterType(appliedNativeFilter)).toBeTruthy();
+  expect(isAppliedNativeFilterType(appliedCrossFilter)).toBeFalsy();
+});
diff --git a/superset-frontend/spec/fixtures/mockDashboardInfo.js b/superset-frontend/spec/fixtures/mockDashboardInfo.js
@@ -26,6 +26,7 @@ export default {
       {
         id: 'DefaultsID',
         filterType: 'filter_select',
+        chartsInScope: [],
         targets: [{}],
         cascadeParentIds: [],
       },

diff --git a/superset-frontend/spec/fixtures/mockNativeFilters.ts b/superset-frontend/spec/fixtures/mockNativeFilters.ts
@@ -133,6 +133,7 @@ export const singleNativeFiltersState = {
       id: [NATIVE_FILTER_ID],
       name: 'eth',
       type: 'text',
+      filterType: 'filter_select',
       targets: [{ datasetId: 13, column: { name: 'ethnic_minority' } }],
       defaultDataMask: {
         filterState: {

diff --git a/superset-frontend/src/dashboard/actions/dashboardState.js b/superset-frontend/src/dashboard/actions/dashboardState.js
@@ -61,7 +61,7 @@ import {
   dashboardInfoChanged,
   SAVE_CHART_CONFIG_COMPLETE,
 } from './dashboardInfo';
-import { fetchDatasourceMetadata } from './datasources';
+import { fetchDatasourceMetadata, setDatasources } from './datasources';
 import { updateDirectPathToFilter } from './dashboardFilters';
 import { SET_FILTER_CONFIG_COMPLETE } from './nativeFilters';
 import getOverwriteItems from '../util/getOverwriteItems';
@@ -341,6 +341,17 @@ export function saveDashboardRequest(data, id, saveType) {
             filterConfig: metadata.native_filter_configuration,
           });
         }
+
+        // fetch datasets to make sure they are up to date
+        SupersetClient.get({
+          endpoint: `/api/v1/dashboard/${id}/datasets`,
+          headers: { 'Content-Type': 'application/json' },
+        }).then(({ json }) => {
+          const datasources = json?.result ?? [];
+          if (datasources.length) {
+            dispatch(setDatasources(datasources));
+          }
+        });
       }
       if (lastModifiedTime) {
         dispatch(saveDashboardRequestSuccess(lastModifiedTime));

diff --git a/superset-frontend/src/dashboard/components/Dashboard.jsx b/superset-frontend/src/dashboard/components/Dashboard.jsx
@@ -41,6 +41,7 @@ import { areObjectsEqual } from '../../reduxUtils';
 import getLocationHash from '../util/getLocationHash';
 import isDashboardEmpty from '../util/isDashboardEmpty';
 import { getAffectedOwnDataCharts } from '../util/charts/getOwnDataCharts';
+import { getRelatedCharts } from '../util/getRelatedCharts';
 
 const propTypes = {
   actions: PropTypes.shape({
@@ -211,9 +212,10 @@ class Dashboard extends PureComponent {
 
   applyFilters() {
     const { appliedFilters } = this;
-    const { activeFilters, ownDataCharts } = this.props;
+    const { activeFilters, ownDataCharts, datasources, slices } = this.props;
 
     // refresh charts if a filter was removed, added, or changed
+
     const currFilterKeys = Object.keys(activeFilters);
     const appliedFilterKeys = Object.keys(appliedFilters);
 
@@ -228,10 +230,24 @@ class Dashboard extends PureComponent {
         appliedFilterKeys.includes(filterKey)
       ) {
         // filterKey is removed?
-        affectedChartIds.push(...appliedFilters[filterKey].scope);
+        affectedChartIds.push(
+          ...getRelatedCharts(
+            appliedFilters,
+            activeFilters,
+            slices,
+            datasources,
+          )[filterKey],
+        );
       } else if (!appliedFilterKeys.includes(filterKey)) {
         // filterKey is newly added?
-        affectedChartIds.push(...activeFilters[filterKey].scope);
+        affectedChartIds.push(
+          ...getRelatedCharts(
+            activeFilters,
+            appliedFilters,
+            slices,
+            datasources,
+          )[filterKey],
+        );
       } else {
         // if filterKey changes value,
         // update charts in its scope
@@ -244,7 +260,14 @@ class Dashboard extends PureComponent {
             },
           )
         ) {
-          affectedChartIds.push(...activeFilters[filterKey].scope);
+          affectedChartIds.push(
+            ...getRelatedCharts(
+              activeFilters,
+              appliedFilters,
+              slices,
+              datasources,
+            )[filterKey],
+          );
         }
 
         // if filterKey changes scope,

diff --git a/superset-frontend/src/dashboard/components/Dashboard.test.jsx b/superset-frontend/src/dashboard/components/Dashboard.test.jsx
@@ -37,6 +37,9 @@ import { dashboardLayout } from 'spec/fixtures/mockDashboardLayout';
 import dashboardState from 'spec/fixtures/mockDashboardState';
 import { sliceEntitiesForChart as sliceEntities } from 'spec/fixtures/mockSliceEntities';
 import { getAllActiveFilters } from 'src/dashboard/util/activeAllDashboardFilters';
+import { getRelatedCharts } from 'src/dashboard/util/getRelatedCharts';
+
+jest.mock('src/dashboard/util/getRelatedCharts');
 
 describe('Dashboard', () => {
   const props = {
@@ -130,6 +133,7 @@ describe('Dashboard', () => {
 
     afterEach(() => {
       refreshSpy.restore();
+      jest.clearAllMocks();
     });
 
     it('should not call refresh when is editMode', () => {
@@ -153,6 +157,9 @@ describe('Dashboard', () => {
     });
 
     it('should call refresh when native filters changed', () => {
+      getRelatedCharts.mockReturnValue({
+        [NATIVE_FILTER_ID]: [230],
+      });
       wrapper.setProps({
         activeFilters: {
           ...OVERRIDE_FILTERS,
@@ -170,11 +177,27 @@ describe('Dashboard', () => {
         [NATIVE_FILTER_ID]: {
           scope: [230],
           values: extraFormData,
+          filterType: 'filter_select',
+          targets: [
+            {
+              datasetId: 13,
+              column: {
+                name: 'ethnic_minority',
+              },
+            },
+          ],
         },
       });
     });
 
     it('should call refresh if a filter is added', () => {
+      getRelatedCharts.mockReturnValue({
+        '1_region': [1],
+        '2_country_name': [1, 2],
+        '3_region': [1],
+        '3_country_name': [],
+        gender: [1],
+      });
       const newFilter = {
         gender: { values: ['boy', 'girl'], scope: [1] },
       };
@@ -186,6 +209,12 @@ describe('Dashboard', () => {
     });
 
     it('should call refresh if a filter is removed', () => {
+      getRelatedCharts.mockReturnValue({
+        '1_region': [1],
+        '2_country_name': [1, 2],
+        '3_region': [1],
+        '3_country_name': [],
+      });
       wrapper.setProps({
         activeFilters: {},
       });

diff --git a/superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.test.tsx b/superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.test.tsx
@@ -51,6 +51,11 @@ test('renders diff viewer when it contains overwriteConfirmMetadata', async () =
 
 test('requests update dashboard api when save button is clicked', async () => {
   const updateDashboardEndpoint = `glob:*/api/v1/dashboard/${overwriteConfirmMetadata.dashboardId}`;
+  const fetchDatasetsEndpoint = `glob:*/api/v1/dashboard/${overwriteConfirmMetadata.dashboardId}/datasets`;
+
+  // mock fetch datasets
+  fetchMock.get(fetchDatasetsEndpoint, []);
+
   fetchMock.put(updateDashboardEndpoint, {
     id: overwriteConfirmMetadata.dashboardId,
     last_modified_time: +new Date(),

diff --git a/superset-frontend/src/dashboard/fixtures/mockNativeFilters.ts b/superset-frontend/src/dashboard/fixtures/mockNativeFilters.ts
@@ -39,6 +39,7 @@ export const nativeFiltersInfo: NativeFiltersState = {
       id: 'DefaultsID',
       name: 'test',
       filterType: 'filter_select',
+      chartsInScope: [],
       targets: [
         {
           datasetId: 0,

diff --git a/superset-frontend/src/dashboard/util/activeAllDashboardFilters.ts b/superset-frontend/src/dashboard/util/activeAllDashboardFilters.ts
@@ -54,9 +54,13 @@ export const getAllActiveFilters = ({
       chartConfiguration?.[filterId]?.crossFilters?.chartsInScope ??
       allSliceIds ??
       [];
+    const filterType = nativeFilters?.[filterId]?.filterType;
+    const targets = nativeFilters?.[filterId]?.targets ?? scope;
     // Iterate over all roots to find all affected charts
     activeFilters[filterId] = {
       scope,
+      filterType,
+      targets,
       values: extraFormData,
     };
   });

diff --git a/superset-frontend/src/dashboard/util/crossFilters.test.ts b/superset-frontend/src/dashboard/util/crossFilters.test.ts
@@ -294,14 +294,14 @@ test('Recalculate charts in global filter scope when charts change', () => {
         id: 2,
         crossFilters: {
           scope: 'global',
-          chartsInScope: [1],
+          chartsInScope: [1, 3],
         },
       },
       '3': {
         id: 3,
         crossFilters: {
           scope: 'global',
-          chartsInScope: [],
+          chartsInScope: [1, 2],
         },
       },
     },

diff --git a/superset-frontend/src/dashboard/util/crossFilters.ts b/superset-frontend/src/dashboard/util/crossFilters.ts
@@ -52,20 +52,6 @@ export const getCrossFiltersConfiguration = (
     return undefined;
   }
 
-  const chartsByDataSource: Record<string, Set<number>> = Object.values(
-    charts,
-  ).reduce((acc: Record<string, Set<number>>, chart) => {
-    if (!chart.form_data) {
-      return acc;
-    }
-    const { datasource } = chart.form_data;
-    if (!acc[datasource]) {
-      acc[datasource] = new Set();
-    }
-    acc[datasource].add(chart.id);
-    return acc;
-  }, {});
-
   const globalChartConfiguration = metadata.global_chart_configuration?.scope
     ? {
         scope: metadata.global_chart_configuration.scope,
@@ -111,13 +97,10 @@ export const getCrossFiltersConfiguration = (
           },
         };
       }
-      const chartDataSource = charts[chartId].form_data.datasource;
       chartConfiguration[chartId].crossFilters.chartsInScope =
         isCrossFilterScopeGlobal(chartConfiguration[chartId].crossFilters.scope)
           ? globalChartConfiguration.chartsInScope.filter(
-              id =>
-                id !== Number(chartId) &&
-                chartsByDataSource[chartDataSource]?.has(id),
+              id => id !== Number(chartId),
             )
           : getChartIdsInFilterScope(
               chartConfiguration[chartId].crossFilters.scope,