mikebridge commented on PR #41551: URL: https://github.com/apache/superset/pull/41551#issuecomment-5118292724
Fixed a permission bug found in local integration testing: **"View version history" never appeared for admins**, so on a fresh install the feature was effectively undiscoverable. The menu was gated on `can_overwrite`, which `hydrateExplore` computes purely as "the current user is among the slice's editors". Every seeded and example chart ships with an empty editors list, so that yields false for everyone — administrators included. Adding a user to a chart's `editors` made the item appear immediately, with nothing else changed. The server disagreed with that gate in two ways. `security_manager.is_editor` returns true for admins outright, and it unions `extra_editors` — editorship granted by a deployment's `EXTRA_EDITORS_RESOLVER` — into the editor set before comparing. So the UI was hiding an action the API would have allowed, for two distinct groups of users. Three commits: - **`b43f8c3392`** — extract the permission into `explore/exploreUtils/canOverwriteSlice`, matching the server: admin, or a subject in `editors` or `extra_editors`, and never for an externally managed slice. `SaveModal` already worked around the same gap with a broader predicate of its own — which is why an admin could *save* a chart but not see its history — so it now uses the shared helper and the two cannot drift apart again. - **`ed6b6706a8`** — the same predicate for the panel's URL-param entry point and the chart preview's restore control. Both derived `canRestore` from `can_overwrite` too, so fixing only the menu would have let an admin open version history and then find Restore missing. - **`c8fb8ba886`** — tests at both levels. Two things worth recording for reviewers: **Why the tests missed it.** The existing suites mock redux with `can_overwrite: true`, so the gate was never exercised against what `hydrateExplore` actually produces. The new menu tests render from the real state shape — a slice with `editors: []` — and are verified as regression guards: with the gate reverted to `can_overwrite`, exactly the admin case fails. **A leftover from the owners → editors rename.** `SaveModal`'s `isCurrentUserOwner()` fallback checked `slice.owners`, but `Slice.data` ships `editors`, `extra_editors` and `viewers` and no `owners` at all, so that branch could never fire. Dropped rather than carried into the shared helper. The dashboard side needed no change: its gate uses `canUserEditDashboard`, which already unions `isUserAdmin` with dashboard editorship. That asymmetry is why version history worked for dashboards while charts hid it, and why this initially looked feature-flag shaped rather than permission shaped. -- 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]
