Copilot commented on code in PR #34418:
URL: https://github.com/apache/superset/pull/34418#discussion_r2260936972
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -139,7 +139,47 @@ def update_id_refs( # pylint: disable=too-many-locals #
noqa: C901
native_filter["scope"]["excluded"] = [
id_map[old_id] for old_id in scope_excluded if old_id in id_map
]
+ fixed = update_cross_filter_scoping(fixed, id_map)
+ return fixed
+
+def update_cross_filter_scoping(
+ config: dict[str, Any], id_map: dict[int, int]
+) -> dict[str, Any]:
+ # fix cross filter references
+ fixed = config.copy()
+
+ cross_filter_global_config = fixed.get("metadata", {}).get(
+ "global_chart_configuration", {}
+ )
+ scope_excluded = cross_filter_global_config.get("scope",
{}).get("excluded", [])
+ if scope_excluded:
+ cross_filter_global_config["scope"]["excluded"] = [
+ id_map[old_id] for old_id in scope_excluded if old_id in id_map
+ ]
+
+ if "chart_configuration" in (metadata := fixed.get("metadata", {})):
+ # in cross_filter_scopes the key is the chart ID as a string; we need
to update
+ # them to be the new ID as a string:
+ metadata["chart_configuration"] = {
+ str(id_map[int(old_id)]): columns
+ for old_id, columns in metadata["chart_configuration"].items()
+ if int(old_id) in id_map
+ }
+ # now update scope excluded to use new IDs:
+ for chart_config in metadata["chart_configuration"].values():
+ if "id" in chart_config and chart_config["id"] in id_map:
+ chart_config["id"] = id_map[chart_config["id"]]
Review Comment:
The condition `chart_config["id"] in id_map` assumes the chart ID is an
integer, but based on line 165-167, the chart_config keys are string IDs
converted from the id_map. This condition should check if the ID exists as an
integer: `if "id" in chart_config and chart_config["id"] in id_map:`
```suggestion
if "id" in chart_config and int(chart_config["id"]) in id_map:
chart_config["id"] = id_map[int(chart_config["id"])]
```
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -139,7 +139,47 @@ def update_id_refs( # pylint: disable=too-many-locals #
noqa: C901
native_filter["scope"]["excluded"] = [
id_map[old_id] for old_id in scope_excluded if old_id in id_map
]
+ fixed = update_cross_filter_scoping(fixed, id_map)
+ return fixed
+
+def update_cross_filter_scoping(
+ config: dict[str, Any], id_map: dict[int, int]
+) -> dict[str, Any]:
+ # fix cross filter references
+ fixed = config.copy()
+
+ cross_filter_global_config = fixed.get("metadata", {}).get(
+ "global_chart_configuration", {}
+ )
+ scope_excluded = cross_filter_global_config.get("scope",
{}).get("excluded", [])
+ if scope_excluded:
+ cross_filter_global_config["scope"]["excluded"] = [
+ id_map[old_id] for old_id in scope_excluded if old_id in id_map
+ ]
+
+ if "chart_configuration" in (metadata := fixed.get("metadata", {})):
Review Comment:
[nitpick] The walrus operator assignment in the condition creates code
that's harder to read. Consider separating the assignment: `metadata =
fixed.get("metadata", {})` followed by `if "chart_configuration" in metadata:`
```suggestion
metadata = fixed.get("metadata", {})
if "chart_configuration" in metadata:
```
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
