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

Add user group modal #1673

Merged
merged 3 commits into from
Oct 25, 2024
Merged

Conversation

CodyWMitchell
Copy link
Contributor

@CodyWMitchell CodyWMitchell commented Oct 17, 2024

Description

Creates Add to user group modal that can be triggered by selecting rows and clicking "Add to user group" button or by selecting "Add to user group" in the action menu for a row. Inactive users cannot be added to user groups.

Modal reuses the existing UserGroupsTable component.

RHCLOUD-34262


Screenshots

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 add-user-group-modal branch 2 times, most recently from bd704cf to c1ac030 Compare October 17, 2024 14:57
@@ -86,7 +122,7 @@ const UserGroupsTable: React.FunctionComponent = () => {
group.roleCount,
group.workspaces || '?', // not currently in API
formatDistanceToNow(new Date(group.modified), { addSuffix: true }),
{
enableActions && {
cell: <ActionsColumn items={ROW_ACTIONS} />,
props: { isActionCell: true },
},
Copy link
Contributor

Choose a reason for hiding this comment

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

@CodyWMitchell you can also pass ID to the rows like so

const rows = groups.map((group: any) => ({
    id: group.uuid,
    row: [
      group.name,
      group.description ? (
        <Tooltip isContentLeftAligned content={group.description}>
          <span>{group.description.length > 23 ? group.description.slice(0, 20) + '...' : group.description}</span>
        </Tooltip>
      ) : (
        <div className="pf-v5-u-color-400">No description</div>
      ),
      group.principalCount,
      group.serviceAccounts || '?', // not currently in API
      group.roleCount,
      group.workspaces || '?', // not currently in API
      formatDistanceToNow(new Date(group.modified), { addSuffix: true }),
      enableActions && {
        cell: <ActionsColumn items={ROW_ACTIONS} />,
        props: { isActionCell: true },
      },
    ],
  }));

this way you can get rid of the useEffect, you can use the ID in matchOption, and the add modal can receive just a list of IDs that will be sent to the API

const selectedUsernames = selectedUsers.map((user) => ({username: user[0]})); // TODO: fix - this seems gross
selectedGroups.forEach((group) => {
console.log(`Adding ${JSON.stringify(selectedUsernames)} to group ${group.name} - ${group.uuid}`);
//dispatch(addMembersToGroup(group.uuid, selectedUsernames)); // TODO: fix 'user' not found 404 error
Copy link
Contributor

@fhlavac fhlavac Oct 18, 2024

Choose a reason for hiding this comment

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

I couldn't get 404 locally, but I got 400 - we should not try to update system roles, these have to be excluded from the list. Please, let me know if you still have issues figuring that out

@CodyWMitchell CodyWMitchell force-pushed the add-user-group-modal branch 2 times, most recently from affc69f to 8ed12b1 Compare October 23, 2024 14:15
@CodyWMitchell CodyWMitchell marked this pull request as ready for review October 23, 2024 14:36
Comment on lines 31 to 61
title={'Add to user group'}
isOpen={isOpen}
onClose={handleCloseModal}
actions={[
<Button key="add" variant="primary" onClick={handleAddUsers} isDisabled={selectedGroups.length === 0}>
Add
</Button>,
<Button key="cancel" variant="link" onClick={handleCloseModal}>
Cancel
</Button>,
]}
ouiaId={'add-user-group-modal'}
>
Select a user group to add{' '}
<span className="pf-v5-u-font-weight-bold">
{selectedUsers.length} user{selectedUsers.length > 1 && 's'}{' '}
</span>
to. These are all the user groups in your account. To manage user groups, go to user groups.
<UserGroupsTable
defaultPerPage={10}
useUrlParams={false}
ouiaId="iam-add-users-modal-table"
onChange={handleUserGroupsChange}
enableActions={false}
/>
</Modal>
Copy link
Contributor

Choose a reason for hiding this comment

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

the strings should be localized

Copy link
Contributor Author

@CodyWMitchell CodyWMitchell Oct 24, 2024

Choose a reason for hiding this comment

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

Because this is dynamic, I'm not sure the best way to localize it:

Select a user group to add{' '}
<span className="pf-v5-u-font-weight-bold">
  {selectedUsers.length} user{selectedUsers.length > 1 && 's'}{' '}
</span>
to. These are all the user groups in your account. To manage user groups, go to user groups.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a similar use case present in remove-group-service-accounts.tsx

<FormattedMessage
        {...messages.removeServiceAccountsText}
        values={{
          b: (text) => <b>{text}</b>,
          count: accountsCount,
          name: selectedServiceAccounts[0]?.name,
          group: group.name,
        }}
      />

Let me know if you have more questions 🙂

Comment on lines 95 to 97
if (onChange) {
onChange(selected);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (onChange) {
onChange(selected);
}
onChange?.(selected);

<span>{group.description.length > 23 ? group.description.slice(0, 20) + '...' : group.description}</span>
</Tooltip>
) : (
<div className="pf-v5-u-color-400">No description</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

should be localized

Comment on lines 89 to 90
user.is_active ? 'Active' : 'Inactive',
user.is_org_admin ? 'Yes' : 'No',
Copy link
Contributor

Choose a reason for hiding this comment

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

^^

isDisabled={selected.length === 0}
ouiaId={`${OUIA_ID}-add-user-button`}
>
Add to user group
Copy link
Contributor

Choose a reason for hiding this comment

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

^^

@fhlavac
Copy link
Contributor

fhlavac commented Oct 23, 2024

@CodyWMitchell looks great, can you please add a simple cypress tests checking that the new modal works?

{intl.formatMessage(messages['usersAndUserGroupsCancel'])}
</Button>,
]}
ouiaId={'add-user-group-modal'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ouiaId={'add-user-group-modal'}
ouiaId="add-user-group-modal"

Comment on lines 46 to 50
Select a user group to add{' '}
<span className="pf-v5-u-font-weight-bold">
{selectedUsers.length} user{selectedUsers.length > 1 && 's'}{' '}
</span>
to. These are all the user groups in your account. To manage user groups, go to user groups.
Copy link
Contributor

Choose a reason for hiding this comment

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

This one can be localized using FormattedMessage

Comment on lines 37 to 38
{ title: 'Add to user group', onClick: () => onAddUserClick([user]) },
{ title: 'Remove from user group', onClick: () => console.log('REMOVE FROM USER GROUP') },
Copy link
Contributor

@fhlavac fhlavac Oct 24, 2024

Choose a reason for hiding this comment

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

Needs to be localized

@fhlavac
Copy link
Contributor

fhlavac commented Oct 24, 2024

Just found two more strings, it looks great!

@CodyWMitchell CodyWMitchell force-pushed the add-user-group-modal branch 2 times, most recently from 95cc2bf to de85942 Compare October 24, 2024 22:08
@fhlavac fhlavac merged commit 49f96b8 into RedHatInsights:master Oct 25, 2024
9 of 10 checks passed
Copy link
Contributor

@fhlavac fhlavac left a comment

Choose a reason for hiding this comment

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

LGTM!

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