codeant-ai-for-open-source[bot] commented on code in PR #36012: URL: https://github.com/apache/superset/pull/36012#discussion_r2766392841
########## 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 Review Comment: **Suggestion:** The test currently asserts that a numeric default value of 0 is invalid for non-range filters, but 0 is a perfectly valid value for numeric filter defaults; encoding this as invalid in tests will force `isValidFilterValue` to reject legitimate defaults and cause the UI to show a validation error when a user picks 0. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Numeric default value 0 rejected in filter configuration. - ⚠️ Users see validation error when selecting 0. - ⚠️ Filter creation UX blocked for zero-valued defaults. ``` </details> ```suggestion // 0 is a valid numeric default and should be accepted expect(isValidFilterValue(0, false)).toBe(true); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the test file at superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.test.ts and locate the number-value test at lines 247-250 which asserts 0 is invalid. 2. Run unit tests: npm run test -- src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.test.ts. The test suite will execute the assertion in that test (line 247) which expects isValidFilterValue(0, false) to be false. 3. If production code uses the same validator implementation from ./utils (imported at top of the test file), a legitimate numeric default value of 0 passed from the UI (e.g., FiltersConfigForm default-value input) would be treated as invalid, causing the form to display the "Please choose a valid value" error when a user selects 0. 4. Reproducing in the running app: open FiltersConfigForm, set a numeric default of 0 for a non-range filter and enable "has default value" — the validator path defined in src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts (imported by the form and tested here) would reject 0, producing an unexpected validation error in the UI. Note: This test asserts current behavior; changing the test to accept 0 documents the intended (correct) behavior that numeric 0 is a valid default. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.test.ts **Line:** 249:249 **Comment:** *Logic Error: The test currently asserts that a numeric default value of 0 is invalid for non-range filters, but 0 is a perfectly valid value for numeric filter defaults; encoding this as invalid in tests will force `isValidFilterValue` to reject legitimate defaults and cause the UI to show a validation error when a user picks 0. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts: ########## @@ -78,13 +78,38 @@ 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) || FILTER_SUPPORTED_TYPES[ 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); Review Comment: **Suggestion:** The range-filter branch of `isValidFilterValue` treats an empty array or an array of `undefined` values as a valid default (because `undefined !== null` is true), so a semantically "no value" range like `[]` or `[undefined, undefined]` will pass validation and avoid showing the "Please choose a valid value" error even though no actual bounds are set; adding a proper length check and treating both `null` and `undefined` as empty fixes this logic error. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Native filter range default validation in FiltersConfigModal. - ⚠️ Pre-filter default behavior for range filters (dashboard UX). - ⚠️ Unit tests around isValidFilterValue (utils.test.ts). ``` </details> ```suggestion if (!Array.isArray(value) || value.length < 2) { return false; } const [start, end] = value; // Treat both null and undefined as "no bound" return start != null || end != null; ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the FiltersConfigModal UI flow that configures a range-type native filter (entry point: user opens filter edit modal). The modal code path calls validation helpers in FiltersConfigForm.tsx — see call site isValidFilterValue referenced at `superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:1570`. 2. In the modal, enable the "Has default value" option for a range filter and leave both bounds empty (no inputs). The form code will assemble the default value and pass it to isValidFilterValue for validation. 3. The current implementation at `.../FiltersConfigForm/utils.ts:98-111` executes the range branch: it checks Array.isArray(value) and then tests `value[0] !== null || value[1] !== null`. For an empty array (`[]`) or `[undefined, undefined]`, Array.isArray may be true but accessing elements yields `undefined !== null` which is true — causing the function to return true and treat the default as valid. 4. Observe that the modal accepts the effectively-empty default and does not show the expected "Please choose a valid value" validation error. This behavior is covered by unit tests around these lines in `.../utils.test.ts` (see tests asserting range behavior at `superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.test.ts:264-277`) — the failing/incorrect case ([], [undefined, undefined]) is not currently asserted and therefore passes unnoticed. 5. Applying the improved code (length check + treating undefined as no-bound) changes step 3: an empty array or arrays with missing elements fail early (length < 2) or both undefined/null bounds are treated as empty, causing the UI validation to show the expected error and preventing saving an empty range default. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts **Line:** 103:103 **Comment:** *Logic Error: The range-filter branch of `isValidFilterValue` treats an empty array or an array of `undefined` values as a valid default (because `undefined !== null` is true), so a semantically "no value" range like `[]` or `[undefined, undefined]` will pass validation and avoid showing the "Please choose a valid value" error even though no actual bounds are set; adding a proper length check and treating both `null` and `undefined` as empty fixes this logic error. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## 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); +}); + Review Comment: **Suggestion:** There is no test for boolean defaults, and the current `isValidFilterValue` logic (truthiness check) will treat `false` as invalid even though `false` is a legitimate default for boolean columns, so adding an explicit test that expects `false` to be valid prevents regressions where boolean defaults are wrongly rejected. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Boolean default false rejected in filter configuration. - ⚠️ Users cannot set false as default for boolean filters. - ⚠️ CI may miss regressions without explicit boolean test. ``` </details> ```suggestion test('isValidFilterValue returns true for boolean false value (non-range filter)', () => { // false is a valid default for boolean filters and should not be treated as missing expect(isValidFilterValue(false, false)).toBe(true); }); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Inspect the test file at superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.test.ts and observe the array-value test at lines 243-245; there is no boolean-false test present. 2. Run the test suite: npm run test -- src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.test.ts. The suite will not exercise the case isValidFilterValue(false, false). 3. In the application, when configuring a boolean filter default value to false in FiltersConfigForm, the validator in ./utils (imported at top of the test file) could treat false as falsy/missing if implementation uses a plain truthiness check, causing the UI to show an invalid default error. 4. Adding the suggested test (expect(isValidFilterValue(false, false)).toBe(true)) directly exercises and pins the intended behavior so regressions (where false becomes invalid) are detected by CI. Note: This reproduction ties the missing test case to a real UI flow — setting a boolean default in FiltersConfigForm — and demonstrates how absence of coverage allows regressions. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.test.ts **Line:** 246:246 **Comment:** *Logic Error: There is no test for boolean defaults, and the current `isValidFilterValue` logic (truthiness check) will treat `false` as invalid even though `false` is a legitimate default for boolean columns, so adding an explicit test that expects `false` to be valid prevents regressions where boolean defaults are wrongly rejected. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> -- 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]
