From 4da4fe4136a6b2f0687d8a699a472fb7962c5576 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Wed, 29 Sep 2021 13:04:07 -0300 Subject: [PATCH] fix: Removing parent filter causes incorrect state of child filter (#16876) * fix: Removing parent filter causes incorrect state of child filter * removed to isRemoved * Shows (deleted) when the parent is removed * Removes unnecessary code --- .../FilterScope/FilterScope.test.tsx | 1 + .../FiltersConfigForm/FiltersConfigForm.tsx | 128 +++++++++++------- .../FiltersConfigModal/FiltersConfigModal.tsx | 24 +++- .../nativeFilters/FiltersConfigModal/types.ts | 2 +- .../nativeFilters/FiltersConfigModal/utils.ts | 6 +- 5 files changed, 105 insertions(+), 56 deletions(-) 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 e9c35d10cd..cf0c87400d 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 @@ -37,6 +37,7 @@ describe('FilterScope', () => { restoreFilter: jest.fn(), parentFilters: [], save, + removedFilters: {}, }; 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 60badd2466..726560f723 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -44,7 +44,7 @@ import React, { useState, } from 'react'; import { useSelector } from 'react-redux'; -import { isEqual } from 'lodash'; +import { isEqual, isEmpty } from 'lodash'; import { FormItem } from 'src/components/Form'; import { Input } from 'src/common/components'; import { Select } from 'src/components'; @@ -70,7 +70,7 @@ import { } from 'src/dashboard/types'; import Loading from 'src/components/Loading'; import { ColumnSelect } from './ColumnSelect'; -import { NativeFiltersForm } from '../types'; +import { NativeFiltersForm, FilterRemoval } from '../types'; import { FILTER_SUPPORTED_TYPES, hasTemporalColumns, @@ -264,7 +264,7 @@ const FilterPanels = { export interface FiltersConfigFormProps { filterId: string; filterToEdit?: Filter; - removed?: boolean; + removedFilters: Record; restoreFilter: (filterId: string) => void; form: FormInstance; parentFilters: { id: string; title: string }[]; @@ -300,21 +300,22 @@ const FiltersConfigForm = ( { filterId, filterToEdit, - removed, + removedFilters, restoreFilter, form, parentFilters, }: FiltersConfigFormProps, ref: React.RefObject, ) => { + const isRemoved = !!removedFilters[filterId]; const [error, setError] = useState(''); const [metrics, setMetrics] = useState([]); const [activeTabKey, setActiveTabKey] = useState( FilterTabs.configuration.key, ); const [activeFilterPanelKey, setActiveFilterPanelKey] = useState< - string | string[] - >(FilterPanels.basic.key); + string | string[] | undefined + >(); const [undoFormValues, setUndoFormValues] = useState + parentFilters.map(filter => ({ + value: filter.id, + label: filter.title, + })), + [parentFilters], + ); + + const parentId = + formFilter?.parentFilter?.value || filterToEdit?.cascadeParentIds?.[0]; + + const parentFilter = parentFilterOptions.find( + ({ value }) => value === parentId, + ); + + const hasParentFilter = !!parentFilter; + const nativeFilterItems = getChartMetadataRegistry().items; const nativeFilterVizTypes = Object.entries(nativeFilterItems) // @ts-ignore @@ -374,7 +393,7 @@ const FiltersConfigForm = ( filterType: formFilter?.filterType, filterToEdit, formFilter, - removed, + removed: isRemoved, }) : {}; const hasColumn = !!mainControlItems.groupby; @@ -506,19 +525,6 @@ const FiltersConfigForm = ( [filterId, form, formChanged], ); - const parentFilterOptions = parentFilters.map(filter => ({ - value: filter.id, - label: filter.title, - })); - - const parentFilter = parentFilterOptions.find( - ({ value }) => - value === formFilter?.parentFilter?.value || - value === filterToEdit?.cascadeParentIds?.[0], - ); - - const hasParentFilter = !!parentFilter; - const hasPreFilter = !!formFilter?.adhoc_filters || !!formFilter?.time_range || @@ -583,28 +589,32 @@ const FiltersConfigForm = ( return Promise.reject(new Error(t('Pre-filter is required'))); }; - let hasCheckedAdvancedControl = hasParentFilter || hasPreFilter || hasSorting; - if (!hasCheckedAdvancedControl) { - hasCheckedAdvancedControl = Object.keys(controlItems) - .filter(key => !BASIC_CONTROL_ITEMS.includes(key)) - .some(key => controlItems[key].checked); - } - const ParentSelect = ({ value, ...rest }: { value?: { value: string | number }; - }) => ( - + ); + }; useEffect(() => { if (datasetId) { @@ -647,12 +657,28 @@ const FiltersConfigForm = ( ]); useEffect(() => { - const activeFilterPanelKey = [FilterPanels.basic.key]; - if (hasCheckedAdvancedControl) { - activeFilterPanelKey.push(FilterPanels.advanced.key); + // Run only once when the control items are available + if (!activeFilterPanelKey && !isEmpty(controlItems)) { + const hasCheckedAdvancedControl = + hasParentFilter || + hasPreFilter || + hasSorting || + Object.keys(controlItems) + .filter(key => !BASIC_CONTROL_ITEMS.includes(key)) + .some(key => controlItems[key].checked); + setActiveFilterPanelKey( + hasCheckedAdvancedControl + ? [FilterPanels.basic.key, FilterPanels.advanced.key] + : FilterPanels.basic.key, + ); } - setActiveFilterPanelKey(activeFilterPanelKey); - }, [hasCheckedAdvancedControl]); + }, [ + activeFilterPanelKey, + hasParentFilter, + hasPreFilter, + hasSorting, + controlItems, + ]); const initiallyExcludedCharts = useMemo(() => { const excluded: number[] = []; @@ -678,20 +704,20 @@ const FiltersConfigForm = ( useEffect(() => { // just removed, saving current form items for eventual undo - if (removed) { + if (isRemoved) { setUndoFormValues(formValues); } - }, [removed]); + }, [isRemoved]); useEffect(() => { // the filter was just restored after undo - if (undoFormValues && !removed) { + if (undoFormValues && !isRemoved) { setNativeFilterFieldValues(form, filterId, undoFormValues); setUndoFormValues(null); } - }, [formValues, filterId, form, removed, undoFormValues]); + }, [formValues, filterId, form, isRemoved, undoFormValues]); - if (removed) { + if (isRemoved) { return restoreFilter(filterId)} />; } @@ -718,13 +744,13 @@ const FiltersConfigForm = ( name={['filters', filterId, 'name']} label={{t('Filter name')}} initialValue={filterToEdit?.name} - rules={[{ required: !removed, message: t('Name is required') }]} + rules={[{ required: !isRemoved, message: t('Name is required') }]} > {t('Filter Type')}} {...getFiltersConfigModalTestId('filter-type')} @@ -782,7 +808,7 @@ const FiltersConfigForm = ( : undefined } rules={[ - { required: !removed, message: t('Dataset is required') }, + { required: !isRemoved, message: t('Dataset is required') }, ]} {...getFiltersConfigModalTestId('datasource-input')} > @@ -837,7 +863,7 @@ const FiltersConfigForm = ( formChanged(); }} > - {!removed && ( + {!isRemoved && ( { + Object.keys(filterConfigMap).forEach(key => { + const filter = filterConfigMap[key]; + const parentId = filter.cascadeParentIds?.[0]; + if (parentId && removedFilters[parentId]) { + filter.cascadeParentIds = []; + } + }); + + const filters = values?.filters; + if (filters) { + Object.keys(filters).forEach(key => { + const filter = filters[key]; + const parentId = filter.parentFilter?.value; + if (parentId && removedFilters[parentId]) { + filter.parentFilter = undefined; + } + }); + } + }; + const handleSave = async () => { const values: NativeFiltersForm | null = await validateForm( form, @@ -207,6 +228,7 @@ export function FiltersConfigModal({ ); if (values) { + cleanDeletedParents(values); createHandleSave( filterConfigMap, filterIds, @@ -295,7 +317,7 @@ export function FiltersConfigModal({ form={form} filterId={id} filterToEdit={filterConfigMap[id]} - removed={!!removedFilters[id]} + removedFilters={removedFilters} restoreFilter={restoreFilter} parentFilters={getParentFilters(id)} /> diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/types.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/types.ts index 2dba43e93f..246f413bcc 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/types.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/types.ts @@ -36,7 +36,7 @@ export interface NativeFiltersFormItem { }; defaultValue: any; defaultDataMask: DataMask; - parentFilter: { + parentFilter?: { value: string; label: string; }; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts index de990349b1..fd699e9479 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts @@ -65,8 +65,8 @@ export const validateForm = async ( 'Cannot create cyclic hierarchy', ); } - const parentId = formValues.filters[filterId] - ? formValues.filters[filterId].parentFilter?.value + const parentId = formValues.filters?.[filterId] + ? formValues.filters[filterId]?.parentFilter?.value : filterConfigMap[filterId]?.cascadeParentIds?.[0]; if (parentId) { validateCycles(parentId, [...trace, filterId]); @@ -111,7 +111,7 @@ export const createHandleSave = ( .filter(id => !removedFilters[id]) .map(id => { // create a filter config object from the form inputs - const formInputs = values.filters[id]; + const formInputs = values.filters?.[id]; // if user didn't open a filter, return the original config if (!formInputs) return filterConfigMap[id]; const target: Partial = {};