reynoldmorel commented on code in PR #37042:
URL: https://github.com/apache/superset/pull/37042#discussion_r2700547749
##########
superset/commands/chart/delete.py:
##########
@@ -68,3 +73,132 @@ def validate(self) -> None:
security_manager.raise_for_ownership(model)
except SupersetSecurityException as ex:
raise ChartForbiddenError() from ex
+
+ def _cleanup_dashboard_metadata( # noqa: C901
+ self, chart_id: int
+ ) -> None:
+ """
+ Remove references to this chart from all dashboard metadata.
+
+ When a chart is deleted, dashboards may still contain references to the
+ chart ID in various metadata fields (expanded_slices, filter_scopes,
etc.).
+ This method cleans up those references to prevent issues during
dashboard
+ export/import.
+ """
+ # Find all dashboards that contain this chart
+ dashboards = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.slices.any(id=chart_id)) # type:
ignore[attr-defined]
+ .all()
+ )
+
+ for dashboard in dashboards:
+ metadata = dashboard.params_dict
+ modified = False
+
+ # Clean up expanded_slices
+ if "expanded_slices" in metadata:
+ chart_id_str = str(chart_id)
+ if chart_id_str in metadata["expanded_slices"]:
+ del metadata["expanded_slices"][chart_id_str]
+ modified = True
+ logger.info(
+ "Removed chart %s from expanded_slices in dashboard
%s",
+ chart_id,
+ dashboard.id,
+ )
Review Comment:
Looks like this pattern is repeated a lot, can't we just put it behind a
function and do something like:
```
(modified, updated_expanded_slices_metadata) =
delete_metadata_by_chart("expanded_slices", metadata, chart_id) or modified
(modified, updated_timed_refresh_immune_slices_metadata) =
delete_metadata_by_chart("timed_refresh_immune_slices",
updated_expanded_slices_metadata, chart_id) or modified
```
For different patterns you can write a condition to check whether the
property needs a unique pattern or not to get its value where `chart_id` might
be located
Also I don't think you should be updating metadata and dashboard properties
by object reference, this could lead to unpredictable behavior if this function
keeps growing and could become harder to maintain.
I would return a new updated metadata reference in general from
`delete_metadata_by_chart`, you could use `copy.deepcopy` or just build a new
reference of metadata once, before start calling `delete_metadata_by_chart` and
then you could modify the props directly to the reference instead of touching
the existing one
--
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]
