-
Notifications
You must be signed in to change notification settings - Fork 332
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
upcoming: [M3-7460] - Child View: Users & Grants page #10076
upcoming: [M3-7460] - Child View: Users & Grants page #10076
Conversation
1cfe954
to
95c468a
Compare
<Box> | ||
{showProxyUserTable && ( | ||
<Typography | ||
sx={(theme) => ({ | ||
marginTop: theme.spacing(3), | ||
[theme.breakpoints.down('md')]: { | ||
marginLeft: theme.spacing(1), | ||
}, | ||
})} | ||
variant="h3" | ||
> | ||
User settings | ||
</Typography> | ||
)} | ||
<Box display="flex" justifyContent="flex-end" sx={{ marginBottom: 1 }}> | ||
<AddNewLink | ||
disabledReason={ | ||
isRestrictedUser | ||
? 'You cannot create other users as a restricted user.' | ||
: undefined | ||
} | ||
disabled={isRestrictedUser} | ||
label="Add a User" | ||
onClick={() => setIsCreateDrawerOpen(true)} | ||
/> | ||
</Box> |
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.
When viewing the Users & Grants page as a child user, table headers will label each table and the Add User
button will be moved from the top of the page to above the User settings table to avoid implying that child users can create new proxy users. (Child users can create regular users.)
expect(screen.getByRole('tooltip')).toBeInTheDocument(); | ||
}); | ||
|
||
expect(screen.getByText('Test tooltip')).toBeVisible(); |
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.
We needed to wait for the tooltip to display after mousing over the button. This test was misleadingly passing because the button's name is 'Test', not because the tooltip was being displayed.
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.
β
Verified you cannot close accounts and tooltip text changes accordingly
β
Verified profile does not display for proxy user
β
Verified Account Access column for child users
Should we align the "User Settings" title with the "Add A User" button on the "Users & Grants" page? |
@jaalah-akamai Yeah, that's a good thought. I was following the mocks, which have it below, but I bet it'd look better aligned. π |
This PR looks good, I'll approve once you make the |
Coverage Report: β
|
@jaalah-akamai Done in 82c7284. Switched over to I also ended up rewriting some of an e2e test for coverage of this stuff while I was there anyway to fix a failure caused by the addition of the "Manage Access" button in this PR. |
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.
β
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.
UI looks great and implementation matches the description!
I left some questions/comments for code improvement
packages/manager/src/features/Account/CloseAccountSetting.test.tsx
Outdated
Show resolved
Hide resolved
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.
Proxy user displays in a table of proxy users (known to the user as "business partners") above the table of other users on the child account β
Only the Username, Email, and Account Access fields display for a proxy user and there are no User Profile or Delete buttons β
The different tables are labeled β
Clicking the Manage Access button for a proxy β
The Close Account button is disabled with a tooltip explanation β
Active profile user_type set to 'proxy' --> the Close Account button is disabled with the same tooltip explanation β
No regressions to the Users & Grants page and User Permissions pages, like no changes are visible when the feature flag is toggled off β
Tests pass β
This goes beyond this PR, but the account hierarchy feels a bit confusing sometimes. For example, a proxy user can't see themselves displayed on the Users & Grants page, but the child account can
packages/manager/src/features/Account/CloseAccountSetting.test.tsx
Outdated
Show resolved
Hide resolved
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.
Nice work and thanks for addressing all the changes!
Feature looks great and confirming steps outlined in the description β
Approving pending the outstanding question with the paginated table
Description π
This PR implements the child user view of the Users & Grants page with a new table to separate out Business Partners (the proxy users by which Parent users log into their child accounts) from other users on the child account. Proxy users do not have viewable profiles or last logins, nor can they be deleted. Child users also cannot delete their own accounts.
Adjustments to the Users & Grants page, User Permissions page, and Settings page have been made to account for this.
Changes π
UserRow
andCloseAccountSettings
.Additionally:
Button.test.tsx
.Preview π·
How to test π§ͺ
Prerequisites
(How to setup test environment)
yarn dev
.Verification steps
(How to verify changes)
ParentCompany
) displays in a table of proxy users (known to the user as "business partners") above the table of other users on the child account.serverHandlers.ts
and change the active profileuser_type
to'proxy'
. Verify the Close Account button is disabled with a different tooltip explanation - same tooltip as is visible for parent accounts.As an Author I have considered π€
Check all that apply