codeant-ai-for-open-source[bot] commented on code in PR #38568: URL: https://github.com/apache/superset/pull/38568#discussion_r2930839385
########## superset-frontend/src/filters/components/TimeGrain/TimeGrainPreFilter.integration.test.tsx: ########## @@ -0,0 +1,141 @@ +/** + * 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. + */ + +/** + * Integration Test: Time Grain Pre-filter Feature (End-to-End) + * + * Tests the full flow: + * 1. Dashboard config: User enables pre-filter and selects allowed time grains + * 2. Dashboard persistence: Config is saved with time_grains array + * 3. Runtime filter: Dashboard displays only the pre-filtered time grains + * + * Note: This documents the expected behavior. Full E2E testing requires + * Playwright/browser tests since it involves dashboard state + filter interactions. + */ + +import { render, screen, userEvent, waitFor } from 'spec/helpers/testing-library'; +import PluginFilterTimegrain from 'src/filters/components/TimeGrain/TimeGrainFilterPlugin'; + +/** + * Scenario: Dashboard owner configures a time grain filter to show only Hour, Day, Week. + * End-user opens the dashboard and can only select from those three options. + */ +test('time grain pre-filter restricts dashboard filter options', async () => { + // Step 1: Simulate saved dashboard config + // (User previously set pre-filter to ['PT1H', 'P1D', 'P1W']) + const dashboardConfig = { + data: [ + { duration: 'PT1M', name: 'Minute' }, + { duration: 'PT1H', name: 'Hour' }, + { duration: 'P1D', name: 'Day' }, + { duration: 'P1W', name: 'Week' }, + { duration: 'P1M', name: 'Month' }, + ], + formData: { + nativeFilterId: 'time_grain_1', + defaultValue: null, + viz_type: 'filter_timegrain', + // This is what was saved by the config form: + time_grains: ['PT1H', 'P1D', 'P1W'], + }, + filterState: { + value: null, + validateStatus: undefined, + validateMessage: undefined, + }, + height: 100, + width: 300, + setDataMask: jest.fn(), + setFilterActive: jest.fn(), + setHoveredFilter: jest.fn(), + unsetHoveredFilter: jest.fn(), + setFocusedFilter: jest.fn(), + unsetFocusedFilter: jest.fn(), + inputRef: { current: null }, + }; + + // Step 2: Render the dashboard filter + render(<PluginFilterTimegrain {...(dashboardConfig as any)} />); + + // Step 3: Verify only pre-filtered options appear + const select = screen.getByRole('combobox'); + await userEvent.click(select); + + await waitFor(() => { + const options = screen.getAllByRole('option'); + const labels = options.map(o => o.textContent); + + // Should only show Hour, Day, Week (in database order) + expect(labels).toEqual(['Hour', 'Day', 'Week']); + + // Should NOT show Minute or Month + expect(labels).not.toContain('Minute'); + expect(labels).not.toContain('Month'); + }); Review Comment: **Suggestion:** This test only checks rendered option labels and never validates that selecting an allowed option updates the filter state payload. As written, it can pass even if user selection is broken at runtime, which gives a false positive for the end-to-end behavior it claims to verify. Extend the test to click an option and assert `setDataMask` receives the expected time grain. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ CI can miss broken time-grain selection payloads. - ❌ Dashboard chart queries may ignore selected time grain. - ⚠️ Pre-filter tests overstate end-to-end coverage accuracy. ``` </details> ```suggestion const options = await screen.findAllByRole('option'); const labels = options.map(o => o.textContent); // Should only show Hour, Day, Week (in database order) expect(labels).toEqual(['Hour', 'Day', 'Week']); // Should NOT show Minute or Month expect(labels).not.toContain('Minute'); expect(labels).not.toContain('Month'); await userEvent.click(screen.getByText('Week')); expect(dashboardConfig.setDataMask).toHaveBeenLastCalledWith( expect.objectContaining({ extraFormData: { time_grain_sqla: 'P1W' }, filterState: expect.objectContaining({ label: 'Week', value: ['P1W'], }), }), ); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `superset-frontend/src/filters/components/TimeGrain/TimeGrainPreFilter.integration.test.tsx:39-91`; this test only asserts option labels inside `waitFor` (`lines 80-90`) and never asserts `setDataMask`. 2. Follow the real runtime selection path in `superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.tsx`: selecting an option triggers `Select.onChange` (`line 134`), which calls `handleChange` (`line 67`) and should emit `setDataMask` with `time_grain_sqla` (`lines 77-83`). 3. Introduce a realistic regression in `handleChange` (for example, remove/wrongly shape `setDataMask` payload at `lines 77-83`) while keeping allowlist rendering (`lines 118-123`) unchanged. 4. Run this test file; it still passes because it validates only rendered options, not side effects. In dashboard runtime, chart queries depend on data mask aggregation via `getFormDataWithExtraFilters` (`src/dashboard/components/gridComponents/Chart/Chart.tsx:430` -> `src/dashboard/util/charts/getFormDataWithExtraFilters.ts:87-94`), so broken `setDataMask` causes time-grain filtering not to apply. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/filters/components/TimeGrain/TimeGrainPreFilter.integration.test.tsx **Line:** 80:90 **Comment:** *Logic Error: This test only checks rendered option labels and never validates that selecting an allowed option updates the filter state payload. As written, it can pass even if user selection is broken at runtime, which gives a false positive for the end-to-end behavior it claims to verify. Extend the test to click an option and assert `setDataMask` receives the expected time grain. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=2bd3ecb98e4a5d12b887ea2aeb22d360085766c49b5c185e6bd5e2fd236f66c8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=2bd3ecb98e4a5d12b887ea2aeb22d360085766c49b5c185e6bd5e2fd236f66c8&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx: ########## @@ -1206,6 +1212,75 @@ const FiltersConfigForm = ( </CollapsibleControl> </FormItem> )} + {itemTypeField === 'filter_timegrain' && + hasDataset && + datasetDetails?.time_grain_sqla && + datasetDetails.time_grain_sqla.length > 0 && ( + <FormItem + name={[ + 'filters', + filterId, + 'preFilterTimegrain', + ]} + > + <CollapsibleControl + initialValue={hasTimeGrainPreFilter} + title={t('Pre-filter available values')} + tooltip={t( + 'Select which time grains are available in the filter control. This is a UI allow list only and does not add extra conditions to the underlying queries.', + )} + onChange={checked => { + if (!checked) { + setNativeFilterFieldValues( + form, + filterId, + { time_grains: undefined }, + ); + forceUpdate(); + } + formChanged(); + }} + > + <FormItem + name={[ + 'filters', + filterId, + 'time_grains', + ]} + initialValue={ + filterToEdit?.time_grains + } + {...getFiltersConfigModalTestId( + 'time-grain-allowlist', + )} + > + <Select + mode="multiple" + ariaLabel={t('Time grain options')} + options={getTimeGrainOptions( + datasetDetails.time_grain_sqla, + )} + sortComparator={() => 0} + onChange={(values: string[]) => { + setNativeFilterFieldValues( + form, + filterId, + { + time_grains: + values.length > 0 + ? values + : undefined, + }, + ); + forceUpdate(); + formChanged(); + }} Review Comment: **Suggestion:** The allowlist currently persists any non-empty selection, including the "all options selected" case. This breaks the intended contract where selecting all grains should be treated as no pre-filter (`undefined`) for backward compatibility and smaller saved configs. Normalize the selection by storing `undefined` when the selected count matches all available time grains. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Redundant full allowlist stored in filter configuration. - ❌ Newly added dataset grains can stay hidden. - ⚠️ Backward-compatibility contract for "all selected" violated. ``` </details> ```suggestion onChange={(values: string[]) => { const allTimeGrains = getTimeGrainOptions( datasetDetails.time_grain_sqla, ).map(option => option.value); setNativeFilterFieldValues( form, filterId, { time_grains: values.length > 0 && values.length < allTimeGrains.length ? values : undefined, }, ); forceUpdate(); formChanged(); }} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In dashboard filter configuration UI, edit a `filter_timegrain` filter (rendered in `FiltersConfigForm.tsx:1215-1282`) and select every available option in the multi-select (`FiltersConfigForm.tsx:1264-1273`). 2. The current handler stores any non-empty selection as `time_grains` (`FiltersConfigForm.tsx:1269-1272`), so a full selection is persisted instead of normalized to `undefined`. 3. Save the modal; save flow calls `transformFilterForSave` from `useModalSaveLogic.ts:309-314`, and `filterTransformer.ts:130` writes `time_grains: formInputs.time_grains` unchanged into saved filter config. 4. Runtime filter loading reads this persisted allowlist and applies it in `applyTimeGrainAllowlist` (`FilterValue.tsx:91-113`, used at `FilterValue.tsx:301-304`), so config is treated as restricted list, not unrestricted. If dataset-supported grains later expand, newly available grains are filtered out because they are absent from saved full-at-that-time list. ``` </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/FiltersConfigForm.tsx **Line:** 1264:1277 **Comment:** *Logic Error: The allowlist currently persists any non-empty selection, including the "all options selected" case. This breaks the intended contract where selecting all grains should be treated as no pre-filter (`undefined`) for backward compatibility and smaller saved configs. Normalize the selection by storing `undefined` when the selected count matches all available time grains. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=c5aae10995cdd7a0bc2f3cf8b56fa8c3855031300de44dd0f4d40fd6c8fad1c6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=c5aae10995cdd7a0bc2f3cf8b56fa8c3855031300de44dd0f4d40fd6c8fad1c6&reaction=dislike'>👎</a> -- 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]
