-
Notifications
You must be signed in to change notification settings - Fork 364
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-7857] - Account Management Copy Updates & Improvements #10270
Conversation
isProxyUser | ||
? 'You can only create tokens for your own company.' | ||
: undefined | ||
<Button |
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.
Changed this to a Button
in order to show the tooltip icon for the disabled state. AddNewLink
should probably be deprecated in the future.
Coverage Report: ✅ |
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 good, thanks @jaalah-akamai! Approved pending update for wording in personal-access-tokens.spec.ts
here:
manager/packages/manager/cypress/e2e/core/account/personal-access-tokens.spec.ts
Line 279 in 882c435
.findByText('You can only create tokens for your own company.') |
Edit: And maybe line 295 too
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.
- Update Proxy Account Tooltip (Proxy User):
- When attempting to close an account as a proxy user, the tooltip should now read: "Contact customer support to close this account." ✅
- Clarify Account Closure Instructions (Proxy User):
- Change the tooltip text from "Remove indirect customers before closing the account." to "Remove child accounts before closing the account."
- Testing this with the mocks seemed inconsistent/tricky. When I first turned the MSW on and changed L558 to
proxy
, I saw this tooltip text, and continued to see it after commenting out L1364. Then I circled back after testing a few other things, and saw the"Contact customer support to close this account."
text although there was a child user on the Users & Grants page. Then with L558 set toparent
and L1364 commented out, I see this text althoughchildAccountUser
isn't getting returned in the response to*/account/users'
- Update Account Login Indicator (General):
- Modify the account indicator text from "You are currently logged in as:" to "Current account:" ✅
- New PAT Tooltip for Proxy View (Proxy User):
- For proxy users, introduce a new tooltip for disabled Personal Access Tokens (PAT) creation with the text: "You can’t create access tokens for child accounts. Instead, switch to your account and create the token there." ✅
- Fix Billing Contact Info Bug (Parent User):
- Address the bug preventing parent users from updating billing contact information due to the company field not being omitted from the payload. ✅
- Limit Proxy Account Login Toast (Proxy User):
- Ensure the login toast for proxy accounts only appears once per session by utilizing local storage to manage its visibility.
- If this can be tested with the MSW, are there any precise steps to take?
The User Settings
table looks like it needs some attention for Proxy users:
@dwiley-akamai We'll look into updating MSW, since it's not always a perfect reflection of the feature. |
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.
Confirmed:
✅ Copy update to PAT button as proxy user
✅ Copy updates for Close Account buttons depending on user type
✅ Only saw the toast once and see the local storage value set - I just think we should document what we're doing here
✅ Can edit a parent account's billing contact info without error
✅ Copy update to user menu
Approving pending test updates and some minor nitpicks/comments.
@dwiley-akamai - to your point about the proxy user row: Thanks for checking on that, it is one case where the mocks don't reflect the feature. The proxy user will never appear in the general users table. Instead, it lives in its own table above the users table and is visible to child users and unrestricted proxy users. Table column count should be correct in that case, but not when it's currently mocked in the users table since we are not displaying some cols for the proxy user.
@@ -270,7 +272,7 @@ export const UserMenu = React.memo(() => { | |||
> | |||
<Stack data-qa-user-menu minWidth={250} spacing={2}> | |||
{canSwitchBetweenParentOrProxyAccount && ( | |||
<Typography>You are currently logged in as:</Typography> | |||
<Typography>Current account:</Typography> |
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.
For posterity: this was an ask by UX, so it's an update to stay true to the mocks. It does feel a bit different from other Cloud Manager helper text in language/tone. (To me, "Current user" seems like a console statement just printed to the screen 😅.) Perhaps we can revisit in the future with feedback from stakeholders (Product, resellers).
const caseKey = isCloseAccountDisabled ? userType : 'default'; | ||
|
||
switch (caseKey) { |
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.
Slightly unfortunate to also have a user_type
named "default" when the default case of the switch is for the parent user. Wondering if this will be a cause for confusion in the future.
packages/manager/cypress/e2e/core/account/personal-access-tokens.spec.ts
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.
Verified all copy changes. One change is different from the PR description.
Testing via MSW, to unable to verify bug fixes to Billing Contact Info and Login Toast.
: undefined | ||
<Button | ||
tooltipText={ | ||
isProxyUser ? PROXY_USER_RESTRICTED_TOOLTIP_TEXT : undefined |
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 tooltip now reads "You can't perform this action on child accounts." instead of "You can’t create access tokens for child accounts. Instead, switch to your account and create the token there." -- just verifying this is an intentional change.
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.
Yes - let me update PR 👍
Description 📝
This PR addresses some updating messaging and account management fixes / improvements.
Changes 🔄
Target release date 🗓️
3/18
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
parent
toproxy
in [MSW] (https://github.com/linode/manager/blob/develop/packages/manager/src/mocks/serverHandlers.ts#L558) when needed.Verification steps
As an Author I have considered 🤔
Check all that apply