diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts index 7b89b75a4..854f0a588 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts @@ -378,7 +378,7 @@ export function cancelNativeFilterSettings() { .should('be.visible') .should('have.text', 'There are unsaved changes.'); cy.get(nativeFilters.modal.footer) - .find(nativeFilters.modal.yesCancelButton) + .find(nativeFilters.modal.confirmCancelButton) .contains('cancel') .click({ force: true }); cy.get(nativeFilters.modal.container).should('not.exist'); diff --git a/superset-frontend/cypress-base/cypress/support/directories.ts b/superset-frontend/cypress-base/cypress/support/directories.ts index 77268a5e0..b59aa1bf8 100644 --- a/superset-frontend/cypress-base/cypress/support/directories.ts +++ b/superset-frontend/cypress-base/cypress/support/directories.ts @@ -322,7 +322,9 @@ export const nativeFilters = { footer: '.ant-modal-footer', saveButton: dataTestLocator('native-filter-modal-save-button'), cancelButton: dataTestLocator('native-filter-modal-cancel-button'), - yesCancelButton: '[type="button"]', + confirmCancelButton: dataTestLocator( + 'native-filter-modal-confirm-cancel-button', + ), alertXUnsavedFilters: '.ant-alert-message', tabsList: { filterItemsContainer: dataTestLocator('filter-title-container'), diff --git a/superset-frontend/src/dashboard/components/Dashboard.jsx b/superset-frontend/src/dashboard/components/Dashboard.jsx index 1953889c5..1a852edda 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 d5b554a2f..682894acc 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 @@ -17,13 +17,8 @@ * under the License. */ -import { FC, useCallback, useRef, useState } from 'react'; -import { - NativeFilterScope, - styled, - t, - useComponentDidUpdate, -} from '@superset-ui/core'; +import { FC, useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { NativeFilterScope, styled, t } from '@superset-ui/core'; import { Radio } from 'src/components/Radio'; import { AntdForm, Typography } from 'src/components'; import { ScopingType } from './types'; @@ -32,7 +27,7 @@ import { getDefaultScopeValue, isScopingAll } from './utils'; type FilterScopeProps = { pathToFormValue?: string[]; - updateFormValues: (values: any) => void; + updateFormValues: (values: any, triggerFormChange?: boolean) => void; formFilterScope?: NativeFilterScope; forceUpdate: Function; filterScope?: NativeFilterScope; @@ -64,17 +59,19 @@ const FilterScope: FC = ({ chartId, initiallyExcludedCharts, }) => { - const [initialFilterScope] = useState( - filterScope || getDefaultScopeValue(chartId, initiallyExcludedCharts), + const initialFilterScope = useMemo( + () => filterScope || getDefaultScopeValue(chartId, initiallyExcludedCharts), + [chartId, filterScope, initiallyExcludedCharts], ); const lastSpecificScope = useRef(initialFilterScope); - const [initialScopingType] = useState( - isScopingAll(initialFilterScope, chartId) - ? ScopingType.All - : ScopingType.Specific, + const initialScopingType = useMemo( + () => + isScopingAll(initialFilterScope, chartId) + ? ScopingType.All + : ScopingType.Specific, + [chartId, initialFilterScope], ); - const [hasScopeBeenModified, setHasScopeBeenModified] = - useState(!!filterScope); + const [hasScopeBeenModified, setHasScopeBeenModified] = useState(false); const onUpdateFormValues = useCallback( (formValues: any) => { @@ -87,26 +84,24 @@ const FilterScope: FC = ({ [formScopingType, updateFormValues], ); - const updateScopes = useCallback(() => { - if (filterScope || hasScopeBeenModified) { - return; - } + const updateScopes = useCallback( + updatedFormValues => { + if (hasScopeBeenModified) { + return; + } - const newScope = getDefaultScopeValue(chartId, initiallyExcludedCharts); - updateFormValues({ - scope: newScope, - scoping: isScopingAll(newScope, chartId) - ? ScopingType.All - : ScopingType.Specific, - }); - }, [ - chartId, - filterScope, - hasScopeBeenModified, - initiallyExcludedCharts, - updateFormValues, - ]); - useComponentDidUpdate(updateScopes); + updateFormValues(updatedFormValues, false); + }, + [hasScopeBeenModified, updateFormValues], + ); + + useEffect(() => { + const updatedFormValues = { + scope: initialFilterScope, + scoping: initialScopingType, + }; + updateScopes(updatedFormValues); + }, [initialFilterScope, initialScopingType, updateScopes]); return ( diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx index b7c66412f..e498d2cd6 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -583,9 +583,9 @@ const FiltersConfigForm = ( !datasetId || datasetDetails || formFilter?.dataset?.label; const updateFormValues = useCallback( - (values: any) => { + (values: any, triggerFormChange = true) => { setNativeFilterFieldValues(form, filterId, values); - formChanged(); + if (triggerFormChange) formChanged(); }, [filterId, form, formChanged], ); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx index 037f461bc..649ac735d 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx @@ -297,7 +297,6 @@ function FiltersConfigModal({ setRemovedFilters, setOrderedFilters, setSaveAlertVisible, - filterChanges, filterId => { setFilterChanges(prev => ({ ...prev, @@ -510,7 +509,8 @@ function FiltersConfigModal({ unsavedFiltersIds.length > 0 || form.isFieldsTouched() || changed || - didChangeOrder + didChangeOrder || + Object.values(removedFilters).some(f => f?.isPending) ) { setSaveAlertVisible(true); } else { diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/Footer/CancelConfirmationAlert.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/Footer/CancelConfirmationAlert.tsx index b36769ada..16d15a06b 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/Footer/CancelConfirmationAlert.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/Footer/CancelConfirmationAlert.tsx @@ -61,6 +61,7 @@ export function CancelConfirmationAlert({ buttonSize="small" buttonStyle="primary" onClick={onConfirm} + data-test="native-filter-modal-confirm-cancel-button" > {t('Yes, cancel')} diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts index f0d09d9aa..af51dedbe 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts @@ -170,7 +170,6 @@ export const createHandleRemoveItem = val: string[] | ((prevState: string[]) => string[]), ) => void, setSaveAlertVisible: Function, - filterChanges: FilterChangesType, removeFilter: (filterId: string) => void, ) => (filterId: string) => { diff --git a/superset-frontend/src/dashboard/util/getRelatedCharts.test.ts b/superset-frontend/src/dashboard/util/getRelatedCharts.test.ts index 94762e8f7..874b912d9 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,54 +49,54 @@ test('Return chart ids matching the dataset id with native filter', () => { } as unknown as Filter, }; - const result = getRelatedCharts(filters, null, slices, datasources); + const result = getRelatedCharts(filters, null, slices); expect(result).toEqual({ - filterKey1: [1], + filterKey1: [1, 2, 3], }); }); -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', () => { +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 = { + '3': { + filterType: undefined, + scope: [1, 2, 3], + targets: [], + values: null, + } as AppliedCrossFilterType, + }; + + const result = getRelatedCharts(filters, null, slices); + expect(result).toEqual({ + '3': [1, 2], + }); +}); + +test('Return only chart ids in specific scope with cross filter', () => { const filters = { '1': { filterType: undefined, @@ -147,108 +108,8 @@ test('Return chart ids matching the column name with cross filter', () => { } as AppliedCrossFilterType, }; - const result = getRelatedCharts(filters, null, slices, datasources); + const result = getRelatedCharts(filters, null, slices); 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 index aba56ba3b..a75d9a717 100644 --- a/superset-frontend/src/dashboard/util/getRelatedCharts.ts +++ b/superset-frontend/src/dashboard/util/getRelatedCharts.ts @@ -20,56 +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) { - 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 isGlobalScope(scope: number[], slices: Record) { + return scope.length === Object.keys(slices).length; } 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 { 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); } @@ -78,52 +53,23 @@ function getRelatedChartsForCrossFilter( 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; - } + // 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; }) @@ -140,7 +86,6 @@ export function getRelatedCharts( AppliedNativeFilterType | AppliedCrossFilterType | Filter > | null, slices: Record, - datasources: DatasourcesState, ) { const related = Object.entries(filters).reduce((acc, [filterKey, filter]) => { const isCrossFilter = @@ -168,7 +113,6 @@ export function getRelatedCharts( actualCrossFilter, slices, chartsInScope, - datasources, ), }; } @@ -186,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 74d31f60d..b28d0a377 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, null, slices, - datasources, ); const highlightedFilterId =