diff --git a/superset-frontend/spec/fixtures/mockNativeFilters.ts b/superset-frontend/spec/fixtures/mockNativeFilters.ts index 9d14a074d32b..9aa6e1010d06 100644 --- 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 index 87f6c5120eb3..972498eb7acc 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, + 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 index d90bf8114e03..5080f531f194 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/components/Dashboard.jsx b/superset-frontend/src/dashboard/components/Dashboard.jsx index 9606fc8ae50b..3f3a79284ace 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/containers/Chart.jsx b/superset-frontend/src/dashboard/containers/Chart.jsx index abd784a5c70f..9c48a8948844 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 f644dc67aef4..07fc924011ff 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 b4c76a135e30..b69ccd5dc95a 100644 --- a/superset-frontend/src/dashboard/reducers/nativeFilters.ts +++ b/superset-frontend/src/dashboard/reducers/nativeFilters.ts @@ -36,6 +36,7 @@ export function getInitialFilterState(id: string): FilterState { export function getInitialState( filterConfig: FilterConfiguration, + prevFiltersState: { [filterId: string]: FilterState }, ): NativeFiltersState { const filters = {}; const filtersState = {}; @@ -43,7 +44,7 @@ export function getInitialState( 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 index b66332a0b39e..86fa4bf00f5d 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 000000000000..e1431642283c --- /dev/null +++ b/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts @@ -0,0 +1,105 @@ +/** + * 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: { [key: string]: 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: { [key: string]: LayoutItem }; +}): ActiveFilters => { + const activeNativeFilters = {}; + if (!nativeFilters?.filtersState) { + return activeNativeFilters; + } + Object.values(nativeFilters.filtersState).forEach( + ({ id: filterId, extraFormData }) => { + 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) => { + // 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 446e924a3f4b..6d7e56b49399 100644 --- a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts +++ b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts @@ -21,19 +21,21 @@ 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 const cachedFiltersByChart = {}; const cachedFormdataByChart = {}; -interface GetFormDataWithExtraFiltersArguments { +export interface GetFormDataWithExtraFiltersArguments { chart: ChartQueryPayload; filters: DataRecordFilters; + layout: { [key: string]: 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_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;