refactor: Remove users redirect to active filter (#4056)

2026-09-24 15:04:27 +08:00 · 2022-09-15 10:05:33 -03:00
parent b78ab9e028
commit 40c0fc285c
10 changed files with 205 additions and 178 deletions
@@ -48,7 +48,10 @@ const NavItems: React.FC<
        </NavLink>
      </ListItem>
      <ListItem button className={styles.item}>
-        <NavLink className={styles.link} to="/users">
+        <NavLink
+          className={styles.link}
+          to={`/users?filter=${encodeURIComponent("status:active")}`}
+        >
          {Language.users}
        </NavLink>
      </ListItem>
@@ -1,4 +1,4 @@
-import { fireEvent, screen } from "@testing-library/react"
+import { screen } from "@testing-library/react"
 import userEvent from "@testing-library/user-event"
 import { render } from "../../testHelpers/renderHelpers"
 import { SearchBarWithFilter } from "./SearchBarWithFilter"
@@ -21,18 +21,6 @@ describe("SearchBarWithFilter", () => {
    await userEvent.type(searchInput, "workspace") // 9 characters

    // Then
-    expect(onFilter).toBeCalledTimes(10) // 9 characters + 1 on component mount
-  })
-
-  it("calls the onFilter handler on submit", async () => {
-    // When
-    const onFilter = jest.fn()
-    render(<SearchBarWithFilter onFilter={onFilter} />)
-
-    const searchInput = screen.getByRole("textbox")
-    await fireEvent.keyDown(searchInput, { key: "Enter", code: "Enter", charCode: 13 })
-
-    // Then
-    expect(onFilter).toBeCalledTimes(1)
+    expect(onFilter).toBeCalledTimes(9) // 9 characters
  })
 })
@@ -7,9 +7,8 @@ import OutlinedInput from "@material-ui/core/OutlinedInput"
 import { makeStyles } from "@material-ui/core/styles"
 import { Theme } from "@material-ui/core/styles/createMuiTheme"
 import SearchIcon from "@material-ui/icons/Search"
-import { FormikErrors, useFormik } from "formik"
 import debounce from "just-debounce-it"
-import { useCallback, useEffect, useState } from "react"
+import { useCallback, useRef, useState } from "react"
 import { getValidationErrorMessage } from "../../api/errors"
 import { CloseDropdown, OpenDropdown } from "../DropdownArrows/DropdownArrows"
 import { Stack } from "../Stack/Stack"
@@ -30,12 +29,6 @@ export interface PresetFilter {
  query: string
 }

-interface FilterFormValues {
-  query: string
-}
-
-export type FilterFormErrors = FormikErrors<FilterFormValues>
-
 export const SearchBarWithFilter: React.FC<React.PropsWithChildren<SearchBarWithFilterProps>> = ({
  filter,
  onFilter,
@@ -43,16 +36,7 @@ export const SearchBarWithFilter: React.FC<React.PropsWithChildren<SearchBarWith
  error,
 }) => {
  const styles = useStyles({ error: Boolean(error) })
-
-  const form = useFormik<FilterFormValues>({
-    enableReinitialize: true,
-    initialValues: {
-      query: filter ?? "",
-    },
-    onSubmit: ({ query }) => {
-      onFilter(query)
-    },
-  })
+  const searchInputRef = useRef<HTMLInputElement>(null)

  // debounce query string entry by user
  // we want the dependency array empty here
@@ -65,12 +49,6 @@ export const SearchBarWithFilter: React.FC<React.PropsWithChildren<SearchBarWith
    [],
  )

-  // update the query params while typing
-  useEffect(() => {
-    debouncedOnFilter(form.values.query)
-    return () => debouncedOnFilter.cancel()
-  }, [debouncedOnFilter, form.values.query])
-
  const [anchorEl, setAnchorEl] = useState<null | HTMLElement>(null)

  const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {
@@ -82,8 +60,15 @@ export const SearchBarWithFilter: React.FC<React.PropsWithChildren<SearchBarWith
  }

  const setPresetFilter = (query: string) => () => {
-    void form.setFieldValue("query", query)
-    void form.submitForm()
+    if (!searchInputRef.current) {
+      throw new Error("Search input not found.")
+    }
+
+    onFilter(query)
+    // Update this to the input directly instead of create a new state and
+    // re-render the component since the onFilter is already calling the
+    // filtering process
+    searchInputRef.current.value = query
    handleClose()
  }

@@ -103,21 +88,24 @@ export const SearchBarWithFilter: React.FC<React.PropsWithChildren<SearchBarWith
          </Button>
        )}

-        <form onSubmit={form.handleSubmit} className={styles.filterForm}>
+        <div role="form" className={styles.filterForm}>
          <OutlinedInput
            id="query"
            name="query"
-            value={form.values.query}
+            defaultValue={filter}
            error={Boolean(error)}
            className={styles.inputStyles}
-            onChange={form.handleChange}
+            onChange={(event) => {
+              debouncedOnFilter(event.currentTarget.value)
+            }}
+            inputRef={searchInputRef}
            startAdornment={
              <InputAdornment position="start" className={styles.searchIcon}>
                <SearchIcon fontSize="small" />
              </InputAdornment>
            }
          />
-        </form>
+        </div>

        {presetFilters && presetFilters.length && (
          <Menu
@@ -4,11 +4,20 @@ import { rest } from "msw"
 import * as API from "../../../api/api"
 import { Language as FormLanguage } from "../../../components/CreateUserForm/CreateUserForm"
 import { Language as FooterLanguage } from "../../../components/FormFooter/FormFooter"
-import { history, render } from "../../../testHelpers/renderHelpers"
+import {
+  history,
+  renderWithAuth,
+  waitForLoaderToBeRemoved,
+} from "../../../testHelpers/renderHelpers"
 import { server } from "../../../testHelpers/server"
-import { Language as UserLanguage } from "../../../xServices/users/usersXService"
+import { Language as CreateUserLanguage } from "../../../xServices/users/createUserXService"
 import { CreateUserPage } from "./CreateUserPage"

+const renderCreateUserPage = async () => {
+  renderWithAuth(<CreateUserPage />)
+  await waitForLoaderToBeRemoved()
+}
+
 const fillForm = async ({
  username = "someuser",
  email = "someone@coder.com",
@@ -34,7 +43,7 @@ describe("Create User Page", () => {
  })

  it("shows validation error message", async () => {
-    render(<CreateUserPage />)
+    await renderCreateUserPage()
    await fillForm({ email: "test" })
    const errorMessage = await screen.findByText(FormLanguage.emailInvalid)
    expect(errorMessage).toBeDefined()
@@ -44,9 +53,9 @@ describe("Create User Page", () => {
    jest.spyOn(API, "createUser").mockRejectedValueOnce({
      data: "unknown error",
    })
-    render(<CreateUserPage />)
+    await renderCreateUserPage()
    await fillForm({})
-    const errorMessage = await screen.findByText(UserLanguage.createUserError)
+    const errorMessage = await screen.findByText(CreateUserLanguage.createUserError)
    expect(errorMessage).toBeDefined()
  })

@@ -68,30 +77,16 @@ describe("Create User Page", () => {
        )
      }),
    )
-    render(<CreateUserPage />)
+    await renderCreateUserPage()
    await fillForm({})
    const errorMessage = await screen.findByText(fieldErrorMessage)
    expect(errorMessage).toBeDefined()
  })

  it("shows success notification and redirects to users page", async () => {
-    render(<CreateUserPage />)
+    await renderCreateUserPage()
    await fillForm({})
-    const successMessage = screen.findByText(UserLanguage.createUserSuccess)
+    const successMessage = screen.findByText(CreateUserLanguage.createUserSuccess)
    expect(successMessage).toBeDefined()
  })
-
-  it("redirects to users page on cancel", async () => {
-    render(<CreateUserPage />)
-    const cancelButton = await screen.findByText(FooterLanguage.cancelLabel)
-    cancelButton.click()
-    expect(history.location.pathname).toEqual("/users")
-  })
-
-  it("redirects to users page on close", async () => {
-    render(<CreateUserPage />)
-    const closeButton = await screen.findByText("ESC")
-    closeButton.click()
-    expect(history.location.pathname).toEqual("/users")
-  })
 })
@@ -1,24 +1,29 @@
-import { useActor, useSelector } from "@xstate/react"
-import React, { useContext } from "react"
+import { useMachine } from "@xstate/react"
+import { useOrganizationId } from "hooks/useOrganizationId"
+import React from "react"
 import { Helmet } from "react-helmet-async"
 import { useNavigate } from "react-router"
+import { createUserMachine } from "xServices/users/createUserXService"
 import * as TypesGen from "../../../api/typesGenerated"
 import { CreateUserForm } from "../../../components/CreateUserForm/CreateUserForm"
 import { Margins } from "../../../components/Margins/Margins"
 import { pageTitle } from "../../../util/page"
-import { selectOrgId } from "../../../xServices/auth/authSelectors"
-import { XServiceContext } from "../../../xServices/StateContext"

 export const Language = {
  unknownError: "Oops, an unknown error occurred.",
 }

 export const CreateUserPage: React.FC = () => {
-  const xServices = useContext(XServiceContext)
-  const myOrgId = useSelector(xServices.authXService, selectOrgId)
-  const [usersState, usersSend] = useActor(xServices.usersXService)
-  const { createUserErrorMessage, createUserFormErrors } = usersState.context
+  const myOrgId = useOrganizationId()
  const navigate = useNavigate()
+  const [createUserState, createUserSend] = useMachine(createUserMachine, {
+    actions: {
+      redirectToUsersPage: () => {
+        navigate("/users")
+      },
+    },
+  })
+  const { createUserErrorMessage, createUserFormErrors } = createUserState.context
  // There is no field for organization id in Community Edition, so handle its field error like a generic error
  const genericError =
    createUserErrorMessage ||
@@ -32,14 +37,14 @@ export const CreateUserPage: React.FC = () => {
      </Helmet>
      <CreateUserForm
        formErrors={createUserFormErrors}
-        onSubmit={(user: TypesGen.CreateUserRequest) => usersSend({ type: "CREATE", user })}
+        onSubmit={(user: TypesGen.CreateUserRequest) => createUserSend({ type: "CREATE", user })}
        onCancel={() => {
-          usersSend("CANCEL_CREATE_USER")
+          createUserSend("CANCEL_CREATE_USER")
          navigate("/users")
        }}
-        isLoading={usersState.hasTag("loading")}
+        isLoading={createUserState.hasTag("loading")}
        error={genericError}
-        myOrgId={myOrgId ?? ""}
+        myOrgId={myOrgId}
      />
    </Margins>
  )
@@ -1,11 +1,11 @@
-import { useActor } from "@xstate/react"
+import { useActor, useMachine } from "@xstate/react"
 import { FC, ReactNode, useContext, useEffect } from "react"
 import { Helmet } from "react-helmet-async"
 import { useNavigate } from "react-router"
 import { useSearchParams } from "react-router-dom"
+import { usersMachine } from "xServices/users/usersXService"
 import { ConfirmDialog } from "../../components/Dialogs/ConfirmDialog/ConfirmDialog"
 import { ResetPasswordDialog } from "../../components/Dialogs/ResetPasswordDialog/ResetPasswordDialog"
-import { userFilterQuery } from "../../util/filters"
 import { pageTitle } from "../../util/page"
 import { XServiceContext } from "../../xServices/StateContext"
 import { UsersPageView } from "./UsersPageView"
@@ -24,7 +24,14 @@ export const Language = {

 export const UsersPage: FC<{ children?: ReactNode }> = () => {
  const xServices = useContext(XServiceContext)
-  const [usersState, usersSend] = useActor(xServices.usersXService)
+  const navigate = useNavigate()
+  const [searchParams, setSearchParams] = useSearchParams()
+  const filter = searchParams.get("filter") ?? undefined
+  const [usersState, usersSend] = useMachine(usersMachine, {
+    context: {
+      filter,
+    },
+  })
  const {
    users,
    getUsersError,
@@ -34,8 +41,7 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => {
    userIdToResetPassword,
    newUserPassword,
  } = usersState.context
-  const navigate = useNavigate()
-  const [searchParams, setSearchParams] = useSearchParams()
+
  const userToBeSuspended = users?.find((u) => u.id === userIdToSuspend)
  const userToBeDeleted = users?.find((u) => u.id === userIdToDelete)
  const userToBeActivated = users?.find((u) => u.id === userIdToActivate)
@@ -60,13 +66,11 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => {

  // Fetch users on component mount
  useEffect(() => {
-    const filter = searchParams.get("filter")
-    const query = filter ?? userFilterQuery.active
    usersSend({
      type: "GET_USERS",
-      query,
+      query: filter,
    })
-  }, [searchParams, usersSend])
+  }, [filter, usersSend])

  // Fetch roles on component mount
  useEffect(() => {
@@ -1,5 +1,10 @@
 import ThemeProvider from "@material-ui/styles/ThemeProvider"
-import { render as wrappedRender, RenderResult } from "@testing-library/react"
+import {
+  render as wrappedRender,
+  RenderResult,
+  screen,
+  waitForElementToBeRemoved,
+} from "@testing-library/react"
 import { createMemoryHistory } from "history"
 import { i18n } from "i18n"
 import { FC, ReactElement } from "react"
@@ -68,4 +73,7 @@ export function renderWithAuth(
  }
 }

+export const waitForLoaderToBeRemoved = (): Promise<void> =>
+  waitForElementToBeRemoved(() => screen.getByRole("progressbar"))
+
 export * from "./entities"
@@ -6,13 +6,11 @@ import { authMachine } from "./auth/authXService"
 import { buildInfoMachine } from "./buildInfo/buildInfoXService"
 import { entitlementsMachine } from "./entitlements/entitlementsXService"
 import { siteRolesMachine } from "./roles/siteRolesXService"
-import { usersMachine } from "./users/usersXService"

 interface XServiceContextType {
  authXService: ActorRefFrom<typeof authMachine>
  buildInfoXService: ActorRefFrom<typeof buildInfoMachine>
  entitlementsXService: ActorRefFrom<typeof entitlementsMachine>
-  usersXService: ActorRefFrom<typeof usersMachine>
  siteRolesXService: ActorRefFrom<typeof siteRolesMachine>
 }

@@ -28,9 +26,6 @@ export const XServiceContext = createContext({} as XServiceContextType)

 export const XServiceProvider: FC<{ children: ReactNode }> = ({ children }) => {
  const navigate = useNavigate()
-  const redirectToUsersPage = () => {
-    navigate("users")
-  }
  const redirectToSetupPage = () => {
    navigate("setup")
  }
@@ -43,9 +38,6 @@ export const XServiceProvider: FC<{ children: ReactNode }> = ({ children }) => {
        ),
        buildInfoXService: useInterpret(buildInfoMachine),
        entitlementsXService: useInterpret(entitlementsMachine),
-        usersXService: useInterpret(() =>
-          usersMachine.withConfig({ actions: { redirectToUsersPage } }),
-        ),
        siteRolesXService: useInterpret(siteRolesMachine),
      }}
    >
@@ -0,0 +1,101 @@
+import { assign, createMachine } from "xstate"
+import * as API from "../../api/api"
+import {
+  ApiError,
+  FieldErrors,
+  getErrorMessage,
+  hasApiFieldErrors,
+  isApiError,
+  mapApiErrorToFieldErrors,
+} from "../../api/errors"
+import * as TypesGen from "../../api/typesGenerated"
+import { displaySuccess } from "../../components/GlobalSnackbar/utils"
+
+export const Language = {
+  createUserSuccess: "Successfully created user.",
+  createUserError: "Error on creating the user.",
+}
+
+export interface CreateUserContext {
+  createUserErrorMessage?: string
+  createUserFormErrors?: FieldErrors
+}
+
+export type CreateUserEvent =
+  | { type: "CREATE"; user: TypesGen.CreateUserRequest }
+  | { type: "CANCEL_CREATE_USER" }
+
+export const createUserMachine = createMachine(
+  {
+    id: "usersState",
+    predictableActionArguments: true,
+    tsTypes: {} as import("./createUserXService.typegen").Typegen0,
+    schema: {
+      context: {} as CreateUserContext,
+      events: {} as CreateUserEvent,
+      services: {} as {
+        createUser: {
+          data: TypesGen.User
+        }
+      },
+    },
+    initial: "idle",
+    states: {
+      idle: {
+        on: {
+          CREATE: "creatingUser",
+          CANCEL_CREATE_USER: { actions: ["clearCreateUserError"] },
+        },
+      },
+      creatingUser: {
+        entry: "clearCreateUserError",
+        invoke: {
+          src: "createUser",
+          id: "createUser",
+          onDone: {
+            target: "idle",
+            actions: ["displayCreateUserSuccess", "redirectToUsersPage"],
+          },
+          onError: [
+            {
+              target: "idle",
+              cond: "hasFieldErrors",
+              actions: ["assignCreateUserFormErrors"],
+            },
+            {
+              target: "idle",
+              actions: ["assignCreateUserError"],
+            },
+          ],
+        },
+        tags: "loading",
+      },
+    },
+  },
+  {
+    services: {
+      createUser: (_, event) => API.createUser(event.user),
+    },
+    guards: {
+      hasFieldErrors: (_, event) => isApiError(event.data) && hasApiFieldErrors(event.data),
+    },
+    actions: {
+      assignCreateUserError: assign({
+        createUserErrorMessage: (_, event) => getErrorMessage(event.data, Language.createUserError),
+      }),
+      assignCreateUserFormErrors: assign({
+        // the guard ensures it is ApiError
+        createUserFormErrors: (_, event) =>
+          mapApiErrorToFieldErrors((event.data as ApiError).response.data),
+      }),
+      clearCreateUserError: assign((context: CreateUserContext) => ({
+        ...context,
+        createUserErrorMessage: undefined,
+        createUserFormErrors: undefined,
+      })),
+      displayCreateUserSuccess: () => {
+        displaySuccess(Language.createUserSuccess)
+      },
+    },
+  },
+)
@@ -1,13 +1,6 @@
 import { assign, createMachine } from "xstate"
 import * as API from "../../api/api"
-import {
-  ApiError,
-  FieldErrors,
-  getErrorMessage,
-  hasApiFieldErrors,
-  isApiError,
-  mapApiErrorToFieldErrors,
-} from "../../api/errors"
+import { getErrorMessage } from "../../api/errors"
 import * as TypesGen from "../../api/typesGenerated"
 import { displayError, displaySuccess } from "../../components/GlobalSnackbar/utils"
 import { queryToFilter } from "../../util/filters"
@@ -15,8 +8,6 @@ import { generateRandomString } from "../../util/random"

 export const Language = {
  getUsersError: "Error getting users.",
-  createUserSuccess: "Successfully created user.",
-  createUserError: "Error on creating the user.",
  suspendUserSuccess: "Successfully suspended the user.",
  suspendUserError: "Error suspending user.",
  deleteUserSuccess: "Successfully deleted the user.",
@@ -34,8 +25,6 @@ export interface UsersContext {
  users?: TypesGen.User[]
  filter?: string
  getUsersError?: Error | unknown
-  createUserErrorMessage?: string
-  createUserFormErrors?: FieldErrors
  // Suspend user
  userIdToSuspend?: TypesGen.User["id"]
  suspendUserError?: Error | unknown
@@ -55,9 +44,7 @@ export interface UsersContext {
 }

 export type UsersEvent =
-  | { type: "GET_USERS"; query: string }
-  | { type: "CREATE"; user: TypesGen.CreateUserRequest }
-  | { type: "CANCEL_CREATE_USER" }
+  | { type: "GET_USERS"; query?: string }
  // Suspend events
  | { type: "SUSPEND_USER"; userId: TypesGen.User["id"] }
  | { type: "CONFIRM_USER_SUSPENSION" }
@@ -109,16 +96,34 @@ export const usersMachine = createMachine(
        }
      },
    },
-    initial: "idle",
+    initial: "gettingUsers",
    states: {
+      gettingUsers: {
+        entry: "clearGetUsersError",
+        invoke: {
+          src: "getUsers",
+          id: "getUsers",
+          onDone: [
+            {
+              target: "#usersState.idle",
+              actions: "assignUsers",
+            },
+          ],
+          onError: [
+            {
+              actions: ["clearUsers", "assignGetUsersError", "displayGetUsersErrorMessage"],
+              target: "#usersState.error",
+            },
+          ],
+        },
+        tags: "loading",
+      },
      idle: {
        on: {
          GET_USERS: {
            actions: "assignFilter",
            target: "gettingUsers",
          },
-          CREATE: "creatingUser",
-          CANCEL_CREATE_USER: { actions: ["clearCreateUserError"] },
          SUSPEND_USER: {
            target: "confirmUserSuspension",
            actions: ["assignUserIdToSuspend"],
@@ -141,49 +146,6 @@ export const usersMachine = createMachine(
          },
        },
      },
-      gettingUsers: {
-        entry: "clearGetUsersError",
-        invoke: {
-          src: "getUsers",
-          id: "getUsers",
-          onDone: [
-            {
-              target: "#usersState.idle",
-              actions: "assignUsers",
-            },
-          ],
-          onError: [
-            {
-              actions: ["clearUsers", "assignGetUsersError", "displayGetUsersErrorMessage"],
-              target: "#usersState.error",
-            },
-          ],
-        },
-        tags: "loading",
-      },
-      creatingUser: {
-        entry: "clearCreateUserError",
-        invoke: {
-          src: "createUser",
-          id: "createUser",
-          onDone: {
-            target: "idle",
-            actions: ["displayCreateUserSuccess", "redirectToUsersPage"],
-          },
-          onError: [
-            {
-              target: "idle",
-              cond: "hasFieldErrors",
-              actions: ["assignCreateUserFormErrors"],
-            },
-            {
-              target: "idle",
-              actions: ["assignCreateUserError"],
-            },
-          ],
-        },
-        tags: "loading",
-      },
      confirmUserSuspension: {
        on: {
          CONFIRM_USER_SUSPENSION: "suspendingUser",
@@ -301,7 +263,6 @@ export const usersMachine = createMachine(
      // when it is mocked. This happen in the UsersPage tests inside of the
      // "shows a success message and refresh the page" test case.
      getUsers: (context) => API.getUsers(queryToFilter(context.filter)),
-      createUser: (_, event) => API.createUser(event.user),
      suspendUser: (context) => {
        if (!context.userIdToSuspend) {
          throw new Error("userIdToSuspend is undefined")
@@ -344,9 +305,7 @@ export const usersMachine = createMachine(
        return API.updateUserRoles(event.roles, context.userIdToUpdateRoles)
      },
    },
-    guards: {
-      hasFieldErrors: (_, event) => isApiError(event.data) && hasApiFieldErrors(event.data),
-    },
+
    actions: {
      assignUsers: assign({
        users: (_, event) => event.data,
@@ -376,14 +335,6 @@ export const usersMachine = createMachine(
        ...context,
        getUsersError: undefined,
      })),
-      assignCreateUserError: assign({
-        createUserErrorMessage: (_, event) => getErrorMessage(event.data, Language.createUserError),
-      }),
-      assignCreateUserFormErrors: assign({
-        // the guard ensures it is ApiError
-        createUserFormErrors: (_, event) =>
-          mapApiErrorToFieldErrors((event.data as ApiError).response.data),
-      }),
      assignSuspendUserError: assign({
        suspendUserError: (_, event) => event.data,
      }),
@@ -403,11 +354,6 @@ export const usersMachine = createMachine(
        ...context,
        users: undefined,
      })),
-      clearCreateUserError: assign((context: UsersContext) => ({
-        ...context,
-        createUserErrorMessage: undefined,
-        createUserFormErrors: undefined,
-      })),
      clearSuspendUserError: assign({
        suspendUserError: (_) => undefined,
      }),
@@ -427,9 +373,6 @@ export const usersMachine = createMachine(
        const message = getErrorMessage(context.getUsersError, Language.getUsersError)
        displayError(message)
      },
-      displayCreateUserSuccess: () => {
-        displaySuccess(Language.createUserSuccess)
-      },
      displaySuspendSuccess: () => {
        displaySuccess(Language.suspendUserSuccess)
      },