Copilot commented on code in PR #43405:
URL: https://github.com/apache/superset/pull/43405#discussion_r3867504401
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -1393,6 +1395,94 @@ test('preserves dependent filter value restored from URL
when it exists in data'
});
});
+test('keeps a dependent filter empty after the user clears it', async () => {
+ // Regression: a dependent filter with "Select first filter value by default"
+ // used to re-apply the first option as soon as the cleared value
round-tripped
+ // through the filter bar, making it impossible to clear.
+ jest.useRealTimers();
+ const setDataMaskMock = jest.fn();
+ const testProps = {
+ ...selectMultipleProps,
+ formData: {
+ ...selectMultipleProps.formData,
+ multiSelect: false,
+ enableEmptyFilter: false,
+ defaultToFirstItem: true,
+ // Non-empty extraFormData is what marks this filter as dependent
+ extraFormData: {
+ filters: [{ col: 'region', op: 'IN', val: ['North America'] }],
+ },
+ },
+ };
+
+ // The filter bar feeds every dispatched dataMask back into the plugin as the
+ // controlled `filterState` prop; the harness reproduces that round-trip.
+ const ControlledSelectFilter = () => {
+ const [filterState, setFilterState] = useState<FilterState>({
+ value: ['boy'],
+ });
+ const handleDataMask = useCallback((dataMask: DataMask) => {
+ setDataMaskMock(dataMask);
+ setFilterState(prev => ({ ...prev, ...dataMask.filterState }));
+ }, []);
+ return (
+ // @ts-expect-error
+ <SelectFilterPlugin
+ // @ts-expect-error
+ {...transformProps({ ...testProps, filterState })}
+ setDataMask={handleDataMask}
+ showOverflow={false}
+ />
+ );
+ };
+
+ render(<ControlledSelectFilter />, {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: {
+ 'test-filter': {
+ name: 'Test Filter',
+ },
+ },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: ['boy'] },
+ },
+ },
+ },
+ });
+
+ userEvent.click(
+ screen.getByRole('img', {
+ name: /close-circle/i,
+ hidden: true,
+ }),
+ );
+
+ await waitFor(() =>
+ expect(setDataMaskMock).toHaveBeenLastCalledWith(
+ expect.objectContaining({
+ extraFormData: {},
+ filterState: expect.objectContaining({ value: null }),
+ }),
+ ),
+ );
+
+ // Let the re-validation effects settle: the value must not come back
+ await act(async () => {
+ await Promise.resolve();
+ });
+ expect(setDataMaskMock).toHaveBeenLastCalledWith(
+ expect.objectContaining({
+ filterState: expect.objectContaining({ value: null }),
+ }),
+ );
+ expect(screen.queryByTitle('boy')).not.toBeInTheDocument();
+});
Review Comment:
This test switches Jest to real timers but never restores the file’s default
fake timers (set at the top of the file). That can leak timer mode into
subsequent tests and make failures order-dependent.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts:
##########
@@ -48,9 +48,10 @@ export const checkIsMissingRequiredValue = (
filter: FilterElement,
filterState?: FilterState,
) => {
- const isRequired =
- !!filter.controlValues?.enableEmptyFilter ||
- !!filter.controlValues?.defaultToFirstItem;
+ // Only `enableEmptyFilter` ("Filter value is required") makes a value
+ // mandatory. `defaultToFirstItem` merely seeds an initial selection, so a
+ // filter that has been cleared by the user must stay clearable and
appliable.
+ const isRequired = !!filter.controlValues?.enableEmptyFilter;
Review Comment:
Typo in the new comment: “appliable” should be “applicable”.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
