This is an automated email from the ASF dual-hosted git repository.
villebro pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 4d04565 feat(native-filters): apply scoping of native filters to
dashboard (#12716)
4d04565 is described below
commit 4d04565c9af477d10b01a1d9156a63af38f381e0
Author: simchaNielsen <[email protected]>
AuthorDate: Tue Jan 26 09:29:49 2021 +0200
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
---
.../spec/fixtures/mockNativeFilters.ts | 114 +++++++++++++++++++++
.../dashboard/components/Dashboard_spec.jsx | 28 +++++
.../util/getFormDataWithExtraFilters_spec.ts | 35 +++++--
.../src/dashboard/components/Dashboard.jsx | 8 +-
.../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 ++
.../dashboard/util/activeDashboardNativeFilters.ts | 105 +++++++++++++++++++
.../util/charts/getFormDataWithExtraFilters.ts | 20 +++-
10 files changed, 318 insertions(+), 19 deletions(-)
diff --git a/superset-frontend/spec/fixtures/mockNativeFilters.ts
b/superset-frontend/spec/fixtures/mockNativeFilters.ts
index 9d14a07..9aa6e10 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 87f6c51..972498e 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 d90bf81..5080f53 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 9606fc8..3f3a792 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 abd784a..9c48a89 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 f644dc6..07fc924 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 b4c76a1..b69ccd5 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 b66332a..86fa4bf 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<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;
+};
diff --git
a/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts
b/superset-frontend/src/dashboard/util/activeDashboardNativeFilters.ts
new file mode 100644
index 0000000..e143164
--- /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 446e924..6d7e56b 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;
