-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat(rolestable): delete roles modal #1696
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1696 +/- ##
==========================================
- Coverage 45.85% 45.73% -0.13%
==========================================
Files 182 182
Lines 4883 4896 +13
Branches 1377 1379 +2
==========================================
Hits 2239 2239
- Misses 2428 2441 +13
Partials 216 216 ☔ View full report in Codecov by Sentry. |
I realized that sometimes selectedRole is undefined so the modal shows undefined instead of the role name. |
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.
Looks like some of the messages needs to be changed to role's messages. Also when I pulled the PR I found it's missing the toolbar actions which should allow to remove multiple roles.
confirmButtonVariant={ButtonVariant.danger} | ||
onClose={() => setIsDeleteModalOpen(false)} | ||
onConfirm={() => { | ||
console.log(`Deleting ${currentRole?.display_name}`); |
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.
No need to console.log
console.log(`Deleting ${currentRole?.display_name}`); |
<WarningModal | ||
ouiaId={`${ouiaId}-remove-role-modal`} | ||
isOpen={isDeleteModalOpen} | ||
title={intl.formatMessage(messages.deleteUserModalTitle)} |
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.
This should be a different message, right now it says Remove from user groups?
it should say Remove role?
{ title: 'Edit role', onClick: () => console.log('Editing role') }, | ||
{ | ||
title: 'Delete role', | ||
onClick: (event: KeyboardEvent | React.MouseEvent, rowId: number, rowData: any) => handleModalToggle(event, rowData), |
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.
No need to use rowData
you have direct access to the role.
onClick: (event: KeyboardEvent | React.MouseEvent, rowId: number, rowData: any) => handleModalToggle(event, rowData), | |
onClick: (event: KeyboardEvent | React.MouseEvent) => handleModalToggle(event, role), |
{isDeleteModalOpen && ( | ||
<WarningModal | ||
ouiaId={`${ouiaId}-remove-role-modal`} | ||
isOpen={isDeleteModalOpen} |
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.
Based on the mocks the modal is missing checkbox
isOpen={isDeleteModalOpen} | |
isOpen={isDeleteModalOpen} | |
withCheckbox |
@InsaneZein I've checked the API and it has a delete role option https://console.redhat.com/docs/api/rbac/v1#operations-Role-deleteRole it should however be available only on custom roles. If the role is custom can be checked in role definition if |
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.
Ah ok the task made it sound like this was just for deleting a singular Role. I can work on adding an action button to delete multiple roles in another PR. |
/retest |
Description
This allows user to delete Roles. Currently deleting roles api doesn't exist so in the meantime, clicking "delete role" button will just spit out a console.log message.
RHCLOUD-32241
Screenshots
Checklist ☑️