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(native-filters): fix loop caused by external state handler #14788

Merged
merged 1 commit into from
May 25, 2021
+54 −68
Merged

fix(native-filters): fix loop caused by external state handler #14788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 .../dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ const DefaultValue: FC<DefaultValueProps> = ({
chartType={formFilter?.filterType}
hooks={{ setDataMask }}
enableNoResults={enableNoResults}
filterState={formFilter.defaultDataMask?.filterState}
/>
);
};
Expand Down
121 changes: 53 additions & 68 deletions superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
} from '@superset-ui/core';
import React, { useCallback, useEffect, useReducer, useState } from 'react';
import { Select } from 'src/common/components';
import { debounce } from 'lodash';
import debounce from 'lodash/debounce';
import { SLOW_DEBOUNCE } from 'src/constants';
import { PluginFilterSelectProps, SelectValue } from './types';
import { StyledSelect, Styles } from '../common';
Expand Down Expand Up @@ -91,7 +91,6 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
appSection,
} = props;
const {
defaultValue,
enableEmptyFilter,
multiSelect,
showSearch,
Expand All @@ -100,29 +99,36 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
defaultToFirstItem,
searchAllOptions,
} = formData;
const groupby = ensureIsArray<string>(formData.groupby);
const [col] = groupby;
const [currentSuggestionSearch, setCurrentSuggestionSearch] = useState('');
const [dataMask, dispatchDataMask] = useReducer<DataMaskReducer>(reducer, {
filterState,
ownState: {
coltypeMap,
},
});
Comment on lines +105 to +110
Copy link
Member Author

@villebro villebro May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This was changed to always initialize ownState to make sure the column type mappings are available if the searchAllOptions option is enabled after initialization.

const updateDataMask = (values: SelectValue) => {
const emptyFilter =
enableEmptyFilter && !inverseSelection && !values?.length;

dispatchDataMask({
type: 'filterState',
extraFormData: getSelectExtraFormData(
col,
values,
emptyFilter,
inverseSelection,
),
filterState: {
value: values,
},
});
};

const isDisabled =
appSection === AppSection.FILTER_CONFIG_MODAL && defaultToFirstItem;

const groupby = ensureIsArray<string>(formData.groupby);
// Correct initial value for Ant Select

// If we are in config modal we always need show empty select for `defaultToFirstItem`
const [values, setValues] = useState<SelectValue>(
!isDisabled && defaultValue?.length ? defaultValue : [],
);
Comment on lines -111 to -113
Copy link
Member Author

Choose a reason for hiding this comment

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

state handling is now deferred to the application, i.e. filter state is only being set via setDataMask.

const [currentSuggestionSearch, setCurrentSuggestionSearch] = useState('');
const [dataMask, dispatchDataMask] = useReducer<DataMaskReducer>(
reducer,
searchAllOptions
? {
ownState: {
coltypeMap,
},
}
: {},
);

const debouncedOwnStateFunc = useCallback(
debounce((val: string) => {
dispatchDataMask({
Expand Down Expand Up @@ -154,67 +160,46 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
}
};

useEffect(() => {
const firstItem: SelectValue = data[0]
? (groupby.map(col => data[0][col]) as string[])
: null;
if (!isDisabled && defaultToFirstItem && firstItem) {
// initialize to first value if set to default to first item
setValues(firstItem);
} else if (!isDisabled && defaultValue?.length) {
// initialize to saved value
setValues(defaultValue);
}
}, [defaultToFirstItem, defaultValue]);

Comment on lines -157 to -169
Copy link
Member Author

Choose a reason for hiding this comment

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

This useEffect has been combined with the other one that was checking for changes in controls (e.g. inverse selection etc). Now there are only two useEffects left - one to check for changes in data mask, and another to check for changes in control values.

const handleBlur = () => {
clearSuggestionSearch();
unsetFocusedFilter();
};

const [col] = groupby;
const datatype: GenericDataType = coltypeMap[col];
const labelFormatter = getDataRecordFormatter({
timeFormatter: smartDateDetailedFormatter,
});

const handleChange = (value?: SelectValue | number | string) => {
setValues(ensureIsArray(value));
const values = ensureIsArray(value);
if (values.length === 0) {
updateDataMask(null);
} else {
updateDataMask(values);
}
};

useEffect(() => {
const firstItem: SelectValue = data[0]
? (groupby.map(col => data[0][col]) as string[])
: null;
if (isDisabled) {
setValues([]);
}
}, [isDisabled]);

useEffect(() => {
const emptyFilter =
enableEmptyFilter && !inverseSelection && values?.length === 0;

dispatchDataMask({
type: 'filterState',
extraFormData: getSelectExtraFormData(
col,
values,
emptyFilter,
inverseSelection,
),
filterState: {
// We need to save in state `FIRST_VALUE` as some const and not as REAL value,
// because when FiltersBar check if all filters initialized it compares `defaultValue` with this value
// and because REAL value can be unpredictable for users that have different data for same dashboard we use `FIRST_VALUE`
value: values,
},
});
}, [col, enableEmptyFilter, inverseSelection, JSON.stringify(values)]);

useEffect(() => {
// handle changes coming from application, e.g. "Clear all" button
if (JSON.stringify(values) !== JSON.stringify(filterState.value)) {
handleChange(filterState.value);
// empty selection if filter is disabled
updateDataMask(null);
} else if (!isDisabled && defaultToFirstItem && firstItem) {
// initialize to first value if set to default to first item
updateDataMask(firstItem);
} else {
// reset data mask based on filter state
updateDataMask(filterState.value);
}
}, [JSON.stringify(filterState.value)]);
}, [
col,
isDisabled,
defaultToFirstItem,
enableEmptyFilter,
inverseSelection,
]);

useEffect(() => {
setDataMask(dataMask);
Expand All @@ -229,7 +214,7 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
<StyledSelect
allowClear={!enableEmptyFilter}
// @ts-ignore
value={values}
value={filterState.value || []}
disabled={isDisabled}
showSearch={showSearch}
mode={multiSelect ? 'multiple' : undefined}
Expand All @@ -253,7 +238,7 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
);
})}
{currentSuggestionSearch &&
!ensureIsArray(values).some(
!ensureIsArray(filterState.value).some(
suggestion => suggestion === currentSuggestionSearch,
) && (
<Option value={currentSuggestionSearch}>
Expand Down