rusackas commented on code in PR #42737:
URL: https://github.com/apache/superset/pull/42737#discussion_r3723206143
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx:
##########
@@ -601,7 +602,7 @@ export const useExploreAdditionalActionsMenu = (
const menuItems = [];
// Edit chart properties
- if (slice) {
+ if (slice && canModifySlice) {
Review Comment:
Good catch on the `can_write Chart` gap. Fixed in ddf32a242f1:
`canModifySlice` is now ANDed with `state.explore.can_add` (which is
`findPermission('can_write', 'Chart', ...)`, already computed by
`hydrateExplore`) for the Edit chart properties menu item specifically, so
visibility matches what `ChartRestApi.put`/`restore_version` actually require.
I left "View version history" on `canModifySlice` alone since its own list/get
endpoints only need read access - only the restore action needs write, and
that's enforced server-side already.
On the admin-role-name part: `isUserAdmin` here is hardcoded to the literal
`'admin'` role name, not `AUTH_ROLE_ADMIN`, but that's an existing pattern
shared with the dashboard editor checks (`permissionUtils.ts`), not something
new to this PR. There's already a comment there acknowledging it ("this should
really be a config value, but is hardcoded in backend logic already"). Fixing
that properly means threading `AUTH_ROLE_ADMIN` through to the frontend
bootstrap data, which is a bigger change than this PR's scope - I'd rather do
that as its own follow-up than bolt it onto this fix.
--
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]
