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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ describe('UserMenu', () => {
expect(getByRole('button')).toBeInTheDocument();
});

it("shows a parent user's username and company name for a parent user", async () => {
it("shows a parent user's username and company name in the TopMenu for a parent user", async () => {
server.use(
rest.get('*/account', (req, res, ctx) => {
return res(
Expand All @@ -44,7 +44,7 @@ describe('UserMenu', () => {
expect(await findByText('Parent Company')).toBeInTheDocument();
});

it("shows the parent user's username and child company name for a proxy user", async () => {
it("shows the parent user's username and child company name in the TopMenu for a proxy user", async () => {
server.use(
rest.get('*/account', (req, res, ctx) => {
return res(
Expand All @@ -71,7 +71,7 @@ describe('UserMenu', () => {
expect(await findByText('Child Company')).toBeInTheDocument();
});

it("shows the child user's username and company name for a child user", async () => {
it("shows the child user's username and company name in the TopMenu for a child user", async () => {
server.use(
rest.get('*/account', (req, res, ctx) => {
return res(
Expand All @@ -95,7 +95,7 @@ describe('UserMenu', () => {
expect(await findByText('Child Company')).toBeInTheDocument();
});

it("shows the user's username and no company name for a regular user", async () => {
it("shows the user's username and no company name in the TopMenu for a regular user", async () => {
server.use(
rest.get('*/account', (req, res, ctx) => {
return res(ctx.json(accountFactory.build({ company: 'Test Company' })));
Expand All @@ -117,9 +117,37 @@ describe('UserMenu', () => {
});

expect(await findByText('regular-user')).toBeInTheDocument();
// Should not be displayed for regular users, only parent/child/proxy users.
expect(queryByText('Test Company')).not.toBeInTheDocument();
});

it("shows the user's username and no email in the TopMenu for a regular user without a company name", async () => {
server.use(
rest.get('*/account', (req, res, ctx) => {
return res(ctx.json(accountFactory.build({ company: undefined })));
}),
rest.get('*/profile', (req, res, ctx) => {
return res(
ctx.json(
profileFactory.build({
email: 'user@email.com',
user_type: 'default',
username: 'regular-user',
})
)
);
})
);

const { findByText, queryByText } = renderWithTheme(<UserMenu />, {
flags: { parentChildAccountAccess: true },
});

expect(await findByText('regular-user')).toBeInTheDocument();
// Should not be displayed for regular restricted users, only restricted parents.
expect(queryByText('user@email.com')).not.toBeInTheDocument();
});

it('shows the parent company name and Switch Account button in the dropdown menu for a parent user', async () => {
server.use(
rest.get('*/account', (req, res, ctx) => {
Expand All @@ -145,7 +173,7 @@ describe('UserMenu', () => {
expect(within(userMenuPopover).getByText('Switch Account')).toBeVisible();
});

it('hides Switch Account button for parent accounts lacking child_account_access', async () => {
it('hides Switch Account button in the dropdown menu for parent accounts lacking child_account_access', async () => {
server.use(
rest.get('*/account/users/*/grants', (req, res, ctx) => {
return res(
Expand Down Expand Up @@ -198,7 +226,7 @@ describe('UserMenu', () => {
expect(within(userMenuPopover).getByText('Switch Account')).toBeVisible();
});

it('shows the parent email for a parent user if their company name is not set', async () => {
it('shows the parent email for a parent user in the top menu and dropdown menu if their company name is unavailable', async () => {
// Mock a forbidden request to the /account endpoint, which happens if Billing (Account) Access is None.
server.use(
rest.get('*/account/users/*/grants', (req, res, ctx) => {
Expand Down
7 changes: 4 additions & 3 deletions packages/manager/src/features/TopMenu/UserMenu/UserMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,15 @@ export const UserMenu = React.memo(() => {
const open = Boolean(anchorEl);
const id = open ? 'user-menu-popover' : undefined;

// If there is no company name to identify an account, fall back on the email.
// Covers an edge case in which a restricted parent user without `account_access` cannot access the account company.
// For parent users lacking `account_access`: without a company name to identify an account, fall back on the email.
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.


const { isParentTokenExpired } = useParentTokenManagement({ isProxyUser });

Expand Down
Loading