This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 4da4fe4 fix: Removing parent filter causes incorrect state of child
filter (#16876)
4da4fe4 is described below
commit 4da4fe4136a6b2f0687d8a699a472fb7962c5576
Author: Michael S. Molina <[email protected]>
AuthorDate: Wed Sep 29 13:04:07 2021 -0300
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 e9c35d1..cf0c874 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 60badd2..726560f 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<string, FilterRemoval>;
restoreFilter: (filterId: string) => void;
form: FormInstance<NativeFiltersForm>;
parentFilters: { id: string; title: string }[];
@@ -300,21 +300,22 @@ const FiltersConfigForm = (
{
filterId,
filterToEdit,
- removed,
+ removedFilters,
restoreFilter,
form,
parentFilters,
}: FiltersConfigFormProps,
ref: React.RefObject<any>,
) => {
+ const isRemoved = !!removedFilters[filterId];
const [error, setError] = useState<string>('');
const [metrics, setMetrics] = useState<Metric[]>([]);
const [activeTabKey, setActiveTabKey] = useState<string>(
FilterTabs.configuration.key,
);
const [activeFilterPanelKey, setActiveFilterPanelKey] = useState<
- string | string[]
- >(FilterPanels.basic.key);
+ string | string[] | undefined
+ >();
const [undoFormValues, setUndoFormValues] = useState<Record<
string,
any
@@ -325,6 +326,24 @@ const FiltersConfigForm = (
const formValues = form.getFieldValue('filters')?.[filterId];
const formFilter = formValues || undoFormValues || defaultFormFilter;
+ const parentFilterOptions = useMemo(
+ () =>
+ 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 };
- }) => (
- <Select
- ariaLabel={t('Parent filter')}
- placeholder={t('None')}
- options={parentFilterOptions}
- allowClear
- value={value?.value}
- {...rest}
- />
- );
+ }) => {
+ const parentId = value?.value;
+ const isParentRemoved = parentId && removedFilters[parentId];
+ let options = parentFilterOptions;
+ if (isParentRemoved) {
+ options = [
+ { label: t('(deleted)'), value: parentId as string },
+ ...parentFilterOptions,
+ ];
+ }
+ return (
+ <Select
+ ariaLabel={t('Parent filter')}
+ placeholder={t('None')}
+ options={options}
+ allowClear
+ value={parentId}
+ {...rest}
+ />
+ );
+ };
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 <RemovedFilter onClick={() => restoreFilter(filterId)} />;
}
@@ -718,13 +744,13 @@ const FiltersConfigForm = (
name={['filters', filterId, 'name']}
label={<StyledLabel>{t('Filter name')}</StyledLabel>}
initialValue={filterToEdit?.name}
- rules={[{ required: !removed, message: t('Name is required') }]}
+ rules={[{ required: !isRemoved, message: t('Name is required') }]}
>
<Input {...getFiltersConfigModalTestId('name-input')} />
</StyledFormItem>
<StyledFormItem
name={['filters', filterId, 'filterType']}
- rules={[{ required: !removed, message: t('Name is required') }]}
+ rules={[{ required: !isRemoved, message: t('Name is required') }]}
initialValue={filterToEdit?.filterType || 'filter_select'}
label={<StyledLabel>{t('Filter Type')}</StyledLabel>}
{...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 && (
<StyledRowSubFormItem
name={['filters', filterId, 'defaultDataMask']}
initialValue={initialDefaultValue}
diff --git
a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
index d72156d..5dbba2f 100644
---
a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
+++
b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
@@ -196,6 +196,27 @@ export function FiltersConfigModal({
title: getFilterTitle(id),
}));
+ const cleanDeletedParents = (values: NativeFiltersForm | null) => {
+ 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 2dba43e..246f413 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 de99034..fd699e9 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<Target> = {};
