Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Removing parent filter causes incorrect state of child filter #16876

Merged
merged 4 commits into from
Sep 29, 2021
Merged

fix: Removing parent filter causes incorrect state of child filter #16876

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60c1ada
fix: Removing parent filter causes incorrect state of child filter
michael-s-molina Sep 28, 2021
ca3bf96
removed to isRemoved
michael-s-molina Sep 29, 2021
6fb7c53
Shows (deleted) when the parent is removed
michael-s-molina Sep 29, 2021
de0bc2d
Removes unnecessary code
michael-s-molina Sep 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...nents/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ describe('FilterScope', () => {
restoreFilter: jest.fn(),
parentFilters: [],
save,
removedFilters: {},
};

const MockModal = ({ scope }: { scope?: object }) => {
Expand Down
88 changes: 56 additions & 32 deletions ...board/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -62,6 +62,7 @@ import Icons from 'src/components/Icons';
import { Tooltip } from 'src/components/Tooltip';
import { Radio } from 'src/components/Radio';
import BasicErrorAlert from 'src/components/ErrorMessage/BasicErrorAlert';
import { usePrevious } from 'src/common/hooks/usePrevious';
import {
Chart,
ChartsState,
Expand All @@ -70,7 +71,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,
Expand Down Expand Up @@ -264,7 +265,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 }[];
Expand Down Expand Up @@ -300,21 +301,22 @@ const FiltersConfigForm = (
{
filterId,
filterToEdit,
removed,
removedFilters,
restoreFilter,
form,
parentFilters,
}: FiltersConfigFormProps,
ref: React.RefObject<any>,
) => {
const removed = !!removedFilters[filterId];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: isRemoved

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
Expand All @@ -324,6 +326,31 @@ const FiltersConfigForm = (
const defaultFormFilter = useMemo(() => ({}), []);
const formValues = form.getFieldValue('filters')?.[filterId];
const formFilter = formValues || undoFormValues || defaultFormFilter;
const previousRemovedFilters = usePrevious(removedFilters);

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 [isHierarchical, setIsHierarchical] = useState(!!parentFilter);

useEffect(() => {
if (!isEqual(removedFilters, previousRemovedFilters)) {
setIsHierarchical(parentId && !removedFilters[parentId]);
}
}, [parentId, previousRemovedFilters, removedFilters]);

const nativeFilterItems = getChartMetadataRegistry().items;
const nativeFilterVizTypes = Object.entries(nativeFilterItems)
Expand Down Expand Up @@ -506,19 +533,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 ||
Expand Down Expand Up @@ -583,13 +597,6 @@ 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
Expand Down Expand Up @@ -647,12 +654,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 =
isHierarchical ||
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,
isHierarchical,
hasPreFilter,
hasSorting,
controlItems,
]);

const initiallyExcludedCharts = useMemo(() => {
const excluded: number[] = [];
Expand Down Expand Up @@ -915,8 +938,9 @@ const FiltersConfigForm = (
{isCascadingFilter && (
<CollapsibleControl
title={t('Filter is hierarchical')}
initialValue={hasParentFilter}
checked={isHierarchical}
onChange={checked => {
setIsHierarchical(checked);
formChanged();
if (checked) {
// execute after render
Expand Down
14 changes: 13 additions & 1 deletion ...frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,17 @@ export function FiltersConfigModal({
title: getFilterTitle(id),
}));

const cleanDeletedParents = () => {
// Clean the deleted parents
Object.keys(filterConfigMap).forEach(key => {
const filter = filterConfigMap[key];
const parentId = filter.cascadeParentIds?.[0];
if (parentId && removedFilters[parentId]) {
filter.cascadeParentIds = [];
}
});
};

const handleSave = async () => {
const values: NativeFiltersForm | null = await validateForm(
form,
Expand All @@ -206,6 +217,7 @@ export function FiltersConfigModal({
);

if (values) {
cleanDeletedParents();
createHandleSave(
filterConfigMap,
filterIds,
Expand Down Expand Up @@ -288,7 +300,7 @@ export function FiltersConfigModal({
form={form}
filterId={id}
filterToEdit={filterConfigMap[id]}
removed={!!removedFilters[id]}
removedFilters={removedFilters}
restoreFilter={restoreFilter}
parentFilters={getParentFilters(id)}
/>
Expand Down