bito-code-review[bot] commented on code in PR #38568:
URL: https://github.com/apache/superset/pull/38568#discussion_r2917687527
##########
tests/unit_tests/dashboards/commands/importers/v1/utils_test.py:
##########
@@ -199,3 +199,88 @@ def
test_update_id_refs_cross_filter_handles_string_excluded():
fixed = update_id_refs(config, chart_ids, dataset_info)
# Should not raise and should remap key
assert "1" in fixed["metadata"]["chart_configuration"]
+
+
+def test_update_id_refs_preserves_time_grains_in_native_filters():
+ """
+ Test that time_grains allowlist is preserved during dashboard import.
+
+ The time_grains field is a top-level filter configuration key that should
+ survive the update_id_refs transformation without modification.
+ """
+ from superset.commands.dashboard.importers.v1.utils import update_id_refs
+
+ config: dict[str, Any] = {
+ "position": {
+ "CHART1": {
+ "id": "CHART1",
+ "meta": {"chartId": 101, "uuid": "uuid1"},
+ "type": "CHART",
+ },
+ },
+ "metadata": {
+ "native_filter_configuration": [
+ {
+ "id": "NATIVE_FILTER-abc123",
+ "filterType": "filter_timegrain",
+ "name": "Time Grain",
+ "scope": {"rootPath": ["ROOT_ID"], "excluded": []},
+ "targets": [{"datasetId": 201, "column": {"name":
"dttm"}}],
+ "controlValues": {},
+ "time_grains": ["P1D", "P1W", "P1M"], # Allowlist should
be preserved
+ }
+ ]
+ },
+ }
+
+ chart_ids = {"uuid1": 1}
+ dataset_info: dict[str, dict[str, Any]] = {}
+
+ fixed = update_id_refs(config, chart_ids, dataset_info)
+
+ # Verify time_grains is preserved unchanged
+ filter_config = fixed["metadata"]["native_filter_configuration"][0]
+ assert filter_config.get("time_grains") == ["P1D", "P1W", "P1M"]
+ assert filter_config.get("filterType") == "filter_timegrain"
+
+
+def test_update_id_refs_handles_missing_time_grains():
+ """
+ Test backward compatibility when time_grains is not present.
+
+ Existing filters without time_grains should not break during import.
+ """
+ from superset.commands.dashboard.importers.v1.utils import update_id_refs
+
+ config: dict[str, Any] = {
+ "position": {
+ "CHART1": {
+ "id": "CHART1",
+ "meta": {"chartId": 101, "uuid": "uuid1"},
+ "type": "CHART",
+ },
+ },
+ "metadata": {
+ "native_filter_configuration": [
+ {
+ "id": "NATIVE_FILTER-legacy",
+ "filterType": "filter_timegrain",
+ "name": "Legacy Time Grain",
+ "scope": {"rootPath": ["ROOT_ID"], "excluded": []},
+ "targets": [{"datasetId": 201, "column": {"name":
"dttm"}}],
+ "controlValues": {},
+ # Note: no time_grains key (legacy filter)
+ }
+ ]
+ },
+ }
+
+ chart_ids = {"uuid1": 1}
+ dataset_info: dict[str, dict[str, Any]] = {}
+
+ fixed = update_id_refs(config, chart_ids, dataset_info)
+
+ # Verify filter is still valid and time_grains is either absent or
undefined
+ filter_config = fixed["metadata"]["native_filter_configuration"][0]
+ assert filter_config.get("filterType") == "filter_timegrain"
+ assert filter_config.get("time_grains") is None or "time_grains" not in
filter_config
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Weak test assertion</b></div>
<div id="fix">
The assertion should precisely verify the absence of the time_grains key to
avoid masking potential bugs where it might be incorrectly set to None.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/tests/unit_tests/dashboards/commands/importers/v1/utils_test.py
+++ b/tests/unit_tests/dashboards/commands/importers/v1/utils_test.py
@@ -286,1 +286,1 @@
- assert filter_config.get("time_grains") is None or "time_grains" not
in filter_config
+ assert "time_grains" not in filter_config
```
</div>
</details>
</div>
<small><i>Code Review Run #4121f4</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/TimeGrainPreFilter.test.tsx:
##########
@@ -0,0 +1,127 @@
+/**
+ * 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 { render, screen, userEvent, waitFor } from
'spec/helpers/testing-library';
+import { Form, Select } from '@superset-ui/core/components';
+import { CollapsibleControl } from './CollapsibleControl';
+import { getTimeGrainOptions } from './utils';
+
+/**
+ * Tests for the Time Grain Pre-filter feature (CollapsibleControl + Select).
+ *
+ * These tests verify:
+ * - Options are rendered in database order (no client-side sorting)
+ * - A saved subset of time_grains is loaded and shown as selected values
+ * - The CollapsibleControl checkbox shows/hides the Select
+ * - Toggling the checkbox off clears the underlying selection
+ */
+
+const TIME_GRAIN_TUPLES: [string, string][] = [
+ ['PT1S', 'Second'],
+ ['PT1M', 'Minute'],
+ ['PT1H', 'Hour'],
+ ['P1D', 'Day'],
+ ['P1W', 'Week'],
+];
+
+const renderPreFilter = (props: {
+ savedGrains?: string[];
+ initialChecked?: boolean;
+ onChangeMock?: jest.Mock;
+}) => {
+ const { savedGrains, initialChecked = false, onChangeMock = jest.fn() } =
props;
+ const options = getTimeGrainOptions(TIME_GRAIN_TUPLES);
+ return render(
+ <Form>
+ <Form.Item name="time_grains" initialValue={savedGrains}>
+ <CollapsibleControl
+ title="Pre-filter available values"
+ initialValue={initialChecked}
+ onChange={onChangeMock}
+ >
+ <Select
+ mode="multiple"
+ ariaLabel="Time grain options"
+ options={options}
+ sortComparator={() => 0}
+ />
+ </CollapsibleControl>
+ </Form.Item>
+ </Form>,
+ );
+};
+
+test('time grain options are displayed in database order (no sorting)', async
() => {
+ renderPreFilter({ initialChecked: true });
+
+ // Open the dropdown
+ const select = screen.getByRole('combobox');
+ await userEvent.click(select);
+
+ await waitFor(() => {
+ const options = screen.getAllByRole('option');
+ const labels = options.map(o => o.textContent);
+ // Options must follow database order: Second, Minute, Hour, Day, Week
+ expect(labels).toEqual(['Second', 'Minute', 'Hour', 'Day', 'Week']);
+ });
+});
+
+test('CollapsibleControl checkbox shows and hides the grain Select', async ()
=> {
+ renderPreFilter({});
+
+ // Initially unchecked — select should be hidden
+ expect(screen.queryByRole('combobox')).not.toBeInTheDocument();
+
+ const checkbox = screen.getByRole('checkbox', {
+ name: /Pre-filter available values/i,
+ });
+ await userEvent.click(checkbox);
+
+ // After checking — select should appear
+ expect(screen.getByRole('combobox')).toBeInTheDocument();
+
+ await userEvent.click(checkbox);
+
+ // After unchecking — select should disappear
+ expect(screen.queryByRole('combobox')).not.toBeInTheDocument();
+});
+
+test('CollapsibleControl starts expanded when initialValue is true', () => {
+ renderPreFilter({ initialChecked: true });
+
+ // When initialValue=true the checkbox is checked and children are visible
+ expect(screen.getByRole('combobox')).toBeInTheDocument();
+ const checkbox = screen.getByRole('checkbox', {
+ name: /Pre-filter available values/i,
+ });
+ expect(checkbox).toBeChecked();
+});
+
+test('onChange is called with correct value when checkbox is toggled', async
() => {
+ const onChangeMock = jest.fn();
+ renderPreFilter({ onChangeMock });
+
+ const checkbox = screen.getByRole('checkbox', {
+ name: /Pre-filter available values/i,
+ });
+ await userEvent.click(checkbox);
+ expect(onChangeMock).toHaveBeenCalledWith(true);
+
+ await userEvent.click(checkbox);
+ expect(onChangeMock).toHaveBeenCalledWith(false);
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test for saved time grain selection</b></div>
<div id="fix">
The test comments indicate verification of 'A saved subset of time_grains is
loaded and shown as selected values', but no test checks that savedGrains
values appear selected in the multiple Select. Add a test to render with
savedGrains=['PT1D', 'P1W'] and assert the selected values match.
</div>
</div>
<small><i>Code Review Run #1c78ce</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Missing test for selection clearing</b></div>
<div id="fix">
The test suite verifies UI visibility changes but misses asserting that
unchecking the CollapsibleControl clears the underlying `time_grains`
selection, as implemented in the real component's onChange handler. This gaps
coverage for the clearing behavior described in the test comments.
</div>
</div>
<small><i>Code Review Run #4121f4</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
