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

fix: [M3-7826] - Display parent email in user menu when no company name is available for restricted parent user #10248

Merged

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Mar 1, 2024

Description 📝

We learned on the backend that company may be null for some users, even if they payByAkamai. (Though awaiting final confirmation that this won't change going forward and can be retroactively applied to parent/child accounts when the relationship made on the backend.) Assuming company can still be null, we won't always be able to display the company under the helper text "You are currently logged in as:" in the user menu dropdown for parent and proxy users.

This PR falls back on username in those cases, so we are no longer rendering an empty string in the UserMenu dropdown and success toast.

UPDATE (3/4): Marked as Do Not Merge and Requires Changes because:

  • Product wants us to fall back on email rather than username (pending final confirmation)
    • We'd need to ensure parent email cannot be updated to null
  • The child account list in the Switch Account Drawer uses company name; we would also need to replace with email

UPDATE (3/8): Reopening this PR because we found a case where company is not available: when a restricted parent user has the child_account_access grant enabled (and therefore account-switching abilities) but does not have any Billing Access (account_access: null) -- this means that are not authorized to access account/ and retrieve the company.

Changes 🔄

  • For a proxy user, we fall back on proxy email rather than parent username + child company if we can’t display the child company name.
  • For a parent user, we fall back on parent email if we can’t display the parent company name.
  • Updates test coverage in UserMenu.test.tsx to reflect this.

Preview 📷

Before After
Screenshot 2024-03-01 at 11 35 24 AM Screenshot 2024-03-01 at 11 30 15 AM
Screenshot 2024-03-01 at 7 05 36 AM Screenshot 2024-03-01 at 11 30 30 AM

How to test 🧪

Prerequisites

  • This can be tested with mocks more easily than in the dev environment because you'd need to ask me for provisioned parent/child account access, then log into alpha admin and delete a child account's company name, and make one manual code edit due to an API bug with child_account_access (the grant isn't being returned correctly) to eliminate the final !isChildAccountAccessRestricted check from const canSwitchBetweenParentOrProxyAccount if you want the Switch Account button to show up.

Reproduction steps

  • Look at the diff and notice how this code before was defaulting to company being an empty string if the conditional was false, and then displaying that empty string in the user menu and tooltip.

Verification steps

  • Check out this PR and yarn up.
  • Ensure the parent/child feature flag is on.
    With mocks:
  • Edit the account endpoint here in serverHandlers.ts to return a null company name. Confirm your user_type is parent.
  • Open the user menu and confirm the username is visible. Click the Switch Account button and switch into a proxy account by selecting a child.
  • Edit the profile endpoint in serverHandlers.ts to have a user_type of proxy.
  • Confirm the success toast includes the username.
  • Open the user menu and confirm the username is visible.
  • (Note that it will be the same username ("mock-user") since mocks will use the same profile endpoint.)

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@mjac0bs mjac0bs marked this pull request as ready for review March 1, 2024 20:49
@mjac0bs mjac0bs requested review from a team as code owners March 1, 2024 20:49
@mjac0bs mjac0bs requested review from cliu-akamai, carrillo-erik, hkhalil-akamai and jaalah-akamai and removed request for a team March 1, 2024 20:49
@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Mar 1, 2024
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Looks good when testing with mocks. Unable to verify account switching toasts.

@mjac0bs
Copy link
Contributor Author

mjac0bs commented Mar 5, 2024

Closing this PR as it has been determined that we do not need a fallback for companyName. Elsewhere on the backend, we were incorrectly retrieving that data from a value that could be optional. This will be changed on the backend to a value that is required, so the companyName will always exist.

@mjac0bs mjac0bs closed this Mar 5, 2024
@mjac0bs mjac0bs reopened this Mar 8, 2024
@mjac0bs mjac0bs changed the title fix: [M3-7269] - Display parent/proxy username in user menu and toast if no company name is available fix: [M3-7269] - Display parent/proxy email in user menu and toast if no company name is available Mar 8, 2024
Copy link

github-actions bot commented Mar 8, 2024

Coverage Report:
Base Coverage: 81.4%
Current Coverage: 81.4%

@mjac0bs mjac0bs changed the title fix: [M3-7269] - Display parent/proxy email in user menu and toast if no company name is available fix: [M3-7269] - Display parent email in user menu when no company name is available for restricted parent user Mar 8, 2024
@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Do Not Merge Add'tl Approval Needed Waiting on another approval! Requires Changes labels Mar 8, 2024
@mjac0bs
Copy link
Contributor Author

mjac0bs commented Mar 8, 2024

For posterity: we reopened this PR after finding an edge case where if a restricted parent account does not have Billing Access but does have child_account_access, we can’t access the account endpoint to grab the company. However, with the enabled grant, they do have account switching access, so we had a blank in the user menu again.

Following guidance from Product, we can fall back on parent email here.

Issue:
Screenshot 2024-03-08 at 9 01 19 AM

Fix:
Screenshot 2024-03-08 at 10 00 40 AM

@mjac0bs
Copy link
Contributor Author

mjac0bs commented Mar 11, 2024

We received approval on this from Product so I'm going to go ahead and merge this ahead of the demo since it is a bug that otherwise will be visible.

@mjac0bs mjac0bs merged commit 20c0869 into linode:develop Mar 11, 2024
18 checks passed
vrajesh73 added a commit to vrajesh73/manager that referenced this pull request Mar 12, 2024
…eature/namespace-create

* 'develop' of https://github.com/vrajesh73/manager: (89 commits)
  fix: [M3-7269] - Display parent email in user menu when no company name is available for restricted parent user (linode#10248)
  fix: [M3-7817] - Show correct status of Child Account Enabled column for parent users (linode#10233)
  upcoming: [M3-7616] - Add Placement Groups Events and Notifications (linode#10221)
  upcoming: [M3-7816-v2] - Adjust logic for when to show Switch Account button (linode#10266)
  fix: [M3-7831] - Persisting error messages in ACLB delete dialogs (linode#10254)
  upcoming: [M3-7842] - Update Assign Linode Drawer and improve query skipping (linode#10263)
  upcoming: [M3-7704] - Disable Cloning, Private IP, Backups for edge regions (linode#10222)
  test: Fix test flake for Images landing page test (linode#10267)
  fix: [M3-7824] - ACLB TCP Rule Creation and other fixes (linode#10264)
  refactor: [M3-7687] - Linodes Restricted User Experience 2/2 (linode#10227)
  test: Resolve OBJ create and delete E2E test flake (linode#10245)
  upcoming: [M3-7723] - Placement Group feature flag as object (linode#10256)
  chore(deps): Bump sanitize-html from 2.11.0 to 2.12.1 (linode#10247)
  change: [M3-7813] - Allow the disabling of the TypeToConfirm input (linode#10251)
  upcoming: [M3-7839] - Change Business Partner to Parent User (linode#10259)
  upcoming: [M3-7835] - Adjust user table column count (linode#10252)
  upcoming: [M3 7738] - Update Placement Group Create & Edit Drawers (linode#10205)
  refactor: [M3-7437] - Use `@lukemorales/query-key-factory` for Profile Queries (linode#10241)
  fix: React Query `updateInPaginatedStore` helper function not working as expected (linode#10249)
  test: [M3-7497] - Add tests for child user verification banner (linode#10204)
  ...

# Conflicts:
#	packages/manager/src/MainContent.tsx
#	packages/manager/src/dev-tools/FeatureFlagTool.tsx
@mjac0bs mjac0bs changed the title fix: [M3-7269] - Display parent email in user menu when no company name is available for restricted parent user fix: [M3-7826] - Display parent email in user menu when no company name is available for restricted parent user Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Parent / Child Account
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants