From fab0ce69e1d44a8cf5e1e6cc863cf075a83216a1 Mon Sep 17 00:00:00 2001 From: Simcha Shats Date: Sun, 24 Jan 2021 16:32:56 +0200 Subject: [PATCH 1/8] feat: scoping native filters in dashboard --- .../src/dashboard/components/Dashboard.jsx | 6 +- .../src/dashboard/containers/Chart.jsx | 2 + .../src/dashboard/containers/Dashboard.jsx | 10 +- .../src/dashboard/reducers/nativeFilters.ts | 5 +- superset-frontend/src/dashboard/types.ts | 10 ++ .../util/activeDashboardNativeFilters.ts | 99 +++++++++++++++++++ .../charts/getFormDataWithExtraFilters.ts | 20 +++- 7 files changed, 140 insertions(+), 12 deletions(-) create mode 100644 superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts diff --git a/superset-frontend/src/dashboard/components/Dashboard.jsx b/superset-frontend/src/dashboard/components/Dashboard.jsx index 9606fc8ae50bd..0400e26d7c557 100644 --- a/superset-frontend/src/dashboard/components/Dashboard.jsx +++ b/superset-frontend/src/dashboard/components/Dashboard.jsx @@ -150,9 +150,9 @@ class Dashboard extends React.PureComponent { 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)); - } + // if (!areObjectsEqual(prevProps.nativeFilters, nativeFilters)) { + // this.refreshCharts(this.getAllCharts().map(chart => chart.id)); + // } 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 index abd784a5c70f4..9c48a89488441 100644 --- 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 index f644dc67aef46..07fc924011ff1 100644 --- 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 index b4c76a135e304..1e49b2834a71b 100644 --- a/superset-frontend/src/dashboard/reducers/nativeFilters.ts +++ b/superset-frontend/src/dashboard/reducers/nativeFilters.ts @@ -19,7 +19,6 @@ import { SET_EXTRA_FORM_DATA, AnyFilterAction, - SET_FILTER_CONFIG_COMPLETE, } from 'src/dashboard/actions/nativeFilters'; import { FilterConfiguration, @@ -66,9 +65,7 @@ export default function nativeFilterReducer( }, }; - case SET_FILTER_CONFIG_COMPLETE: - return getInitialState(action.filterConfig); - + // TODO handle SET_FILTER_CONFIG_COMPLETE action if needed // TODO handle SET_FILTER_CONFIG_FAIL action default: return state; diff --git a/superset-frontend/src/dashboard/types.ts b/superset-frontend/src/dashboard/types.ts index b66332a0b39e2..86fa4bf00f5de 100644 --- 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 { + 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; +}; diff --git a/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts b/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts new file mode 100644 index 0000000000000..e35a9867f71c2 --- /dev/null +++ b/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts @@ -0,0 +1,99 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { CHART_TYPE } from './componentTypes'; +import { NativeFiltersState, Scope } from '../components/nativeFilters/types'; +import { ActiveFilters, LayoutItem } from '../types'; + +// Looking for affected chart scopes and values +export const findAffectedCharts = ({ + child, + layout, + scope, + activeNativeFilters, + filterId, + extraFormData, +}: { + child: string; + layout: LayoutItem; + scope: Scope; + activeNativeFilters: ActiveFilters; + filterId: string; + extraFormData: any; +}) => { + const chartId = layout[child]?.meta?.chartId; + if (layout[child].type === CHART_TYPE) { + // Ignore excluded charts + if (scope.excluded.includes(chartId)) { + return; + } + if (!activeNativeFilters[filterId]) { + // Small mutation but simplify logic + // eslint-disable-next-line no-param-reassign + activeNativeFilters[filterId] = { + scope: [], + values: [], + }; + } + // Add not excluded chart scopes(to know what charts refresh) and values(refresh only if its value changed) + activeNativeFilters[filterId].scope.push(chartId); + activeNativeFilters[filterId].values.push(extraFormData); + return; + } + // If child is not chart, recursive iterate over its children + layout[child].children.forEach((child: string) => + findAffectedCharts({ + child, + layout, + scope, + activeNativeFilters, + filterId, + extraFormData, + }), + ); +}; + +export const getActiveNativeFilters = ({ + nativeFilters, + layout, +}: { + nativeFilters: NativeFiltersState; + layout: LayoutItem; +}): ActiveFilters => { + const activeNativeFilters = {}; + Object.values(nativeFilters.filtersState).forEach( + ({ id: filterId, extraFormData }) => { + const { scope } = nativeFilters.filters[filterId]; + // Iterate over all roots to find all affected charts + scope.rootPath.forEach(layoutItemId => { + layout[layoutItemId].children.forEach((child: string) => { + // Need exclude from affected charts, charts that located in scope `excluded` + findAffectedCharts({ + child, + layout, + scope, + activeNativeFilters, + filterId, + extraFormData, + }); + }); + }); + }, + ); + return activeNativeFilters; +}; diff --git a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts index 446e924a3f4bb..d868e775d5081 100644 --- a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts +++ b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts @@ -21,10 +21,11 @@ import { CategoricalColorNamespace, DataRecordFilters, } from '@superset-ui/core'; -import { ChartQueryPayload } from 'src/dashboard/types'; +import { ChartQueryPayload, LayoutItem } from 'src/dashboard/types'; import { NativeFiltersState } from 'src/dashboard/components/nativeFilters/types'; import { getExtraFormData } from 'src/dashboard/components/nativeFilters/utils'; import getEffectiveExtraFilters from './getEffectiveExtraFilters'; +import { getActiveNativeFilters } from '../activeDashboardNativeFilters'; // We cache formData objects so that our connected container components don't always trigger // render cascades. we cannot leverage the reselect library because our cache size is >1 @@ -34,6 +35,7 @@ const cachedFormdataByChart = {}; interface GetFormDataWithExtraFiltersArguments { chart: ChartQueryPayload; filters: DataRecordFilters; + layout: LayoutItem; colorScheme?: string; colorNamespace?: string; sliceId: number; @@ -49,6 +51,7 @@ export default function getFormDataWithExtraFilters({ colorScheme, colorNamespace, sliceId, + layout, nativeFilters, }: GetFormDataWithExtraFiltersArguments) { // Propagate color mapping to chart @@ -68,12 +71,23 @@ export default function getFormDataWithExtraFilters({ return cachedFormdataByChart[sliceId]; } + let extraData = {}; + const activeNativeFilters = getActiveNativeFilters({ nativeFilters, layout }); + const isAffectedChart = Object.values(activeNativeFilters).some(({ scope }) => + scope.includes(chart.id), + ); + if (isAffectedChart) { + extraData = { + extra_filters: getEffectiveExtraFilters(filters), + extra_form_data: getExtraFormData(nativeFilters), + }; + } + const formData = { ...chart.formData, ...(colorScheme && { color_scheme: colorScheme }), label_colors: labelColors, - extra_filters: getEffectiveExtraFilters(filters), - extra_form_data: getExtraFormData(nativeFilters), + ...extraData, }; cachedFiltersByChart[sliceId] = filters; cachedFormdataByChart[sliceId] = formData; From 961a633daf3025aadbbe4fe8a64578c0abdbba89 Mon Sep 17 00:00:00 2001 From: Simcha Shats Date: Sun, 24 Jan 2021 17:20:50 +0200 Subject: [PATCH 2/8] test: add tests / fix reducer --- .../spec/fixtures/mockNativeFilters.ts | 113 ++++++++++++++++++ .../dashboard/components/Dashboard_spec.jsx | 31 +++++ .../src/dashboard/components/Dashboard.jsx | 8 +- .../src/dashboard/reducers/nativeFilters.ts | 10 +- 4 files changed, 154 insertions(+), 8 deletions(-) create mode 100644 superset-frontend/spec/fixtures/mockNativeFilters.ts diff --git a/superset-frontend/spec/fixtures/mockNativeFilters.ts b/superset-frontend/spec/fixtures/mockNativeFilters.ts new file mode 100644 index 0000000000000..7504a2a20ee52 --- /dev/null +++ b/superset-frontend/spec/fixtures/mockNativeFilters.ts @@ -0,0 +1,113 @@ +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 nativeFiltersState = { + 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-p4LImrSgA', + extraFormData, + }, + }, +}; + +export const layoutForNativeFilter = { + 'CHART-ZHVS7YasaQ': { + children: [], + id: 'CHART-ZHVS7YasaQ', + meta: { + chartId: 230, + height: 50, + sliceName: 'Pie', + 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: 'Select filter', + 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: 'Bars 2', + 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: '[ untitled 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 index 87f6c5120eb3a..e6c0833099bd9 100644 --- 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, + layoutForNativeFilter, + nativeFiltersState, +} 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,30 @@ describe('Dashboard', () => { expect(wrapper.instance().appliedFilters).toBe(OVERRIDE_FILTERS); }); + it('should call refresh when native filters changed', () => { + const mockGetActiveNativeFilters = jest + .fn() + .mockImplementation(getActiveNativeFilters); + wrapper.setProps({ + activeFilters: { + ...OVERRIDE_FILTERS, + ...mockGetActiveNativeFilters({ + nativeFilters: nativeFiltersState, + layout: layoutForNativeFilter, + }), + }, + }); + 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/src/dashboard/components/Dashboard.jsx b/superset-frontend/src/dashboard/components/Dashboard.jsx index 0400e26d7c557..3f3a79284aceb 100644 --- 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/reducers/nativeFilters.ts b/superset-frontend/src/dashboard/reducers/nativeFilters.ts index 1e49b2834a71b..0d42513068416 100644 --- a/superset-frontend/src/dashboard/reducers/nativeFilters.ts +++ b/superset-frontend/src/dashboard/reducers/nativeFilters.ts @@ -19,6 +19,7 @@ import { SET_EXTRA_FORM_DATA, AnyFilterAction, + SET_FILTER_CONFIG_COMPLETE, } from 'src/dashboard/actions/nativeFilters'; import { FilterConfiguration, @@ -35,6 +36,7 @@ export function getInitialFilterState(id: string): FilterState { export function getInitialState( filterConfig: FilterConfiguration, + prevFiltersState: { [filterId: string]: FilterState }, ): NativeFiltersState { const filters = {}; const filtersState = {}; @@ -42,7 +44,9 @@ export function getInitialState( filterConfig.forEach(filter => { const { id } = filter; filters[id] = filter; - filtersState[id] = getInitialFilterState(id); + filtersState[id] = !prevFiltersState?.[id] + ? getInitialFilterState(id) + : prevFiltersState[id]; }); return state; } @@ -65,7 +69,9 @@ export default function nativeFilterReducer( }, }; - // TODO handle SET_FILTER_CONFIG_COMPLETE action if needed + case SET_FILTER_CONFIG_COMPLETE: + return getInitialState(action.filterConfig, filtersState); + // TODO handle SET_FILTER_CONFIG_FAIL action default: return state; From 329facbd48780b2b90df9b88c89332bb6363053f Mon Sep 17 00:00:00 2001 From: Simcha Shats Date: Sun, 24 Jan 2021 18:58:40 +0200 Subject: [PATCH 3/8] test: fix tests --- .../util/getFormDataWithExtraFilters_spec.ts | 35 +++++++++++++++---- .../util/activeDashboardNativeFilters.ts | 4 +-- .../charts/getFormDataWithExtraFilters.ts | 4 +-- 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/superset-frontend/spec/javascripts/dashboard/util/getFormDataWithExtraFilters_spec.ts b/superset-frontend/spec/javascripts/dashboard/util/getFormDataWithExtraFilters_spec.ts index d90bf8114e03b..5080f531f1940 100644 --- 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/util/activeDashboardNativeFilters.ts b/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts index e35a9867f71c2..769c616e29318 100644 --- a/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts +++ b/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts @@ -30,7 +30,7 @@ export const findAffectedCharts = ({ extraFormData, }: { child: string; - layout: LayoutItem; + layout: { [key: string]: LayoutItem }; scope: Scope; activeNativeFilters: ActiveFilters; filterId: string; @@ -73,7 +73,7 @@ export const getActiveNativeFilters = ({ layout, }: { nativeFilters: NativeFiltersState; - layout: LayoutItem; + layout: { [key: string]: LayoutItem }; }): ActiveFilters => { const activeNativeFilters = {}; Object.values(nativeFilters.filtersState).forEach( diff --git a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts index d868e775d5081..51ca3ec28c1cd 100644 --- a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts +++ b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts @@ -32,10 +32,10 @@ import { getActiveNativeFilters } from '../activeDashboardNativeFilters'; const cachedFiltersByChart = {}; const cachedFormdataByChart = {}; -interface GetFormDataWithExtraFiltersArguments { +export interface GetFormDataWithExtraFiltersArguments { chart: ChartQueryPayload; filters: DataRecordFilters; - layout: LayoutItem; + layout: { [key: string]: LayoutItem }; colorScheme?: string; colorNamespace?: string; sliceId: number; From b1e83e6f1debc76fa4314cc2f4d68468a868e1f0 Mon Sep 17 00:00:00 2001 From: Simcha Shats Date: Sun, 24 Jan 2021 20:50:29 +0200 Subject: [PATCH 4/8] chore: merge with master --- .../spec/fixtures/mockNativeFilters.ts | 6 +++--- .../dashboard/components/Dashboard_spec.jsx | 15 ++++++--------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/superset-frontend/spec/fixtures/mockNativeFilters.ts b/superset-frontend/spec/fixtures/mockNativeFilters.ts index 9c7e9b5feb08a..ed0c725204a71 100644 --- a/superset-frontend/spec/fixtures/mockNativeFilters.ts +++ b/superset-frontend/spec/fixtures/mockNativeFilters.ts @@ -103,7 +103,7 @@ export const extraFormData = { export const NATIVE_FILTER_ID = 'NATIVE_FILTER-p4LImrSgA'; -export const nativeFiltersState = { +export const singleNativeFiltersState = { filters: { [NATIVE_FILTER_ID]: { id: [NATIVE_FILTER_ID], @@ -121,13 +121,13 @@ export const nativeFiltersState = { }, filtersState: { [NATIVE_FILTER_ID]: { - id: 'NATIVE_FILTER-p4LImrSgA', + id: NATIVE_FILTER_ID, extraFormData, }, }, }; -export const layoutForNativeFilter = { +export const layoutForSingleNativeFilter = { 'CHART-ZHVS7YasaQ': { children: [], id: 'CHART-ZHVS7YasaQ', diff --git a/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx index e6c0833099bd9..1cce107a7256b 100644 --- a/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx +++ b/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx @@ -31,9 +31,9 @@ import datasources from 'spec/fixtures/mockDatasource'; import { extraFormData, NATIVE_FILTER_ID, - layoutForNativeFilter, - nativeFiltersState, -} from 'spec/fixtures/mockNativefilters'; + layoutForSingleNativeFilter, + singleNativeFiltersState, +} from 'spec/fixtures/mockNativeFilters'; import dashboardInfo from 'spec/fixtures/mockDashboardInfo'; import { dashboardLayout } from 'spec/fixtures/mockDashboardLayout'; import dashboardState from 'spec/fixtures/mockDashboardState'; @@ -149,15 +149,12 @@ describe('Dashboard', () => { }); it('should call refresh when native filters changed', () => { - const mockGetActiveNativeFilters = jest - .fn() - .mockImplementation(getActiveNativeFilters); wrapper.setProps({ activeFilters: { ...OVERRIDE_FILTERS, - ...mockGetActiveNativeFilters({ - nativeFilters: nativeFiltersState, - layout: layoutForNativeFilter, + ...getActiveNativeFilters({ + nativeFilters: singleNativeFiltersState, + layout: layoutForSingleNativeFilter, }), }, }); From a593ca804935c81ecc91cad3ce952af81f09762e Mon Sep 17 00:00:00 2001 From: Simcha Shats Date: Mon, 25 Jan 2021 08:18:15 +0200 Subject: [PATCH 5/8] fix: fix undefined cases --- .../src/dashboard/util/activeDashboardNativeFilters.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts b/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts index 769c616e29318..e1431642283c0 100644 --- a/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts +++ b/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts @@ -76,9 +76,15 @@ export const getActiveNativeFilters = ({ layout: { [key: string]: LayoutItem }; }): ActiveFilters => { const activeNativeFilters = {}; + if (!nativeFilters?.filtersState) { + return activeNativeFilters; + } Object.values(nativeFilters.filtersState).forEach( ({ id: filterId, extraFormData }) => { - const { scope } = nativeFilters.filters[filterId]; + const scope = nativeFilters?.filters?.[filterId]?.scope; + if (!scope) { + return; + } // Iterate over all roots to find all affected charts scope.rootPath.forEach(layoutItemId => { layout[layoutItemId].children.forEach((child: string) => { From 0cc68fa24b07f6e0bb2bc136a3900b600005012b Mon Sep 17 00:00:00 2001 From: Simcha Shats Date: Mon, 25 Jan 2021 10:02:22 +0200 Subject: [PATCH 6/8] fix: fix code according cypress --- .../src/dashboard/util/charts/getFormDataWithExtraFilters.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts index 51ca3ec28c1cd..6d7e56b493990 100644 --- a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts +++ b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts @@ -78,7 +78,6 @@ export default function getFormDataWithExtraFilters({ ); if (isAffectedChart) { extraData = { - extra_filters: getEffectiveExtraFilters(filters), extra_form_data: getExtraFormData(nativeFilters), }; } @@ -87,6 +86,7 @@ export default function getFormDataWithExtraFilters({ ...chart.formData, ...(colorScheme && { color_scheme: colorScheme }), label_colors: labelColors, + extra_filters: getEffectiveExtraFilters(filters), ...extraData, }; cachedFiltersByChart[sliceId] = filters; From ca6337422a66ae3d7cb48c9004e17c21a39ed214 Mon Sep 17 00:00:00 2001 From: Simcha Shats Date: Mon, 25 Jan 2021 11:00:13 +0200 Subject: [PATCH 7/8] refactor: fix mocks according CRs --- superset-frontend/spec/fixtures/mockNativeFilters.ts | 8 ++++---- .../javascripts/dashboard/components/Dashboard_spec.jsx | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/superset-frontend/spec/fixtures/mockNativeFilters.ts b/superset-frontend/spec/fixtures/mockNativeFilters.ts index ed0c725204a71..9aa6e1010d067 100644 --- a/superset-frontend/spec/fixtures/mockNativeFilters.ts +++ b/superset-frontend/spec/fixtures/mockNativeFilters.ts @@ -134,7 +134,7 @@ export const layoutForSingleNativeFilter = { meta: { chartId: 230, height: 50, - sliceName: 'Pie', + sliceName: 'Pie Chart', uuid: '05ef6145-3950-4f59-891f-160852613eca', width: 12, }, @@ -147,7 +147,7 @@ export const layoutForSingleNativeFilter = { meta: { chartId: 227, height: 50, - sliceName: 'Select filter', + sliceName: 'Another Chart', uuid: 'ddb78f6c-7876-47fc-ae98-70183b05ba90', width: 4, }, @@ -160,7 +160,7 @@ export const layoutForSingleNativeFilter = { meta: { chartId: 229, height: 47, - sliceName: 'Bars 2', + sliceName: 'Bar Chart', uuid: 'e1501e54-d632-4fdc-ae16-07cafee31093', width: 12, }, @@ -177,7 +177,7 @@ export const layoutForSingleNativeFilter = { HEADER_ID: { id: 'HEADER_ID', type: 'HEADER', - meta: { text: '[ untitled dashboard ]' }, + meta: { text: 'My Native Filter Dashboard' }, }, ROOT_ID: { children: ['GRID_ID'], id: 'ROOT_ID', type: 'ROOT' }, 'ROW-NweUz7oC0': { diff --git a/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx index 1cce107a7256b..972498eb7acc5 100644 --- a/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx +++ b/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx @@ -38,7 +38,7 @@ 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'; +import { getActiveNativeFilters } from 'src/dashboard/util/activeDashboardNativeFilters'; describe('Dashboard', () => { const props = { From c91cacc560d1042e5dc654524c548d9fa255a3e0 Mon Sep 17 00:00:00 2001 From: Simcha Shats Date: Mon, 25 Jan 2021 16:46:40 +0200 Subject: [PATCH 8/8] chore: re run pipeline --- superset-frontend/src/dashboard/reducers/nativeFilters.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/superset-frontend/src/dashboard/reducers/nativeFilters.ts b/superset-frontend/src/dashboard/reducers/nativeFilters.ts index 0d42513068416..b69ccd5dc95ad 100644 --- a/superset-frontend/src/dashboard/reducers/nativeFilters.ts +++ b/superset-frontend/src/dashboard/reducers/nativeFilters.ts @@ -44,9 +44,7 @@ export function getInitialState( filterConfig.forEach(filter => { const { id } = filter; filters[id] = filter; - filtersState[id] = !prevFiltersState?.[id] - ? getInitialFilterState(id) - : prevFiltersState[id]; + filtersState[id] = prevFiltersState?.[id] || getInitialFilterState(id); }); return state; }