From 362948324c7718e74c0a9655332249c0e1328703 Mon Sep 17 00:00:00 2001 From: Geido <60598000+geido@users.noreply.github.com> Date: Wed, 16 Oct 2024 02:13:05 +0300 Subject: [PATCH] fix(Filters): Apply native & cross filters on common columns (#30438) --- docs/static/resources/openapi.json | 6 + .../superset-ui-chart-controls/src/types.ts | 1 + .../src/query/types/Dashboard.ts | 36 +++ .../test/query/types/Dashboard.test.ts | 28 ++ .../spec/fixtures/mockDashboardInfo.js | 1 + .../spec/fixtures/mockNativeFilters.ts | 1 + .../src/dashboard/actions/dashboardState.js | 13 +- .../src/dashboard/components/Dashboard.jsx | 31 ++- .../dashboard/components/Dashboard.test.jsx | 29 ++ .../OverwriteConfirmModal.test.tsx | 5 + .../dashboard/fixtures/mockNativeFilters.ts | 1 + .../util/activeAllDashboardFilters.ts | 4 + .../src/dashboard/util/crossFilters.test.ts | 4 +- .../src/dashboard/util/crossFilters.ts | 19 +- .../dashboard/util/getRelatedCharts.test.ts | 254 ++++++++++++++++++ .../src/dashboard/util/getRelatedCharts.ts | 200 ++++++++++++++ .../useFilterFocusHighlightStyles.test.tsx | 22 ++ .../util/useFilterFocusHighlightStyles.ts | 22 +- superset-frontend/src/types/Chart.ts | 2 + superset/connectors/sqla/models.py | 14 +- superset/dashboards/schemas.py | 1 + 21 files changed, 656 insertions(+), 38 deletions(-) create mode 100644 superset-frontend/src/dashboard/util/getRelatedCharts.test.ts create mode 100644 superset-frontend/src/dashboard/util/getRelatedCharts.ts diff --git a/docs/static/resources/openapi.json b/docs/static/resources/openapi.json index 43781ac9658b1..c2cadfd1f5767 100644 --- 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 index d4170be17c623..86e7b1c04e518 100644 --- 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 index cb299c3ac96c4..ac4b19cae55ae 100644 --- 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[]; + } | null; +}; + +export type AppliedCrossFilterType = { + filterType: undefined; + targets: number[]; + scope: number[]; +} & AppliedFilter; + +export type AppliedNativeFilterType = { + filterType: 'filter_select'; + scope: number[]; + targets: Partial[]; +} & AppliedFilter; + export type FilterWithDataMask = Filter & { dataMask: DataMaskWithId }; export type Divider = Partial> & { @@ -89,6 +107,24 @@ export type Divider = Partial> & { 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 index 79d7980940867..c1c714395779a 100644 --- 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 index 2f747fd07b557..a046a554d0965 100644 --- 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 index 070f48ab06bd0..b83cdcc8dccdd 100644 --- 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 index 9b0c1818212a9..945dda32a319f 100644 --- 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 index 8cc0d8103026e..1953889c5d418 100644 --- 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 index b33fe9f6388c9..cd76787a21209 100644 --- 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 index 01e9e825bf222..5d93fd6900b04 100644 --- 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 index d1d37cbf6aecb..32f54cbe269db 100644 --- 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 index f9e98e85365cb..3bde1d2e6f856 100644 --- 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 index e811856a4a39c..b88579cc843a0 100644 --- 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 index 880b826870a1a..903a1f74fbe55 100644 --- 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> = Object.values( - charts, - ).reduce((acc: Record>, 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, diff --git a/superset-frontend/src/dashboard/util/getRelatedCharts.test.ts b/superset-frontend/src/dashboard/util/getRelatedCharts.test.ts new file mode 100644 index 0000000000000..94762e8f780dd --- /dev/null +++ b/superset-frontend/src/dashboard/util/getRelatedCharts.test.ts @@ -0,0 +1,254 @@ +/** + * 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 { + AppliedCrossFilterType, + DatasourceType, + Filter, + NativeFilterType, +} from '@superset-ui/core'; +import { DatasourcesState } from '../types'; +import { getRelatedCharts } from './getRelatedCharts'; + +const slices = { + '1': { datasource: 'ds1', slice_id: 1 }, + '2': { datasource: 'ds2', slice_id: 2 }, + '3': { datasource: 'ds1', slice_id: 3 }, +} as any; + +const datasources: DatasourcesState = { + ds1: { + uid: 'ds1', + datasource_name: 'ds1', + table_name: 'table1', + description: '', + id: 100, + columns: [{ column_name: 'column1' }, { column_name: 'column2' }], + column_names: ['column1', 'column2'], + column_types: [], + type: DatasourceType.Table, + metrics: [], + column_formats: {}, + currency_formats: {}, + verbose_map: {}, + main_dttm_col: '', + filter_select_enabled: true, + }, + ds2: { + uid: 'ds2', + datasource_name: 'ds2', + table_name: 'table2', + description: '', + id: 200, + columns: [{ column_name: 'column3' }, { column_name: 'column4' }], + column_names: ['column3', 'column4'], + column_types: [], + type: DatasourceType.Table, + metrics: [], + column_formats: {}, + currency_formats: {}, + verbose_map: {}, + main_dttm_col: '', + filter_select_enabled: true, + }, +}; + +test('Return chart ids matching the dataset id with native filter', () => { + const filters = { + filterKey1: { + filterType: 'filter_select', + chartsInScope: [1, 2], + scope: { + excluded: [], + rootPath: [], + }, + targets: [ + { + column: { name: 'column1' }, + datasetId: 100, + }, + ], + type: NativeFilterType.NativeFilter, + } as unknown as Filter, + }; + + const result = getRelatedCharts(filters, null, slices, datasources); + expect(result).toEqual({ + filterKey1: [1], + }); +}); + +test('Return chart ids matching the dataset id with cross filter', () => { + const filters = { + '3': { + filterType: undefined, + scope: [1, 2], + targets: [], + values: null, + } as AppliedCrossFilterType, + }; + + const result = getRelatedCharts(filters, null, slices, datasources); + expect(result).toEqual({ + '3': [1], + }); +}); + +test('Return chart ids matching the column name with native filter', () => { + const filters = { + filterKey1: { + filterType: 'filter_select', + chartsInScope: [1, 2], + scope: { + excluded: [], + rootPath: [], + }, + targets: [ + { + column: { name: 'column3' }, + datasetId: 999, + }, + ], + type: NativeFilterType.NativeFilter, + } as unknown as Filter, + }; + + const result = getRelatedCharts(filters, null, slices, datasources); + expect(result).toEqual({ + filterKey1: [2], + }); +}); + +test('Return chart ids matching the column name with cross filter', () => { + const filters = { + '1': { + filterType: undefined, + scope: [1, 2], + targets: [], + values: { + filters: [{ col: 'column3' }], + }, + } as AppliedCrossFilterType, + }; + + const result = getRelatedCharts(filters, null, slices, datasources); + expect(result).toEqual({ + '1': [2], + }); +}); + +test('Return chart ids when column display name matches with native filter', () => { + const filters = { + filterKey1: { + filterType: 'filter_select', + chartsInScope: [1, 2], + scope: { + excluded: [], + rootPath: [], + }, + targets: [ + { + column: { name: 'column4', displayName: 'column4' }, + datasetId: 999, + }, + ], + type: NativeFilterType.NativeFilter, + } as unknown as Filter, + }; + + const result = getRelatedCharts(filters, null, slices, datasources); + expect(result).toEqual({ + filterKey1: [2], + }); +}); + +test('Return chart ids when column display name matches with cross filter', () => { + const filters = { + '1': { + filterType: undefined, + scope: [1, 2], + targets: [], + values: { + filters: [{ col: 'column4' }], + }, + } as AppliedCrossFilterType, + }; + + const result = getRelatedCharts(filters, null, slices, datasources); + expect(result).toEqual({ + '1': [2], + }); +}); + +test('Return scope when filterType is not filter_select', () => { + const filters = { + filterKey1: { + filterType: 'filter_time', + chartsInScope: [3, 4], + } as Filter, + }; + + const result = getRelatedCharts(filters, null, slices, datasources); + expect(result).toEqual({ + filterKey1: [3, 4], + }); +}); + +test('Return an empty array if no matching charts found with native filter', () => { + const filters = { + filterKey1: { + filterType: 'filter_select', + chartsInScope: [1, 2], + scope: { + excluded: [], + rootPath: [], + }, + targets: [ + { + column: { name: 'nonexistent_column' }, + datasetId: 300, + }, + ], + type: NativeFilterType.NativeFilter, + } as unknown as Filter, + }; + + const result = getRelatedCharts(filters, null, slices, datasources); + expect(result).toEqual({ + filterKey1: [], + }); +}); + +test('Return an empty array if no matching charts found with cross filter', () => { + const filters = { + '1': { + filterType: undefined, + scope: [1, 2], + targets: [], + values: { + filters: [{ col: 'nonexistent_column' }], + }, + } as AppliedCrossFilterType, + }; + + const result = getRelatedCharts(filters, null, slices, datasources); + expect(result).toEqual({ + '1': [], + }); +}); diff --git a/superset-frontend/src/dashboard/util/getRelatedCharts.ts b/superset-frontend/src/dashboard/util/getRelatedCharts.ts new file mode 100644 index 0000000000000..aba56ba3b6c19 --- /dev/null +++ b/superset-frontend/src/dashboard/util/getRelatedCharts.ts @@ -0,0 +1,200 @@ +/** + * 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 { + AppliedCrossFilterType, + AppliedNativeFilterType, + ensureIsArray, + Filter, + isAppliedCrossFilterType, + isAppliedNativeFilterType, + isNativeFilter, +} from '@superset-ui/core'; +import { Slice } from 'src/types/Chart'; +import { DatasourcesState } from '../types'; + +function checkForExpression(formData?: Record) { + const groupby = ensureIsArray(formData?.groupby) ?? []; + const allColumns = ensureIsArray(formData?.all_columns) ?? []; + const checkColumns = groupby.concat(allColumns); + return checkColumns.some( + (col: string | Record) => + typeof col !== 'string' && col.expressionType !== undefined, + ); +} + +function getRelatedChartsForSelectFilter( + nativeFilter: AppliedNativeFilterType | Filter, + slices: Record, + chartsInScope: number[], + datasources: DatasourcesState, +) { + return Object.values(slices) + .filter(slice => { + const { datasource, slice_id } = slice; + // not in scope, ignore + if (!chartsInScope.includes(slice_id)) return false; + + const chartDatasource = datasource + ? datasources[datasource] + : Object.values(datasources).find(ds => ds.id === slice.datasource_id); + + const { column, datasetId } = nativeFilter.targets?.[0] ?? {}; + const datasourceColumnNames = chartDatasource?.column_names ?? []; + + // same datasource, always apply + if (chartDatasource?.id === datasetId) return true; + + // let backend deal with adhoc columns and jinja + const hasSqlExpression = checkForExpression(slice.form_data); + if (hasSqlExpression) { + return true; + } + + return datasourceColumnNames.some( + col => col === column?.name || col === column?.displayName, + ); + }) + .map(slice => slice.slice_id); +} +function getRelatedChartsForCrossFilter( + filterKey: string, + crossFilter: AppliedCrossFilterType, + slices: Record, + scope: number[], + datasources: DatasourcesState, +): number[] { + const sourceSlice = slices[filterKey]; + const filters = crossFilter?.values?.filters ?? []; + + if (!sourceSlice) return []; + + const sourceDatasource = sourceSlice.datasource + ? datasources[sourceSlice.datasource] + : Object.values(datasources).find( + ds => ds.id === sourceSlice.datasource_id, + ); + + return Object.values(slices) + .filter(slice => { + // cross-filter emitter + if (slice.slice_id === Number(filterKey)) return false; + // not in scope, ignore + if (!scope.includes(slice.slice_id)) return false; + + const targetDatasource = slice.datasource + ? datasources[slice.datasource] + : Object.values(datasources).find(ds => ds.id === slice.datasource_id); + + // same datasource, always apply + if (targetDatasource === sourceDatasource) return true; + + const targetColumnNames = targetDatasource?.column_names ?? []; + + // let backend deal with adhoc columns and jinja + const hasSqlExpression = checkForExpression(slice.form_data); + if (hasSqlExpression) { + return true; + } + + for (const filter of filters) { + // let backend deal with adhoc columns + if ( + typeof filter.col !== 'string' && + filter.col.expressionType !== undefined + ) { + return true; + } + // shared column names, different datasources, apply filter + if (targetColumnNames.includes(filter.col)) return true; + } + + return false; + }) + .map(slice => slice.slice_id); +} + +export function getRelatedCharts( + filters: Record< + string, + AppliedNativeFilterType | AppliedCrossFilterType | Filter + >, + checkFilters: Record< + string, + AppliedNativeFilterType | AppliedCrossFilterType | Filter + > | null, + slices: Record, + datasources: DatasourcesState, +) { + const related = Object.entries(filters).reduce((acc, [filterKey, filter]) => { + const isCrossFilter = + Object.keys(slices).includes(filterKey) && + isAppliedCrossFilterType(filter); + + const chartsInScope = Array.isArray(filter.scope) + ? filter.scope + : ((filter as Filter).chartsInScope ?? []); + + if (isCrossFilter) { + const checkFilter = checkFilters?.[filterKey] as AppliedCrossFilterType; + const crossFilter = filter as AppliedCrossFilterType; + const wasRemoved = !!( + ((filter.values && filter.values.filters === undefined) || + filter.values?.filters?.length === 0) && + checkFilter?.values?.filters?.length + ); + const actualCrossFilter = wasRemoved ? checkFilter : crossFilter; + + return { + ...acc, + [filterKey]: getRelatedChartsForCrossFilter( + filterKey, + actualCrossFilter, + slices, + chartsInScope, + datasources, + ), + }; + } + + const nativeFilter = filter as AppliedNativeFilterType | Filter; + // on highlight, a standard native filter is passed + // on apply, an applied native filter is passed + if ( + isAppliedNativeFilterType(nativeFilter) || + isNativeFilter(nativeFilter) + ) { + return { + ...acc, + [filterKey]: getRelatedChartsForSelectFilter( + nativeFilter, + slices, + chartsInScope, + datasources, + ), + }; + } + return { + ...acc, + [filterKey]: chartsInScope, + }; + }, {}); + + return related; +} diff --git a/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.test.tsx b/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.test.tsx index d3010376da210..ca19c63d51642 100644 --- a/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.test.tsx +++ b/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.test.tsx @@ -24,6 +24,9 @@ import reducerIndex from 'spec/helpers/reducerIndex'; import { screen, render } from 'spec/helpers/testing-library'; import { initialState } from 'src/SqlLab/fixtures'; import useFilterFocusHighlightStyles from './useFilterFocusHighlightStyles'; +import { getRelatedCharts } from './getRelatedCharts'; + +jest.mock('./getRelatedCharts'); const TestComponent = ({ chartId }: { chartId: number }) => { const styles = useFilterFocusHighlightStyles(chartId); @@ -38,6 +41,7 @@ describe('useFilterFocusHighlightStyles', () => { { ...mockState, ...(initialState as any), ...customState }, compose(applyMiddleware(thunk)), ); + const mockGetRelatedCharts = getRelatedCharts as jest.Mock; const renderWrapper = (chartId: number, store = createMockStore()) => render(, { @@ -57,6 +61,9 @@ describe('useFilterFocusHighlightStyles', () => { }); it('should return unfocused styles if chart is not in scope of focused native filter', async () => { + mockGetRelatedCharts.mockReturnValue({ + 'test-filter': [], + }); const store = createMockStore({ nativeFilters: { focusedFilterId: 'test-filter', @@ -76,6 +83,9 @@ describe('useFilterFocusHighlightStyles', () => { }); it('should return unfocused styles if chart is not in scope of hovered native filter', async () => { + mockGetRelatedCharts.mockReturnValue({ + 'test-filter': [], + }); const store = createMockStore({ nativeFilters: { hoveredFilterId: 'test-filter', @@ -96,6 +106,9 @@ describe('useFilterFocusHighlightStyles', () => { it('should return focused styles if chart is in scope of focused native filter', async () => { const chartId = 18; + mockGetRelatedCharts.mockReturnValue({ + testFilter: [chartId], + }); const store = createMockStore({ nativeFilters: { focusedFilterId: 'testFilter', @@ -116,6 +129,9 @@ describe('useFilterFocusHighlightStyles', () => { it('should return focused styles if chart is in scope of hovered native filter', async () => { const chartId = 18; + mockGetRelatedCharts.mockReturnValue({ + testFilter: [chartId], + }); const store = createMockStore({ nativeFilters: { hoveredFilterId: 'testFilter', @@ -136,6 +152,9 @@ describe('useFilterFocusHighlightStyles', () => { it('should return unfocused styles if focusedFilterField is targeting a different chart', async () => { const chartId = 18; + mockGetRelatedCharts.mockReturnValue({ + testFilter: [], + }); const store = createMockStore({ dashboardState: { focusedFilterField: { @@ -159,6 +178,9 @@ describe('useFilterFocusHighlightStyles', () => { it('should return focused styles if focusedFilterField chart equals our own', async () => { const chartId = 18; + mockGetRelatedCharts.mockReturnValue({ + testFilter: [chartId], + }); const store = createMockStore({ dashboardState: { focusedFilterField: { diff --git a/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts b/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts index f1f428240c169..74d31f60d6567 100644 --- a/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts +++ b/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts @@ -16,11 +16,12 @@ * specific language governing permissions and limitations * under the License. */ -import { useTheme } from '@superset-ui/core'; +import { Filter, useTheme } from '@superset-ui/core'; import { useSelector } from 'react-redux'; import { getChartIdsInFilterScope } from 'src/dashboard/util/activeDashboardFilters'; import { DashboardState, RootState } from 'src/dashboard/types'; +import { getRelatedCharts } from './getRelatedCharts'; const selectFocusedFilterScope = ( dashboardState: DashboardState, @@ -41,6 +42,7 @@ const useFilterFocusHighlightStyles = (chartId: number) => { const dashboardState = useSelector( (state: RootState) => state.dashboardState, ); + const dashboardFilters = useSelector( (state: RootState) => state.dashboardFilters, ); @@ -49,6 +51,18 @@ const useFilterFocusHighlightStyles = (chartId: number) => { dashboardFilters, ); + const datasources = + useSelector((state: RootState) => state.datasources) || {}; + const slices = + useSelector((state: RootState) => state.sliceEntities.slices) || {}; + + const relatedCharts = getRelatedCharts( + nativeFilters.filters as Record, + null, + slices, + datasources, + ); + const highlightedFilterId = nativeFilters?.focusedFilterId || nativeFilters?.hoveredFilterId; if (!(focusedFilterScope || highlightedFilterId)) { @@ -69,11 +83,7 @@ const useFilterFocusHighlightStyles = (chartId: number) => { }; if (highlightedFilterId) { - if ( - nativeFilters.filters[highlightedFilterId]?.chartsInScope?.includes( - chartId, - ) - ) { + if (relatedCharts[highlightedFilterId]?.includes(chartId)) { return focusedChartStyles; } } else if ( diff --git a/superset-frontend/src/types/Chart.ts b/superset-frontend/src/types/Chart.ts index f26525cd33580..b7c439cb211f0 100644 --- a/superset-frontend/src/types/Chart.ts +++ b/superset-frontend/src/types/Chart.ts @@ -74,6 +74,8 @@ export type Slice = { query_context?: object; is_managed_externally: boolean; owners?: number[]; + datasource?: string; + datasource_id?: number; }; export default Chart; diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 82980cef161d8..e8aa0d705b8f8 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -489,6 +489,11 @@ def data_for_slices( # pylint: disable=too-many-locals del data["description"] data.update({"metrics": filtered_metrics}) data.update({"columns": filtered_columns}) + + all_columns = { + column_["column_name"]: column_["verbose_name"] or column_["column_name"] + for column_ in filtered_columns + } verbose_map = {"__timestamp": "Time"} verbose_map.update( { @@ -496,14 +501,9 @@ def data_for_slices( # pylint: disable=too-many-locals for metric in filtered_metrics } ) - verbose_map.update( - { - column_["column_name"]: column_["verbose_name"] - or column_["column_name"] - for column_ in filtered_columns - } - ) + verbose_map.update(all_columns) data["verbose_map"] = verbose_map + data["column_names"] = set(all_columns.values()) | set(self.column_names) return data diff --git a/superset/dashboards/schemas.py b/superset/dashboards/schemas.py index b0a47aba414ce..d63e79336c377 100644 --- a/superset/dashboards/schemas.py +++ b/superset/dashboards/schemas.py @@ -262,6 +262,7 @@ class DashboardDatasetSchema(Schema): owners = fields.List(fields.Dict()) columns = fields.List(fields.Dict()) column_types = fields.List(fields.Int()) + column_names = fields.List(fields.Str()) metrics = fields.List(fields.Dict()) order_by_choices = fields.List(fields.List(fields.Str())) verbose_map = fields.Dict(fields.Str(), fields.Str())