From 2e463078a2c48a1fe3b00a8ccb76a03c303a809b Mon Sep 17 00:00:00 2001 From: Joe Li Date: Thu, 5 Feb 2026 10:48:55 -0800 Subject: [PATCH] refactor(filters): extract shouldShowTimeRangePicker and improve test coverage (#36012) Co-authored-by: Claude Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../FiltersConfigForm/FiltersConfigForm.tsx | 11 +- .../FiltersConfigForm/utils.test.ts | 278 ++++++++++++++++++ .../FiltersConfigForm/utils.ts | 25 ++ .../FiltersConfigModal.test.tsx | 161 +++++----- 4 files changed, 393 insertions(+), 82 deletions(-) create mode 100644 superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.test.ts 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 07d31932758..a10f2702451 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -103,8 +103,10 @@ import RemovedFilter from './RemovedFilter'; import { useBackendFormUpdate, useDefaultValue } from './state'; import { hasTemporalColumns, + isValidFilterValue, mostUsedDataset, setNativeFilterFieldValues, + shouldShowTimeRangePicker, useForceUpdate, } from './utils'; import { @@ -355,8 +357,7 @@ const FiltersConfigForm = ( const currentDataset = Object.values(loadedDatasets).find( dataset => dataset.id === formFilter?.dataset?.value, ); - - return currentDataset ? hasTemporalColumns(currentDataset) : true; + return shouldShowTimeRangePicker(currentDataset); }, [formFilter?.dataset?.value, loadedDatasets]); const itemTypeField = @@ -818,12 +819,6 @@ const FiltersConfigForm = ( /> ); - const isValidFilterValue = (value: unknown, isRangeFilter: boolean) => { - if (isRangeFilter) { - return Array.isArray(value) && (value[0] !== null || value[1] !== null); - } - return !!value; - }; return ( [0]; + +const createDataset = ( + columnTypes: GenericDataType[] | undefined, +): DatasetParam => ({ column_types: columnTypes }) as DatasetParam; + +// Typed fixture helpers for mostUsedDataset tests +const createDatasourcesState = ( + entries: Array<{ key: string; id: number }>, +): DatasourcesState => + Object.fromEntries( + entries.map(({ key, id }) => [key, { id } as Partial]), + ) as DatasourcesState; + +const createChartsState = ( + entries: Array<{ key: string; datasource?: string }>, +): ChartsState => + Object.fromEntries( + entries.map(({ key, datasource }) => [ + key, + datasource !== undefined + ? ({ form_data: { datasource } } as Partial) + : ({} as Partial), + ]), + ) as ChartsState; + +// Typed fixture helper for doesColumnMatchFilterType tests +const createColumn = ( + column_name: string, + type_generic?: GenericDataType, +): Column => ({ column_name, type_generic }) as Column; + +test('hasTemporalColumns returns true when column_types is undefined (precautionary default)', () => { + const dataset = createDataset(undefined); + expect(hasTemporalColumns(dataset)).toBe(true); +}); + +test('hasTemporalColumns returns true when column_types is empty array (precautionary default)', () => { + const dataset = createDataset([]); + expect(hasTemporalColumns(dataset)).toBe(true); +}); + +test('hasTemporalColumns returns true when column_types includes Temporal', () => { + const dataset = createDataset([ + GenericDataType.String, + GenericDataType.Temporal, + GenericDataType.Numeric, + ]); + expect(hasTemporalColumns(dataset)).toBe(true); +}); + +test('hasTemporalColumns returns true when column_types is only Temporal', () => { + const dataset = createDataset([GenericDataType.Temporal]); + expect(hasTemporalColumns(dataset)).toBe(true); +}); + +test('hasTemporalColumns returns false when column_types has no Temporal columns', () => { + const dataset = createDataset([ + GenericDataType.String, + GenericDataType.Numeric, + ]); + expect(hasTemporalColumns(dataset)).toBe(false); +}); + +test('hasTemporalColumns returns false when column_types has only Numeric columns', () => { + const dataset = createDataset([GenericDataType.Numeric]); + expect(hasTemporalColumns(dataset)).toBe(false); +}); + +test('hasTemporalColumns returns false when column_types has only String columns', () => { + const dataset = createDataset([GenericDataType.String]); + expect(hasTemporalColumns(dataset)).toBe(false); +}); + +test('hasTemporalColumns returns false when column_types has Boolean but no Temporal', () => { + const dataset = createDataset([ + GenericDataType.Boolean, + GenericDataType.String, + ]); + expect(hasTemporalColumns(dataset)).toBe(false); +}); + +test('hasTemporalColumns handles null dataset gracefully', () => { + // @ts-expect-error testing null input + expect(hasTemporalColumns(null)).toBe(true); +}); + +// Test shouldShowTimeRangePicker - wrapper function used by FiltersConfigForm +// to determine if time range picker should be displayed in pre-filter settings + +test('shouldShowTimeRangePicker returns true when dataset is undefined (precautionary default)', () => { + expect(shouldShowTimeRangePicker(undefined)).toBe(true); +}); + +test('shouldShowTimeRangePicker returns true when dataset has temporal columns', () => { + const dataset = createDataset([ + GenericDataType.String, + GenericDataType.Temporal, + ]); + expect(shouldShowTimeRangePicker(dataset)).toBe(true); +}); + +test('shouldShowTimeRangePicker returns false when dataset has no temporal columns', () => { + const dataset = createDataset([ + GenericDataType.String, + GenericDataType.Numeric, + ]); + expect(shouldShowTimeRangePicker(dataset)).toBe(false); +}); + +// Test mostUsedDataset - finds the dataset used by the most charts +// Used to pre-select dataset when creating new filters + +test('mostUsedDataset returns the dataset ID used by most charts', () => { + const datasets = createDatasourcesState([ + { key: '7__table', id: 7 }, + { key: '8__table', id: 8 }, + ]); + const charts = createChartsState([ + { key: '1', datasource: '7__table' }, + { key: '2', datasource: '7__table' }, + { key: '3', datasource: '8__table' }, + ]); + expect(mostUsedDataset(datasets, charts)).toBe(7); +}); + +test('mostUsedDataset returns undefined when charts is empty', () => { + const datasets = createDatasourcesState([{ key: '7__table', id: 7 }]); + const charts = createChartsState([]); + expect(mostUsedDataset(datasets, charts)).toBeUndefined(); +}); + +test('mostUsedDataset returns undefined when dataset not in datasets map', () => { + const datasets = createDatasourcesState([]); + const charts = createChartsState([{ key: '1', datasource: '7__table' }]); + expect(mostUsedDataset(datasets, charts)).toBeUndefined(); +}); + +test('mostUsedDataset skips charts without form_data', () => { + const datasets = createDatasourcesState([{ key: '7__table', id: 7 }]); + // Charts without datasource are created without form_data + const charts = createChartsState([ + { key: '1', datasource: '7__table' }, + { key: '2' }, // No form_data + { key: '3' }, // No form_data + ]); + expect(mostUsedDataset(datasets, charts)).toBe(7); +}); + +test('mostUsedDataset handles single chart correctly', () => { + const datasets = createDatasourcesState([{ key: '8__table', id: 8 }]); + const charts = createChartsState([{ key: '1', datasource: '8__table' }]); + expect(mostUsedDataset(datasets, charts)).toBe(8); +}); + +// Test doesColumnMatchFilterType - validates column compatibility with filter types +// Used to filter column options in the filter configuration UI + +test('doesColumnMatchFilterType returns true when column has no type_generic', () => { + const column = createColumn('name'); + expect(doesColumnMatchFilterType('filter_select', column)).toBe(true); +}); + +test('doesColumnMatchFilterType returns true for unknown filter type', () => { + const column = createColumn('name', GenericDataType.String); + expect(doesColumnMatchFilterType('unknown_filter', column)).toBe(true); +}); + +test('doesColumnMatchFilterType returns true when column type matches filter_select', () => { + const stringColumn = createColumn('name', GenericDataType.String); + const numericColumn = createColumn('count', GenericDataType.Numeric); + const boolColumn = createColumn('active', GenericDataType.Boolean); + expect(doesColumnMatchFilterType('filter_select', stringColumn)).toBe(true); + expect(doesColumnMatchFilterType('filter_select', numericColumn)).toBe(true); + expect(doesColumnMatchFilterType('filter_select', boolColumn)).toBe(true); +}); + +test('doesColumnMatchFilterType returns true when column type matches filter_range', () => { + const numericColumn = createColumn('count', GenericDataType.Numeric); + expect(doesColumnMatchFilterType('filter_range', numericColumn)).toBe(true); +}); + +test('doesColumnMatchFilterType returns false when column type does not match filter_range', () => { + const stringColumn = createColumn('name', GenericDataType.String); + expect(doesColumnMatchFilterType('filter_range', stringColumn)).toBe(false); +}); + +test('doesColumnMatchFilterType returns true when column type matches filter_time', () => { + const temporalColumn = createColumn('created_at', GenericDataType.Temporal); + expect(doesColumnMatchFilterType('filter_time', temporalColumn)).toBe(true); +}); + +test('doesColumnMatchFilterType returns false when column type does not match filter_time', () => { + const stringColumn = createColumn('name', GenericDataType.String); + expect(doesColumnMatchFilterType('filter_time', stringColumn)).toBe(false); +}); + +// Test isValidFilterValue - validates default value field when "has default value" is enabled +// This is the validation logic used by FiltersConfigForm to show "Please choose a valid value" error + +test('isValidFilterValue returns true for non-empty string value (non-range filter)', () => { + expect(isValidFilterValue('some value', false)).toBe(true); +}); + +test('isValidFilterValue returns true for non-empty array value (non-range filter)', () => { + expect(isValidFilterValue(['option1', 'option2'], false)).toBe(true); +}); + +test('isValidFilterValue returns true for number value (non-range filter)', () => { + expect(isValidFilterValue(42, false)).toBe(true); + expect(isValidFilterValue(0, false)).toBe(false); // 0 is falsy +}); + +test('isValidFilterValue returns false for empty/null/undefined (non-range filter)', () => { + expect(isValidFilterValue('', false)).toBe(false); + expect(isValidFilterValue(null, false)).toBe(false); + expect(isValidFilterValue(undefined, false)).toBe(false); +}); + +test('isValidFilterValue returns false for empty array (non-range filter)', () => { + // For multi-select filters, [] means "no selection was made" + // This should be invalid when "has default value" is enabled + expect(isValidFilterValue([], false)).toBe(false); +}); + +test('isValidFilterValue returns true when range filter has at least one non-null value', () => { + expect(isValidFilterValue([1, 10], true)).toBe(true); + expect(isValidFilterValue([1, null], true)).toBe(true); + expect(isValidFilterValue([null, 10], true)).toBe(true); +}); + +test('isValidFilterValue returns false when range filter has both values null', () => { + expect(isValidFilterValue([null, null], true)).toBe(false); +}); + +test('isValidFilterValue returns false when range filter value is not an array', () => { + expect(isValidFilterValue('not an array', true)).toBe(false); + expect(isValidFilterValue(null, true)).toBe(false); + expect(isValidFilterValue(undefined, true)).toBe(false); +}); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts index 452e93c47f5..82c3319660e 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts @@ -78,6 +78,12 @@ export const hasTemporalColumns = ( ); }; +// Determines whether to show the time range picker in pre-filter settings. +// Returns true if dataset is undefined (precautionary default) or has temporal columns. +export const shouldShowTimeRangePicker = ( + currentDataset: (Dataset & { column_types: GenericDataType[] }) | undefined, +): boolean => (currentDataset ? hasTemporalColumns(currentDataset) : true); + export const doesColumnMatchFilterType = (filterType: string, column: Column) => !column.type_generic || !(filterType in FILTER_SUPPORTED_TYPES) || @@ -85,6 +91,25 @@ export const doesColumnMatchFilterType = (filterType: string, column: Column) => filterType as keyof typeof FILTER_SUPPORTED_TYPES ]?.includes(column.type_generic); +// Validates that a filter default value is present when the default value option is enabled. +// For range filters, at least one of the two values must be non-null. +// For other filters (e.g., filter_select), the value must be non-empty. +// Arrays must have at least one element (empty array means no selection). +export const isValidFilterValue = ( + value: unknown, + isRangeFilter: boolean, +): boolean => { + if (isRangeFilter) { + return Array.isArray(value) && (value[0] !== null || value[1] !== null); + } + // For multi-select filters, an empty array means no selection was made + if (Array.isArray(value)) { + return value.length > 0; + } + // For other values, check if truthy (note: 0 is falsy but unlikely for non-range filters) + return !!value; +}; + export const mostUsedDataset = ( datasets: DatasourcesState, charts: ChartsState, diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx index e27c90395ae..961a558bb96 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx @@ -69,9 +69,7 @@ const defaultState = () => ({ const noTemporalColumnsState = () => { const state = defaultState(); return { - charts: { - ...state.charts, - }, + ...state, datasources: { ...state.datasources, [datasourceId]: { @@ -126,22 +124,38 @@ const datasetResult = (id: number) => ({ show_columns: ['id', 'table_name'], }); -fetchMock.get('glob:*/api/v1/dataset/1', datasetResult(1)); -fetchMock.get(`glob:*/api/v1/dataset/${id}`, datasetResult(id)); +function setupFetchMocks() { + fetchMock.get(`glob:*/api/v1/dataset/${id}`, datasetResult(id)); + // Mock dataset 1 for buildNativeFilter fixtures which use datasetId: 1 + fetchMock.get('glob:*/api/v1/dataset/1', datasetResult(1)); + // Mock the dataset list endpoint for the dataset selector dropdown + // Uses `id` constant (matches mockDatasource.id) for fixture data consistency + fetchMock.get('glob:*/api/v1/dataset/?*', { + result: [ + { + id, + table_name: 'birth_names', + database: { database_name: 'examples' }, + schema: 'public', + }, + ], + count: 1, + }); -fetchMock.post('glob:*/api/v1/chart/data', { - result: [ - { - status: 'success', - data: [ - { name: 'Aaron', count: 453 }, - { name: 'Abigail', count: 228 }, - { name: 'Adam', count: 454 }, - ], - applied_filters: [{ column: 'name' }], - }, - ], -}); + fetchMock.post('glob:*/api/v1/chart/data', { + result: [ + { + status: 'success', + data: [ + { name: 'Aaron', count: 453 }, + { name: 'Abigail', count: 228 }, + { name: 'Adam', count: 454 }, + ], + applied_filters: [{ column: 'name' }], + }, + ], + }); +} const FILTER_TYPE_REGEX = /^filter type$/i; const FILTER_NAME_REGEX = /^filter name$/i; @@ -165,10 +179,8 @@ const SORT_REGEX = /^sort filter values$/i; const SAVE_REGEX = /^save$/i; const NAME_REQUIRED_REGEX = /^name is required$/i; const COLUMN_REQUIRED_REGEX = /^column is required$/i; -const DEFAULT_VALUE_REQUIRED_REGEX = /^default value is required$/i; const PRE_FILTER_REQUIRED_REGEX = /^pre-filter is required$/i; -const FILL_REQUIRED_FIELDS_REGEX = /fill all required fields to enable/; -const TIME_RANGE_PREFILTER_REGEX = /^time range$/i; +const DEFAULT_VALUE_INVALID_REGEX = /choose.*valid value/i; const props: FiltersConfigModalProps = { isOpen: true, @@ -181,13 +193,21 @@ beforeAll(() => { new MainPreset().register(); }); +beforeEach(() => { + setupFetchMocks(); +}); + afterEach(() => { jest.runOnlyPendingTimers(); jest.useRealTimers(); jest.restoreAllMocks(); + fetchMock.removeRoutes(); }); -function defaultRender(initialState: any = defaultState(), modalProps = props) { +function defaultRender( + initialState: ReturnType = defaultState(), + modalProps: FiltersConfigModalProps = props, +) { return render(, { initialState, useDnd: true, @@ -327,67 +347,60 @@ test('validates the column', async () => { ).toBeInTheDocument(); }); -// eslint-disable-next-line jest/no-disabled-tests -test.skip('validates the default value', async () => { - defaultRender(noTemporalColumnsState()); - expect(await screen.findByText('birth_names')).toBeInTheDocument(); - await userEvent.type(screen.getByRole('combobox'), `Column A{Enter}`); - await userEvent.click(getCheckbox(DEFAULT_VALUE_REGEX)); - await waitFor(() => { - expect( - screen.queryByText(FILL_REQUIRED_FIELDS_REGEX), - ).not.toBeInTheDocument(); +// This test validates the "default value" field validation. +// +// LIMITATION: Does not exercise the full dataset/column selection flow. +// With createNewOnOpen: true, the modal renders in a state where form fields +// are visible but selecting dataset/column through async selects requires +// complex setup that proved unreliable in this unit test environment. +// +// What this test covers: +// - Default value checkbox can be enabled +// - Validation error appears when default value is enabled without a value +// - The underlying validation logic (isValidFilterValue) is unit tested in utils.test.ts +// +// What would require E2E testing (tracked in issue #36964): +// - Full flow: open modal → select dataset → select column → enable default value → validate +// - This flow is better tested with Playwright where the full component lifecycle is available +// +// The core validation logic is still covered - this guards against regressions where +// the "Please choose a valid value" error fails to appear when default value is enabled. +test('validates the default value', async () => { + defaultRender(); + // Wait for the default value checkbox to appear + const defaultValueCheckbox = await screen.findByRole('checkbox', { + name: DEFAULT_VALUE_REGEX, }); + // Verify default value error is NOT present before enabling checkbox expect( - await screen.findByText(DEFAULT_VALUE_REQUIRED_REGEX), + screen.queryByText(DEFAULT_VALUE_INVALID_REGEX), + ).not.toBeInTheDocument(); + // Enable default value checkbox without setting a value + await userEvent.click(defaultValueCheckbox); + // Try to save - should show validation error + await userEvent.click(screen.getByRole('button', { name: SAVE_REGEX })); + // Verify validation error appears (actual message is "Please choose a valid value") + expect( + await screen.findByText(DEFAULT_VALUE_INVALID_REGEX, {}, { timeout: 3000 }), ).toBeInTheDocument(); -}); +}, 50000); test('validates the pre-filter value', async () => { - jest.useFakeTimers(); - try { - defaultRender(); + // Use real timers to avoid userEvent + fake timers compatibility issues + defaultRender(); - await userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX)); - await userEvent.click(getCheckbox(PRE_FILTER_REGEX)); - - jest.runAllTimers(); - - await waitFor(() => { - const errorMessages = screen.getAllByText(PRE_FILTER_REQUIRED_REGEX); - expect(errorMessages.length).toBeGreaterThan(0); - }); - } finally { - jest.runOnlyPendingTimers(); - jest.useRealTimers(); - } - - // Wait for validation to complete after timer switch - await waitFor( - () => { - const errorMessages = screen.queryAllByText(PRE_FILTER_REQUIRED_REGEX); - expect(errorMessages.length).toBeGreaterThan(0); - }, - { timeout: 15000 }, - ); -}, 50000); // Slow-running test, increase timeout to 50 seconds. - -// eslint-disable-next-line jest/no-disabled-tests -test.skip("doesn't render time range pre-filter if there are no temporal columns in datasource", async () => { - defaultRender(noTemporalColumnsState()); - await userEvent.click(screen.getByText(DATASET_REGEX)); - await waitFor(async () => { - expect(screen.queryByLabelText('Loading')).not.toBeInTheDocument(); - await userEvent.click(screen.getByText('birth_names')); - }); await userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX)); await userEvent.click(getCheckbox(PRE_FILTER_REGEX)); - await waitFor(() => - expect( - screen.queryByText(TIME_RANGE_PREFILTER_REGEX), - ).not.toBeInTheDocument(), + + // Wait for validation error to appear + await waitFor( + () => { + const errorMessages = screen.getAllByText(PRE_FILTER_REQUIRED_REGEX); + expect(errorMessages.length).toBeGreaterThan(0); + }, + { timeout: 10000 }, ); -}); +}, 50000); // Slow-running test, increase timeout to 50 seconds. test('filters are draggable', async () => { const nativeFilterConfig = [