This is an automated email from the ASF dual-hosted git repository.
diegopucci pushed a commit to branch geido/fix/cross-filtering-jinja
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to
refs/heads/geido/fix/cross-filtering-jinja by this push:
new 53fd866a03 chore(Dashboard): Simplify scoping logic for cross/native
filters
53fd866a03 is described below
commit 53fd866a0367e98cef3f21e0e560549f6f171e18
Author: Diego Pucci <[email protected]>
AuthorDate: Fri Oct 25 19:45:43 2024 +0300
chore(Dashboard): Simplify scoping logic for cross/native filters
---
.../src/dashboard/components/Dashboard.jsx | 25 +--
.../FiltersConfigForm/FilterScope/FilterScope.tsx | 4 +-
.../src/dashboard/util/getRelatedCharts.test.ts | 177 +++------------------
.../src/dashboard/util/getRelatedCharts.ts | 90 ++---------
.../util/useFilterFocusHighlightStyles.ts | 3 -
5 files changed, 44 insertions(+), 255 deletions(-)
diff --git a/superset-frontend/src/dashboard/components/Dashboard.jsx
b/superset-frontend/src/dashboard/components/Dashboard.jsx
index 1953889c5d..1a852edda8 100644
--- a/superset-frontend/src/dashboard/components/Dashboard.jsx
+++ b/superset-frontend/src/dashboard/components/Dashboard.jsx
@@ -212,7 +212,7 @@ class Dashboard extends PureComponent {
applyFilters() {
const { appliedFilters } = this;
- const { activeFilters, ownDataCharts, datasources, slices } = this.props;
+ const { activeFilters, ownDataCharts, slices } = this.props;
// refresh charts if a filter was removed, added, or changed
@@ -231,22 +231,12 @@ class Dashboard extends PureComponent {
) {
// filterKey is removed?
affectedChartIds.push(
- ...getRelatedCharts(
- appliedFilters,
- activeFilters,
- slices,
- datasources,
- )[filterKey],
+ ...getRelatedCharts(appliedFilters, activeFilters,
slices)[filterKey],
);
} else if (!appliedFilterKeys.includes(filterKey)) {
// filterKey is newly added?
affectedChartIds.push(
- ...getRelatedCharts(
- activeFilters,
- appliedFilters,
- slices,
- datasources,
- )[filterKey],
+ ...getRelatedCharts(activeFilters, appliedFilters,
slices)[filterKey],
);
} else {
// if filterKey changes value,
@@ -261,12 +251,9 @@ class Dashboard extends PureComponent {
)
) {
affectedChartIds.push(
- ...getRelatedCharts(
- activeFilters,
- appliedFilters,
- slices,
- datasources,
- )[filterKey],
+ ...getRelatedCharts(activeFilters, appliedFilters, slices)[
+ filterKey
+ ],
);
}
diff --git
a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.tsx
b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.tsx
index d5b554a2f5..673377e1b8 100644
---
a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.tsx
+++
b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.tsx
@@ -134,7 +134,9 @@ const FilterScope: FC<FilterScopeProps> = ({
<Typography.Text type="secondary">
{(formScopingType ?? initialScopingType) === ScopingType.Specific
? t('Only selected panels will be affected by this filter')
- : t('All panels with this column will be affected by this filter')}
+ : t(
+ 'All panels with this column name will be affected by this
filter',
+ )}
</Typography.Text>
{(formScopingType ?? initialScopingType) === ScopingType.Specific && (
<ScopingTree
diff --git a/superset-frontend/src/dashboard/util/getRelatedCharts.test.ts
b/superset-frontend/src/dashboard/util/getRelatedCharts.test.ts
index 94762e8f78..874b912d90 100644
--- a/superset-frontend/src/dashboard/util/getRelatedCharts.test.ts
+++ b/superset-frontend/src/dashboard/util/getRelatedCharts.test.ts
@@ -19,11 +19,9 @@
import {
AppliedCrossFilterType,
- DatasourceType,
Filter,
NativeFilterType,
} from '@superset-ui/core';
-import { DatasourcesState } from '../types';
import { getRelatedCharts } from './getRelatedCharts';
const slices = {
@@ -32,48 +30,11 @@ const slices = {
'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', () => {
+test('Return all chart ids in global scope with native filters', () => {
const filters = {
filterKey1: {
filterType: 'filter_select',
- chartsInScope: [1, 2],
+ chartsInScope: [1, 2, 3],
scope: {
excluded: [],
rootPath: [],
@@ -88,167 +49,67 @@ test('Return chart ids matching the dataset id with native
filter', () => {
} 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);
+ const result = getRelatedCharts(filters, null, slices);
expect(result).toEqual({
- '3': [1],
+ filterKey1: [1, 2, 3],
});
});
-test('Return chart ids matching the column name with native filter', () => {
+test('Return only chart ids in specific scope with native filters', () => {
const filters = {
filterKey1: {
filterType: 'filter_select',
- chartsInScope: [1, 2],
+ chartsInScope: [1, 3],
scope: {
excluded: [],
rootPath: [],
},
targets: [
{
- column: { name: 'column3' },
- datasetId: 999,
+ column: { name: 'column1' },
+ datasetId: 100,
},
],
type: NativeFilterType.NativeFilter,
} as unknown as Filter,
};
- const result = getRelatedCharts(filters, null, slices, datasources);
+ const result = getRelatedCharts(filters, null, slices);
expect(result).toEqual({
- filterKey1: [2],
+ filterKey1: [1, 3],
});
});
-test('Return chart ids matching the column name with cross filter', () => {
+test('Return all chart ids with cross filter in global scope', () => {
const filters = {
- '1': {
+ '3': {
filterType: undefined,
- scope: [1, 2],
+ scope: [1, 2, 3],
targets: [],
- values: {
- filters: [{ col: 'column3' }],
- },
+ values: null,
} 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);
+ const result = getRelatedCharts(filters, null, slices);
expect(result).toEqual({
- filterKey1: [2],
+ '3': [1, 2],
});
});
-test('Return chart ids when column display name matches with cross filter', ()
=> {
+test('Return only chart ids in specific scope with cross filter', () => {
const filters = {
'1': {
filterType: undefined,
scope: [1, 2],
targets: [],
values: {
- filters: [{ col: 'column4' }],
+ filters: [{ col: 'column3' }],
},
} as AppliedCrossFilterType,
};
- const result = getRelatedCharts(filters, null, slices, datasources);
+ const result = getRelatedCharts(filters, null, slices);
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
index c019baf98e..a75d9a7173 100644
--- a/superset-frontend/src/dashboard/util/getRelatedCharts.ts
+++ b/superset-frontend/src/dashboard/util/getRelatedCharts.ts
@@ -20,57 +20,31 @@
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<string, any>) {
- const groupby = ensureIsArray(formData?.groupby);
- const allColumns = ensureIsArray(formData?.all_columns);
- const adhocFilters = ensureIsArray(formData?.adhoc_filters);
- const checkExpressions = groupby.concat(allColumns).concat(adhocFilters);
- return checkExpressions.some(
- (ex: string | Record<string, any>) =>
- ex && typeof ex === 'object' && ex.expressionType === 'SQL',
- );
+function isGlobalScope(scope: number[], slices: Record<string, Slice>) {
+ return scope.length === Object.keys(slices).length;
}
function getRelatedChartsForSelectFilter(
nativeFilter: AppliedNativeFilterType | Filter,
slices: Record<string, Slice>,
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 { slice_id } = slice;
+ // all have been selected, always apply
+ if (isGlobalScope(chartsInScope, slices)) return true;
+ // hand-picked in scope, always apply
+ if (chartsInScope.includes(slice_id)) return true;
- 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,
- );
+ return false;
})
.map(slice => slice.slice_id);
}
@@ -79,52 +53,23 @@ function getRelatedChartsForCrossFilter(
crossFilter: AppliedCrossFilterType,
slices: Record<string, Slice>,
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;
- }
+ // hand-picked in the scope, always apply
+ const fullScope = [
+ ...scope.filter(s => String(s) !== filterKey),
+ Number(filterKey),
+ ];
+ if (isGlobalScope(fullScope, slices)) return true;
+ // hand-picked in the scope, always apply
+ if (scope.includes(slice.slice_id)) return true;
return false;
})
@@ -141,7 +86,6 @@ export function getRelatedCharts(
AppliedNativeFilterType | AppliedCrossFilterType | Filter
> | null,
slices: Record<string, Slice>,
- datasources: DatasourcesState,
) {
const related = Object.entries(filters).reduce((acc, [filterKey, filter]) =>
{
const isCrossFilter =
@@ -169,7 +113,6 @@ export function getRelatedCharts(
actualCrossFilter,
slices,
chartsInScope,
- datasources,
),
};
}
@@ -187,7 +130,6 @@ export function getRelatedCharts(
nativeFilter,
slices,
chartsInScope,
- datasources,
),
};
}
diff --git
a/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts
b/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts
index 74d31f60d6..b28d0a3771 100644
--- a/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts
+++ b/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts
@@ -51,8 +51,6 @@ const useFilterFocusHighlightStyles = (chartId: number) => {
dashboardFilters,
);
- const datasources =
- useSelector((state: RootState) => state.datasources) || {};
const slices =
useSelector((state: RootState) => state.sliceEntities.slices) || {};
@@ -60,7 +58,6 @@ const useFilterFocusHighlightStyles = (chartId: number) => {
nativeFilters.filters as Record<string, Filter>,
null,
slices,
- datasources,
);
const highlightedFilterId =
