fix(explore): hide edit-properties menu item for non-owner/non-editor users (#38884) - #42737
Open
rusackas wants to merge 3 commits into
Open
fix(explore): hide edit-properties menu item for non-owner/non-editor users (#38884)#42737rusackas wants to merge 3 commits into
rusackas wants to merge 3 commits into
Conversation
…ers (#38884) useExploreAdditionalActionsMenu shows the "Edit chart properties" menu item whenever a slice exists, with no check against the current user's ownership/editor rights on it -- so a user who can open the modal but whose UpdateChartCommand save will be rejected server-side still sees a fully-clickable "Edit chart properties" entry (dosubot pinned the same root cause: the frontend gate is missing while the backend enforces ownership correctly, and ExploreChartHeader's own title-edit affordance already gates on the equivalent editors/user_subjects check this menu item lacks). Red: the menu item renders for a slice whose editors list doesn't include the current user. `type-checking-frontend` skipped -- pre-existing failures in unrelated files (superset-frontend/src/components/Datasource/FoldersEditor/*) needing a prebuilt lib/ this fresh worktree doesn't have; not caused by this change. oxlint/custom-rules/stylelint all pass on the changed file.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #dcbc6f
Actionable Suggestions - 1
-
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.tsx - 1
- Test validates unimplemented gate · Line 124-134
Review Details
-
Files reviewed - 1 · Commit Range:
2fd27c4..2fd27c4- superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
… users The "Edit chart properties" menu item rendered unconditionally whenever a chart was loaded, with no ownership/editor check, even though UpdateChartCommand already rejects the save server-side for a user without write access -- letting a user open a modal for an edit they can never persist. Gate it the same way ExploreChartHeader already gates its editable-title affordance: visible when the user can overwrite the chart, or is listed in the chart's editors. type-checking-frontend skipped: this worktree lacks the prebuilt lib/ artifacts it needs to run; oxfmt/oxlint/custom-rules/stylelint all pass. Fixes #38884
rusackas
changed the title
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…ting tests (#38884) The prior commit's test fixture for useExploreAdditionalActionsMenu.test.tsx passed slice.editors through an untyped object literal, which TypeScript's excess-property check rejected once compiled for real -- the production Slice type already declares editors, so this was purely a test-fixture typing gap; fixed by explicitly typing the fixture as Slice. Also updates three pre-existing ExploreChartHeader tests that exercised "Edit chart properties" without previously needing an explicit permission grant, since nothing gated that path before this issue's fix. They now set can_overwrite: true in the Redux initialState they already render with, matching how the fix's new permission check actually reads that signal. type-checking-frontend skipped: this worktree lacks the prebuilt lib/*.d.ts output the full project type-check needs. jest, oxlint, custom-rules, and stylelint all pass on the changed files.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42737 +/- ##
=======================================
Coverage 65.57% 65.57%
=======================================
Files 2818 2818
Lines 160038 160044 +6
Branches 36557 36561 +4
=======================================
+ Hits 104942 104948 +6
Misses 53051 53051
Partials 2045 2045
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Code Review Agent Run #dd730fActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Fixes #38884: an Alpha user with
can write on Chart(but no ownership or editor rights on a specific chart) could open the "Edit chart properties" modal for a chart they don't own, only to have the save silently rejected server-side (UpdateChartCommandcorrectly enforces ownership) -- a confusing dead-end UI flow.Root cause.
useExploreAdditionalActionsMenupushed the "Edit chart properties" menu item whenever asliceexisted, with no check against the current user's ownership/editor rights.ExploreChartHeader's own title-edit affordance already gates on the equivalentcan_overwritesignal (sourced from Reduxstate.explore.can_overwrite, itself derived from the chart'seditorsat hydration time), so the signal was already available in the codebase, just not applied to this menu item.Fix. Mirror that same gate: the menu item now only renders when
state.explore.can_overwriteis true.Follow-up fixes needed after the initial commit:
slicefixture was an untyped object literal, so TypeScript inferred a narrow type withouteditorsand rejected the override literals that added it once compiled for real (productionSlicealready declareseditors?: number[], so this was purely a test-fixture typing gap). Fixed by explicitly typing the fixture asSlice.ExploreChartHeader.test.tsxtests exercised "Edit chart properties" without ever needing an explicit permission grant, since nothing gated that path before this fix. They now setcan_overwrite: truein the ReduxinitialStatethey already render with.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A -- no visual change for owners/editors; the item is hidden for non-owner/non-editor users instead of leading to a dead-end save failure.
TESTING INSTRUCTIONS
hides Edit chart properties from a user who is not an owner/editor of the chart (regression #38884)now passes (was red), and a companion testshows Edit chart properties for a chart editorconfirms the item still renders for a user with edit rights. All 41 pre-existingExploreChartHeadertests pass.ADDITIONAL INFORMATION