mikebridge commented on PR #41550: URL: https://github.com/apache/superset/pull/41550#issuecomment-5130771481
## Review fixes: both blockers and all six HIGHs are in Working through the combined review above. Head is now `e20ba79686`. | Commit | What | |---|---| | `ed5f8e322f` | **BLOCKER 1** — purge resolves only the type the route authorized. Also the 200-OK-for-failed-purges bug, the select-then-act race, the `skip_base_filter` parity gap, the blanket `except`, `dao: Any`, and the audit-actor fallback | | `3e6b4a4262` | **BLOCKER 2** — the bootstrap now carries the *effective* retention window, not a config key that never existed | | `ac53b92609` | **HIGH** — disabling `SOFT_DELETE` no longer leaves the purge APIs callable | | `c498fc144e` | Keyboard-reachable row actions, restore/purge double-fire guard, the toast's missing `%s`, docs drift | | `d8826e5d88` | The type selector offers only the types the viewer can read | | `e20ba79686` | **HIGH** — preview links removed | Every new test was checked against the previous code and fails there, so they are regression guards rather than decoration. ### Three findings worth calling out, because the reviews were not quite right **The retention-window blocker reproduces in a browser.** On a running instance with `SOFT_DELETE` on, both `SOFT_DELETE_RETENTION_DAYS` and `SUPERSET_SOFT_DELETE_RETENTION_DAYS` read `None` in the bootstrap payload. Rather than just correcting the key, the payload now carries what `resolve_retention_window()` resolves — so an operator who narrows the window with `deletion-retention set_window` gets *that* number in the modal, not the config seed it overrides. Telling users they have 30 days to recover something the purge removes after 7 seemed worth avoiding. **The preview-link finding was half right.** Driving a live instance: an archived dashboard's page does 404 as reported, but an archived chart's explore page answers **200 with no chart and no error** — the reader gets what looks like an empty new chart and is told nothing. The silent case is the more misleading of the two and had not been spotted. Neither target can show an archived object, so no row links out; all three types render as text with a tooltip pointing at recovery. A read-only view for the restore audience is the honest long-term answer and is tracked separately. **Gating purge on the feature flag exposed a latent test problem.** Every purge test was running with `SOFT_DELETE` *off* and passing — which is itself the clearest evidence the gate was missing. Two tests would have gone on passing for the wrong reason, asserting a 404 and a non-200 that the new gate produces regardless of the behaviour under test. All purge tests now declare the flag state they exercise instead of inheriting the config default. ### Deliberately not done - **The page-admission gate** stays on `can_read` for Chart. Widening it to any archivable type means replacing `@has_access`, which is also what redirects unauthenticated requests to the login page, and `abort(403)` has no precedent in `superset/views/`. That is an auth change deserving its own commit, not a rider on a permission tweak. The limitation is now written into the view's docstring, and the affected user is not stranded — the list and `/restore` endpoints answer them directly. The client-side half *is* fixed, which matters because it was reachable today: any role lacking all three reads was previously offered a type whose info endpoint 403s. - **The triplicated restore/purge route bodies.** A real refactor onto `SoftDeleteApiMixin`, and `SoftDeleteBinding` already captures the per-entity variance it would need. Better before a fourth entity is wired up than inside this PR. Both are ticketed, along with the read-only archived view, so none of them rests on this thread. Still draft pending CI and a first human review — this PR has never had one, which remains the real risk on it rather than anything above. -- 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]
