feat(native-filters): apply scoping of native filters to dashboard (#…

…12716) * feat: scoping native filters in dashboard * test: add tests / fix reducer * test: fix tests * chore: merge with master * fix: fix undefined cases * fix: fix code according cypress * refactor: fix mocks according CRs * chore: re run pipeline
apache · Jan 26, 2021 · 4d04565 · 4d04565
1 parent 0c08bb8
commit 4d04565
Show file tree

Hide file tree

Showing 10 changed files with 318 additions and 19 deletions.
diff --git a/superset-frontend/spec/fixtures/mockNativeFilters.ts b/superset-frontend/spec/fixtures/mockNativeFilters.ts
@@ -88,3 +88,117 @@ export const nativeFilters: NativeFiltersState = {
     },
   },
 };
+
+export const extraFormData = {
+  append_form_data: {
+    filters: [
+      {
+        col: 'ethnic_minority',
+        op: 'IN',
+        val: 'No, not an ethnic minority',
+      },
+    ],
+  },
+};
+
+export const NATIVE_FILTER_ID = 'NATIVE_FILTER-p4LImrSgA';
+
+export const singleNativeFiltersState = {
+  filters: {
+    [NATIVE_FILTER_ID]: {
+      id: [NATIVE_FILTER_ID],
+      name: 'eth',
+      type: 'text',
+      targets: [{ datasetId: 13, column: { name: 'ethnic_minority' } }],
+      defaultValue: null,
+      cascadeParentIds: [],
+      scope: { rootPath: ['ROOT_ID'], excluded: [227, 229] },
+      inverseSelection: false,
+      isInstant: true,
+      allowsMultipleValues: false,
+      isRequired: false,
+    },
+  },
+  filtersState: {
+    [NATIVE_FILTER_ID]: {
+      id: NATIVE_FILTER_ID,
+      extraFormData,
+    },
+  },
+};
+
+export const layoutForSingleNativeFilter = {
+  'CHART-ZHVS7YasaQ': {
+    children: [],
+    id: 'CHART-ZHVS7YasaQ',
+    meta: {
+      chartId: 230,
+      height: 50,
+      sliceName: 'Pie Chart',
+      uuid: '05ef6145-3950-4f59-891f-160852613eca',
+      width: 12,
+    },
+    parents: ['ROOT_ID', 'GRID_ID', 'ROW-NweUz7oC0'],
+    type: 'CHART',
+  },
+  'CHART-gsGu8NIKQT': {
+    children: [],
+    id: 'CHART-gsGu8NIKQT',
+    meta: {
+      chartId: 227,
+      height: 50,
+      sliceName: 'Another Chart',
+      uuid: 'ddb78f6c-7876-47fc-ae98-70183b05ba90',
+      width: 4,
+    },
+    parents: ['ROOT_ID', 'GRID_ID', 'ROW-QkiTjeZGs'],
+    type: 'CHART',
+  },
+  'CHART-hgYjD8axJX': {
+    children: [],
+    id: 'CHART-hgYjD8axJX',
+    meta: {
+      chartId: 229,
+      height: 47,
+      sliceName: 'Bar Chart',
+      uuid: 'e1501e54-d632-4fdc-ae16-07cafee31093',
+      width: 12,
+    },
+    parents: ['ROOT_ID', 'GRID_ID', 'ROW-mcdVZi0rL'],
+    type: 'CHART',
+  },
+  DASHBOARD_VERSION_KEY: 'v2',
+  GRID_ID: {
+    children: ['ROW-mcdVZi0rL', 'ROW-NweUz7oC0', 'ROW-QkiTjeZGs'],
+    id: 'GRID_ID',
+    parents: ['ROOT_ID'],
+    type: 'GRID',
+  },
+  HEADER_ID: {
+    id: 'HEADER_ID',
+    type: 'HEADER',
+    meta: { text: 'My Native Filter Dashboard' },
+  },
+  ROOT_ID: { children: ['GRID_ID'], id: 'ROOT_ID', type: 'ROOT' },
+  'ROW-NweUz7oC0': {
+    children: ['CHART-ZHVS7YasaQ'],
+    id: 'ROW-NweUz7oC0',
+    meta: { background: 'BACKGROUND_TRANSPARENT' },
+    parents: ['ROOT_ID', 'GRID_ID'],
+    type: 'ROW',
+  },
+  'ROW-QkiTjeZGs': {
+    children: ['CHART-gsGu8NIKQT'],
+    id: 'ROW-QkiTjeZGs',
+    meta: { background: 'BACKGROUND_TRANSPARENT' },
+    parents: ['ROOT_ID', 'GRID_ID'],
+    type: 'ROW',
+  },
+  'ROW-mcdVZi0rL': {
+    children: ['CHART-hgYjD8axJX'],
+    id: 'ROW-mcdVZi0rL',
+    meta: { '0': 'ROOT_ID', background: 'BACKGROUND_TRANSPARENT' },
+    parents: ['ROOT_ID', 'GRID_ID'],
+    type: 'ROW',
+  },
+};
diff --git a/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx
@@ -28,10 +28,17 @@ import newComponentFactory from 'src/dashboard/util/newComponentFactory';
 // mock data
 import chartQueries from 'spec/fixtures/mockChartQueries';
 import datasources from 'spec/fixtures/mockDatasource';
+import {
+  extraFormData,
+  NATIVE_FILTER_ID,
+  layoutForSingleNativeFilter,
+  singleNativeFiltersState,
+} from 'spec/fixtures/mockNativeFilters';
 import dashboardInfo from 'spec/fixtures/mockDashboardInfo';
 import { dashboardLayout } from 'spec/fixtures/mockDashboardLayout';
 import dashboardState from 'spec/fixtures/mockDashboardState';
 import { sliceEntitiesForChart as sliceEntities } from 'spec/fixtures/mockSliceEntities';
+import { getActiveNativeFilters } from 'src/dashboard/util/activeDashboardNativeFilters';
 
 describe('Dashboard', () => {
   const props = {
@@ -141,6 +148,27 @@ describe('Dashboard', () => {
       expect(wrapper.instance().appliedFilters).toBe(OVERRIDE_FILTERS);
     });
 
+    it('should call refresh when native filters changed', () => {
+      wrapper.setProps({
+        activeFilters: {
+          ...OVERRIDE_FILTERS,
+          ...getActiveNativeFilters({
+            nativeFilters: singleNativeFiltersState,
+            layout: layoutForSingleNativeFilter,
+          }),
+        },
+      });
+      wrapper.instance().componentDidUpdate(prevProps);
+      expect(refreshSpy.callCount).toBe(1);
+      expect(wrapper.instance().appliedFilters).toEqual({
+        ...OVERRIDE_FILTERS,
+        [NATIVE_FILTER_ID]: {
+          scope: [230],
+          values: [extraFormData],
+        },
+      });
+    });
+
     it('should call refresh if a filter is added', () => {
       const newFilter = {
         gender: { values: ['boy', 'girl'], scope: [1] },

diff --git a/superset-frontend/spec/javascripts/dashboard/util/getFormDataWithExtraFilters_spec.ts b/superset-frontend/spec/javascripts/dashboard/util/getFormDataWithExtraFilters_spec.ts
@@ -16,11 +16,18 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import getFormDataWithExtraFilters from 'src/dashboard/util/charts/getFormDataWithExtraFilters';
+import getFormDataWithExtraFilters, {
+  GetFormDataWithExtraFiltersArguments,
+} from 'src/dashboard/util/charts/getFormDataWithExtraFilters';
+import { DASHBOARD_ROOT_ID } from 'src/dashboard/util/constants';
+import { Filter } from 'src/dashboard/components/nativeFilters/types';
+import { LayoutItem } from 'src/dashboard/types';
+import { dashboardLayout } from '../../../fixtures/mockDashboardLayout';
+import { sliceId as chartId } from '../../../fixtures/mockChartQueries';
 
 describe('getFormDataWithExtraFilters', () => {
-  const chartId = 8675309;
-  const mockArgs = {
+  const filterId = 'native-filter-1';
+  const mockArgs: GetFormDataWithExtraFiltersArguments = {
     chart: {
       id: chartId,
       formData: {
@@ -37,11 +44,27 @@ describe('getFormDataWithExtraFilters', () => {
       region: ['Spain'],
       color: ['pink', 'purple'],
     },
+    sliceId: chartId,
     nativeFilters: {
-      filters: {},
-      filtersState: {},
+      filters: {
+        [filterId]: ({
+          id: filterId,
+          scope: {
+            rootPath: [DASHBOARD_ROOT_ID],
+            excluded: [],
+          },
+        } as unknown) as Filter,
+      },
+      filtersState: {
+        [filterId]: {
+          id: filterId,
+          extraFormData: {},
+        },
+      },
+    },
+    layout: (dashboardLayout.present as unknown) as {
+      [key: string]: LayoutItem;
     },
-    sliceId: chartId,
   };
 
   it('should include filters from the passed filters', () => {

diff --git a/superset-frontend/src/dashboard/components/Dashboard.jsx b/superset-frontend/src/dashboard/components/Dashboard.jsx
@@ -144,15 +144,11 @@ class Dashboard extends React.PureComponent {
     }
   }
 
-  componentDidUpdate(prevProps) {
+  componentDidUpdate() {
     const { hasUnsavedChanges, editMode } = this.props.dashboardState;
 
     const { appliedFilters } = this;
-    const { activeFilters, nativeFilters } = this.props;
-    // do not apply filter when dashboard in edit mode
-    if (!areObjectsEqual(prevProps.nativeFilters, nativeFilters)) {
-      this.refreshCharts(this.getAllCharts().map(chart => chart.id));
-    }
+    const { activeFilters } = this.props;
     if (!editMode && !areObjectsEqual(appliedFilters, activeFilters)) {
       this.applyFilters();
     }

diff --git a/superset-frontend/src/dashboard/containers/Chart.jsx b/superset-frontend/src/dashboard/containers/Chart.jsx
@@ -43,6 +43,7 @@ function mapStateToProps(
     charts: chartQueries,
     dashboardInfo,
     dashboardState,
+    dashboardLayout,
     datasources,
     sliceEntities,
     nativeFilters,
@@ -57,6 +58,7 @@ function mapStateToProps(
 
   // note: this method caches filters if possible to prevent render cascades
   const formData = getFormDataWithExtraFilters({
+    layout: dashboardLayout.present,
     chart,
     filters: getAppliedFilterValues(id),
     colorScheme,

diff --git a/superset-frontend/src/dashboard/containers/Dashboard.jsx b/superset-frontend/src/dashboard/containers/Dashboard.jsx
@@ -28,6 +28,7 @@ import {
 import { triggerQuery } from '../../chart/chartAction';
 import { logEvent } from '../../logger/actions';
 import { getActiveFilters } from '../util/activeDashboardFilters';
+import { getActiveNativeFilters } from '../util/activeDashboardNativeFilters';
 
 function mapStateToProps(state) {
   const {
@@ -54,10 +55,15 @@ function mapStateToProps(state) {
     // When dashboard is first loaded into browser,
     // its value is from preselect_filters that dashboard owner saved in dashboard's meta data
     // When user start interacting with dashboard, it will be user picked values from all filter_box
-    activeFilters: getActiveFilters(),
+    activeFilters: {
+      ...getActiveFilters(),
+      ...getActiveNativeFilters({
+        nativeFilters,
+        layout: dashboardLayout.present,
+      }),
+    },
     slices: sliceEntities.slices,
     layout: dashboardLayout.present,
-    nativeFilters,
     impressionId,
   };
 }

diff --git a/superset-frontend/src/dashboard/reducers/nativeFilters.ts b/superset-frontend/src/dashboard/reducers/nativeFilters.ts
@@ -36,14 +36,15 @@ export function getInitialFilterState(id: string): FilterState {
 
 export function getInitialState(
   filterConfig: FilterConfiguration,
+  prevFiltersState: { [filterId: string]: FilterState },
 ): NativeFiltersState {
   const filters = {};
   const filtersState = {};
   const state = { filters, filtersState };
   filterConfig.forEach(filter => {
     const { id } = filter;
     filters[id] = filter;
-    filtersState[id] = getInitialFilterState(id);
+    filtersState[id] = prevFiltersState?.[id] || getInitialFilterState(id);
   });
   return state;
 }
@@ -67,7 +68,7 @@ export default function nativeFilterReducer(
       };
 
     case SET_FILTER_CONFIG_COMPLETE:
-      return getInitialState(action.filterConfig);
+      return getInitialState(action.filterConfig, filtersState);
 
     // TODO handle SET_FILTER_CONFIG_FAIL action
     default:

diff --git a/superset-frontend/src/dashboard/types.ts b/superset-frontend/src/dashboard/types.ts
@@ -25,6 +25,7 @@ export type ChartReducerInitialState = typeof chart;
 // chart query built from initialState
 // Ref: https://github.com/apache/superset/blob/dcac860f3e5528ecbc39e58f045c7388adb5c3d0/superset-frontend/src/dashboard/reducers/getInitialState.js#L120
 export interface ChartQueryPayload extends Partial<ChartReducerInitialState> {
+  id: number;
   formData: ChartProps['formData'];
   form_data?: ChartProps['rawFormData'];
   [key: string]: unknown;
@@ -69,3 +70,12 @@ export type LayoutItem = {
     width: number;
   };
 };
+
+type ActiveFilter = {
+  scope: number[];
+  values: any[];
+};
+
+export type ActiveFilters = {
+  [key: string]: ActiveFilter;
+};