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-v2] - Fix email displaying in top menu for all users without a company name #10276

Merged

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Mar 12, 2024

Description 📝

In #10248, I didn't test the regular user experience thoroughly enough, because the conditional logic was defaulting to the profile email of the user if no company name was set, even if the user was not a parent/child/proxy account. We don't want this.

We should see the following:

  • For a regular (default) user, there should be no changes to the top menu and user menu (i.e. it should display username only).
  • For parent/child/proxy users, the top menu should display username and company names.
  • For parent/proxy users, the user menu dropdown should display the company name.
  • For restricted parent users who do not have account_access set to at least read_only, the top menu should display username and email (in place of company).
    For restricted parent users who do not have account_access set to at least read_only, the dropdown menu should display email (in place of company).

Changes 🔄

  • Fixed the conditional logic to check if the user is a parent user and is missing an account company in order to resort to email, otherwise defaults to undefined.
  • Added the other piece of unit test coverage that was missing for this edge-case, confirming a regular user does not see their email in the top menu or dropdown user menu.

Target release date 🗓️

3/18

Preview 📷

Before After
Screenshot 2024-03-12 at 8 53 37 AM Screenshot 2024-03-12 at 8 52 57 AM

How to test 🧪

Prerequisites

(How to setup test environment)

  • You can use the MSW to toggle the different user types, or you can use real parent/child/proxy and default user accounts (ask me).

Reproduction steps

(How to reproduce the issue, if applicable)

  • Check out develop.
  • Observe for your regular user account, the top menu now includes your email if you don't have a company name associated with your account.

Verification steps

(How to verify changes)

  • Check out this branch.
  • With mocks off: confirm that the top menu and user menu dropdown include only your username (no email). There should be no change from prod.
  • With mocks on: update the user_type to parent, proxy, child, and default in the MSW. For a restricted parent user, you'd want to mock a 403 on the account endpoint.
      rest.get('*/account', (req, res, ctx) => {
        return res(ctx.status(403));
      }),
  • For each user_type, confirm that the top menu and user menu match the description outlined in the PR description.

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 - Note: this was a bug fix to a bug fix, and did not ever make it into a release so I left the changeset off.
  • 🧪 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 added Bug Fixes for regressions or bugs Parent / Child Account labels Mar 12, 2024
@mjac0bs mjac0bs self-assigned this Mar 12, 2024
Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for the tests! These changes are now working - not seeing email above my normal user accounts

@mjac0bs mjac0bs marked this pull request as ready for review March 12, 2024 19:19
@mjac0bs mjac0bs requested a review from a team as a code owner March 12, 2024 19:19
@mjac0bs mjac0bs requested review from bnussman-akamai and abailly-akamai and removed request for a team March 12, 2024 19:19
Comment on lines 85 to 92
const companyNameOrEmail =
hasParentChildAccountAccess &&
profile?.user_type !== 'default' &&
account?.company
? account.company
: profile?.email;
: isParentUser && profile?.email
? profile.email
: undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Should we break this out into a util function and unit test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's fair, it's probably the better approach than to add every test case related to this to the UserMenu. I'll revisit.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good! I think both testing approaches are fine, but I do think a helper function using if/else would be more readable than the ternaries here.

Feel free to revisit later if it will keep parent/child unblocked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Mar 12, 2024
Copy link

github-actions bot commented Mar 13, 2024

Coverage Report:
Base Coverage: 81.39%
Current Coverage: 81.39%

@mjac0bs mjac0bs merged commit 3cd0132 into linode:develop Mar 13, 2024
18 checks passed
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! Bug Fixes for regressions or bugs Parent / Child Account
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants