mikebridge commented on PR #45538:
URL: https://github.com/apache/superset/pull/45538#issuecomment-6374795877
_AI-assisted reply on Mike's behalf._
@fitzee: thanks for the second pass. Pushed in 500576470f, then a master
merge (fe6e6a6d67).
**Detached chart still in the layout.** This is intended for this PR, and
there's now a test for it (`test_update_dashboard_layout_detached_chart`).
- Only `dashboard_slices` rows count as membership. A stale tile neither
keeps a chart a member nor lets a save re-attach it.
- A caller who can't read the chart gets the 422 until the tile is removed.
Saving without the tile works.
- A caller who can read it, such as an admin, re-attaches it on the next
save, as before this PR.
- The cost is the UX you described: an editor can get stuck on a tile they
can't see. Accepting the stale tile would reopen exactly the path this PR
closes, so I'd fix the UX in a follow-up. The options:
- your suggestion: treat a chart that isn't a member, and that the caller
can't read, like a dangling reference, i.e. render a placeholder and don't link
it. This also removes the existence oracle;
- or, when a chart is detached through the chart API, drop its tile from
the layout, which fixes the drift at its source.
**Test gaps.**
- Added `POST /api/v1/dashboard/`: own chart and readable chart give 201; a
foreign chart gives 422.
- Added string and float `chartId`: both give the access-denied 422, and
membership is unchanged.
- Added the `duplicate_slices` copy.
- The surviving mutant at `daos/dashboard.py:1110` is equivalent. The clone
loop already links each clone to the new dashboard, so
`_member_chart_ids(dashboard.id)` admits the clones before the allowance is
consulted. With chart reads denied, removing either one alone still passes, and
removing both fails the new test. I can drop the redundant branch here or in a
follow-up, whichever you prefer.
**Double access check on PUT.** Agreed, it's non-blocking. A follow-up could
skip the `set_dash_metadata` check when the update command has already
validated the same layout, or cache the datasource decision per datasource
within a request. I'd keep the backstop for create, copy and MCP either way.
--
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]
