This is an automated email from the ASF dual-hosted git repository. diegopucci pushed a commit to branch geido/refactor/native-filters-form-perf in repository https://gitbox.apache.org/repos/asf/superset.git
commit 2e44285172580f50813646cfef6c08b05fb42458 Author: Diego Pucci <[email protected]> AuthorDate: Tue Oct 15 16:47:53 2024 +0300 refactor(Dashboard): Native filters form update endpoint --- .../src/dashboard/actions/dashboardInfo.ts | 6 + .../src/dashboard/actions/dashboardState.js | 4 +- .../src/dashboard/actions/nativeFilters.ts | 108 +++++------- .../FilterBar/FilterConfigurationLink/index.tsx | 8 +- .../FilterBar/FilterControls/FilterValue.tsx | 1 - .../FilterScope/FilterScope.test.tsx | 1 + .../FiltersConfigForm/FiltersConfigForm.tsx | 61 +++++-- .../FiltersConfigForm/getControlItemsMap.test.tsx | 1 + .../FiltersConfigForm/getControlItemsMap.tsx | 4 + .../FiltersConfigModal/FiltersConfigModal.test.tsx | 196 ++++++++++++++++++--- .../FiltersConfigModal/FiltersConfigModal.tsx | 147 ++++++++++++---- .../nativeFilters/FiltersConfigModal/types.ts | 11 ++ .../nativeFilters/FiltersConfigModal/utils.ts | 115 ++++++------ .../src/dashboard/reducers/dashboardInfo.js | 37 ++++ .../src/dashboard/reducers/nativeFilters.ts | 45 ++++- superset-frontend/src/dataMask/actions.ts | 35 ++-- superset-frontend/src/dataMask/reducer.ts | 41 ++++- superset/commands/dashboard/exceptions.py | 4 + superset/commands/dashboard/update.py | 17 +- superset/constants.py | 1 + superset/daos/dashboard.py | 64 +++++++ superset/dashboards/api.py | 97 +++++++++- superset/dashboards/schemas.py | 6 + tests/integration_tests/dashboards/api_tests.py | 175 ++++++++++++++++++ 24 files changed, 960 insertions(+), 225 deletions(-) diff --git a/superset-frontend/src/dashboard/actions/dashboardInfo.ts b/superset-frontend/src/dashboard/actions/dashboardInfo.ts index 14659b9fc3..ea25e8c715 100644 --- a/superset-frontend/src/dashboard/actions/dashboardInfo.ts +++ b/superset-frontend/src/dashboard/actions/dashboardInfo.ts @@ -29,11 +29,17 @@ import { import { onSave } from './dashboardState'; export const DASHBOARD_INFO_UPDATED = 'DASHBOARD_INFO_UPDATED'; +export const DASHBOARD_INFO_FILTERS_CHANGED = 'DASHBOARD_INFO_FILTERS_CHANGED'; // updates partially changed dashboard info export function dashboardInfoChanged(newInfo: { metadata: any }) { return { type: DASHBOARD_INFO_UPDATED, newInfo }; } + +export function nativeFiltersConfigChanged(newInfo: Record<string, any>) { + return { type: DASHBOARD_INFO_FILTERS_CHANGED, newInfo }; +} + export const SAVE_CHART_CONFIG_BEGIN = 'SAVE_CHART_CONFIG_BEGIN'; export const SAVE_CHART_CONFIG_COMPLETE = 'SAVE_CHART_CONFIG_COMPLETE'; export const SAVE_CHART_CONFIG_FAIL = 'SAVE_CHART_CONFIG_FAIL'; diff --git a/superset-frontend/src/dashboard/actions/dashboardState.js b/superset-frontend/src/dashboard/actions/dashboardState.js index 9b0c181821..443db787e0 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.js @@ -63,7 +63,7 @@ import { } from './dashboardInfo'; import { fetchDatasourceMetadata } from './datasources'; import { updateDirectPathToFilter } from './dashboardFilters'; -import { SET_FILTER_CONFIG_COMPLETE } from './nativeFilters'; +import { SET_IN_SCOPE_STATUS_OF_FILTERS } from './nativeFilters'; import getOverwriteItems from '../util/getOverwriteItems'; import { applyColors, @@ -337,7 +337,7 @@ export function saveDashboardRequest(data, id, saveType) { } if (metadata.native_filter_configuration) { dispatch({ - type: SET_FILTER_CONFIG_COMPLETE, + type: SET_IN_SCOPE_STATUS_OF_FILTERS, filterConfig: metadata.native_filter_configuration, }); } diff --git a/superset-frontend/src/dashboard/actions/nativeFilters.ts b/superset-frontend/src/dashboard/actions/nativeFilters.ts index 968805dbb7..f594080b70 100644 --- a/superset-frontend/src/dashboard/actions/nativeFilters.ts +++ b/superset-frontend/src/dashboard/actions/nativeFilters.ts @@ -19,28 +19,31 @@ import { FilterConfiguration, Filters, makeApi } from '@superset-ui/core'; import { Dispatch } from 'redux'; import { cloneDeep } from 'lodash'; -import { - SET_DATA_MASK_FOR_FILTER_CONFIG_FAIL, - setDataMaskForFilterConfigComplete, -} from 'src/dataMask/actions'; +import { setDataMaskForFilterChangesComplete } from 'src/dataMask/actions'; import { HYDRATE_DASHBOARD } from './hydrate'; -import { dashboardInfoChanged } from './dashboardInfo'; -import { DashboardInfo } from '../types'; +import { + dashboardInfoChanged, + nativeFiltersConfigChanged, +} from './dashboardInfo'; +import { SaveFilterChangesType } from '../components/nativeFilters/FiltersConfigModal/types'; -export const SET_FILTER_CONFIG_BEGIN = 'SET_FILTER_CONFIG_BEGIN'; -export interface SetFilterConfigBegin { - type: typeof SET_FILTER_CONFIG_BEGIN; +export const SET_NATIVE_FILTERS_CONFIG_BEGIN = + 'SET_NATIVE_FILTERS_CONFIG_BEGIN'; +export interface SetNativeFiltersConfigBegin { + type: typeof SET_NATIVE_FILTERS_CONFIG_BEGIN; filterConfig: FilterConfiguration; } -export const SET_FILTER_CONFIG_COMPLETE = 'SET_FILTER_CONFIG_COMPLETE'; -export interface SetFilterConfigComplete { - type: typeof SET_FILTER_CONFIG_COMPLETE; - filterConfig: FilterConfiguration; +export const SET_NATIVE_FILTERS_CONFIG_COMPLETE = + 'SET_NATIVE_FILTERS_CONFIG_COMPLETE'; +export interface SetNativeFiltersConfigComplete { + type: typeof SET_NATIVE_FILTERS_CONFIG_COMPLETE; + filterChanges: SaveFilterChangesType; } -export const SET_FILTER_CONFIG_FAIL = 'SET_FILTER_CONFIG_FAIL'; -export interface SetFilterConfigFail { - type: typeof SET_FILTER_CONFIG_FAIL; + +export const SET_NATIVE_FILTERS_CONFIG_FAIL = 'SET_NATIVE_FILTERS_CONFIG_FAIL'; +export interface SetNativeFiltersConfigFail { + type: typeof SET_NATIVE_FILTERS_CONFIG_FAIL; filterConfig: FilterConfiguration; } export const SET_IN_SCOPE_STATUS_OF_FILTERS = 'SET_IN_SCOPE_STATUS_OF_FILTERS'; @@ -49,60 +52,45 @@ export interface SetInScopeStatusOfFilters { filterConfig: FilterConfiguration; } +const isFilterChangesEmpty = (filterChanges: SaveFilterChangesType) => + Object.values(filterChanges).every( + array => Array.isArray(array) && !array.length, + ); + export const setFilterConfiguration = - (filterConfig: FilterConfiguration) => + (filterChanges: SaveFilterChangesType) => async (dispatch: Dispatch, getState: () => any) => { + if (isFilterChangesEmpty(filterChanges)) { + return; + } + + const { id } = getState().dashboardInfo; + const oldFilters = getState().nativeFilters?.filters; + dispatch({ - type: SET_FILTER_CONFIG_BEGIN, - filterConfig, + type: SET_NATIVE_FILTERS_CONFIG_BEGIN, + filterChanges, }); - const { id, metadata } = getState().dashboardInfo; - const oldFilters = getState().nativeFilters?.filters; - // TODO extract this out when makeApi supports url parameters - const updateDashboard = makeApi< - Partial<DashboardInfo>, - { result: DashboardInfo } + const updateFilters = makeApi< + SaveFilterChangesType, + { result: SaveFilterChangesType } >({ method: 'PUT', - endpoint: `/api/v1/dashboard/${id}`, + endpoint: `/api/v1/dashboard/${id}/filters`, }); - - const mergedFilterConfig = filterConfig.map(filter => { - const oldFilter = oldFilters[filter.id]; - if (!oldFilter) { - return filter; - } - return { ...oldFilter, ...filter }; - }); - try { - const response = await updateDashboard({ - json_metadata: JSON.stringify({ - ...metadata, - native_filter_configuration: mergedFilterConfig, - }), - }); - dispatch( - dashboardInfoChanged({ - metadata: JSON.parse(response.result.json_metadata), - }), - ); + const response = await updateFilters(filterChanges); + dispatch(nativeFiltersConfigChanged(response.result)); dispatch({ - type: SET_FILTER_CONFIG_COMPLETE, - filterConfig: mergedFilterConfig, + type: SET_NATIVE_FILTERS_CONFIG_COMPLETE, + filterChanges, }); - dispatch( - setDataMaskForFilterConfigComplete(mergedFilterConfig, oldFilters), - ); + dispatch(setDataMaskForFilterChangesComplete(filterChanges, oldFilters)); } catch (err) { dispatch({ - type: SET_FILTER_CONFIG_FAIL, - filterConfig: mergedFilterConfig, - }); - dispatch({ - type: SET_DATA_MASK_FOR_FILTER_CONFIG_FAIL, - filterConfig: mergedFilterConfig, + type: SET_NATIVE_FILTERS_CONFIG_FAIL, + filterConfig: filterChanges, }); } }; @@ -221,9 +209,9 @@ export function updateCascadeParentIds( } export type AnyFilterAction = - | SetFilterConfigBegin - | SetFilterConfigComplete - | SetFilterConfigFail + | SetNativeFiltersConfigBegin + | SetNativeFiltersConfigComplete + | SetNativeFiltersConfigFail | SetInScopeStatusOfFilters | SetBootstrapData | SetFocusedNativeFilter diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx index d7e8564eb5..7be1e7814c 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx @@ -21,9 +21,10 @@ import { ReactNode, FC, useCallback, useState, memo } from 'react'; import { useDispatch } from 'react-redux'; import { setFilterConfiguration } from 'src/dashboard/actions/nativeFilters'; import Button from 'src/components/Button'; -import { FilterConfiguration, styled } from '@superset-ui/core'; +import { styled } from '@superset-ui/core'; import FiltersConfigModal from 'src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal'; import { getFilterBarTestId } from '../utils'; +import { SaveFilterChangesType } from '../../FiltersConfigModal/types'; export interface FCBProps { createNewOnOpen?: boolean; @@ -46,14 +47,13 @@ export const FilterConfigurationLink: FC<FCBProps> = ({ }) => { const dispatch = useDispatch(); const [isOpen, setOpen] = useState(false); - const close = useCallback(() => { setOpen(false); }, [setOpen]); const submit = useCallback( - async (filterConfig: FilterConfiguration) => { - dispatch(await setFilterConfiguration(filterConfig)); + async (filterChanges: SaveFilterChangesType) => { + dispatch(await setFilterConfiguration(filterChanges)); close(); }, [dispatch, close], diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx index 2d85f413c3..45ccd4dd41 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx @@ -182,7 +182,6 @@ const FilterValue: FC<FilterControlProps> = ({ if (isFeatureEnabled(FeatureFlag.GlobalAsyncQueries)) { // deal with getChartDataRequest transforming the response data const result = 'result' in json ? json.result[0] : json; - if (response.status === 200) { setState([result]); handleFilterLoadFinish(); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.test.tsx index 93f3dd6ec1..aece899a16 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.test.tsx @@ -47,6 +47,7 @@ describe('FilterScope', () => { activeFilterPanelKeys: `DefaultFilterId-${FilterPanels.configuration.key}`, isActive: true, validateDependencies: jest.fn(), + onModifyFilter: jest.fn(), }; const MockModal = ({ scope }: { scope?: object }) => { 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 77a423df76..b7c66412fa 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -39,8 +39,9 @@ import { t, ClientErrorObject, getClientErrorObject, + SLOW_DEBOUNCE, } from '@superset-ui/core'; -import { isEqual } from 'lodash'; +import { debounce, isEqual } from 'lodash'; import { forwardRef, useCallback, @@ -306,6 +307,7 @@ export interface FiltersConfigFormProps { filterToEdit?: Filter; removedFilters: Record<string, FilterRemoval>; restoreFilter: (filterId: string) => void; + onModifyFilter: (filterId: string) => void; form: FormInstance<NativeFiltersForm>; getAvailableFilters: ( filterId: string, @@ -346,6 +348,7 @@ const FiltersConfigForm = ( restoreFilter, handleActiveFilterPanelChange, setErroredFilters, + onModifyFilter, validateDependencies, getDependencySuggestion, isActive, @@ -372,6 +375,12 @@ const FiltersConfigForm = ( const formValues = filters?.[filterId]; const formFilter = formValues || undoFormValues || defaultFormFilter; + const handleModifyFilter = useCallback(() => { + if (onModifyFilter) { + onModifyFilter(filterId); + } + }, [onModifyFilter, filterId]); + const dependencies: string[] = formFilter?.dependencies || filterToEdit?.cascadeParentIds || []; @@ -412,12 +421,28 @@ const FiltersConfigForm = ( filterToEdit?.targets[0]?.datasetId ?? mostUsedDataset(loadedDatasets, charts); + const formChanged = useCallback(() => { + form.setFields([ + { + name: 'changed', + value: true, + }, + ]); + handleModifyFilter(); + }, [form, handleModifyFilter]); + + const debouncedFormChanged = useCallback( + debounce(formChanged, SLOW_DEBOUNCE), + [], + ); + const { controlItems = {}, mainControlItems = {} } = formFilter ? getControlItemsMap({ expanded, datasetId, disabled: false, forceUpdate, + formChanged, form, filterId, filterType: formFilter?.filterType, @@ -488,7 +513,6 @@ const FiltersConfigForm = ( groupby: formFilter?.column, ...formFilter, }); - formData.extra_form_data = dependenciesDefaultValues; setNativeFilterFieldValuesWrapper({ @@ -549,6 +573,7 @@ const FiltersConfigForm = ( groupby: hasColumn ? formFilter?.column : undefined, ...formFilter, }); + newFormData.extra_form_data = dependenciesDefaultValues; const [hasDefaultValue, isRequired, defaultValueTooltip, setHasDefaultValue] = @@ -557,15 +582,6 @@ const FiltersConfigForm = ( const showDataset = !datasetId || datasetDetails || formFilter?.dataset?.label; - const formChanged = useCallback(() => { - form.setFields([ - { - name: 'changed', - value: true, - }, - ]); - }, [form]); - const updateFormValues = useCallback( (values: any) => { setNativeFilterFieldValues(form, filterId, values); @@ -794,6 +810,7 @@ const FiltersConfigForm = ( granularity_sqla: column, }); forceUpdate(); + formChanged(); }} /> </StyledRowFormItem> @@ -817,7 +834,7 @@ const FiltersConfigForm = ( hidden initialValue={NativeFilterType.NativeFilter} > - <Input /> + <Input onChange={formChanged} /> </StyledFormItem> <StyledFormItem expanded={expanded} @@ -826,7 +843,10 @@ const FiltersConfigForm = ( initialValue={filterToEdit?.name} rules={[{ required: !isRemoved, message: t('Name is required') }]} > - <Input {...getFiltersConfigModalTestId('name-input')} /> + <Input + {...getFiltersConfigModalTestId('name-input')} + onChange={debouncedFormChanged} + /> </StyledFormItem> <StyledFormItem expanded={expanded} @@ -870,6 +890,7 @@ const FiltersConfigForm = ( column: null, }); forceUpdate(); + formChanged(); }} /> </StyledFormItem> @@ -920,6 +941,7 @@ const FiltersConfigForm = ( }); } forceUpdate(); + formChanged(); }} /> </StyledFormItem> @@ -1018,6 +1040,7 @@ const FiltersConfigForm = ( adhoc_filters: filters, }); forceUpdate(); + formChanged(); validatePreFilter(); }} label={ @@ -1050,6 +1073,7 @@ const FiltersConfigForm = ( time_range: timeRange, }); forceUpdate(); + formChanged(); validatePreFilter(); }} /> @@ -1085,6 +1109,7 @@ const FiltersConfigForm = ( <Radio.Group onChange={value => { onSortChanged(value.target.value); + formChanged(); }} > <Radio value>{t('Sort ascending')}</Radio> @@ -1124,6 +1149,7 @@ const FiltersConfigForm = ( }); forceUpdate(); } + formChanged(); }} /> </StyledRowSubFormItem> @@ -1156,9 +1182,10 @@ const FiltersConfigForm = ( } > <Radio.Group - onChange={value => - onEnableSingleValueChanged(value.target.value) - } + onChange={value => { + onEnableSingleValueChanged(value.target.value); + formChanged(); + }} > <Radio value={SingleValueType.Minimum}> {t('Minimum')} @@ -1187,7 +1214,7 @@ const FiltersConfigForm = ( initialValue={filterToEdit?.description} label={<StyledLabel>{t('Description')}</StyledLabel>} > - <TextArea /> + <TextArea onChange={debouncedFormChanged} /> </StyledFormItem> <CleanFormItem name={['filters', filterId, 'defaultValueQueriesData']} diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx index 93d31990ad..9d2fc12e0c 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx @@ -72,6 +72,7 @@ const createProps: () => ControlItemsProps = () => ({ filterId: 'filterId', filterToEdit: filterMock, filterType: 'filterType', + formChanged: jest.fn(), }); const createControlItems = () => [ diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx index 62fd4917ec..ed92809988 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx @@ -48,6 +48,7 @@ export interface ControlItemsProps { datasetId: number; disabled: boolean; forceUpdate: Function; + formChanged: Function; form: FormInstance<NativeFiltersForm>; filterId: string; filterType: string; @@ -65,6 +66,7 @@ export default function getControlItemsMap({ datasetId, disabled, forceUpdate, + formChanged, form, filterId, filterType, @@ -137,6 +139,7 @@ export default function getControlItemsMap({ defaultDataMask: null, }); forceUpdate(); + formChanged(); }} /> </StyledFormItem> @@ -200,6 +203,7 @@ export default function getControlItemsMap({ defaultDataMask: null, }); } + formChanged(); forceUpdate(); }} > diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx index a56e969f1b..8e61ee736a 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx @@ -23,7 +23,12 @@ import chartQueries from 'spec/fixtures/mockChartQueries'; import { dashboardLayout } from 'spec/fixtures/mockDashboardLayout'; import mockDatasource, { datasourceId, id } from 'spec/fixtures/mockDatasource'; import { buildNativeFilter } from 'spec/fixtures/mockNativeFilters'; -import { render, screen, waitFor } from 'spec/helpers/testing-library'; +import { + fireEvent, + render, + screen, + waitFor, +} from 'spec/helpers/testing-library'; import { RangeFilterPlugin, SelectFilterPlugin, @@ -147,6 +152,10 @@ beforeAll(() => { new MainPreset().register(); }); +afterEach(() => { + jest.restoreAllMocks(); +}); + function defaultRender(initialState: any = defaultState(), modalProps = props) { return render(<FiltersConfigModal {...modalProps} />, { initialState, @@ -373,29 +382,24 @@ test('deletes a filter', async () => { dashboardLayout, }; const onSave = jest.fn(); + defaultRender(state, { ...props, createNewOnOpen: false, onSave, }); const removeButtons = screen.getAllByRole('img', { name: 'trash' }); - // remove NATIVE_FILTER-3 which isn't a dependency of any other filter userEvent.click(removeButtons[2]); + userEvent.click(screen.getByRole('button', { name: SAVE_REGEX })); + await waitFor(() => expect(onSave).toHaveBeenCalledWith( - expect.arrayContaining([ - expect.objectContaining({ - type: 'NATIVE_FILTER', - id: 'NATIVE_FILTER-1', - cascadeParentIds: ['NATIVE_FILTER-2'], - }), - expect.objectContaining({ - type: 'NATIVE_FILTER', - id: 'NATIVE_FILTER-2', - cascadeParentIds: [], - }), - ]), + expect.objectContaining({ + deleted: expect.arrayContaining(['NATIVE_FILTER-3']), + modified: expect.arrayContaining([]), + reordered: expect.arrayContaining([]), + }), ), ); }); @@ -420,23 +424,161 @@ test('deletes a filter including dependencies', async () => { onSave, }); const removeButtons = screen.getAllByRole('img', { name: 'trash' }); - // remove NATIVE_FILTER-2 which is a dependency of NATIVE_FILTER-1 userEvent.click(removeButtons[1]); userEvent.click(screen.getByRole('button', { name: SAVE_REGEX })); await waitFor(() => expect(onSave).toHaveBeenCalledWith( - expect.arrayContaining([ - expect.objectContaining({ - type: 'NATIVE_FILTER', - id: 'NATIVE_FILTER-1', - cascadeParentIds: [], - }), - expect.objectContaining({ - type: 'NATIVE_FILTER', - id: 'NATIVE_FILTER-3', - cascadeParentIds: [], - }), - ]), + expect.objectContaining({ + deleted: ['NATIVE_FILTER-2'], + modified: expect.arrayContaining([ + expect.objectContaining({ + id: 'NATIVE_FILTER-1', + }), + ]), + reordered: [], + }), + ), + ); +}); + +test('switches the order between two filters', async () => { + const nativeFilterState = [ + buildNativeFilter('NATIVE_FILTER-1', 'state', []), + buildNativeFilter('NATIVE_FILTER-2', 'country', []), + ]; + + const state = { + ...defaultState(), + dashboardInfo: { + metadata: { native_filter_configuration: nativeFilterState }, + }, + dashboardLayout, + }; + + const onSave = jest.fn(); + + defaultRender(state, { + ...props, + createNewOnOpen: false, + onSave, + }); + + const draggableFilters = screen.getAllByRole('tab'); + + fireEvent.dragStart(draggableFilters[0]); + + fireEvent.dragOver(draggableFilters[1]); + + fireEvent.drop(draggableFilters[1]); + + fireEvent.dragEnd(draggableFilters[0]); + + userEvent.click(screen.getByRole('button', { name: SAVE_REGEX })); + + await waitFor(() => + expect(onSave).toHaveBeenCalledWith( + expect.objectContaining({ + deleted: [], + modified: [], + reordered: expect.arrayContaining([ + 'NATIVE_FILTER-2', + 'NATIVE_FILTER-1', + ]), + }), + ), + ); +}); + +test('rearranges three filters and deletes one of them', async () => { + const nativeFilterState = [ + buildNativeFilter('NATIVE_FILTER-1', 'state', []), + buildNativeFilter('NATIVE_FILTER-2', 'country', []), + buildNativeFilter('NATIVE_FILTER-3', 'product', []), + ]; + + const state = { + ...defaultState(), + dashboardInfo: { + metadata: { native_filter_configuration: nativeFilterState }, + }, + dashboardLayout, + }; + + const onSave = jest.fn(); + + defaultRender(state, { + ...props, + createNewOnOpen: false, + onSave, + }); + + const draggableFilters = screen.getAllByRole('tab'); + const deleteButtons = screen.getAllByRole('img', { name: 'trash' }); + userEvent.click(deleteButtons[1]); + + fireEvent.dragStart(draggableFilters[0]); + fireEvent.dragOver(draggableFilters[2]); + fireEvent.drop(draggableFilters[2]); + fireEvent.dragEnd(draggableFilters[0]); + + userEvent.click(screen.getByRole('button', { name: SAVE_REGEX })); + + await waitFor(() => + expect(onSave).toHaveBeenCalledWith( + expect.objectContaining({ + modified: [], + deleted: ['NATIVE_FILTER-2'], + reordered: expect.arrayContaining([ + 'NATIVE_FILTER-2', + 'NATIVE_FILTER-3', + 'NATIVE_FILTER-1', + ]), + }), + ), + ); +}); + +test('modifies the name of a filter', async () => { + jest.useFakeTimers(); + const nativeFilterState = [ + buildNativeFilter('NATIVE_FILTER-1', 'state', []), + buildNativeFilter('NATIVE_FILTER-2', 'country', []), + ]; + + const state = { + ...defaultState(), + dashboardInfo: { + metadata: { native_filter_configuration: nativeFilterState }, + }, + dashboardLayout, + }; + + const onSave = jest.fn(); + + defaultRender(state, { + ...props, + createNewOnOpen: false, + onSave, + }); + + const filterNameInput = screen.getByRole('textbox', { + name: FILTER_NAME_REGEX, + }); + + userEvent.clear(filterNameInput); + userEvent.type(filterNameInput, 'New Filter Name'); + + jest.runAllTimers(); + + userEvent.click(screen.getByRole('button', { name: SAVE_REGEX })); + + await waitFor(() => + expect(onSave).toHaveBeenCalledWith( + expect.objectContaining({ + modified: expect.arrayContaining([ + expect.objectContaining({ name: 'New Filter Name' }), + ]), + }), ), ); }); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx index 7ad2838d26..9432075689 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx @@ -20,7 +20,6 @@ import { memo, useEffect, useCallback, useMemo, useState, useRef } from 'react'; import { uniq, isEqual, sortBy, debounce, isEmpty } from 'lodash'; import { Filter, - FilterConfiguration, NativeFilterType, Divider, styled, @@ -44,7 +43,12 @@ import FiltersConfigForm, { } from './FiltersConfigForm/FiltersConfigForm'; import Footer from './Footer/Footer'; import { useOpenModal, useRemoveCurrentFilter } from './state'; -import { FilterRemoval, NativeFiltersForm } from './types'; +import { + FilterChangesType, + FilterRemoval, + NativeFiltersForm, + SaveFilterChangesType, +} from './types'; import { createHandleSave, createHandleRemoveItem, @@ -114,7 +118,7 @@ export interface FiltersConfigModalProps { isOpen: boolean; initialFilterId?: string; createNewOnOpen?: boolean; - onSave: (filterConfig: FilterConfiguration) => Promise<void>; + onSave: (filterChanges: SaveFilterChangesType) => Promise<void>; onCancel: () => void; } export const ALLOW_DEPENDENCIES = [ @@ -128,6 +132,11 @@ const DEFAULT_REMOVED_FILTERS: Record<string, FilterRemoval> = {}; const DEFAULT_FORM_VALUES: NativeFiltersForm = { filters: {}, }; +const DEFAULT_FILTER_CHANGES: FilterChangesType = { + modified: [], + deleted: [], + reordered: [], +}; /** * This is the modal to configure all the dashboard-native filters. @@ -155,6 +164,27 @@ function FiltersConfigModal({ const filterConfig = useFilterConfiguration(); const filterConfigMap = useFilterConfigMap(); + // this state contains the changes that we'll be sent through the PATCH endpoint + const [filterChanges, setFilterChanges] = useState<FilterChangesType>( + DEFAULT_FILTER_CHANGES, + ); + + const resetFilterChanges = () => { + setFilterChanges(DEFAULT_FILTER_CHANGES); + }; + + const handleModifyFilter = useCallback( + (filterId: string) => { + if (!filterChanges.modified.includes(filterId)) { + setFilterChanges(prev => ({ + ...prev, + modified: [...prev.modified, filterId], + })); + } + }, + [filterChanges.modified], + ); + // new filter ids belong to filters have been added during // this configuration session, and only exist in the form state until we submit. const [newFilterIds, setNewFilterIds] = useState<string[]>( @@ -200,11 +230,17 @@ function FiltersConfigModal({ const restoreFilter = useCallback( (id: string) => { const removal = removedFilters[id]; - // gotta clear the removal timeout to prevent the filter from getting deleted + // Clear the removal timeout if the filter is pending deletion if (removal?.isPending) clearTimeout(removal.timerId); + setRemovedFilters(current => ({ ...current, [id]: null })); + + setFilterChanges(prev => ({ + ...prev, + deleted: prev.deleted.filter(deletedId => deletedId !== id), + })); }, - [removedFilters], + [removedFilters, setRemovedFilters], ); const initialFilterOrder = useMemo( () => Object.keys(filterConfigMap), @@ -239,18 +275,13 @@ function FiltersConfigModal({ (type: NativeFilterType) => { const newFilterId = generateFilterId(type); setNewFilterIds([...newFilterIds, newFilterId]); + handleModifyFilter(newFilterId); setCurrentFilterId(newFilterId); setSaveAlertVisible(false); setOrderedFilters([...orderedFilters, newFilterId]); setActiveFilterPanelKey(getActiveFilterPanelKey(newFilterId)); }, - [ - newFilterIds, - orderedFilters, - setCurrentFilterId, - setOrderedFilters, - setNewFilterIds, - ], + [newFilterIds, handleModifyFilter, orderedFilters], ); useOpenModal(isOpen, addFilter, createNewOnOpen); @@ -266,6 +297,13 @@ function FiltersConfigModal({ setRemovedFilters, setOrderedFilters, setSaveAlertVisible, + filterChanges, + filterId => { + setFilterChanges(prev => ({ + ...prev, + deleted: [...prev.deleted, filterId], + })); + }, ); // After this, it should be as if the modal was just opened fresh. @@ -276,6 +314,7 @@ function FiltersConfigModal({ setRemovedFilters(DEFAULT_REMOVED_FILTERS); setSaveAlertVisible(false); setFormValues(DEFAULT_FORM_VALUES); + resetFilterChanges(); setErroredFilters(DEFAULT_EMPTY_FILTERS); if (filterIds.length > 0) { setActiveFilterPanelKey(getActiveFilterPanelKey(filterIds[0])); @@ -329,19 +368,32 @@ function FiltersConfigModal({ value: id, type: filterConfigMap[id]?.filterType, })), - [canBeUsedAsDependency, filterIds, getFilterTitle], + [canBeUsedAsDependency, filterConfigMap, filterIds, getFilterTitle], ); + /** + * Manages dependencies of filters associated with a deleted filter. + * + * @param values the native filters form + * @returns the updated filterConfigMap + */ const cleanDeletedParents = (values: NativeFiltersForm | null) => { + const modifiedParentFilters = new Set<string>(); const updatedFilterConfigMap = Object.keys(filterConfigMap).reduce( (acc, key) => { const filter = filterConfigMap[key]; const cascadeParentIds = filter.cascadeParentIds?.filter(id => canBeUsedAsDependency(id), ); - if (cascadeParentIds) { + + if ( + cascadeParentIds && + !isEqual(cascadeParentIds, filter.cascadeParentIds) + ) { dispatch(updateCascadeParentIds(key, cascadeParentIds)); + modifiedParentFilters.add(key); } + return { ...acc, [key]: { @@ -357,18 +409,24 @@ function FiltersConfigModal({ if (filters) { Object.keys(filters).forEach(key => { const filter = filters[key]; + if (!('dependencies' in filter)) { return; } - const { dependencies } = filter; - if (dependencies) { - filter.dependencies = dependencies.filter(id => - canBeUsedAsDependency(id), - ); + + const originalDependencies = filter.dependencies || []; + const cleanedDependencies = originalDependencies.filter(id => + canBeUsedAsDependency(id), + ); + + if (!isEqual(cleanedDependencies, originalDependencies)) { + filter.dependencies = cleanedDependencies; + modifiedParentFilters.add(key); } }); } - return updatedFilterConfigMap; + + return [updatedFilterConfigMap, modifiedParentFilters]; }; const handleErroredFilters = useCallback(() => { @@ -407,18 +465,35 @@ function FiltersConfigModal({ handleErroredFilters(); if (values) { - const updatedFilterConfigMap = cleanDeletedParents(values); - createHandleSave( - updatedFilterConfigMap, - orderedFilters, - removedFilters, - onSave, - values, - )(); + const [updatedFilterConfigMap, modifiedParentFilters] = + cleanDeletedParents(values); + + const allModified = [ + ...new Set([ + ...(modifiedParentFilters as Set<string>), + ...filterChanges.modified, + ]), + ]; + + const actualChanges = { + ...filterChanges, + modified: + allModified.length && filterChanges.deleted.length + ? allModified.filter(id => !filterChanges.deleted.includes(id)) + : allModified, + reordered: + filterChanges.reordered.length && + !isEqual(filterChanges.reordered, initialFilterOrder) + ? filterChanges.reordered + : [], + }; + + createHandleSave(onSave, actualChanges, values, updatedFilterConfigMap)(); resetForm(true); } else { configFormRef.current?.changeTab?.('configuration'); } + resetFilterChanges(); }; const handleConfirmCancel = () => { @@ -447,6 +522,10 @@ function FiltersConfigModal({ const removed = newOrderedFilter.splice(dragIndex, 1)[0]; newOrderedFilter.splice(targetIndex, 0, removed); setOrderedFilters(newOrderedFilter); + setFilterChanges(prev => ({ + ...prev, + reordered: newOrderedFilter, + })); }; const buildDependencyMap = useCallback(() => { @@ -596,28 +675,34 @@ function FiltersConfigModal({ setErroredFilters={setErroredFilters} validateDependencies={validateDependencies} getDependencySuggestion={getDependencySuggestion} + onModifyFilter={handleModifyFilter} /> )} </div> ); }), [ - renderedFilters, orderedFilters, + renderedFilters, currentFilterId, filterConfigMap, + expanded, form, removedFilters, restoreFilter, getAvailableFilters, activeFilterPanelKey, + handleActiveFilterPanelChange, validateDependencies, getDependencySuggestion, - handleActiveFilterPanelChange, - expanded, + handleModifyFilter, ], ); + useEffect(() => { + resetFilterChanges(); + }, []); + return ( <StyledModalWrapper visible={isOpen} diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/types.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/types.ts index 6f40ea6542..41d53791f6 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/types.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/types.ts @@ -21,6 +21,7 @@ import { DataMask, NativeFilterType, NativeFilterScope, + Filter, } from '@superset-ui/core'; export interface NativeFiltersFormItem { @@ -60,6 +61,16 @@ export interface NativeFiltersForm { changed?: boolean; } +export type FilterChangesType = { + modified: string[]; + deleted: string[]; + reordered: string[]; +}; + +export type SaveFilterChangesType = { + modified: Filter[]; +} & Omit<FilterChangesType, 'modified'>; + export type FilterRemoval = | null | { diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts index ffe53378ab..f0d09d9aa3 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts @@ -20,15 +20,15 @@ import { FormInstance } from 'src/components'; import { nanoid } from 'nanoid'; import { getInitialDataMask } from 'src/dataMask/reducer'; import { - Filter, FilterConfiguration, NativeFilterType, - Divider, NativeFilterTarget, logging, + Filter, + Divider, } from '@superset-ui/core'; import { DASHBOARD_ROOT_ID } from 'src/dashboard/util/constants'; -import { FilterRemoval, NativeFiltersForm } from './types'; +import { FilterChangesType, FilterRemoval, NativeFiltersForm } from './types'; export const REMOVAL_DELAY_SECS = 5; @@ -92,62 +92,69 @@ export const validateForm = async ( export const createHandleSave = ( - filterConfigMap: Record<string, Filter | Divider>, - filterIds: string[], - removedFilters: Record<string, FilterRemoval>, saveForm: Function, + filterChanges: FilterChangesType, values: NativeFiltersForm, + filterConfigMap: Record<string, Filter | Divider>, ) => async () => { - const newFilterConfig: FilterConfiguration = filterIds - .filter(id => !removedFilters[id]) - .map(id => { - // create a filter config object from the form inputs - const formInputs = values.filters?.[id]; - // if user didn't open a filter, return the original config - if (!formInputs) return filterConfigMap[id]; - if (formInputs.type === NativeFilterType.Divider) { - return { - id, - type: NativeFilterType.Divider, - scope: { - rootPath: [DASHBOARD_ROOT_ID], - excluded: [], - }, - title: formInputs.title, - description: formInputs.description, - }; - } - const target: Partial<NativeFilterTarget> = {}; - if (formInputs.dataset) { - target.datasetId = formInputs.dataset.value; - } - if (formInputs.dataset && formInputs.column) { - target.column = { name: formInputs.column }; - } + const transformFilter = (id: string) => { + const formInputs = values.filters?.[id] || filterConfigMap[id]; + if (!formInputs) { + return undefined; + } + if (formInputs.type === NativeFilterType.Divider) { return { id, - adhoc_filters: formInputs.adhoc_filters, - time_range: formInputs.time_range, - controlValues: formInputs.controlValues ?? {}, - granularity_sqla: formInputs.granularity_sqla, - requiredFirst: Object.values(formInputs.requiredFirst ?? {}).find( - rf => rf, - ), - name: formInputs.name, - filterType: formInputs.filterType, - // for now there will only ever be one target - targets: [target], - defaultDataMask: formInputs.defaultDataMask ?? getInitialDataMask(), - cascadeParentIds: formInputs.dependencies || [], - scope: formInputs.scope, - sortMetric: formInputs.sortMetric, - type: formInputs.type, - description: (formInputs.description || '').trim(), + type: NativeFilterType.Divider, + scope: { + rootPath: [DASHBOARD_ROOT_ID], + excluded: [], + }, + title: formInputs.title, + description: formInputs.description, }; - }); + } - await saveForm(newFilterConfig); + const target: Partial<NativeFilterTarget> = {}; + if (formInputs.dataset) { + target.datasetId = formInputs.dataset.value; + } + if (formInputs.dataset && formInputs.column) { + target.column = { name: formInputs.column }; + } + + return { + id, + adhoc_filters: formInputs.adhoc_filters, + time_range: formInputs.time_range, + controlValues: formInputs.controlValues ?? {}, + granularity_sqla: formInputs.granularity_sqla, + requiredFirst: Object.values(formInputs.requiredFirst ?? {}).find( + rf => rf, + ), + name: formInputs.name, + filterType: formInputs.filterType, + targets: [target], + defaultDataMask: formInputs.defaultDataMask ?? getInitialDataMask(), + cascadeParentIds: formInputs.dependencies || [], + scope: formInputs.scope, + sortMetric: formInputs.sortMetric, + type: formInputs.type, + description: (formInputs.description || '').trim(), + }; + return undefined; + }; + + const transformedModified = filterChanges.modified + .map(transformFilter) + .filter(Boolean); + + const newFilterChanges = { + ...filterChanges, + modified: transformedModified, + }; + await saveForm(newFilterChanges); }; export const createHandleRemoveItem = @@ -163,6 +170,8 @@ export const createHandleRemoveItem = val: string[] | ((prevState: string[]) => string[]), ) => void, setSaveAlertVisible: Function, + filterChanges: FilterChangesType, + removeFilter: (filterId: string) => void, ) => (filterId: string) => { const completeFilterRemoval = (filterId: string) => { @@ -187,12 +196,14 @@ export const createHandleRemoveItem = ...removedFilters, [filterId]: { isPending: true, timerId }, })); + removeFilter(filterId); + // eslint-disable-next-line no-param-reassign setSaveAlertVisible(false); }; export const NATIVE_FILTER_PREFIX = 'NATIVE_FILTER-'; export const NATIVE_FILTER_DIVIDER_PREFIX = 'NATIVE_FILTER_DIVIDER-'; -export const generateFilterId = (type: NativeFilterType) => { +export const generateFilterId = (type: NativeFilterType): string => { const prefix = type === NativeFilterType.NativeFilter ? NATIVE_FILTER_PREFIX diff --git a/superset-frontend/src/dashboard/reducers/dashboardInfo.js b/superset-frontend/src/dashboard/reducers/dashboardInfo.js index 3503c441b6..29f4dd2d3b 100644 --- a/superset-frontend/src/dashboard/reducers/dashboardInfo.js +++ b/superset-frontend/src/dashboard/reducers/dashboardInfo.js @@ -21,6 +21,7 @@ import { DASHBOARD_INFO_UPDATED, SET_FILTER_BAR_ORIENTATION, SET_CROSS_FILTERS_ENABLED, + DASHBOARD_INFO_FILTERS_CHANGED, } from '../actions/dashboardInfo'; import { HYDRATE_DASHBOARD } from '../actions/hydrate'; @@ -33,6 +34,42 @@ export default function dashboardStateReducer(state = {}, action) { // server-side compare last_modified_time in second level last_modified_time: Math.round(new Date().getTime() / 1000), }; + case DASHBOARD_INFO_FILTERS_CHANGED: { + const { modified = [], deleted = [], reordered = [] } = action.newInfo; + let updatedFilters = state.metadata.native_filter_configuration || []; + if (deleted.length > 0) { + updatedFilters = updatedFilters.filter( + filter => !deleted.includes(filter.id), + ); + } + if (modified.length > 0) { + modified.forEach(modifiedFilter => { + const existingFilterIndex = updatedFilters.findIndex( + filter => filter.id === modifiedFilter.id, + ); + if (existingFilterIndex > -1) { + updatedFilters[existingFilterIndex] = modifiedFilter; + } else { + updatedFilters = [...updatedFilters, modifiedFilter]; + } + }); + } + if (reordered.length > 0) { + updatedFilters = reordered + .map(reorderedId => + updatedFilters.find(filter => filter.id === reorderedId), + ) + .filter(Boolean); + } + return { + ...state, + metadata: { + ...state.metadata, + native_filter_configuration: updatedFilters, + }, + last_modified_time: Math.round(new Date().getTime() / 1000), + }; + } case HYDRATE_DASHBOARD: return { ...state, diff --git a/superset-frontend/src/dashboard/reducers/nativeFilters.ts b/superset-frontend/src/dashboard/reducers/nativeFilters.ts index c68ff3c19b..8ca3f0eea6 100644 --- a/superset-frontend/src/dashboard/reducers/nativeFilters.ts +++ b/superset-frontend/src/dashboard/reducers/nativeFilters.ts @@ -18,7 +18,7 @@ */ import { AnyFilterAction, - SET_FILTER_CONFIG_COMPLETE, + SET_NATIVE_FILTERS_CONFIG_COMPLETE, SET_IN_SCOPE_STATUS_OF_FILTERS, SET_FOCUSED_NATIVE_FILTER, UNSET_FOCUSED_NATIVE_FILTER, @@ -28,6 +28,7 @@ import { } from 'src/dashboard/actions/nativeFilters'; import { FilterConfiguration, NativeFiltersState } from '@superset-ui/core'; import { HYDRATE_DASHBOARD } from '../actions/hydrate'; +import { SaveFilterChangesType } from '../components/nativeFilters/FiltersConfigModal/types'; export function getInitialState({ filterConfig, @@ -37,7 +38,6 @@ export function getInitialState({ state?: NativeFiltersState; }): NativeFiltersState { const state: Partial<NativeFiltersState> = {}; - const filters = {}; if (filterConfig) { filterConfig.forEach(filter => { @@ -52,6 +52,43 @@ export function getInitialState({ return state as NativeFiltersState; } +function handleFilterChangesComplete( + state: NativeFiltersState, + changes: SaveFilterChangesType, +) { + const { modified = [], deleted = [], reordered = [] } = changes; + + let updatedFilters = { ...state.filters }; + + if (deleted.length > 0) { + deleted.forEach(id => { + if (updatedFilters[id]) { + delete updatedFilters[id]; + } + }); + } + + if (modified.length > 0) { + const modifiedFilters = Object.fromEntries( + modified.map(filter => [filter.id, filter]), + ); + updatedFilters = { ...updatedFilters, ...modifiedFilters }; + } + + if (reordered.length > 0) { + updatedFilters = Object.fromEntries( + reordered + .map(id => [id, updatedFilters[id]]) + .filter(([, filter]) => filter), + ); + } + + return { + ...state, + filters: updatedFilters, + } as NativeFiltersState; +} + export default function nativeFilterReducer( state: NativeFiltersState = { filters: {}, @@ -64,10 +101,12 @@ export default function nativeFilterReducer( filters: action.data.nativeFilters.filters, }; - case SET_FILTER_CONFIG_COMPLETE: case SET_IN_SCOPE_STATUS_OF_FILTERS: return getInitialState({ filterConfig: action.filterConfig, state }); + case SET_NATIVE_FILTERS_CONFIG_COMPLETE: + return handleFilterChangesComplete(state, action.filterChanges); + case SET_FOCUSED_NATIVE_FILTER: return { ...state, diff --git a/superset-frontend/src/dataMask/actions.ts b/superset-frontend/src/dataMask/actions.ts index cb7ed393d7..e8c7d6c4f3 100644 --- a/superset-frontend/src/dataMask/actions.ts +++ b/superset-frontend/src/dataMask/actions.ts @@ -16,7 +16,8 @@ * specific language governing permissions and limitations * under the License. */ -import { DataMask, FilterConfiguration, Filters } from '@superset-ui/core'; +import { DataMask, Filters } from '@superset-ui/core'; +import { SaveFilterChangesType } from 'src/dashboard/components/nativeFilters/FiltersConfigModal/types'; import { getInitialDataMask } from './reducer'; export const CLEAR_DATA_MASK_STATE = 'CLEAR_DATA_MASK_STATE'; @@ -37,32 +38,25 @@ export interface INITDATAMASK { dataMask: DataMask; } -export const SET_DATA_MASK_FOR_FILTER_CONFIG_COMPLETE = - 'SET_DATA_MASK_FOR_FILTER_CONFIG_COMPLETE'; - -export interface SetDataMaskForFilterConfigComplete { - type: typeof SET_DATA_MASK_FOR_FILTER_CONFIG_COMPLETE; - filterConfig: FilterConfiguration; +export const SET_DATA_MASK_FOR_FILTER_CHANGES_COMPLETE = + 'SET_DATA_MASK_FOR_FILTER_CHANGES_COMPLETE'; +export interface SetDataMaskForFilterChangesComplete { + type: typeof SET_DATA_MASK_FOR_FILTER_CHANGES_COMPLETE; + filterChanges: SaveFilterChangesType; filters?: Filters; } -export const SET_DATA_MASK_FOR_FILTER_CONFIG_FAIL = - 'SET_DATA_MASK_FOR_FILTER_CONFIG_FAIL'; - -export interface SetDataMaskForFilterConfigFail { - type: typeof SET_DATA_MASK_FOR_FILTER_CONFIG_FAIL; - filterConfig: FilterConfiguration; -} -export function setDataMaskForFilterConfigComplete( - filterConfig: FilterConfiguration, +export function setDataMaskForFilterChangesComplete( + filterChanges: SaveFilterChangesType, filters?: Filters, -): SetDataMaskForFilterConfigComplete { +): SetDataMaskForFilterChangesComplete { return { - type: SET_DATA_MASK_FOR_FILTER_CONFIG_COMPLETE, - filterConfig, + type: SET_DATA_MASK_FOR_FILTER_CHANGES_COMPLETE, + filterChanges, filters, }; } + export function updateDataMask( filterId: string | number, dataMask: DataMask, @@ -87,5 +81,4 @@ export function clearDataMaskState(): ClearDataMaskState { export type AnyDataMaskAction = | ClearDataMaskState | UpdateDataMask - | SetDataMaskForFilterConfigFail - | SetDataMaskForFilterConfigComplete; + | SetDataMaskForFilterChangesComplete; diff --git a/superset-frontend/src/dataMask/reducer.ts b/superset-frontend/src/dataMask/reducer.ts index 28158630ad..b79f17fb1d 100644 --- a/superset-frontend/src/dataMask/reducer.ts +++ b/superset-frontend/src/dataMask/reducer.ts @@ -32,10 +32,11 @@ import { } from '@superset-ui/core'; import { NATIVE_FILTER_PREFIX } from 'src/dashboard/components/nativeFilters/FiltersConfigModal/utils'; import { HYDRATE_DASHBOARD } from 'src/dashboard/actions/hydrate'; +import { SaveFilterChangesType } from 'src/dashboard/components/nativeFilters/FiltersConfigModal/types'; import { AnyDataMaskAction, CLEAR_DATA_MASK_STATE, - SET_DATA_MASK_FOR_FILTER_CONFIG_COMPLETE, + SET_DATA_MASK_FOR_FILTER_CHANGES_COMPLETE, UPDATE_DATA_MASK, } from './actions'; import { areObjectsEqual } from '../reduxUtils'; @@ -100,6 +101,37 @@ function fillNativeFilters( }); } +function updateDataMaskForFilterChanges( + filterChanges: SaveFilterChangesType, + mergedDataMask: DataMaskStateWithId, + draftDataMask: DataMaskStateWithId, + initialDataMask?: Filters, +) { + const dataMask = initialDataMask || {}; + + Object.entries(dataMask).forEach(([key, value]) => { + mergedDataMask[key] = { ...value, ...value.defaultDataMask }; + }); + + filterChanges.deleted.forEach((filterId: string) => { + delete mergedDataMask[filterId]; + }); + + filterChanges.modified.forEach((filter: Filter) => { + mergedDataMask[filter.id] = { + ...getInitialDataMask(filter.id), + ...filter.defaultDataMask, + ...filter, + }; + }); + + Object.values(draftDataMask).forEach(filter => { + if (!String(filter?.id).startsWith(NATIVE_FILTER_PREFIX)) { + mergedDataMask[filter?.id] = filter; + } + }); +} + const dataMaskReducer = produce( (draft: DataMaskStateWithId, action: AnyDataMaskAction) => { const cleanState = {}; @@ -136,15 +168,14 @@ const dataMaskReducer = produce( action.data.dataMask, ); return cleanState; - case SET_DATA_MASK_FOR_FILTER_CONFIG_COMPLETE: - fillNativeFilters( - action.filterConfig ?? [], + case SET_DATA_MASK_FOR_FILTER_CHANGES_COMPLETE: + updateDataMaskForFilterChanges( + action.filterChanges, cleanState, draft, action.filters, ); return cleanState; - default: return draft; } diff --git a/superset/commands/dashboard/exceptions.py b/superset/commands/dashboard/exceptions.py index 9281119b32..a4b7be1261 100644 --- a/superset/commands/dashboard/exceptions.py +++ b/superset/commands/dashboard/exceptions.py @@ -58,6 +58,10 @@ class DashboardUpdateFailedError(UpdateFailedError): message = _("Dashboard could not be updated.") +class DashboardNativeFiltersUpdateFailedError(UpdateFailedError): + message = _("Dashboard native filters could not be patched.") + + class DashboardDeleteFailedError(DeleteFailedError): message = _("Dashboard could not be deleted.") diff --git a/superset/commands/dashboard/update.py b/superset/commands/dashboard/update.py index 031db1af31..a87ac899d1 100644 --- a/superset/commands/dashboard/update.py +++ b/superset/commands/dashboard/update.py @@ -27,6 +27,7 @@ from superset.commands.base import BaseCommand, UpdateMixin from superset.commands.dashboard.exceptions import ( DashboardForbiddenError, DashboardInvalidError, + DashboardNativeFiltersUpdateFailedError, DashboardNotFoundError, DashboardSlugExistsValidationError, DashboardUpdateFailedError, @@ -67,7 +68,6 @@ class UpdateDashboardCommand(UpdateMixin, BaseCommand): dashboard, data=json.loads(self._properties.get("json_metadata", "{}")), ) - return dashboard def validate(self) -> None: @@ -187,3 +187,18 @@ class UpdateDashboardCommand(UpdateMixin, BaseCommand): deleted_tabs = find_deleted_tabs() reports = find_reports_containing_tabs(deleted_tabs) deactivate_reports(reports) + + +class UpdateDashboardNativeFiltersCommand(UpdateDashboardCommand): + @transaction( + on_error=partial(on_error, reraise=DashboardNativeFiltersUpdateFailedError) + ) + def run(self) -> Model: + super().validate() + assert self._model + + dashboard = DashboardDAO.update_native_filters_config( + self._model, self._properties + ) + + return dashboard diff --git a/superset/constants.py b/superset/constants.py index d233f271c6..f6cf2a1157 100644 --- a/superset/constants.py +++ b/superset/constants.py @@ -173,6 +173,7 @@ MODEL_API_RW_METHOD_PERMISSION_MAP = { "columnar_metadata": "columnar_upload", "csv_metadata": "csv_upload", "slack_channels": "write", + "put_filters": "write", } EXTRA_FORM_DATA_APPEND_KEYS = { diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py index 8196c197b2..1f1f0fc015 100644 --- a/superset/daos/dashboard.py +++ b/superset/daos/dashboard.py @@ -28,6 +28,7 @@ from superset.commands.dashboard.exceptions import ( DashboardAccessDeniedError, DashboardForbiddenError, DashboardNotFoundError, + DashboardUpdateFailedError, ) from superset.daos.base import BaseDAO from superset.dashboards.filters import DashboardAccessFilter, is_uuid @@ -318,6 +319,69 @@ class DashboardDAO(BaseDAO[Dashboard]): db.session.add(dash) return dash + @classmethod + def update_native_filters_config( + cls, + dashboard: Dashboard | None = None, + attributes: dict[str, Any] | None = None, + ) -> Dashboard: + if not dashboard: + raise DashboardUpdateFailedError("Dashboard not found") + + if attributes: + metadata = json.loads(dashboard.json_metadata or "{}") + native_filter_configuration = metadata.get( + "native_filter_configuration", [] + ) + updated_configuration = [] + + # Modify / Delete existing filters + for conf in native_filter_configuration: + deleted_filter = next( + (f for f in attributes.get("deleted", []) if f == conf.get("id")), + None, + ) + if deleted_filter: + continue + + modified_filter = next( + ( + f + for f in attributes.get("modified", []) + if f.get("id") == conf.get("id") + ), + None, + ) + if modified_filter: + updated_configuration.append(modified_filter) + else: + updated_configuration.append(conf) + + # Append new filters + for new_filter in attributes.get("modified", []): + if new_filter.get("id") not in [ + f.get("id") for f in updated_configuration + ]: + updated_configuration.append(new_filter) + + # Reorder filters + reordered_filter_ids = attributes.get("reordered", []) + if reordered_filter_ids: + filter_map = { + filter_config["id"]: filter_config + for filter_config in updated_configuration + } + updated_configuration = [ + filter_map[filter_id] + for filter_id in reordered_filter_ids + if filter_id in filter_map + ] + + metadata["native_filter_configuration"] = updated_configuration + dashboard.json_metadata = json.dumps(metadata) + + return dashboard + @staticmethod def add_favorite(dashboard: Dashboard) -> None: ids = DashboardDAO.favorited_ids([dashboard]) diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index cb7e30ef02..2f510f0380 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -47,6 +47,7 @@ from superset.commands.dashboard.exceptions import ( DashboardDeleteFailedError, DashboardForbiddenError, DashboardInvalidError, + DashboardNativeFiltersUpdateFailedError, DashboardNotFoundError, DashboardUpdateFailedError, ) @@ -55,7 +56,10 @@ from superset.commands.dashboard.fave import AddFavoriteDashboardCommand from superset.commands.dashboard.importers.dispatcher import ImportDashboardsCommand from superset.commands.dashboard.permalink.create import CreateDashboardPermalinkCommand from superset.commands.dashboard.unfave import DelFavoriteDashboardCommand -from superset.commands.dashboard.update import UpdateDashboardCommand +from superset.commands.dashboard.update import ( + UpdateDashboardCommand, + UpdateDashboardNativeFiltersCommand, +) from superset.commands.database.exceptions import DatasetValidationError from superset.commands.exceptions import TagForbiddenError from superset.commands.importers.exceptions import NoValidFilesFoundError @@ -80,6 +84,7 @@ from superset.dashboards.schemas import ( DashboardCopySchema, DashboardDatasetSchema, DashboardGetResponseSchema, + DashboardNativeFiltersConfigUpdateSchema, DashboardPostSchema, DashboardPutSchema, EmbeddedDashboardConfigSchema, @@ -175,6 +180,7 @@ class DashboardRestApi(BaseSupersetModelRestApi): "copy_dash", "cache_dashboard_screenshot", "screenshot", + "put_filters", } resource_name = "dashboard" allow_browser_login = True @@ -262,6 +268,7 @@ class DashboardRestApi(BaseSupersetModelRestApi): add_model_schema = DashboardPostSchema() edit_model_schema = DashboardPutSchema() + update_filters_model_schema = DashboardNativeFiltersConfigUpdateSchema() chart_entity_response_schema = ChartEntityResponseSchema() dashboard_get_response_schema = DashboardGetResponseSchema() dashboard_dataset_schema = DashboardDatasetSchema() @@ -675,6 +682,94 @@ class DashboardRestApi(BaseSupersetModelRestApi): response = self.response_422(message=str(ex)) return response + @expose("/<pk>/filters", methods=("PUT",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put", + log_to_statsd=False, + ) + @requires_json + def put_filters(self, pk: int) -> Response: + """ + Modify native filters configuration for a dashboard. + --- + put: + summary: Update native filters configuration for a dashboard. + parameters: + - in: path + schema: + type: integer + name: pk + requestBody: + description: Native filters configuration + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/{{self.__class__.__name__}}.put' + responses: + 200: + description: Dashboard native filters updated + content: + application/json: + schema: + type: object + properties: + id: + type: number + result: + $ref: '#/components/schemas/{{self.__class__.__name__}}.put' + last_modified_time: + type: number + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + try: + item = self.update_filters_model_schema.load(request.json, partial=True) + except ValidationError as error: + return self.response_400(message=error.messages) + + try: + changed_model = UpdateDashboardNativeFiltersCommand(pk, item).run() + last_modified_time = changed_model.changed_on.replace( + microsecond=0 + ).timestamp() + response = self.response( + 200, + id=changed_model.id, + result=item, + last_modified_time=last_modified_time, + ) + except DashboardNotFoundError: + response = self.response_404() + except DashboardForbiddenError: + response = self.response_403() + except TagForbiddenError as ex: + response = self.response(403, message=str(ex)) + except DashboardInvalidError as ex: + return self.response_422(message=ex.normalized_messages()) + except DashboardNativeFiltersUpdateFailedError as ex: + logger.error( + "Error changing native filters for dashboard %s: %s", + self.__class__.__name__, + str(ex), + exc_info=True, + ) + response = self.response_422(message=str(ex)) + return response + @expose("/<pk>", methods=("DELETE",)) @protect() @safe diff --git a/superset/dashboards/schemas.py b/superset/dashboards/schemas.py index b0a47aba41..8f6c56738c 100644 --- a/superset/dashboards/schemas.py +++ b/superset/dashboards/schemas.py @@ -398,6 +398,12 @@ class DashboardPutSchema(BaseDashboardSchema): ) +class DashboardNativeFiltersConfigUpdateSchema(BaseDashboardSchema): + deleted = fields.List(fields.String(), allow_none=False) + modified = fields.List(fields.Raw(), allow_none=False) + reordered = fields.List(fields.String(), allow_none=False) + + class DashboardScreenshotPostSchema(Schema): dataMask = fields.Dict( keys=fields.Str(), diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index f88bdccc0e..b6d7dc2cad 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -85,6 +85,15 @@ class TestDashboardApi(ApiOwnersTestCaseMixin, InsertChartMixin, SupersetTestCas "published": False, } + dashboard_put_filters_data = { + "modified": [ + {"id": "native_filter_1", "name": "Filter 1"}, + {"id": "native_filter_2", "name": "Filter 2"}, + ], + "deleted": [], + "reordered": [], + } + @pytest.fixture() def create_dashboards(self): with self.create_app().app_context(): @@ -1719,6 +1728,172 @@ class TestDashboardApi(ApiOwnersTestCaseMixin, InsertChartMixin, SupersetTestCas db.session.delete(model) db.session.commit() + def test_add_dashboard_filters(self): + """ + Dashboard API: Test that a filter was added + """ + admin = self.get_user("admin") + admin_role = self.get_role("Admin") + dashboard_id = self.insert_dashboard( + "title1", "slug1", [admin.id], roles=[admin_role.id] + ).id + self.login(ADMIN_USERNAME) + uri = f"api/v1/dashboard/{dashboard_id}/filters" + rv = self.put_assert_metric(uri, self.dashboard_put_filters_data, "put_filters") + self.assertEqual(rv.status_code, 200) + model = db.session.query(Dashboard).get(dashboard_id) + json_metadata = model.json_metadata + native_filter_config = json.loads(json_metadata)["native_filter_configuration"] + + self.assertEqual(native_filter_config[0]["name"], "Filter 1") + db.session.delete(model) + db.session.commit() + + def test_modify_dashboard_filters_values(self): + """ + Dashboard API: Test that a filter was added + """ + admin = self.get_user("admin") + admin_role = self.get_role("Admin") + json_metadata = { + "native_filter_configuration": [ + { + "id": "native_filter_1", + "name": "Filter X", + "filterType": "filter_select", + "cascadeParentIds": [], + } + ] + } + dashboard_id = self.insert_dashboard( + "title1", + "slug1", + [admin.id], + roles=[admin_role.id], + json_metadata=json.dumps(json_metadata), + ).id + self.login(ADMIN_USERNAME) + uri = f"api/v1/dashboard/{dashboard_id}/filters" + rv = self.put_assert_metric(uri, self.dashboard_put_filters_data, "put_filters") + + self.assertEqual(rv.status_code, 200) + model = db.session.query(Dashboard).get(dashboard_id) + json_metadata = model.json_metadata + native_filter_config = json.loads(json_metadata)["native_filter_configuration"] + + self.assertEqual(native_filter_config[0]["name"], "Filter 1") + db.session.delete(model) + db.session.commit() + + def test_modfify_dashboard_filters_order(self): + """ + Dashboard API: Test filters reordered + """ + admin = self.get_user("admin") + admin_role = self.get_role("Admin") + json_metadata = { + "native_filter_configuration": [ + { + "id": "native_filter_1", + "name": "Filter 1", + "filterType": "filter_select", + "cascadeParentIds": [], + }, + { + "id": "native_filter_2", + "name": "Filter 2", + "filterType": "filter_select", + "cascadeParentIds": [], + }, + ] + } + dashboard_id = self.insert_dashboard( + "title1", + "slug1", + [admin.id], + roles=[admin_role.id], + json_metadata=json.dumps(json_metadata), + ).id + self.login(ADMIN_USERNAME) + uri = f"api/v1/dashboard/{dashboard_id}/filters" + put_data = { + **self.dashboard_put_filters_data, + "reordered": ["native_filter_2", "native_filter_1"], + } + rv = self.put_assert_metric(uri, put_data, "put_filters") + self.assertEqual(rv.status_code, 200) + model = db.session.query(Dashboard).get(dashboard_id) + json_metadata = model.json_metadata + native_filter_config = json.loads(json_metadata)["native_filter_configuration"] + + self.assertEqual(native_filter_config[0]["name"], "Filter 2") + db.session.delete(model) + db.session.commit() + + def test_dashboard_filters_deleted(self): + """ + Dashboard API: Test filters deleted + """ + admin = self.get_user("admin") + admin_role = self.get_role("Admin") + json_metadata = { + "native_filter_configuration": [ + { + "id": "native_filter_1", + "name": "Filter 1", + "filterType": "filter_select", + "cascadeParentIds": [], + }, + { + "id": "native_filter_2", + "name": "Filter 2", + "filterType": "filter_select", + "cascadeParentIds": [], + }, + ] + } + dashboard_id = self.insert_dashboard( + "title1", + "slug1", + [admin.id], + roles=[admin_role.id], + json_metadata=json.dumps(json_metadata), + ).id + self.login(ADMIN_USERNAME) + uri = f"api/v1/dashboard/{dashboard_id}/filters" + put_data = { + **self.dashboard_put_filters_data, + "deleted": ["native_filter_1"], + } + rv = self.put_assert_metric(uri, put_data, "put_filters") + self.assertEqual(rv.status_code, 200) + model = db.session.query(Dashboard).get(dashboard_id) + json_metadata = model.json_metadata + native_filter_config = json.loads(json_metadata)["native_filter_configuration"] + + self.assertEqual(native_filter_config[0]["name"], "Filter 2") + db.session.delete(model) + db.session.commit() + + def test_modify_dashboard_filters_invalid_data(self): + """ + Dashboard API: Test modify filters with invalid data + """ + admin = self.get_user("admin") + admin_role = self.get_role("Admin") + dashboard_id = self.insert_dashboard( + "title1", "slug1", [admin.id], roles=[admin_role.id] + ).id + self.login(ADMIN_USERNAME) + uri = f"api/v1/dashboard/{dashboard_id}/filters" + put_data = {"invalid_key": "invalid_value"} + rv = self.put_assert_metric(uri, put_data, "put_filters") + self.assertEqual(rv.status_code, 400) + + model = db.session.query(Dashboard).get(dashboard_id) + db.session.delete(model) + db.session.commit() + def test_dashboard_get_list_no_username(self): """ Dashboard API: Tests that no username is returned
