sts-sponsors team mailing list archive

[Jones Ogolo <mp+441453@xxxxxxxxxxxxxxxxxx>]

Jones Ogolo has proposed merging ~jonesogolo/maas-site-manager:1570-fix-login-navbar into maas-site-manager:main.

Commit message:
Enable sidebar on login page

Requested reviews:
  MAAS Lander (maas-lander): unittests
  MAAS Committers (maas-committers)

For more details, see:
https://code.launchpad.net/~jonesogolo/maas-site-manager/+git/maas-site-manager/+merge/441453

QA Steps:
- Logout of the app if logged in.
- on '/login' ensure there is a side nav and no top nav on large screens.
- ensure the in app links are not visible
-- 
Your team MAAS Committers is requested to review the proposed merge of ~jonesogolo/maas-site-manager:1570-fix-login-navbar into maas-site-manager:main.

diff --git a/frontend/src/components/MainLayout/MainLayout.tsx b/frontend/src/components/MainLayout/MainLayout.tsx
index 4ab076f..a73b8bb 100644
--- a/frontend/src/components/MainLayout/MainLayout.tsx
+++ b/frontend/src/components/MainLayout/MainLayout.tsx
@@ -1,7 +1,7 @@
 import type { PropsWithChildren } from "react";
 import { useEffect } from "react";
 
-import { Col, Row, Strip, useOnEscapePressed, usePrevious } from "@canonical/react-components";
+import { Col, Row, useOnEscapePressed, usePrevious } from "@canonical/react-components";
 import classNames from "classnames";
 import { Outlet, useLocation } from "react-router-dom";
 
@@ -9,9 +9,8 @@ import { routesConfig } from "@/base/routesConfig";
 import type { RoutePath } from "@/base/routesConfig";
 import DocumentTitle from "@/components/DocumentTitle/DocumentTitle";
 import Navigation from "@/components/Navigation";
-import NavigationBanner from "@/components/Navigation/NavigationBanner";
 import RemoveRegions from "@/components/RemoveRegions";
-import { useAppContext } from "@/context";
+import { useAppContext, useAuthContext } from "@/context";
 import TokensCreate from "@/pages/tokens/create";
 
 export const sidebarLabels: Record<"removeRegions" | "createToken", string> = {
@@ -44,33 +43,12 @@ const getPageTitle = (pathname: RoutePath) => {
   return title ? `${title} | MAAS Site Manager` : "MAAS Site Manager";
 };
 
-const LoginLayout: React.FC = () => {
-  return (
-    <div className="l-application">
-      <header className="l-navigation-bar is-pinned">
-        <div className="p-panel is-dark">
-          <div className="p-panel__header">
-            <NavigationBanner />
-          </div>
-        </div>
-      </header>
-      <main className="l-main">
-        <div>
-          <Strip element="section" includeCol={false} shallow>
-            <Col size={12}>
-              <Outlet />
-            </Col>
-          </Strip>
-        </div>
-      </main>
-    </div>
-  );
-};
-
 const MainLayout: React.FC = () => {
   const { sidebar, setSidebar } = useAppContext();
   const { pathname } = useLocation();
   const previousPathname = usePrevious(pathname);
+  const { status } = useAuthContext();
+  const isLoggedIn = status === "authenticated";
 
   // close any open panels on route change
   useEffect(() => {
@@ -82,7 +60,7 @@ const MainLayout: React.FC = () => {
   return (
     <>
       <div className="l-application">
-        <Navigation />
+        <Navigation isLoggedIn={isLoggedIn} />
         <main className="l-main is-maas-site-manager">
           <div className="row">
             <div className="col-12">
@@ -108,7 +86,7 @@ const Layout = () => {
   return (
     <>
       <DocumentTitle>{getPageTitle(pathname as RoutePath)}</DocumentTitle>
-      {pathname === "/login" ? <LoginLayout /> : <MainLayout />}
+      <MainLayout />
     </>
   );
 };
diff --git a/frontend/src/components/Navigation/Navigation.test.tsx b/frontend/src/components/Navigation/Navigation.test.tsx
index e0609fc..a5715e2 100644
--- a/frontend/src/components/Navigation/Navigation.test.tsx
+++ b/frontend/src/components/Navigation/Navigation.test.tsx
@@ -8,7 +8,7 @@ describe("Navigation", () => {
   it("displays navigation", () => {
     render(
       <MemoryRouter initialEntries={[{ pathname: "/", key: "testKey" }]}>
-        <Navigation />
+        <Navigation isLoggedIn />
       </MemoryRouter>,
     );
     expect(screen.getByRole("navigation")).toBeInTheDocument();
@@ -18,7 +18,7 @@ describe("Navigation", () => {
     it(`highlights ${label} navigation item when active`, () => {
       render(
         <MemoryRouter initialEntries={[{ pathname: url, key: "testKey" }]}>
-          <Navigation />
+          <Navigation isLoggedIn />
         </MemoryRouter>,
       );
 
@@ -32,7 +32,7 @@ describe("Navigation", () => {
   it.skip("highlights 'Settings' when active", () => {
     const { rerender } = render(
       <MemoryRouter initialEntries={[{ pathname: "/tokens", key: "testKey" }]}>
-        <Navigation />
+        <Navigation isLoggedIn />
       </MemoryRouter>,
     );
 
@@ -42,7 +42,7 @@ describe("Navigation", () => {
 
     rerender(
       <MemoryRouter initialEntries={[{ pathname: "/requests", key: "testKey" }]}>
-        <Navigation />
+        <Navigation isLoggedIn />
       </MemoryRouter>,
     );
 
@@ -52,7 +52,7 @@ describe("Navigation", () => {
 
     rerender(
       <MemoryRouter initialEntries={[{ pathname: "/users", key: "testKey" }]}>
-        <Navigation />
+        <Navigation isLoggedIn />
       </MemoryRouter>,
     );
 
@@ -64,7 +64,7 @@ describe("Navigation", () => {
   it("is collapsed by default", () => {
     render(
       <MemoryRouter initialEntries={[{ pathname: "/", key: "testKey" }]}>
-        <Navigation />
+        <Navigation isLoggedIn />
       </MemoryRouter>,
     );
     expect(screen.getByRole("navigation")).toHaveClass("is-collapsed");
@@ -73,7 +73,7 @@ describe("Navigation", () => {
   it("persists collapsed state", async () => {
     const { rerender } = render(
       <MemoryRouter initialEntries={[{ pathname: "/", key: "testKey" }]}>
-        <Navigation />
+        <Navigation isLoggedIn />
       </MemoryRouter>,
     );
 
@@ -83,10 +83,42 @@ describe("Navigation", () => {
 
     rerender(
       <MemoryRouter initialEntries={[{ pathname: "/", key: "testKey" }]}>
-        <Navigation />
+        <Navigation isLoggedIn />
       </MemoryRouter>,
     );
 
     expect(primaryNavigation).toHaveClass("is-pinned");
   });
+
+  it("does not display collapsible button when user is logged out", () => {
+    render(
+      <MemoryRouter initialEntries={[{ pathname: "/login", key: "testKey" }]}>
+        <Navigation isLoggedIn={false} />
+      </MemoryRouter>,
+    );
+
+    expect(screen.queryByRole("button", { name: /expand main navigation/i })).not.toBeInTheDocument();
+  });
+
+  it("does not display in-app navigation links when logged out", () => {
+    render(
+      <MemoryRouter initialEntries={[{ pathname: "/", key: "testKey" }]}>
+        <Navigation isLoggedIn={false} />
+      </MemoryRouter>,
+    );
+
+    navItems.map(({ label }) =>
+      expect(screen.queryByRole("link", { name: new RegExp(label, "i") })).not.toBeInTheDocument(),
+    );
+  });
+
+  it("should not be collapsed when user is logged out", () => {
+    render(
+      <MemoryRouter initialEntries={[{ pathname: "/", key: "testKey" }]}>
+        <Navigation isLoggedIn={false} />
+      </MemoryRouter>,
+    );
+
+    expect(screen.getByRole("navigation")).toHaveClass("is-pinned");
+  });
 });
diff --git a/frontend/src/components/Navigation/Navigation.tsx b/frontend/src/components/Navigation/Navigation.tsx
index a61ee34..5cd0934 100644
--- a/frontend/src/components/Navigation/Navigation.tsx
+++ b/frontend/src/components/Navigation/Navigation.tsx
@@ -31,7 +31,11 @@ const navItemsBottom: NavGroup[] = [
 
 const navItemsAccount = [{ label: "Log out", url: "/logout" }];
 
-const Navigation = (): JSX.Element => {
+type NavProps = {
+  isLoggedIn: boolean;
+};
+
+const Navigation = ({ isLoggedIn }: NavProps): JSX.Element => {
   const [isCollapsed, setIsCollapsed] = useLocalStorageState<boolean>("appSideNavIsCollapsed", { defaultValue: true });
   const location = useLocation();
   const path = location.pathname;
@@ -58,22 +62,27 @@ const Navigation = (): JSX.Element => {
       </header>
       <nav
         aria-label="main"
-        className={classNames("l-navigation", { "is-collapsed": isCollapsed, "is-pinned": !isCollapsed })}
+        className={classNames("l-navigation", {
+          "is-collapsed": isCollapsed,
+          "is-pinned": !isCollapsed || !isLoggedIn,
+        })}
       >
         <div className="l-navigation__drawer">
           <div className="p-panel is-dark">
             <div className="p-panel__header is-sticky">
               <NavigationBanner>
                 <div className="l-navigation__controls">
-                  <NavigationCollapseToggle isCollapsed={isCollapsed} setIsCollapsed={setIsCollapsed} />
+                  {isLoggedIn && <NavigationCollapseToggle isCollapsed={isCollapsed} setIsCollapsed={setIsCollapsed} />}
                 </div>
               </NavigationBanner>
             </div>
-            <div className="p-panel__content">
-              <NavigationList hasIcons isDark items={navItems} path={path} />
-              <NavigationList hasIcons isDark items={navItemsBottom} path={path} />
-              <NavigationList hasIcons isDark items={navItemsAccount} path={path} />
-            </div>
+            {isLoggedIn && (
+              <div className="p-panel__content">
+                <NavigationList hasIcons isDark items={navItems} path={path} />
+                <NavigationList hasIcons isDark items={navItemsBottom} path={path} />
+                <NavigationList hasIcons isDark items={navItemsAccount} path={path} />
+              </div>
+            )}
           </div>
         </div>
       </nav>