Skip to content
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: create delete user groups modal #1703

Merged

Conversation

CodyWMitchell
Copy link
Contributor

@CodyWMitchell CodyWMitchell commented Nov 13, 2024

Description

RHCLOUD-32237

Create the modal that handled deletion of User Groups fitting the criteria below:

  • I can delete an existing user group.
  • I am prompted to verify the deletion and informed of the potential impacts (e.g. all associated role bindings for my organization with the group will be broken and any existing permissions associated with the bindings will be lost.)
  • I can confirm or cancel my request.
  • Upon cancellation, I am redirected to the previous page/task.
  • Upon confirming, I am presented with a success message and redirected to the User Group list.
  • I am presented with an error message if the user group is not deleted successfully.

Screenshots

image
image


Checklist ☑️

  • PR only fixes one issue or story
  • Change reviewed for extraneous code
  • UI best practices adhered to
  • Commits squashed and meaningfully named
  • All PR checks pass locally (build, lint, test, E2E)

  • (Optional) QE: Needs QE attention (OUIA changed, perceived impact to tests, no test coverage)
  • (Optional) QE: Has been mentioned
  • (Optional) UX: Needs UX attention (end user UX modified, missing designs)
  • (Optional) UX: Has been mentioned

@CodyWMitchell CodyWMitchell force-pushed the create-delete-groups-modal branch from 078cfaf to 3b08e37 Compare November 13, 2024 06:59
@CodyWMitchell CodyWMitchell changed the title Create delete groups modal feat: create delete user groups modal Nov 13, 2024
@CodyWMitchell CodyWMitchell marked this pull request as ready for review November 13, 2024 07:00
@CodyWMitchell CodyWMitchell requested a review from a team as a code owner November 13, 2024 07:00
Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Just a few tweaks here and there.

Comment on lines 198 to 206
title={
<FormattedMessage
{...messages.deleteUserGroupModalTitle}
values={{
count: currentGroups.length,
plural: currentGroups.length > 1 ? intl.formatMessage(messages.groups) : intl.formatMessage(messages.group),
}}
/>
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like title props can only be string. You already have a message preconfigured to work with plural values

Suggested change
title={
<FormattedMessage
{...messages.deleteUserGroupModalTitle}
values={{
count: currentGroups.length,
plural: currentGroups.length > 1 ? intl.formatMessage(messages.groups) : intl.formatMessage(messages.group),
}}
/>
}
title={intl.formatMessage(messages.deleteUserGroupModalTitle, { count: currentGroups.length })}

Comment on lines 251 to 253
onClick: () => {
handleDeleteModalToggle(groups.filter((group) => selected.some((selectedGroup) => selectedGroup.id === group.uuid)));
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selectedGroup has implicit any type

Suggested change
onClick: () => {
handleDeleteModalToggle(groups.filter((group) => selected.some((selectedGroup) => selectedGroup.id === group.uuid)));
},
onClick: () => {
handleDeleteModalToggle(
groups.filter((group) => selected.some((selectedGroup: { id: string }) => selectedGroup.id === group.uuid))
);
},

Comment on lines 141 to 155
cell: (
<ActionsColumn
items={[
{
title: intl.formatMessage(messages['usersAndUserGroupsEditUserGroup']),
onClick: () => console.log('EDIT USER GROUP'),
},
{
title: intl.formatMessage(messages['usersAndUserGroupsDeleteUserGroup']),
onClick: () => handleDeleteModalToggle([group]),
},
]}
rowData={group}
/>
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the group has either system=true or platform_default=true flag enabled we can't show the kebab menu at all. These groups can't be changed or removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Actions are now disabled for system and platform_default rows, both individually and when one is included within a bulk selection.

}
actions={
<div data-ouia-component-id={`${ouiaId}-actions-dropdown`}>
<ActionsColumn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use newer functionality from DataView https://react-data-view-pr-data-view-131.surge.sh/extensions/data-view/components#actions-example this would allow us to keep actions button visible all the times.

Copy link
Contributor Author

@CodyWMitchell CodyWMitchell Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karelhala The <ResponsiveActions> component doesn't currently support disabling actions through any props like isDisabled or disabled. I'll switch to using it once that functionality is added. We need that here in the case that no rows are selected or any rows are system or platform_default (see #1703 (comment)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other feedback should be resolved ✅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can disable each action by setting isDisabled on ResponsiveAction

<ResponsiveActions breakpoint="lg">
    <ResponsiveAction isPersistent isDisabled>
        Persistent Action
    </ResponsiveAction>
    <ResponsiveAction isDisabled isPinned variant='secondary'>
        Pinned Action
    </ResponsiveAction>
    <ResponsiveAction>
        Overflow Action
    </ResponsiveAction>
  </ResponsiveActions>

Copy link
Contributor Author

@CodyWMitchell CodyWMitchell Nov 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karelhala It's working when the ResponsiveAction is marked isPersistent, but for actions within the dropdown (not isPersistent or isPinned), isDisabled isn't disabling them.
image
For example, both actions in the screenshot are isDisabled={true}, but only the Edit button is being disabled.

@CodyWMitchell
Copy link
Contributor Author

looks like some tests are failing for the roles page:

22:42:22   2 failing
22:42:22 
22:42:22   1) Roles page
22:42:22        should display the Roles table and correct data:
22:42:22      AssertionError: Timed out retrying after 4000ms: Expected to find element: `[data-ouia-component-id="RolesTable-detail-drawer"]`, but never found it.
22:42:22       at Context.eval (webpack://insights-****/./cypress/e2e/roles.cy.ts:54:0)
22:42:22 
22:42:22   2) Roles page
22:42:22        should display the details drawer:
22:42:22      AssertionError: Timed out retrying after 4000ms: Expected to find element: `[data-ouia-component-id^="RolesTable-table-tr-0"]`, but never found it.
22:42:22       at eval (webpack://insights-****/./cypress/e2e/roles.cy.ts:68:0)
22:42:22   at Array.forEach (<anonymous>)
22:42:22       at Context.eval (webpack://insights-****/./cypress/e2e/roles.cy.ts:67:0)

@CodyWMitchell
Copy link
Contributor Author

/retest

@karelhala karelhala merged commit ed28740 into RedHatInsights:master Nov 19, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants