bito-code-review[bot] commented on code in PR #40140:
URL: https://github.com/apache/superset/pull/40140#discussion_r3245379262
##########
tests/unit_tests/dashboards/commands/importers/v1/utils_test.py:
##########
@@ -123,6 +123,99 @@ def test_update_native_filter_config_scope_excluded():
}
+def test_update_id_refs_remaps_charts_in_scope():
+ """
+ Regression for #26338: ``chartsInScope`` on a native filter holds chart
+ IDs and must be remapped from source-env IDs to destination-env IDs
+ during import.
+
+ The export side already converts ``chartsInScope`` IDs to UUIDs (see
+ ``export_example.py:325``). The import side must symmetrically convert
+ them back to the destination environment's chart IDs. Without that
+ remap, the field carries stale source IDs into the imported dashboard
+ and breaks ``filtersInScope`` / ``filtersOutScope`` computation —
+ filters end up applied to the wrong charts (or none at all).
+ """
+ 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",
+ },
+ "CHART2": {
+ "id": "CHART2",
+ "meta": {"chartId": 102, "uuid": "uuid2"},
+ "type": "CHART",
+ },
+ },
+ "metadata": {
+ "native_filter_configuration": [
+ {
+ "id": "NATIVE_FILTER-region",
+ "scope": {"rootPath": ["ROOT_ID"], "excluded": []},
+ # chartsInScope contains source-env chart IDs.
+ "chartsInScope": [101, 102, 103],
+ }
+ ],
+ },
+ }
+ chart_ids = {"uuid1": 1, "uuid2": 2}
+ dataset_info: dict[str, dict[str, Any]] = {}
+
+ fixed = update_id_refs(config, chart_ids, dataset_info)
+ filter_config = fixed["metadata"]["native_filter_configuration"][0]
+
+ # Resolved IDs are remapped; unknown IDs (103) are dropped rather than
+ # left to silently bind to whatever chart owns that integer in the
+ # destination environment.
+ assert filter_config["chartsInScope"] == [1, 2]
+
+
+def test_update_id_refs_remaps_cross_filter_charts_in_scope():
+ """
+ Companion to test_update_id_refs_remaps_charts_in_scope. Cross-filter
+ config also stores ``chartsInScope`` (under ``crossFilters`` per chart)
+ and must be remapped on import for the same reason.
+ """
+ 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",
+ },
+ "CHART2": {
+ "id": "CHART2",
+ "meta": {"chartId": 102, "uuid": "uuid2"},
+ "type": "CHART",
+ },
+ },
+ "metadata": {
+ "chart_configuration": {
+ "101": {
+ "id": 101,
+ "crossFilters": {
+ "scope": {"rootPath": ["ROOT_ID"], "excluded": []},
+ "chartsInScope": [101, 102, 103],
+ },
+ },
+ },
+ },
+ }
+ chart_ids = {"uuid1": 1, "uuid2": 2}
+ dataset_info: dict[str, dict[str, Any]] = {}
+
+ fixed = update_id_refs(config, chart_ids, dataset_info)
+ cross_filters =
fixed["metadata"]["chart_configuration"]["1"]["crossFilters"]
+
+ assert cross_filters["chartsInScope"] == [1, 2]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing chartsInScope Remapping</b></div>
<div id="fix">
The added tests expect the update_id_refs function to remap chartsInScope
from source to destination chart IDs, but the current implementation only
remaps scope excluded. This will cause the tests to fail. The function needs to
be updated to include remapping for chartsInScope in both
native_filter_configuration and chart_configuration.crossFilters.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/commands/dashboard/importers/v1/utils.py
@@ -143,4 +143,7 @@
if scope_excluded:
native_filter["scope"]["excluded"] = [
id_map[old_id] for old_id in scope_excluded if old_id in
id_map
- ]
+ ]
+ native_filter["chartsInScope"] = [
+ id_map[old_id] for old_id in
native_filter.get("chartsInScope", []) if old_id in id_map
+ ]
@@ -213,1 +213,10 @@
]
+ # Update cross filter chartsInScope ids
+ cross_filters = chart_config.get("crossFilters",
{})
+ if isinstance(cross_filters, dict):
+ charts_in_scope =
cross_filters.get("chartsInScope", [])
+ if charts_in_scope:
+
chart_config["crossFilters"]["chartsInScope"] = [
+ id_map[old_id]
+ for old_id in charts_in_scope
+ if old_id in id_map
+ ]
+
new_chart_configuration[str(new_id)] = chart_config
```
</div>
</details>
</div>
<small><i>Code Review Run #c144e5</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]
