refactor: Refactor update check banner (#5708)

2023-01-13 13:48:45 -03:00 · 2023-01-13 13:48:45 -03:00 · de16e29566
parent d6543c042f
commit de16e29566
9 changed files with 258 additions and 233 deletions
--- a/site/src/app.tsx
+++ b/site/src/app.tsx
@ -1,7 +1,5 @@
 import CssBaseline from "@material-ui/core/CssBaseline"
 import ThemeProvider from "@material-ui/styles/ThemeProvider"
-import { LicenseBanner } from "components/LicenseBanner/LicenseBanner"
-import { ServiceBanner } from "components/ServiceBanner/ServiceBanner"
 import { FC } from "react"
 import { HelmetProvider } from "react-helmet-async"
 import { BrowserRouter as Router } from "react-router-dom"
@ -20,8 +18,6 @@ export const App: FC = () => {
          <CssBaseline />
          <ErrorBoundary>
            <XServiceProvider>
-              <ServiceBanner />
-              <LicenseBanner />
              <AppRouter />
              <GlobalSnackbar />
            </XServiceProvider>
--- a/site/src/components/DashboardLayout/DashboardLayout.tsx
+++ b/site/src/components/DashboardLayout/DashboardLayout.tsx
@ -1,49 +1,56 @@
 import { makeStyles } from "@material-ui/core/styles"
-import { useActor } from "@xstate/react"
+import { useMachine } from "@xstate/react"
 import { Loader } from "components/Loader/Loader"
-import { FC, Suspense, useContext, useEffect } from "react"
-import { XServiceContext } from "../../xServices/StateContext"
+import { FC, Suspense } from "react"
 import { Navbar } from "../Navbar/Navbar"
 import { UpdateCheckBanner } from "components/UpdateCheckBanner/UpdateCheckBanner"
 import { Margins } from "components/Margins/Margins"
 import { Outlet } from "react-router-dom"
+import { LicenseBanner } from "components/LicenseBanner/LicenseBanner"
+import { ServiceBanner } from "components/ServiceBanner/ServiceBanner"
+import { updateCheckMachine } from "xServices/updateCheck/updateCheckXService"
+import { usePermissions } from "hooks/usePermissions"
+import { UpdateCheckResponse } from "api/typesGenerated"

 export const DashboardLayout: FC = () => {
  const styles = useStyles()
-  const xServices = useContext(XServiceContext)
-  const [authState] = useActor(xServices.authXService)
-  const [updateCheckState, updateCheckSend] = useActor(
-    xServices.updateCheckXService,
-  )
-
-  useEffect(() => {
-    if (authState.matches("signedIn")) {
-      updateCheckSend("CHECK")
-    } else {
-      updateCheckSend("CLEAR")
-    }
-  }, [authState, updateCheckSend])
+  const permissions = usePermissions()
+  const [updateCheckState, updateCheckSend] = useMachine(updateCheckMachine, {
+    context: {
+      permissions,
+    },
+  })
+  const { error: updateCheckError, updateCheck } = updateCheckState.context

  return (
-    <div className={styles.site}>
-      <Navbar />
-      {updateCheckState.context.show && (
-        <div className={styles.updateCheckBanner}>
-          <Margins>
-            <UpdateCheckBanner
-              updateCheck={updateCheckState.context.updateCheck}
-              error={updateCheckState.context.error}
-              onDismiss={() => updateCheckSend("DISMISS")}
-            />
-          </Margins>
+    <>
+      <ServiceBanner />
+      <LicenseBanner />
+
+      <div className={styles.site}>
+        <Navbar />
+
+        {updateCheckState.matches("show") && (
+          <div className={styles.updateCheckBanner}>
+            <Margins>
+              <UpdateCheckBanner
+                // We can trust when it is show, the update check is filled
+                // unfortunately, XState does not has typed state - context yet
+                updateCheck={updateCheck as UpdateCheckResponse}
+                error={updateCheckError}
+                onDismiss={() => updateCheckSend("DISMISS")}
+              />
+            </Margins>
+          </div>
+        )}
+
+        <div className={styles.siteContent}>
+          <Suspense fallback={<Loader />}>
+            <Outlet />
+          </Suspense>
        </div>
-      )}
-      <div className={styles.siteContent}>
-        <Suspense fallback={<Loader />}>
-          <Outlet />
-        </Suspense>
      </div>
-    </div>
+    </>
  )
 }

--- a/site/src/components/UpdateCheckBanner/UpdateCheckBanner.test.tsx
+++ b/site/src/components/UpdateCheckBanner/UpdateCheckBanner.test.tsx
@ -1,51 +0,0 @@
-import { fireEvent, screen, waitFor } from "@testing-library/react"
-import i18next from "i18next"
-import { MockUpdateCheck, render } from "testHelpers/renderHelpers"
-import { UpdateCheckBanner } from "./UpdateCheckBanner"
-
-describe("UpdateCheckBanner", () => {
-  it("shows an update notification when one is available", () => {
-    const { t } = i18next
-    render(
-      <UpdateCheckBanner
-        updateCheck={{ ...MockUpdateCheck, current: false }}
-      />,
-    )
-
-    const updateText = t("updateCheck.message", {
-      ns: "common",
-      version: MockUpdateCheck.version,
-    })
-    // Message contatins HTML elements so we check it in parts.
-    for (const text of updateText.split(/<\/?[0-9]+>/)) {
-      expect(screen.getByText(text, { exact: false })).toBeInTheDocument()
-    }
-
-    expect(screen.getAllByRole("link")[0]).toHaveAttribute(
-      "href",
-      MockUpdateCheck.url,
-    )
-  })
-
-  it("is hidden when dismissed", async () => {
-    const dismiss = jest.fn()
-    const { container } = render(
-      <UpdateCheckBanner
-        onDismiss={dismiss}
-        updateCheck={{ ...MockUpdateCheck, current: false }}
-      />,
-    )
-
-    fireEvent.click(screen.getByRole("button"))
-    await waitFor(() => expect(dismiss).toBeCalledTimes(1), { timeout: 2000 })
-
-    expect(container.firstChild).toBeNull()
-  })
-
-  it("does not show when up-to-date", async () => {
-    const { container } = render(
-      <UpdateCheckBanner updateCheck={{ ...MockUpdateCheck, current: true }} />,
-    )
-    expect(container.firstChild).toBeNull()
-  })
-})
--- a/site/src/components/UpdateCheckBanner/UpdateCheckBanner.tsx
+++ b/site/src/components/UpdateCheckBanner/UpdateCheckBanner.tsx
@ -2,12 +2,12 @@ import Link from "@material-ui/core/Link"
 import { AlertBanner } from "components/AlertBanner/AlertBanner"
 import { Trans, useTranslation } from "react-i18next"
 import * as TypesGen from "api/typesGenerated"
-import { FC, useState } from "react"
+import { FC } from "react"

 export interface UpdateCheckBannerProps {
-  updateCheck?: TypesGen.UpdateCheckResponse
-  error?: Error | unknown
-  onDismiss?: () => void
+  updateCheck: TypesGen.UpdateCheckResponse
+  error?: unknown
+  onDismiss: () => void
 }

 export const UpdateCheckBanner: FC<
@ -15,45 +15,33 @@ export const UpdateCheckBanner: FC<
 > = ({ updateCheck, error, onDismiss }) => {
  const { t } = useTranslation("common")

-  const isOutdated = updateCheck && !updateCheck.current
-
-  const [show, setShow] = useState(error || isOutdated)
-
-  const dismiss = () => {
-    onDismiss && onDismiss()
-    setShow(false)
-  }
-
  return (
-    <>
-      {show && (
-        <AlertBanner
-          severity={error ? "error" : "info"}
-          error={error}
-          onDismiss={dismiss}
-          dismissible
-        >
-          <>
-            {error && <>{t("updateCheck.error")} </>}
-            {isOutdated && (
-              <div>
-                <Trans
-                  t={t}
-                  i18nKey="updateCheck.message"
-                  values={{ version: updateCheck.version }}
-                >
-                  Coder {"{{version}}"} is now available. View the{" "}
-                  <Link href={updateCheck.url}>release notes</Link> and{" "}
-                  <Link href="https://coder.com/docs/coder-oss/latest/admin/upgrade">
-                    upgrade instructions
-                  </Link>{" "}
-                  for more information.
-                </Trans>
-              </div>
-            )}
-          </>
-        </AlertBanner>
-      )}
-    </>
+    <AlertBanner
+      severity={error ? "error" : "info"}
+      error={error}
+      onDismiss={onDismiss}
+      dismissible
+    >
+      <>
+        {error ? (
+          t("updateCheck.error")
+        ) : (
+          <div>
+            <Trans
+              t={t}
+              i18nKey="updateCheck.message"
+              values={{ version: updateCheck.version }}
+            >
+              Coder {"{{version}}"} is now available. View the{" "}
+              <Link href={updateCheck.url}>release notes</Link> and{" "}
+              <Link href="https://coder.com/docs/coder-oss/latest/admin/upgrade">
+                upgrade instructions
+              </Link>{" "}
+              for more information.
+            </Trans>
+          </div>
+        )}
+      </>
+    </AlertBanner>
  )
 }
--- a/site/src/testHelpers/entities.ts
+++ b/site/src/testHelpers/entities.ts
@ -1096,6 +1096,7 @@ export const MockPermissions: Permissions = {
  updateUsers: true,
  viewAuditLog: true,
  viewDeploymentConfig: true,
+  viewUpdateCheck: true,
 }

 export const MockAppearance: TypesGen.AppearanceConfig = {
--- a/site/src/xServices/StateContext.tsx
+++ b/site/src/xServices/StateContext.tsx
@ -3,7 +3,6 @@ import { createContext, FC, ReactNode } from "react"
 import { ActorRefFrom } from "xstate"
 import { authMachine } from "./auth/authXService"
 import { buildInfoMachine } from "./buildInfo/buildInfoXService"
-import { updateCheckMachine } from "./updateCheck/updateCheckXService"
 import { entitlementsMachine } from "./entitlements/entitlementsXService"
 import { appearanceMachine } from "./appearance/appearanceXService"

@ -12,7 +11,6 @@ interface XServiceContextType {
  buildInfoXService: ActorRefFrom<typeof buildInfoMachine>
  entitlementsXService: ActorRefFrom<typeof entitlementsMachine>
  appearanceXService: ActorRefFrom<typeof appearanceMachine>
-  updateCheckXService: ActorRefFrom<typeof updateCheckMachine>
 }

 /**
@ -33,7 +31,6 @@ export const XServiceProvider: FC<{ children: ReactNode }> = ({ children }) => {
        buildInfoXService: useInterpret(buildInfoMachine),
        entitlementsXService: useInterpret(entitlementsMachine),
        appearanceXService: useInterpret(appearanceMachine),
-        updateCheckXService: useInterpret(updateCheckMachine),
      }}
    >
      {children}
--- a/site/src/xServices/auth/authXService.ts
+++ b/site/src/xServices/auth/authXService.ts
@ -16,6 +16,7 @@ export const checks = {
  viewAuditLog: "viewAuditLog",
  viewDeploymentConfig: "viewDeploymentConfig",
  createGroup: "createGroup",
+  viewUpdateCheck: "viewUpdateCheck",
 } as const

 export const permissionsToCheck = {
@ -67,6 +68,12 @@ export const permissionsToCheck = {
    },
    action: "create",
  },
+  [checks.viewUpdateCheck]: {
+    object: {
+      resource_type: "update_check",
+    },
+    action: "read",
+  },
 } as const

 export type Permissions = Record<keyof typeof permissionsToCheck, boolean>
--- a/site/src/xServices/updateCheck/updateCheckXService.test.ts
+++ b/site/src/xServices/updateCheck/updateCheckXService.test.ts
@ -0,0 +1,138 @@
+import { waitFor } from "@testing-library/react"
+import { MockPermissions, MockUpdateCheck } from "testHelpers/entities"
+import { interpret } from "xstate"
+import {
+  clearDismissedVersionOnLocal,
+  getDismissedVersionOnLocal,
+  saveDismissedVersionOnLocal,
+  updateCheckMachine,
+} from "./updateCheckXService"
+
+describe("updateCheckMachine", () => {
+  beforeEach(() => {
+    clearDismissedVersionOnLocal()
+  })
+
+  it("is dismissed when does not have permission to see it", () => {
+    const machine = updateCheckMachine.withContext({
+      permissions: {
+        ...MockPermissions,
+        viewUpdateCheck: false,
+      },
+    })
+
+    const updateCheckService = interpret(machine)
+    updateCheckService.start()
+    expect(updateCheckService.state.matches("dismissed")).toBeTruthy()
+  })
+
+  it("is dismissed when it is already using current version", async () => {
+    const machine = updateCheckMachine
+      .withContext({
+        permissions: {
+          ...MockPermissions,
+          viewUpdateCheck: true,
+        },
+      })
+      .withConfig({
+        services: {
+          getUpdateCheck: () =>
+            Promise.resolve({
+              ...MockUpdateCheck,
+              current: true,
+            }),
+        },
+      })
+
+    const updateCheckService = interpret(machine)
+    updateCheckService.start()
+
+    await waitFor(() => {
+      expect(updateCheckService.state.matches("dismissed")).toBeTruthy()
+    })
+  })
+
+  it("is dismissed when it was dismissed previously", async () => {
+    const machine = updateCheckMachine
+      .withContext({
+        permissions: {
+          ...MockPermissions,
+          viewUpdateCheck: true,
+        },
+      })
+      .withConfig({
+        services: {
+          getUpdateCheck: () =>
+            Promise.resolve({
+              ...MockUpdateCheck,
+              current: false,
+            }),
+        },
+      })
+
+    saveDismissedVersionOnLocal(MockUpdateCheck.version)
+    const updateCheckService = interpret(machine)
+    updateCheckService.start()
+
+    await waitFor(() => {
+      expect(updateCheckService.state.matches("dismissed")).toBeTruthy()
+    })
+  })
+
+  it("shows when has permission and is outdated", async () => {
+    const machine = updateCheckMachine
+      .withContext({
+        permissions: {
+          ...MockPermissions,
+          viewUpdateCheck: true,
+        },
+      })
+      .withConfig({
+        services: {
+          getUpdateCheck: () =>
+            Promise.resolve({
+              ...MockUpdateCheck,
+              current: false,
+            }),
+        },
+      })
+
+    const updateCheckService = interpret(machine)
+    updateCheckService.start()
+
+    await waitFor(() => {
+      expect(updateCheckService.state.matches("show")).toBeTruthy()
+    })
+  })
+
+  it("it is dismissed when the DISMISS event happens", async () => {
+    const machine = updateCheckMachine
+      .withContext({
+        permissions: {
+          ...MockPermissions,
+          viewUpdateCheck: true,
+        },
+      })
+      .withConfig({
+        services: {
+          getUpdateCheck: () =>
+            Promise.resolve({
+              ...MockUpdateCheck,
+              current: false,
+            }),
+        },
+      })
+
+    const updateCheckService = interpret(machine)
+    updateCheckService.start()
+    await waitFor(() => {
+      expect(updateCheckService.state.matches("show")).toBeTruthy()
+    })
+
+    updateCheckService.send("DISMISS")
+    await waitFor(() => {
+      expect(updateCheckService.state.matches("dismissed")).toBeTruthy()
+      expect(getDismissedVersionOnLocal()).toEqual(MockUpdateCheck.version)
+    })
+  })
+})
--- a/site/src/xServices/updateCheck/updateCheckXService.ts
+++ b/site/src/xServices/updateCheck/updateCheckXService.ts
@ -1,33 +1,15 @@
 import { assign, createMachine } from "xstate"
-import { checkAuthorization, getUpdateCheck } from "api/api"
+import { getUpdateCheck } from "api/api"
 import { AuthorizationResponse, UpdateCheckResponse } from "api/typesGenerated"
-
-export const checks = {
-  viewUpdateCheck: "viewUpdateCheck",
-}
-
-export const permissionsToCheck = {
-  [checks.viewUpdateCheck]: {
-    object: {
-      resource_type: "update_check",
-    },
-    action: "read",
-  },
-}
-
-export type Permissions = Record<keyof typeof permissionsToCheck, boolean>
+import { checks, Permissions } from "xServices/auth/authXService"

 export interface UpdateCheckContext {
-  show: boolean
+  permissions: Permissions
  updateCheck?: UpdateCheckResponse
-  permissions?: Permissions
  error?: Error | unknown
 }

-export type UpdateCheckEvent =
-  | { type: "CHECK" }
-  | { type: "CLEAR" }
-  | { type: "DISMISS" }
+export type UpdateCheckEvent = { type: "DISMISS" }

 export const updateCheckMachine = createMachine(
  {
@ -46,36 +28,8 @@ export const updateCheckMachine = createMachine(
        }
      },
    },
-    context: {
-      show: false,
-    },
-    initial: "idle",
+    initial: "checkingPermissions",
    states: {
-      idle: {
-        on: {
-          CHECK: {
-            target: "fetchingPermissions",
-          },
-        },
-      },
-      fetchingPermissions: {
-        invoke: {
-          src: "checkPermissions",
-          id: "checkPermissions",
-          onDone: [
-            {
-              actions: ["assignPermissions"],
-              target: "checkingPermissions",
-            },
-          ],
-          onError: [
-            {
-              actions: ["assignError"],
-              target: "show",
-            },
-          ],
-        },
-      },
      checkingPermissions: {
        always: [
          {
@ -83,8 +37,7 @@ export const updateCheckMachine = createMachine(
            cond: "canViewUpdateCheck",
          },
          {
-            target: "dismissOrClear",
-            cond: "canNotViewUpdateCheck",
+            target: "dismissed",
          },
        ],
      },
@ -94,36 +47,28 @@ export const updateCheckMachine = createMachine(
          id: "getUpdateCheck",
          onDone: [
            {
-              actions: ["assignUpdateCheck", "clearError"],
+              actions: ["assignUpdateCheck"],
              target: "show",
+              cond: "shouldShowUpdateCheck",
+            },
+            {
+              target: "dismissed",
            },
          ],
          onError: [
            {
-              actions: ["assignError", "clearUpdateCheck"],
-              target: "show",
+              actions: ["assignError"],
+              target: "dismissed",
            },
          ],
        },
      },
      show: {
-        entry: "assignShow",
-        always: [
-          {
-            target: "dismissOrClear",
-          },
-        ],
-      },
-      dismissOrClear: {
        on: {
          DISMISS: {
-            actions: ["assignHide", "setDismissedVersion"],
+            actions: ["setDismissedVersion"],
            target: "dismissed",
          },
-          CLEAR: {
-            actions: ["clearUpdateCheck", "clearError", "assignHide"],
-            target: "idle",
-          },
        },
      },
      dismissed: {
@ -133,48 +78,45 @@ export const updateCheckMachine = createMachine(
  },
  {
    services: {
-      checkPermissions: async () =>
-        checkAuthorization({ checks: permissionsToCheck }),
-      getUpdateCheck: getUpdateCheck,
+      getUpdateCheck,
    },
    actions: {
-      assignPermissions: assign({
-        permissions: (_, event) => event.data as Permissions,
-      }),
-      assignShow: assign((context) => ({
-        show:
-          localStorage.getItem("dismissedVersion") !==
-          context.updateCheck?.version,
-      })),
-      assignHide: assign({
-        show: false,
-      }),
      assignUpdateCheck: assign({
        updateCheck: (_, event) => event.data,
      }),
-      clearUpdateCheck: assign((context) => ({
-        ...context,
-        updateCheck: undefined,
-      })),
      assignError: assign({
        error: (_, event) => event.data,
      }),
-      clearError: assign((context) => ({
-        ...context,
-        error: undefined,
-      })),
-      setDismissedVersion: (context) => {
-        if (context.updateCheck?.version) {
-          // We use localStorage to ensure users who have dismissed the UpdateCheckBanner are not plagued by its reappearance on page reload
-          localStorage.setItem("dismissedVersion", context.updateCheck.version)
+      setDismissedVersion: ({ updateCheck }) => {
+        if (!updateCheck) {
+          throw new Error("Update check is not set")
        }
+
+        saveDismissedVersionOnLocal(updateCheck.version)
      },
    },
    guards: {
-      canViewUpdateCheck: (context) =>
-        context.permissions?.[checks.viewUpdateCheck] || false,
-      canNotViewUpdateCheck: (context) =>
-        !context.permissions?.[checks.viewUpdateCheck],
+      canViewUpdateCheck: ({ permissions }) =>
+        permissions[checks.viewUpdateCheck] || false,
+      shouldShowUpdateCheck: (_, { data }) => {
+        const isNotDismissed = getDismissedVersionOnLocal() !== data.version
+        const isOutdated = !data.current
+
+        return isNotDismissed && isOutdated
+      },
    },
  },
 )
+
+// Exporting to be used in the tests
+export const saveDismissedVersionOnLocal = (version: string): void => {
+  window.localStorage.setItem("dismissedVersion", version)
+}
+
+export const getDismissedVersionOnLocal = (): string | undefined => {
+  return localStorage.getItem("dismissedVersion") ?? undefined
+}
+
+export const clearDismissedVersionOnLocal = (): void => {
+  localStorage.removeItem("dismissedVersion")
+}