-
Notifications
You must be signed in to change notification settings - Fork 373
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-7430] - Implement Account Switching Functionality #10064
Conversation
const proxyToken = await getProxyToken({ euuid, headers }); | ||
|
||
// We don't need to worry about this if we're a proxy user. | ||
if (!isProxyUser) { |
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.
Minor/nit: the readability of this sequence could be improved slightly IMO– I was confused how we could get a proxy token AS another proxy user. (It became clearer once I saw how the headers
argument was used).
Conceptually, maybe the getProxyToken
function itself should decide what the headers should be based on whether it's a parent/sibling account? (Instead of taking in arbitrary headers).
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.
Great suggestion - I like that idea! 👍
}); | ||
} catch (error) { | ||
setIsProxyTokenError(error as APIError[]); | ||
throw error; |
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 will cause the try block in handleSwitchToChildAccount
to error.
euuid, | ||
token: isProxyUser | ||
? getStorage('authentication/parent_token/token') | ||
: currentTokenWithBearer, |
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 'proxy' userType, use the stored parent token in the request. Otherwise, if 'parent' use current token.
}, [childAccounts, error, isLoading]); | ||
updateCurrentTokenBasedOnUserType({ userType: 'parent' }); | ||
handleClose(); | ||
}, [handleClose, isProxyUser]); |
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.
Switching back to parent is simpler. Validate parent token in local storage and swap to it.
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.
Here's some more feedback; I will give this an additional pass through to make sure that all loading and error states are addressed once this stuff is addressed.
Noteworthy:
- Without a page reload, I don't think we can actually account switch, even if we were off mocks. Updating local storage isn't enough without a refresh. Is that not the case?
- We lost a feature flag in
canSwitchBetweenParentOrProxyAccount
and so there's still a regression in the user menu.
Looks like there's a failure on SwitchAccountDrawer.test.tsx
that probably came about when ChildAccountList
was pulled out into its own component:
packages/manager/src/features/Account/SwitchAccounts/ChildAccountList.tsx
Outdated
Show resolved
Hide resolved
if (userToken) { | ||
setStorage('authentication/token', userToken); | ||
setStorage('authentication/scopes', userScope); | ||
setStorage('authentication/expire', userExpiry); | ||
} |
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.
In the POC, after swapping the tokens in local storage, we then had to trigger a page reload in order for the account switch to occur. Wouldn't that still be the case here (even though we can't test this yet easily without API returning a valid user_type
)?
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.
@mjac0bs I'm using the useHistory
as to not force an entire page refresh. We'll have to test this when the API is ready.
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.
I gave that a try back in my old POC branch to try account switching with real accounts off mocks and see what would happen, and wasn't able to successfully account switch so not sure about this. But I've forgotten exactly why it was that we needed the refresh for the new token to take affect, so if you want to leave as is until we can test for certain in the dev environment, that's fine.
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.
Thanks for adding this!
const userName = | ||
(hasParentChildAccountAccess && isProxyUser ? parentProfile : profile) | ||
?.username ?? ''; |
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.
Much cleaner than the function that was doing this before - thanks!
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
…untList.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Coverage Report: ✅ |
I've verified the switch account flow as described in the reproduction steps. I was able to observe the updates to the Regarding the note on the |
Toasts should be removed for error states 👍 |
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.
Thanks for addressing all the little pieces of feedback! 🙌🏼
Confirmed:
- When feature flag is off, no regressions in user menu.
- When feature flag is on with
user_type: null
, no regressions in user menu. - When feature flag is on with
user_type: child
, no regressions in user menu. - When feature flag is on with
user_type: parent
, user menu displays the parent's company name with a clickable Switch Account button, which opens the Switch Account drawer. Clicking a child account swaps the proxy token into local storage active token as saves the current token as the local storage parent token. - When feature flag is on with
user_type: proxy
, user menu displays the child's company name with a clickable Switch Account button, which opens the Switch Account drawer. Drawer allows user to select another proxy account or switch back to parent account (swapping parent token into local storage active token). - Switch Account drawer gracefully handles an error when switching to a child account (shows error notice in drawer).
- Switch Account drawer gracefully handles loading state and error loading child accounts (shows error copy and Try Again button in drawer, which tries to load accounts again when clicked).
- Switch Account drawer gracefully handles error with expired parent token when switching back to parent account from proxy (shows error notice in drawer).
- Same behavior as above for the Switch Account button on the Account Landing page.
Notes:
- I don't think we have any loading state when clicking a child account and completing the account switch. May be worth adding as a follow up.
This LGTM. I'm still not certain the actual switching will work as expected, as I commented, but we know token swapping is. Looking forward to re-confirming all this with actual API data.
I'll make a note, it may get lost with the refresh / switch itself, but something we can test against API. |
Description 📝
Presently, there's no way for resellers to seamlessly log into indirect customer accounts. To overcome this obstacle, we aim to introduce a new feature. This feature allows users within the reseller account to choose an indirect customer account, obtaining the EUUID (Entity Unique User ID) in the process. Subsequently, this EUUID becomes instrumental in making a necessary endpoint call, triggering the generation of an ephemeral token allowing for the reseller to authenticate into a proxy account within the main indirect reseller's account. The primary goal of this token is to streamline the transition, providing reseller users with a frictionless experience when logging into the proxy account.
Changes 🔄
ChildAccountList.tsx
/authentication/
token as the active account.SwitchAccountButton.tsx
Preview 📷
Note
All PAT in this demo have been revoked. 👌
parent-account-switching.mp4
Note
We will most likely be removing the toast when some of the errors are shown (as in not to show both a notice and a toast). While this is still in draft, I wanted to get some UX input.
error-switching-to-child.mp4
- Error if parent token is expired
indirect-customer-login-and-expiry.mp4
How to test 🧪
Important
You won't see any actual UI changes when performing the switch account feature until we're off mocks.
Prerequisites
(How to setup test environment)
REACT_APP_PROXY_PAT
to your.env
file with a temporary PAT that you've created (could be the same account for testing)Parent / Child Account
feature flag{ user_type: 'parent' }
in serverHandlers.tsReproduction & Verification steps
(How to reproduce the issue, if applicable)
⭐ Parent View
authentication/token
authentication/parent_token/token
authentication/token
andauthentication/proxy_token/token
contain the same token indicating we've switched to a proxy account.⭐ Proxy View: Switching Into Another Proxy
authentication/token
authentication/parent_token/token
remains the sameauthentication/token
andauthentication/proxy_token/token
have new token values⭐ Proxy View: Switching Back to Parent
authentication/token
(should still mirror proxy token){ user_type: 'proxy' }
in serverHandlers.tsauthentication/token
token value matchesauthentication/parent_token/token
As an Author I have considered 🤔
Check all that apply