fix(oidc): Display spinner in OIDC login button during loading (#1840)

fix(oidc): Display spinner in OIDC login button during loading
canonical · Jan 31, 2025 · 3b25638 · 3b25638
1 parent d1b7cfe
commit 3b25638
Show file tree

Hide file tree

Showing 13 changed files with 137 additions and 27 deletions.
diff --git a/src/components/LogIn/LogIn.test.tsx b/src/components/LogIn/LogIn.test.tsx
@@ -98,8 +98,10 @@ describe("LogIn", () => {
   it("renders a login error if one exists", () => {
     const state = rootStateFactory.build({
       general: generalStateFactory.withConfig().build({
-        loginErrors: {
-          "wss://controller.example.com": "Controller rejected request",
+        login: {
+          errors: {
+            "wss://controller.example.com": "Controller rejected request",
+          },
         },
         config: configFactory.build({
           authMethod: AuthMethod.LOCAL,
@@ -115,8 +117,8 @@ describe("LogIn", () => {
   it("renders invalid username login errors", () => {
     const state = rootStateFactory.build({
       general: generalStateFactory.withConfig().build({
-        loginErrors: {
-          "wss://controller.example.com": ErrorResponse.INVALID_TAG,
+        login: {
+          errors: { "wss://controller.example.com": ErrorResponse.INVALID_TAG },
         },
         config: configFactory.build({
           authMethod: AuthMethod.LOCAL,
@@ -130,8 +132,10 @@ describe("LogIn", () => {
   it("renders invalid field errors", () => {
     const state = rootStateFactory.build({
       general: generalStateFactory.withConfig().build({
-        loginErrors: {
-          "wss://controller.example.com": ErrorResponse.INVALID_FIELD,
+        login: {
+          errors: {
+            "wss://controller.example.com": ErrorResponse.INVALID_FIELD,
+          },
         },
         config: configFactory.build({
           authMethod: AuthMethod.LOCAL,

diff --git a/src/components/LogIn/OIDCForm/OIDCForm.test.tsx b/src/components/LogIn/OIDCForm/OIDCForm.test.tsx
@@ -1,6 +1,10 @@
 import { screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
 
 import { endpoints } from "juju/jimm/api";
+import * as dashboardStore from "store/store";
+import { rootStateFactory } from "testing/factories";
+import { generalStateFactory } from "testing/factories/general";
 import { renderComponent } from "testing/utils";
 
 import { Label } from "../types";
@@ -14,4 +18,35 @@ describe("OIDCForm", () => {
       screen.getByRole("link", { name: Label.LOGIN_TO_DASHBOARD }),
     ).toHaveAttribute("href", endpoints().login);
   });
+
+  it("should render spinner after getting redirected", () => {
+    const state = rootStateFactory.build({
+      general: generalStateFactory.build({
+        login: {
+          errors: {},
+          loading: true,
+        },
+      }),
+    });
+    renderComponent(<OIDCForm />, { state });
+    expect(
+      screen.getByRole("button", { name: Label.LOADING }),
+    ).toBeInTheDocument();
+  });
+
+  it("should dispatch event to update loading state on click", async () => {
+    const mockUseAppDispatch = vi.fn().mockReturnValue({
+      then: vi.fn().mockReturnValue({ catch: vi.fn() }),
+    });
+    vi.spyOn(dashboardStore, "useAppDispatch").mockReturnValue(
+      mockUseAppDispatch,
+    );
+    renderComponent(<OIDCForm />);
+    await userEvent.click(
+      screen.getByRole("link", { name: Label.LOGIN_TO_DASHBOARD }),
+    );
+    expect(mockUseAppDispatch.mock.calls[0][0]).toMatchObject({
+      type: "general/updateLoginLoading",
+    });
+  });
 });
diff --git a/src/components/LogIn/OIDCForm/OIDCForm.tsx b/src/components/LogIn/OIDCForm/OIDCForm.tsx
@@ -1,16 +1,28 @@
-import { Button } from "@canonical/react-components";
+import { Button, Spinner } from "@canonical/react-components";
 
 import { endpoints } from "juju/jimm/api";
+import { actions as generalActions } from "store/general";
+import { getLoginLoading } from "store/general/selectors";
+import { useAppDispatch, useAppSelector } from "store/store";
 
 import { Label } from "../types";
 
 import { TestId } from "./types";
 
 const OIDCForm = () => {
-  return (
+  const dispatch = useAppDispatch();
+  const loginLoading = useAppSelector(getLoginLoading);
+
+  return loginLoading ? (
+    <button className="p-button--neutral" disabled>
+      <Spinner text="Connecting..." />
+    </button>
+  ) : (
     <Button
       appearance="positive"
       element="a"
+      // this is so that a spinner is shown as soon as user interacts with the button
+      onClick={() => dispatch(generalActions.updateLoginLoading(true))}
       href={endpoints().login}
       data-testid={TestId.OIDC_LOGIN}
     >

diff --git a/src/components/LogIn/types.ts b/src/components/LogIn/types.ts
@@ -8,6 +8,7 @@ export enum Label {
   INVALID_FIELD = "Invalid user name or password",
   POLLING_ERROR = "Error when trying to connect and start polling models.",
   LOGIN_TO_DASHBOARD = "Log in to the dashboard",
+  LOADING = "Connecting...",
 }
 
 export enum TestId {

diff --git a/src/pages/ControllersIndex/ControllersIndex.test.tsx b/src/pages/ControllersIndex/ControllersIndex.test.tsx
@@ -112,8 +112,9 @@ describe("Controllers table", () => {
   });
 
   it("displays login errors", async () => {
-    state.general.loginErrors = {
-      "wss://jimm.jujucharms.com/api": "Uh oh!",
+    state.general.login = {
+      errors: { "wss://jimm.jujucharms.com/api": "Uh oh!" },
+      loading: false,
     };
 
     state.juju = jujuStateFactory.build({

diff --git a/src/store/general/reducers.test.ts b/src/store/general/reducers.test.ts
@@ -59,6 +59,16 @@ describe("reducers", () => {
     });
   });
 
+  it("updateLoginLoading", () => {
+    const state = generalStateFactory.build({
+      login: null,
+    });
+    expect(reducer(state, actions.updateLoginLoading(true))).toStrictEqual({
+      ...state,
+      login: { loading: true },
+    });
+  });
+
   it("storeConfig", () => {
     const state = generalStateFactory.build();
     const newConfig = configFactory.build({
@@ -72,7 +82,7 @@ describe("reducers", () => {
 
   it("storeLoginError", () => {
     const state = generalStateFactory.build({
-      loginErrors: { "wss://example.com": "old error" },
+      login: { errors: { "wss://example.com": "old error" } },
     });
     expect(
       reducer(
@@ -84,13 +94,13 @@ describe("reducers", () => {
       ),
     ).toStrictEqual({
       ...state,
-      loginErrors: { "wss://example.com": "new error" },
+      login: { errors: { "wss://example.com": "new error" }, loading: false },
     });
   });
 
   it("storeLoginError creates login errors object if it is null", () => {
     const state = generalStateFactory.build({
-      loginErrors: null,
+      login: null,
     });
     expect(
       reducer(
@@ -102,7 +112,7 @@ describe("reducers", () => {
       ),
     ).toStrictEqual({
       ...state,
-      loginErrors: { "wss://example.com": "new error" },
+      login: { errors: { "wss://example.com": "new error" }, loading: false },
     });
   });
 
@@ -205,11 +215,13 @@ describe("reducers", () => {
 
   it("cleanupLoginErrors", () => {
     const state = generalStateFactory.build({
-      loginErrors: { "wss://example.com": "Uh oh!" },
+      login: { errors: { "wss://example.com": "Uh oh!" } },
     });
     expect(reducer(state, actions.cleanupLoginErrors())).toStrictEqual({
       ...state,
-      loginErrors: null,
+      login: {
+        errors: {},
+      },
     });
   });
 

diff --git a/src/store/general/selectors.test.ts b/src/store/general/selectors.test.ts
@@ -31,6 +31,7 @@ import {
   getAnalyticsEnabled,
   isReBACEnabled,
   getControllerUserTag,
+  getLoginLoading,
 } from "./selectors";
 
 describe("selectors", () => {
@@ -120,7 +121,7 @@ describe("selectors", () => {
       getLoginErrors(
         rootStateFactory.build({
           general: generalStateFactory.build({
-            loginErrors: { "wss://example.com": "error" },
+            login: { errors: { "wss://example.com": "error" } },
           }),
         }),
       ),
@@ -132,14 +133,26 @@ describe("selectors", () => {
       getLoginError(
         rootStateFactory.build({
           general: generalStateFactory.build({
-            loginErrors: { "wss://example.com": "error" },
+            login: { errors: { "wss://example.com": "error" } },
           }),
         }),
         "wss://example.com",
       ),
     ).toBe("error");
   });
 
+  it("getLoginLoading", () => {
+    expect(
+      getLoginLoading(
+        rootStateFactory.build({
+          general: generalStateFactory.build({
+            login: { loading: true },
+          }),
+        }),
+      ),
+    ).toStrictEqual(true);
+  });
+
   it("getPingerIntervalIds", () => {
     const pingerIntervalIds = {
       "wss://example.com": 12,

diff --git a/src/store/general/selectors.ts b/src/store/general/selectors.ts
@@ -67,7 +67,17 @@ export const getConnectionError = createSelector(
 */
 export const getLoginErrors = createSelector(
   [slice],
-  (sliceState) => sliceState?.loginErrors,
+  (sliceState) => sliceState?.login?.errors,
+);
+
+/**
+  Fetches login loading state from state
+  @param state The application state.
+  @returns The collection of error messages if any.
+*/
+export const getLoginLoading = createSelector(
+  [slice],
+  (sliceState) => sliceState?.login?.loading,
 );
 
 /**

diff --git a/src/store/general/slice.ts b/src/store/general/slice.ts
@@ -12,13 +12,16 @@ const slice = createSlice({
     controllerConnections: null,
     controllerFeatures: null,
     credentials: null,
-    loginErrors: null,
+    login: null,
     pingerIntervalIds: null,
     visitURLs: null,
   } as GeneralState,
   reducers: {
     cleanupLoginErrors: (state) => {
-      state.loginErrors = null;
+      state.login = {
+        ...state.login,
+        errors: {},
+      };
     },
     updateControllerConnection: (state, action) => {
       const connections = state.controllerConnections ?? {};
@@ -46,10 +49,13 @@ const slice = createSlice({
       state,
       action: PayloadAction<{ wsControllerURL: string; error: string }>,
     ) => {
-      if (!state.loginErrors) {
-        state.loginErrors = {};
+      if (!state.login) {
+        state.login = {};
       }
-      state.loginErrors[action.payload.wsControllerURL] = action.payload.error;
+      state.login = {
+        loading: false,
+        errors: { [action.payload.wsControllerURL]: action.payload.error },
+      };
     },
     storeUserPass: (state, action) => {
       const credentials = state.credentials ?? {};
@@ -86,6 +92,12 @@ const slice = createSlice({
       intervals[action.payload.wsControllerURL] = action.payload.intervalId;
       state.pingerIntervalIds = intervals;
     },
+    updateLoginLoading: (state, action: PayloadAction<boolean>) => {
+      if (!state.login) {
+        state.login = {};
+      }
+      state.login.loading = action.payload;
+    },
   },
 });
 

diff --git a/src/store/general/types.ts b/src/store/general/types.ts
@@ -38,7 +38,10 @@ export type ControllerFeatures = {
 
 export type ControllerFeaturesState = Record<string, ControllerFeatures>;
 
-export type LoginErrors = Record<string, string>;
+export type Login = {
+  errors?: Record<string, string>;
+  loading?: boolean;
+};
 
 export type GeneralState = {
   appVersion: string | null;
@@ -47,7 +50,7 @@ export type GeneralState = {
   controllerConnections: ControllerConnections | null;
   controllerFeatures: ControllerFeaturesState | null;
   credentials: Credentials | null;
-  loginErrors: LoginErrors | null;
+  login: Login | null;
   pingerIntervalIds: PingerIntervalIds | null;
   visitURLs: string[] | null;
 };
diff --git a/src/store/middleware/check-auth.ts b/src/store/middleware/check-auth.ts
@@ -73,6 +73,7 @@ export const checkAuthMiddleware: Middleware<
       generalActions.storeVisitURL.type,
       generalActions.updateControllerConnection.type,
       generalActions.updatePingerIntervalId.type,
+      generalActions.updateLoginLoading.type,
       jimmListeners.pollWhoamiStart.type,
       jimmListeners.pollWhoamiStop.type,
       jujuActions.populateMissingAllWatcherData.type,

diff --git a/src/store/middleware/model-poller.ts b/src/store/middleware/model-poller.ts
@@ -64,6 +64,7 @@ export const modelPollerMiddleware: Middleware<
         let intervalId: number | null | undefined;
         if (authMethod === AuthMethod.OIDC) {
           try {
+            reduxStore.dispatch(generalActions.updateLoginLoading(true));
             const whoamiResponse = await reduxStore.dispatch(whoami());
             const user = unwrapResult(whoamiResponse);
             if (user) {
@@ -73,6 +74,7 @@ export const modelPollerMiddleware: Middleware<
             } else {
               // If there's no response that means the user is not
               // authenticated, so halt the connection attempt.
+              reduxStore.dispatch(generalActions.updateLoginLoading(false));
               return;
             }
           } catch (error) {
@@ -113,6 +115,10 @@ export const modelPollerMiddleware: Middleware<
             }),
           );
           return console.log(LoginError.LOG, e, controllerData);
+        } finally {
+          if (authMethod === AuthMethod.OIDC) {
+            reduxStore.dispatch(generalActions.updateLoginLoading(false));
+          }
         }
 
         if (!conn?.info || !Object.keys(conn.info).length) {

diff --git a/src/testing/factories/general.ts b/src/testing/factories/general.ts
@@ -56,7 +56,7 @@ export const generalStateFactory = GeneralStateFactory.define(() => ({
   controllerConnections: null,
   controllerFeatures: null,
   credentials: null,
-  loginErrors: null,
+  login: null,
   pingerIntervalIds: null,
   visitURLs: null,
 }));