refactor(filters): extract shouldShowTimeRangePicker and improve test coverage (#36012)

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

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 = (
       />
     </StyledRowFormItem>
   );
-  const isValidFilterValue = (value: unknown, isRangeFilter: boolean) => {
-    if (isRangeFilter) {
-      return Array.isArray(value) && (value[0] !== null || value[1] !== null);
-    }
-    return !!value;
-  };
   return (
     <Tabs
       activeKey={activeTabKey}

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.test.ts

@@ -0,0 +1,278 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Column } from '@superset-ui/core';
+import { GenericDataType } from '@apache-superset/core/api/core';
+import {
+  ChartsState,
+  DatasourcesState,
+  Datasource,
+  Chart,
+} from 'src/dashboard/types';
+import {
+  hasTemporalColumns,
+  isValidFilterValue,
+  shouldShowTimeRangePicker,
+  mostUsedDataset,
+  doesColumnMatchFilterType,
+} from './utils';
+
+// Test hasTemporalColumns - validates time range pre-filter visibility logic
+// This addresses the coverage gap from the skipped FiltersConfigModal test
+// "doesn't render time range pre-filter if there are no temporal columns in datasource"
+
+type DatasetParam = Parameters<typeof hasTemporalColumns>[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<Datasource>]),
+  ) 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<Chart>)
+        : ({} as Partial<Chart>),
+    ]),
+  ) 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);
+});

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,

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<typeof defaultState> = defaultState(),
+  modalProps: FiltersConfigModalProps = props,
+) {
   return render(<FiltersConfigModal {...modalProps} />, {
     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 = [