mikebridge commented on code in PR #45472:
URL: https://github.com/apache/superset/pull/45472#discussion_r4327056853
##########
superset/daos/dashboard.py:
##########
@@ -857,7 +857,10 @@ def copy_dashboard(
]
)
value["meta"]["chartId"] = new_id
- else:
+ elif metadata.get("positions") is None:
Review Comment:
Agreed. This predates the PR (PUT on a dashboard you own has the same gap),
so I've kept it out of this fix and it's tracked as a follow-up: limit the
layout's chart IDs to the source's members, plus charts from version history
the caller can access, on the server.
##########
superset/daos/dashboard.py:
##########
@@ -857,7 +857,10 @@ def copy_dashboard(
]
)
value["meta"]["chartId"] = new_id
- else:
+ elif metadata.get("positions") is None:
+ # With a layout, set_dash_metadata links exactly the layout's
charts.
+ # Linking the source's charts first and then dropping some makes
the
+ # flush delete dashboard_slices rows that were never inserted.
dash.slices = original_dash.slices
dash.params = original_dash.params
Review Comment:
Right, the copy starts from the source's current params and only overlays
the posted keys, so a version fork inherits today's keys and per-chart entries
for charts the layout dropped. Pre-existing, so not changing it here; tracked
as a follow-up to build the fork's metadata from the snapshot and prune
per-chart keys to the layout's charts.
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -5381,6 +5382,96 @@ def
test_copy_dashboard_layout_error_returns_422_without_writes(
db.session.delete(other)
db.session.commit()
+ @parameterized.expand([(True, False), (False, False), (True, True),
(False, True)])
+ def test_copy_dashboard_layout_without_current_charts(
+ self, keep_first: bool, duplicate_slices: bool
+ ) -> None:
+ """Copy a layout that omits charts the source has, as a version fork
does."""
+ from superset.connectors.sqla.models import SqlaTable
+
+ admin: User = self.get_user(ADMIN_USERNAME)
+ dataset_id: int = (
+ db.session.query(SqlaTable.id).order_by(SqlaTable.id).first()[0]
+ )
+ first: Slice = self.insert_chart("copy-subset-first", [admin.id],
dataset_id)
+ second: Slice = self.insert_chart("copy-subset-second", [admin.id],
dataset_id)
+ dashboard: Dashboard = self.insert_dashboard(
+ "copy-subset-source", None, [admin.id], slices=[first, second]
+ )
+ source_members: set[int] = {first.id, second.id}
+ positions: dict[str, Any] = (
+ {
+ "CHART-first": {
+ "id": "CHART-first",
+ "type": "CHART",
+ "meta": {"chartId": first.id, "width": 4, "height": 50},
+ }
+ }
+ if keep_first
+ else {}
+ )
+ self.login(ADMIN_USERNAME)
+ copied_id: int | None = None
+ try:
+ response: TestResponse = self.client.post(
+ f"/api/v1/dashboard/{dashboard.id}/copy/",
+ json={
+ "dashboard_title": "copy-subset-result",
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps({"positions": positions}),
+ },
+ )
+ assert response.status_code == 200, response.data
+ copied_id = response.json["result"]["id"]
+ db.session.expire_all()
+ copied: Dashboard = db.session.get(Dashboard, copied_id)
+ expected: set[str] = {"copy-subset-first"} if keep_first else set()
+ assert {chart.slice_name for chart in copied.slices} == expected
+ assert {chart.id for chart in dashboard.slices} == source_members
+ finally:
+ db.session.rollback()
+ if copied_id is not None:
+ db.session.delete(db.session.get(Dashboard, copied_id))
+ db.session.delete(dashboard)
+ # Clones carry the source names, including ones the layout left
out.
Review Comment:
Good catch. The clone loop has always cloned every non-archived member
regardless of the layout, so that's pre-existing too. Tracked as a follow-up to
skip clones whose original isn't in the layout and assert the orphan count in
this test.
--
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]
