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(navigation): Prevent secondary navigation from displaying without auth MAASENG-1986 #5067

Conversation

ndv99
Copy link
Contributor

@ndv99 ndv99 commented Jul 19, 2023

Done

  • Prevent secondary navigation from displaying if user is not authenticated or connected
  • Add tests to ensure secondary nav can display on /settings and /account routes
  • Add tests to ensure secondary nav does not display without auth or connection

QA

MAAS deployment

To run this branch you will need access to one of the following MAAS deployments:

Running the branch

You can run this branch by:

QA steps

  1. Go to /settings
  2. Log out
  3. Ensure navigation does not display
  4. Log in
  5. Go to /account
  6. Repeat steps 2-4
  7. Stop MAAS-UI in the terminal
  8. Ensure side navigation does not display

Fixes

Fixes MAASENG-1986

@webteam-app
Copy link

Demo starting at https://maas-ui-5067.demos.haus

Comment on lines +42 to +75
it("shows the secondary navigation for settings", () => {
state.status.authenticated = true;
state.status.connected = true;
renderWithBrowserRouter(
<PageContent
header="Settings"
sidePanelContent={null}
sidePanelTitle={null}
>
content
</PageContent>,
{ route: "/settings/configuration/general", state }
);

expect(screen.getByRole("navigation")).toBeInTheDocument();

settingsNavItems.forEach((item) => {
expect(screen.getByText(item.label)).toBeInTheDocument();
});
});

it("shows the secondary navigation for preferences", () => {
state.status.authenticated = true;
state.status.connected = true;
renderWithBrowserRouter(
<PageContent
header="Preferences"
sidePanelContent={null}
sidePanelTitle={null}
>
content
</PageContent>,
{ route: "/account/prefs/details", state }
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine as is, but generally I'd be for merging these 2 tests into 1.
https://kentcdodds.com/blog/write-fewer-longer-tests

src/app/base/components/PageContent/PageContent.tsx Outdated Show resolved Hide resolved
@ndv99 ndv99 merged commit a71e3d5 into canonical:main Jul 20, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants