-
Notifications
You must be signed in to change notification settings - Fork 49
Add conditional edit/delete button disabling #1850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideImplements permission- and type-based disabling of workspace edit/delete actions by fetching user write permissions from Chrome, centralizing disable logic in helper functions, extends the workspace type enum to include 'ungrouped-hosts', and adds Cypress tests to verify correct button states across workspace types. Sequence Diagram: Fetching and Applying User PermissionssequenceDiagram
title Sequence Diagram: Fetching and Applying User Permissions
participant WLT as WorkspaceListTable Component
participant Chrome as Chrome Service
WLT->>Chrome: getUserPermissions()
activate Chrome
Chrome-->>WLT: userPermissions
deactivate Chrome
WLT->>WLT: Updates state with userPermissions
WLT->>WLT: Applies new logic (canModify) during row rendering
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @apinkert - I've reviewed your changes - here's some feedback:
- The delete button’s isDisabled logic uses
&&
to combine child existence and permission checks—this means if a workspace has no children but the user lacks permission, it won’t be disabled; flip it to disable when either condition is true. - Guard against
permissions.find(...)
returning undefined before callingsetUserPermissions
(or provide a fallback) to avoid settinguserPermissions
to undefined and crashing when accessing its fields. - Rather than two nearly identical switch functions, consider using a single config map or lookup object for edit/delete validity to reduce duplication and simplify future type additions.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really solid! Just a few code cleanup things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked it locally and it looks really good! One minor visual update and we are golden.
For RHCLOUD-39840
Conditionally disables a workspace's edit/delete button based on workspace type as well as user permissions. Also adds basic e2e tests to test workspace type conditions.
Summary by Sourcery
Add conditional disabling of workspace edit and delete actions based on workspace type and user RBAC permissions.
New Features:
Enhancements:
Tests: