fitzee commented on code in PR #45528:
URL: https://github.com/apache/superset/pull/45528#discussion_r4330793742
##########
superset/daos/dashboard.py:
##########
@@ -969,9 +969,15 @@ def copy_dashboard(
# the raw-layout markdown placeholder while preserving geometry;
# the remap below skips that slot because it has no chartId.
reconcile_position_json(metadata.get("positions"),
original_dash.id)
+ chart_id: int | None
+ layout_chart_ids: set[int] = {
+ chart_id
+ for node in metadata["positions"].values()
+ if (chart_id := _layout_chart_id(node)) is not None
+ }
# Duplicating slices as well, mapping old ids to new ones
for slc in original_dash.slices:
- if slc.id in archived_ids:
+ if slc.id in archived_ids or slc.id not in layout_chart_ids:
Review Comment:
Non-blocking. A side effect of skipping omitted charts:
`old_to_new_slice_ids` no longer has entries for them, so
`_remap_filter_scopes` leaves their IDs as the source's IDs. Before this change
they were rewritten to orphan-clone IDs. Example: source has A=1 and B=2, the
layout keeps only A, and A is cloned to 4. The copy's `json_metadata` ends up
with:
- `chart_configuration` keys `"2"` and `"4"`. The `"2"` entry belongs to the
source's live chart B.
- `chart_configuration["4"].crossFilters.chartsInScope == [2]`.
- A native filter's `chartsInScope == [4, 2]` and `scope.excluded == [2]`.
The references were dangling before this change too. Now they point at a
live chart on another dashboard. If B is later added to the copy, it picks up
the stale cross-filter config. The follow-up suggested on #45472 (prune
metadata to the layout's charts) is now the natural complement: after the
remap, drop any ID in these structures that isn't in
`old_to_new_slice_ids.values()`.
##########
superset/daos/dashboard.py:
##########
@@ -969,9 +969,15 @@ def copy_dashboard(
# the raw-layout markdown placeholder while preserving geometry;
# the remap below skips that slot because it has no chartId.
reconcile_position_json(metadata.get("positions"),
original_dash.id)
+ chart_id: int | None
+ layout_chart_ids: set[int] = {
Review Comment:
Non-blocking, pre-existing. `timed_refresh_immune_slices` and
`expanded_slices` are never remapped on the duplicate path, even for charts
that *are* cloned. In the A=1 to clone 4 case above, the copy keeps
`timed_refresh_immune_slices: [1, 2]` and `expanded_slices: {"1": true, "2":
true}`. As a result, every duplicated copy loses refresh immunity and expanded
state for its charts. `_remap_filter_scopes` covers neither key. Out of scope
here, but worth handling in the same follow-up: remap through
`old_to_new_slice_ids` and prune to the layout. (`default_filters` has a
similar problem: its old-ID keys are filtered against the new `slice_ids`, so
they are all dropped.)
--
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]
