mikebridge opened a new pull request, #45482:
URL: https://github.com/apache/superset/pull/45482
### SUMMARY
**TL;DR:** Restoring a dashboard version now gets archived charts'
membership right. Before this fix, a restored version that included an archived
chart left it off the dashboard. A restored version that didn't include one
left it on.
**What users saw.** Say a dashboard had a chart that was later archived
(soft-deleted), and someone used "Restore this version" in version history.
- **The version had the chart:** the layout came back with the chart's slot,
but the dashboard no longer counted the chart as one of its charts, and the
restore reported it as a chart that "no longer exist[s]". Restoring the chart
from the archive later didn't bring it back onto the dashboard.
- **The version didn't have the chart:** the dashboard kept the chart.
Restoring the chart later put it back on a dashboard whose layout had no slot
for it.
Live charts and hard-deleted charts were not affected.
**Why, in plain terms.** An archived chart is hidden from normal lookups but
still belongs to its dashboards, so restoring it from the archive puts it back
where it was. The version restore set the dashboard's charts using normal
lookups. It couldn't see archived charts, either in the version it was
restoring or in the dashboard's current chart list, so it never added or
removed them.
**What this changes.** The restore reads both chart lists with archived
charts visible. An archived chart's membership now follows the restored
version, and the restore no longer reports archived charts as missing.
Permanently deleted charts are still skipped and reported. The version-history
docs now say this.
**Details.** `_restore_dashboard_membership` now does the following:
- It resolves the version's charts inside
`skip_visibility_filter(db.session, Slice)`.
- It takes the dashboard's current charts from `dashboard_slices` under the
same bypass.
- It installs those current charts as the collection's committed value
before assigning.
The current set can't come from `dashboard.slices`. The soft-delete loader
criteria the dashboard was loaded with also apply to its relationship loads,
even inside the bypass, so a lazy load or refresh still leaves archived members
out. The flush would then keep a row it should delete, or insert one that
already exists.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (API behavior).
Before: `POST /api/v1/dashboard/<uuid>/versions/<version_uuid>/restore`
returns 200. An archived chart in the version is not linked, the message says
"1 chart(s) referenced by the snapshot no longer exist and were not
reattached", and an archived chart not in the version stays linked. After: the
archived chart's link matches the restored version, and the message is "OK".
Restoring the chart afterwards brings it back only where the version had it.
### TESTING INSTRUCTIONS
- `pytest tests/integration_tests/dashboards/version_restore_tests.py -k
archived_chart_membership`, on PostgreSQL and SQLite.
- All three cases fail without the fix. The third case is a chart archived
while it is a member and present in the version. Without the fix it fails on
the "no longer exist" message, and it also guards against inserting that
chart's existing row again.
- Each case checks the live membership, the chart's newest
membership-history row and the restore message. It then restores the chart from
the archive.
- Manually, with soft delete and version history on:
1. Add a chart to a dashboard and save.
2. Remove the chart and save again.
3. Archive the chart.
4. Restore the version saved in step 1.
5. Restore the chart from the archive. It is back on the dashboard.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [x] Required feature flags: `SOFT_DELETE` (on by default) and
`VERSION_HISTORY`
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
