From 9e4f999cba7afd6568de1de69a7bf94c77562846 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 20 Oct 2023 13:57:33 +0000 Subject: [PATCH 01/22] chore: add queries for workspace build info --- site/src/api/queries/workspaceBuilds.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/site/src/api/queries/workspaceBuilds.ts b/site/src/api/queries/workspaceBuilds.ts index 98049a89e7ffe..9da020881b2b4 100644 --- a/site/src/api/queries/workspaceBuilds.ts +++ b/site/src/api/queries/workspaceBuilds.ts @@ -1,6 +1,21 @@ -import { UseInfiniteQueryOptions } from "react-query"; +import { QueryOptions, UseInfiniteQueryOptions } from "react-query"; import * as API from "api/api"; -import { WorkspaceBuild, WorkspaceBuildsRequest } from "api/typesGenerated"; +import { + type WorkspaceBuild, + type WorkspaceBuildParameter, + type WorkspaceBuildsRequest, +} from "api/typesGenerated"; + +export function workspaceBuildParametersKey(workspaceBuildId: string) { + return ["workspaceBuilds", workspaceBuildId, "parameters"] as const; +} + +export function workspaceBuildParameters(workspaceBuildId: string) { + return { + queryKey: workspaceBuildParametersKey(workspaceBuildId), + queryFn: () => API.getWorkspaceBuildParameters(workspaceBuildId), + } as const satisfies QueryOptions; +} export const workspaceBuildByNumber = ( username: string, From 112bc95473612dee352a27f4df13010bca5be904 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 20 Oct 2023 13:58:53 +0000 Subject: [PATCH 02/22] refactor: clean up logic for CreateWorkspacePage to support multiple modes --- .../CreateWorkspacePage.tsx | 49 +++++++++++++------ 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index 2c6cff4807e81..e46cd3920be09 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -30,7 +30,8 @@ import { CreateWSPermissions, createWorkspaceChecks } from "./permissions"; import { paramsUsedToCreateWorkspace } from "utils/workspace"; import { useEffectEvent } from "hooks/hookPolyfills"; -type CreateWorkspaceMode = "form" | "auto"; +export const createWorkspaceModes = ["form", "auto", "duplicate"] as const; +export type CreateWorkspaceMode = (typeof createWorkspaceModes)[number]; export type ExternalAuthPollingState = "idle" | "polling" | "abandoned"; @@ -41,10 +42,9 @@ const CreateWorkspacePage: FC = () => { const navigate = useNavigate(); const [searchParams, setSearchParams] = useSearchParams(); const defaultBuildParameters = getDefaultBuildParameters(searchParams); - const mode = (searchParams.get("mode") ?? "form") as CreateWorkspaceMode; + const mode = getWorkspaceMode(searchParams); const customVersionId = searchParams.get("version") ?? undefined; - const defaultName = - mode === "auto" ? generateUniqueName() : searchParams.get("name") ?? ""; + const defaultName = getDefaultName(mode, searchParams); const queryClient = useQueryClient(); const autoCreateWorkspaceMutation = useMutation( @@ -220,18 +220,13 @@ const getDefaultBuildParameters = ( return buildValues; }; -export const orderedTemplateParameters = ( - templateParameters?: TemplateVersionParameter[], -): TemplateVersionParameter[] => { - if (!templateParameters) { - return []; - } - - const immutables = templateParameters.filter( - (parameter) => !parameter.mutable, - ); - const mutables = templateParameters.filter((parameter) => parameter.mutable); - return [...immutables, ...mutables]; +export const orderTemplateParameters = ( + templateParameters?: readonly TemplateVersionParameter[], +) => { + return { + mutable: templateParameters?.filter((p) => p.mutable) ?? [], + immutable: templateParameters?.filter((p) => !p.mutable) ?? [], + } as const; }; const generateUniqueName = () => { @@ -245,3 +240,25 @@ const generateUniqueName = () => { }; export default CreateWorkspacePage; + +function getWorkspaceMode(params: URLSearchParams): CreateWorkspaceMode { + const paramMode = params.get("mode"); + if (createWorkspaceModes.includes(paramMode as CreateWorkspaceMode)) { + return paramMode as CreateWorkspaceMode; + } + + return "form"; +} + +function getDefaultName(mode: CreateWorkspaceMode, params: URLSearchParams) { + if (mode === "auto") { + return generateUniqueName(); + } + + const paramsName = params.get("name"); + if (mode === "duplicate" && paramsName) { + return `${paramsName}-copy`; + } + + return paramsName ?? ""; +} From 15fdfbf18c285609e3bdfc42cce98a4ca03c0466 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 20 Oct 2023 13:59:26 +0000 Subject: [PATCH 03/22] chore: add custom workspace duplication hook --- .../CreateWorkspacePage.tsx | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index e46cd3920be09..bed599f251370 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -29,6 +29,7 @@ import { checkAuthorization } from "api/queries/authCheck"; import { CreateWSPermissions, createWorkspaceChecks } from "./permissions"; import { paramsUsedToCreateWorkspace } from "utils/workspace"; import { useEffectEvent } from "hooks/hookPolyfills"; +import { workspaceBuildParameters } from "api/queries/workspaceBuilds"; export const createWorkspaceModes = ["form", "auto", "duplicate"] as const; export type CreateWorkspaceMode = (typeof createWorkspaceModes)[number]; @@ -262,3 +263,64 @@ function getDefaultName(mode: CreateWorkspaceMode, params: URLSearchParams) { return paramsName ?? ""; } + +function getDuplicationUrlParams( + workspaceParams: readonly WorkspaceBuildParameter[], + workspace: Workspace, +): URLSearchParams { + // Record type makes sure that every property key added starts with "param."; + // page is also set up to parse params with this prefix for auto mode + const consolidatedParams: Record<`param.${string}`, string> = {}; + + for (const p of workspaceParams) { + consolidatedParams[`param.${p.name}`] = p.value; + } + + return new URLSearchParams({ + ...consolidatedParams, + mode: "duplicate" satisfies CreateWorkspaceMode, + name: workspace.name, + version: workspace.template_active_version_id, + }); +} + +/** + * Takes a workspace, and returns out a function that will navigate the user to + * the 'Create Workspace' page, pre-filling the form with as much information + * about the workspace as possible. + */ +// Meant to be consumed by components outside of this file +export function useWorkspaceDuplication(workspace: Workspace) { + const navigate = useNavigate(); + const buildParametersQuery = useQuery( + workspaceBuildParameters(workspace.latest_build.id), + ); + + // Not using useEffectEvent for this, because useEffect isn't really an + // intended use case for this custom hook + const duplicateWorkspace = useCallback(() => { + const buildParams = buildParametersQuery.data; + if (buildParams === undefined) { + return; + } + + const newUrlParams = getDuplicationUrlParams(buildParams, workspace); + + // Necessary for giving modals/popups time to flush their state changes and + // close the popup before actually navigating. MUI does provide the + // disablePortal prop, which also side-steps this issue, but you have to + // remember to put it on any component that calls this function. Better to + // code defensively and have some redundancy in case someone forgets + void Promise.resolve().then(() => { + navigate({ + pathname: `/templates/${workspace.template_name}/workspace`, + search: newUrlParams.toString(), + }); + }); + }, [navigate, workspace, buildParametersQuery.data]); + + return { + duplicateWorkspace, + duplicationReady: buildParametersQuery.isSuccess, + } as const; +} From d007b865e78be9422e3d133c9bac562827633f2a Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 20 Oct 2023 14:13:12 +0000 Subject: [PATCH 04/22] chore: integrate mode into CreateWorkspacePageView --- .../CreateWorkspacePage.tsx | 1 + .../CreateWorkspacePageView.tsx | 279 ++++++++++-------- 2 files changed, 162 insertions(+), 118 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index bed599f251370..74bc53285720a 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -123,6 +123,7 @@ const CreateWorkspacePage: FC = () => { ) : ( = ({ + mode, error, defaultName, defaultOwner, @@ -112,133 +120,143 @@ export const CreateWorkspacePageView: FC = ({ ); return ( - - - {Boolean(error) && } - {/* General info */} - - - - {versionId && versionId !== template.active_version_id && ( - - - - This parameter has been preset, and cannot be modified. - - - )} - - - + <> + {mode === "duplicate" && } - {permissions.createWorkspaceForUser && ( + + + {Boolean(error) && } + {/* General info */} - { - setOwner(user ?? defaultOwner); - }} - label="Owner" - size="medium" + + {versionId && versionId !== template.active_version_id && ( + + + + This parameter has been preset, and cannot be modified. + + + )} + - )} - {externalAuth && externalAuth.length > 0 && ( - - - {externalAuth.map((auth) => ( - + + { + setOwner(user ?? defaultOwner); + }} + label="Owner" + size="medium" /> - ))} - - - )} + + + )} - {parameters && ( - <> - { - return { - ...getFieldHelpers( - "rich_parameter_values[" + index + "].value", - ), - onChange: async (value) => { - await form.setFieldValue("rich_parameter_values." + index, { - name: parameter.name, - value: value, - }); - }, - disabled: - disabledParamsList?.includes( - parameter.name.toLowerCase().replace(/ /g, "_"), - ) || creatingWorkspace, - }; - }} - /> - { - return { - ...getFieldHelpers( - "rich_parameter_values[" + index + "].value", - ), - onChange: async (value) => { - await form.setFieldValue("rich_parameter_values." + index, { - name: parameter.name, - value: value, - }); - }, - disabled: - disabledParamsList?.includes( - parameter.name.toLowerCase().replace(/ /g, "_"), - ) || creatingWorkspace, - }; - }} - /> - - )} + {externalAuth && externalAuth.length > 0 && ( + + + {externalAuth.map((auth) => ( + + ))} + + + )} - - - + {parameters && ( + <> + { + return { + ...getFieldHelpers( + "rich_parameter_values[" + index + "].value", + ), + onChange: async (value) => { + await form.setFieldValue( + "rich_parameter_values." + index, + { + name: parameter.name, + value: value, + }, + ); + }, + disabled: + disabledParamsList?.includes( + parameter.name.toLowerCase().replace(/ /g, "_"), + ) || creatingWorkspace, + }; + }} + /> + { + return { + ...getFieldHelpers( + "rich_parameter_values[" + index + "].value", + ), + onChange: async (value) => { + await form.setFieldValue( + "rich_parameter_values." + index, + { + name: parameter.name, + value: value, + }, + ); + }, + disabled: + disabledParamsList?.includes( + parameter.name.toLowerCase().replace(/ /g, "_"), + ) || creatingWorkspace, + }; + }} + /> + + )} + + + + + ); }; @@ -279,6 +297,31 @@ const useExternalAuthVerification = ( }; }; +function DuplicateWarningMessage() { + const [isDismissed, dismiss] = useReducer(() => true, false); + const theme = useTheme(); + + if (isDismissed) { + return null; + } + + // Setup looks a little hokey (having an Alert already fully configured to + // listen to dismissals, on top of more dismissal state), but relying solely + // on the Alert API wouldn't get rid of the div and horizontal margin helper + // after the dismiss happens. Not using CSS margins because those can be a + // style maintenance nightmare over time + return ( +
+ + + Duplicating a workspace only copies its parameters. No state from the + old workspace is copied over. + + +
+ ); +} + const useStyles = makeStyles((theme) => ({ hasDescription: { paddingBottom: theme.spacing(2), From 294156ec1e27cffc40b13953f5de294147c728d2 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 20 Oct 2023 14:25:37 +0000 Subject: [PATCH 05/22] fix: add mode to CreateWorkspacePageView stories --- .../CreateWorkspacePage/CreateWorkspacePageView.stories.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.stories.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.stories.tsx index fb0546ca41363..dfb43f5c31a9b 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.stories.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.stories.tsx @@ -19,6 +19,7 @@ const meta: Meta = { template: MockTemplate, parameters: [], externalAuth: [], + mode: "form", permissions: { createWorkspaceForUser: true, }, From 4554895aa4d6465d87fc8d7a14355404fb0d0d24 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 20 Oct 2023 14:39:09 +0000 Subject: [PATCH 06/22] refactor: extract workspace duplication outside CreateWorkspacePage file --- .../CreateWorkspacePage.tsx | 62 ---------------- .../useWorkspaceDuplication.ts | 71 +++++++++++++++++++ 2 files changed, 71 insertions(+), 62 deletions(-) create mode 100644 site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.ts diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index 74bc53285720a..7876a40361c58 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -29,7 +29,6 @@ import { checkAuthorization } from "api/queries/authCheck"; import { CreateWSPermissions, createWorkspaceChecks } from "./permissions"; import { paramsUsedToCreateWorkspace } from "utils/workspace"; import { useEffectEvent } from "hooks/hookPolyfills"; -import { workspaceBuildParameters } from "api/queries/workspaceBuilds"; export const createWorkspaceModes = ["form", "auto", "duplicate"] as const; export type CreateWorkspaceMode = (typeof createWorkspaceModes)[number]; @@ -264,64 +263,3 @@ function getDefaultName(mode: CreateWorkspaceMode, params: URLSearchParams) { return paramsName ?? ""; } - -function getDuplicationUrlParams( - workspaceParams: readonly WorkspaceBuildParameter[], - workspace: Workspace, -): URLSearchParams { - // Record type makes sure that every property key added starts with "param."; - // page is also set up to parse params with this prefix for auto mode - const consolidatedParams: Record<`param.${string}`, string> = {}; - - for (const p of workspaceParams) { - consolidatedParams[`param.${p.name}`] = p.value; - } - - return new URLSearchParams({ - ...consolidatedParams, - mode: "duplicate" satisfies CreateWorkspaceMode, - name: workspace.name, - version: workspace.template_active_version_id, - }); -} - -/** - * Takes a workspace, and returns out a function that will navigate the user to - * the 'Create Workspace' page, pre-filling the form with as much information - * about the workspace as possible. - */ -// Meant to be consumed by components outside of this file -export function useWorkspaceDuplication(workspace: Workspace) { - const navigate = useNavigate(); - const buildParametersQuery = useQuery( - workspaceBuildParameters(workspace.latest_build.id), - ); - - // Not using useEffectEvent for this, because useEffect isn't really an - // intended use case for this custom hook - const duplicateWorkspace = useCallback(() => { - const buildParams = buildParametersQuery.data; - if (buildParams === undefined) { - return; - } - - const newUrlParams = getDuplicationUrlParams(buildParams, workspace); - - // Necessary for giving modals/popups time to flush their state changes and - // close the popup before actually navigating. MUI does provide the - // disablePortal prop, which also side-steps this issue, but you have to - // remember to put it on any component that calls this function. Better to - // code defensively and have some redundancy in case someone forgets - void Promise.resolve().then(() => { - navigate({ - pathname: `/templates/${workspace.template_name}/workspace`, - search: newUrlParams.toString(), - }); - }); - }, [navigate, workspace, buildParametersQuery.data]); - - return { - duplicateWorkspace, - duplicationReady: buildParametersQuery.isSuccess, - } as const; -} diff --git a/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.ts b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.ts new file mode 100644 index 0000000000000..9ceaf1f7206e9 --- /dev/null +++ b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.ts @@ -0,0 +1,71 @@ +import { useNavigate } from "react-router-dom"; +import { useQuery } from "react-query"; +import { type CreateWorkspaceMode } from "./CreateWorkspacePage"; +import { + type Workspace, + type WorkspaceBuildParameter, +} from "api/typesGenerated"; +import { workspaceBuildParameters } from "api/queries/workspaceBuilds"; +import { useCallback } from "react"; + +function getDuplicationUrlParams( + workspaceParams: readonly WorkspaceBuildParameter[], + workspace: Workspace, +): URLSearchParams { + // Record type makes sure that every property key added starts with "param."; + // page is also set up to parse params with this prefix for auto mode + const consolidatedParams: Record<`param.${string}`, string> = {}; + + for (const p of workspaceParams) { + consolidatedParams[`param.${p.name}`] = p.value; + } + + return new URLSearchParams({ + ...consolidatedParams, + mode: "duplicate" satisfies CreateWorkspaceMode, + name: workspace.name, + version: workspace.template_active_version_id, + }); +} + +/** + * Takes a workspace, and returns out a function that will navigate the user to + * the 'Create Workspace' page, pre-filling the form with as much information + * about the workspace as possible. + */ +export function useWorkspaceDuplication(workspace?: Workspace) { + const navigate = useNavigate(); + const buildParametersQuery = useQuery( + workspace !== undefined + ? workspaceBuildParameters(workspace.latest_build.id) + : { enabled: false }, + ); + + // Not using useEffectEvent for this, because useEffect isn't really an + // intended use case for this custom hook + const duplicateWorkspace = useCallback(() => { + const buildParams = buildParametersQuery.data; + if (buildParams === undefined || workspace === undefined) { + return; + } + + const newUrlParams = getDuplicationUrlParams(buildParams, workspace); + + // Necessary for giving modals/popups time to flush their state changes and + // close the popup before actually navigating. MUI does provide the + // disablePortal prop, which also side-steps this issue, but you have to + // remember to put it on any component that calls this function. Better to + // code defensively and have some redundancy in case someone forgets + void Promise.resolve().then(() => { + navigate({ + pathname: `/templates/${workspace.template_name}/workspace`, + search: newUrlParams.toString(), + }); + }); + }, [navigate, workspace, buildParametersQuery.data]); + + return { + duplicateWorkspace, + duplicationReady: buildParametersQuery.isSuccess, + } as const; +} From 25bacf2d8cb456555d19f4636ac0b6a0ed8eede8 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 20 Oct 2023 14:59:14 +0000 Subject: [PATCH 07/22] chore: integrate useWorkspaceDuplication into WorkspaceActions --- .../WorkspaceActions/WorkspaceActions.tsx | 33 +++++++++++++++---- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx index 226396720b593..894a7ad6b90fd 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx @@ -19,10 +19,14 @@ import { ButtonTypesEnum, actionsByWorkspaceStatus, } from "./constants"; -import SettingsOutlined from "@mui/icons-material/SettingsOutlined"; -import HistoryOutlined from "@mui/icons-material/HistoryOutlined"; -import DeleteOutlined from "@mui/icons-material/DeleteOutlined"; + import IconButton from "@mui/material/IconButton"; +import DuplicateIcon from "@mui/icons-material/FileCopyOutlined"; +import SettingsIcon from "@mui/icons-material/SettingsOutlined"; +import HistoryIcon from "@mui/icons-material/HistoryOutlined"; +import DeleteIcon from "@mui/icons-material/DeleteOutlined"; + +import { useWorkspaceDuplication } from "pages/CreateWorkspacePage/useWorkspaceDuplication"; export interface WorkspaceActionsProps { workspace: Workspace; @@ -69,6 +73,8 @@ export const WorkspaceActions: FC = ({ const canBeUpdated = workspace.outdated && canAcceptJobs; const menuTriggerRef = useRef(null); const [isMenuOpen, setIsMenuOpen] = useState(false); + const { duplicateWorkspace, duplicationReady } = + useWorkspaceDuplication(workspace); // A mapping of button type to the corresponding React component const buttonMapping: ButtonMapping = { @@ -120,12 +126,16 @@ export const WorkspaceActions: FC = ({ (isUpdating ? buttonMapping[ButtonTypesEnum.updating] : buttonMapping[ButtonTypesEnum.update])} + {isRestarting && buttonMapping[ButtonTypesEnum.restarting]} + {!isRestarting && actionsByStatus.map((action) => ( {buttonMapping[action]} ))} + {canCancel && } +
= ({ > + = ({ onClose={() => setIsMenuOpen(false)} > - + Settings + {canChangeVersions && ( - + Change version… )} + + + + Duplicate… + + - + Delete… From 094703118c5bdd24bf8d1b6dd179d1adb964edcc Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 20 Oct 2023 15:03:00 +0000 Subject: [PATCH 08/22] chore: delete unnecessary function --- .../pages/CreateWorkspacePage/CreateWorkspacePage.tsx | 9 --------- 1 file changed, 9 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index 7876a40361c58..7a0d41ce9fe51 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -221,15 +221,6 @@ const getDefaultBuildParameters = ( return buildValues; }; -export const orderTemplateParameters = ( - templateParameters?: readonly TemplateVersionParameter[], -) => { - return { - mutable: templateParameters?.filter((p) => p.mutable) ?? [], - immutable: templateParameters?.filter((p) => !p.mutable) ?? [], - } as const; -}; - const generateUniqueName = () => { const numberDictionary = NumberDictionary.generate({ min: 0, max: 99 }); return uniqueNamesGenerator({ From d71acf66dafa2ffade938433ee7628e842ea92ae Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 31 Oct 2023 18:12:48 +0000 Subject: [PATCH 09/22] refactor: swap useReducer for useState --- .../CreateWorkspacePage/CreateWorkspacePageView.tsx | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx index 0194dc690f5ab..1248110a58f81 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx @@ -2,7 +2,7 @@ import TextField from "@mui/material/TextField"; import * as TypesGen from "api/typesGenerated"; import { UserAutocomplete } from "components/UserAutocomplete/UserAutocomplete"; import { FormikContextType, useFormik } from "formik"; -import { FC, useEffect, useReducer, useState } from "react"; +import { FC, useEffect, useState } from "react"; import { getFormHelpers, nameValidator, @@ -298,7 +298,7 @@ const useExternalAuthVerification = ( }; function DuplicateWarningMessage() { - const [isDismissed, dismiss] = useReducer(() => true, false); + const [isDismissed, setIsDismissed] = useState(false); const theme = useTheme(); if (isDismissed) { @@ -313,7 +313,11 @@ function DuplicateWarningMessage() { return (
- + setIsDismissed(true)} + > Duplicating a workspace only copies its parameters. No state from the old workspace is copied over. From c0a8c56561e1cc343316f9956941d3d5a60df44c Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 31 Oct 2023 18:20:03 +0000 Subject: [PATCH 10/22] fix: swap warning alert for info alert --- site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx index 1248110a58f81..4d529cd6b0199 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx @@ -314,7 +314,7 @@ function DuplicateWarningMessage() {
setIsDismissed(true)} > From 0b3e954162e20351650bb3d4ea1f8798d46a5bed Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 31 Oct 2023 18:36:16 +0000 Subject: [PATCH 11/22] refactor: move info alert message --- .../CreateWorkspacePageView.tsx | 263 ++++++++---------- 1 file changed, 122 insertions(+), 141 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx index 4d529cd6b0199..7928a1ad6f608 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx @@ -35,8 +35,6 @@ import { } from "./CreateWorkspacePage"; import { useSearchParams } from "react-router-dom"; import { CreateWSPermissions } from "./permissions"; -import { useTheme } from "@emotion/react"; -import { Margins } from "components/Margins/Margins"; import { Alert } from "components/Alert/Alert"; export interface CreateWorkspacePageViewProps { @@ -120,143 +118,135 @@ export const CreateWorkspacePageView: FC = ({ ); return ( - <> - {mode === "duplicate" && } + + + {mode === "duplicate" && } + {Boolean(error) && } - - - {Boolean(error) && } - {/* General info */} + {/* General info */} + + + + {versionId && versionId !== template.active_version_id && ( + + + + This parameter has been preset, and cannot be modified. + + + )} + + + + + {permissions.createWorkspaceForUser && ( - - {versionId && versionId !== template.active_version_id && ( - - - - This parameter has been preset, and cannot be modified. - - - )} - { + setOwner(user ?? defaultOwner); + }} + label="Owner" + size="medium" /> + )} - {permissions.createWorkspaceForUser && ( - - - { - setOwner(user ?? defaultOwner); - }} - label="Owner" - size="medium" + {externalAuth && externalAuth.length > 0 && ( + + + {externalAuth.map((auth) => ( + - - - )} - - {externalAuth && externalAuth.length > 0 && ( - - - {externalAuth.map((auth) => ( - - ))} - - - )} + ))} + + + )} - {parameters && ( - <> - { - return { - ...getFieldHelpers( - "rich_parameter_values[" + index + "].value", - ), - onChange: async (value) => { - await form.setFieldValue( - "rich_parameter_values." + index, - { - name: parameter.name, - value: value, - }, - ); - }, - disabled: - disabledParamsList?.includes( - parameter.name.toLowerCase().replace(/ /g, "_"), - ) || creatingWorkspace, - }; - }} - /> - { - return { - ...getFieldHelpers( - "rich_parameter_values[" + index + "].value", - ), - onChange: async (value) => { - await form.setFieldValue( - "rich_parameter_values." + index, - { - name: parameter.name, - value: value, - }, - ); - }, - disabled: - disabledParamsList?.includes( - parameter.name.toLowerCase().replace(/ /g, "_"), - ) || creatingWorkspace, - }; - }} - /> - - )} + {parameters && ( + <> + { + return { + ...getFieldHelpers( + "rich_parameter_values[" + index + "].value", + ), + onChange: async (value) => { + await form.setFieldValue("rich_parameter_values." + index, { + name: parameter.name, + value: value, + }); + }, + disabled: + disabledParamsList?.includes( + parameter.name.toLowerCase().replace(/ /g, "_"), + ) || creatingWorkspace, + }; + }} + /> + { + return { + ...getFieldHelpers( + "rich_parameter_values[" + index + "].value", + ), + onChange: async (value) => { + await form.setFieldValue("rich_parameter_values." + index, { + name: parameter.name, + value: value, + }); + }, + disabled: + disabledParamsList?.includes( + parameter.name.toLowerCase().replace(/ /g, "_"), + ) || creatingWorkspace, + }; + }} + /> + + )} - - - - + + + ); }; @@ -299,7 +289,6 @@ const useExternalAuthVerification = ( function DuplicateWarningMessage() { const [isDismissed, setIsDismissed] = useState(false); - const theme = useTheme(); if (isDismissed) { return null; @@ -311,18 +300,10 @@ function DuplicateWarningMessage() { // after the dismiss happens. Not using CSS margins because those can be a // style maintenance nightmare over time return ( -
- - setIsDismissed(true)} - > - Duplicating a workspace only copies its parameters. No state from the - old workspace is copied over. - - -
+ setIsDismissed(true)}> + Duplicating a workspace only copies its parameters. No state from the old + workspace is copied over. + ); } From 7a763a9d5b1c52f85b11408ebda2f8aa8715f586 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 31 Oct 2023 18:53:25 +0000 Subject: [PATCH 12/22] refactor: simplify UI logic for mode alerts --- .../CreateWorkspacePageView.tsx | 28 +++++-------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx index 7928a1ad6f608..8077f838af77b 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx @@ -120,9 +120,15 @@ export const CreateWorkspacePageView: FC = ({ return ( - {mode === "duplicate" && } {Boolean(error) && } + {mode === "duplicate" && ( + + Duplicating a workspace only copies its parameters. No state from + the old workspace is copied over. + + )} + {/* General info */} setIsDismissed(true)}> - Duplicating a workspace only copies its parameters. No state from the old - workspace is copied over. - - ); -} - const useStyles = makeStyles((theme) => ({ hasDescription: { paddingBottom: theme.spacing(2), From da488fa12fccd09c9bec77b759c7e1c125e53565 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 31 Oct 2023 18:53:49 +0000 Subject: [PATCH 13/22] fix: prevent dismissed Alerts from affecting layouts --- site/src/components/Alert/Alert.tsx | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/site/src/components/Alert/Alert.tsx b/site/src/components/Alert/Alert.tsx index fb3f5e07c170d..2b8a196056fe6 100644 --- a/site/src/components/Alert/Alert.tsx +++ b/site/src/components/Alert/Alert.tsx @@ -21,6 +21,13 @@ export const Alert: FC = ({ }) => { const [open, setOpen] = useState(true); + // Can't only rely on MUI's hiding behavior inside flex layouts, because even + // though MUI will make the alert have zero height, the alert will still + // behave as a flex child and introduce extra row/column gaps + if (!open) { + return null; + } + return ( Date: Tue, 31 Oct 2023 18:54:39 +0000 Subject: [PATCH 14/22] fix: remove unnecessary prop binding --- site/src/components/Alert/Alert.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/components/Alert/Alert.tsx b/site/src/components/Alert/Alert.tsx index 2b8a196056fe6..efec4029c0654 100644 --- a/site/src/components/Alert/Alert.tsx +++ b/site/src/components/Alert/Alert.tsx @@ -29,7 +29,7 @@ export const Alert: FC = ({ } return ( - + Date: Tue, 31 Oct 2023 18:56:43 +0000 Subject: [PATCH 15/22] docs: reword comment for clarity --- site/src/components/Alert/Alert.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/site/src/components/Alert/Alert.tsx b/site/src/components/Alert/Alert.tsx index efec4029c0654..bba4044c4ce86 100644 --- a/site/src/components/Alert/Alert.tsx +++ b/site/src/components/Alert/Alert.tsx @@ -22,8 +22,8 @@ export const Alert: FC = ({ const [open, setOpen] = useState(true); // Can't only rely on MUI's hiding behavior inside flex layouts, because even - // though MUI will make the alert have zero height, the alert will still - // behave as a flex child and introduce extra row/column gaps + // though MUI will make a dismissed alert have zero height, the alert will + // still behave as a flex child and introduce extra row/column gaps if (!open) { return null; } From aeacda5bff64013e579b0ff296622ae34e5075ce Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 31 Oct 2023 21:52:48 +0000 Subject: [PATCH 16/22] chore: update msw build params to return multiple params --- site/src/testHelpers/handlers.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/site/src/testHelpers/handlers.ts b/site/src/testHelpers/handlers.ts index 8bc5dabba16c4..fe1fceabbcb09 100644 --- a/site/src/testHelpers/handlers.ts +++ b/site/src/testHelpers/handlers.ts @@ -354,7 +354,16 @@ export const handlers = [ rest.get( "/api/v2/workspacebuilds/:workspaceBuildId/parameters", (_, res, ctx) => { - return res(ctx.status(200), ctx.json([M.MockWorkspaceBuildParameter1])); + return res( + ctx.status(200), + ctx.json([ + M.MockWorkspaceBuildParameter1, + M.MockWorkspaceBuildParameter2, + M.MockWorkspaceBuildParameter3, + M.MockWorkspaceBuildParameter4, + M.MockWorkspaceBuildParameter5, + ]), + ); }, ), From 230a4f15b5acacd977661148aba22ab82954ea8d Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 31 Oct 2023 22:11:06 +0000 Subject: [PATCH 17/22] chore: rename duplicationReady to isDuplicationReady --- site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.ts | 2 +- .../pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.ts b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.ts index 9ceaf1f7206e9..6e929dd6f092f 100644 --- a/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.ts +++ b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.ts @@ -66,6 +66,6 @@ export function useWorkspaceDuplication(workspace?: Workspace) { return { duplicateWorkspace, - duplicationReady: buildParametersQuery.isSuccess, + isDuplicationReady: buildParametersQuery.isSuccess, } as const; } diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx index 894a7ad6b90fd..0832c9dcead3a 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx @@ -73,7 +73,7 @@ export const WorkspaceActions: FC = ({ const canBeUpdated = workspace.outdated && canAcceptJobs; const menuTriggerRef = useRef(null); const [isMenuOpen, setIsMenuOpen] = useState(false); - const { duplicateWorkspace, duplicationReady } = + const { duplicateWorkspace, isDuplicationReady } = useWorkspaceDuplication(workspace); // A mapping of button type to the corresponding React component @@ -170,7 +170,7 @@ export const WorkspaceActions: FC = ({ Duplicate… From 75b183949e93d585c3e57a1ac7329611b0c11f51 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 1 Nov 2023 14:23:19 +0000 Subject: [PATCH 18/22] chore: expose root component for testing/re-rendering --- site/src/testHelpers/renderHelpers.tsx | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/site/src/testHelpers/renderHelpers.tsx b/site/src/testHelpers/renderHelpers.tsx index 1de1c1997b12b..fb9fcf5c0ebfe 100644 --- a/site/src/testHelpers/renderHelpers.tsx +++ b/site/src/testHelpers/renderHelpers.tsx @@ -29,12 +29,15 @@ export const renderWithRouter = ( }, }); + const rootComponent = ( + + + + ); + return { - ...tlRender( - - - , - ), + ...tlRender(rootComponent), + rootComponent, router, }; }; From 7cf446fb9116027126d1369b5c82da3212c6912a Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 1 Nov 2023 14:28:04 +0000 Subject: [PATCH 19/22] chore: get tests in place (still have act warnings) --- .../useWorkspaceDuplication.test.tsx | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx diff --git a/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx new file mode 100644 index 0000000000000..7ede996963e88 --- /dev/null +++ b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx @@ -0,0 +1,114 @@ +import { waitFor, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { createMemoryRouter } from "react-router-dom"; +import { renderWithRouter } from "testHelpers/renderHelpers"; + +import * as M from "../../testHelpers/entities"; +import { type Workspace } from "api/typesGenerated"; +import { useWorkspaceDuplication } from "./useWorkspaceDuplication"; +import { MockWorkspace } from "testHelpers/entities"; +import CreateWorkspacePage from "./CreateWorkspacePage"; + +// Tried really hard to get these tests working with RTL's renderHook, but I +// kept running into weird function mismatches, mostly stemming from the fact +// that React Router's RouteProvider does not accept children, meaning that you +// can't inject values into it with renderHook's wrapper +function WorkspaceMock({ workspace }: { workspace?: Workspace }) { + const { duplicateWorkspace, isDuplicationReady } = + useWorkspaceDuplication(workspace); + + return ( + + ); +} + +function renderInMemory(workspace?: Workspace) { + const router = createMemoryRouter([ + { path: "/", element: }, + { + path: "/templates/:template/workspace", + element: , + }, + ]); + + return renderWithRouter(router); +} + +async function performNavigation( + button: HTMLElement, + router: ReturnType, +) { + await waitFor(() => expect(button).not.toBeDisabled()); + + await userEvent.click(button); + await waitFor(() => { + expect(router.state.location.pathname).toEqual( + `/templates/${MockWorkspace.template_name}/workspace`, + ); + }); +} + +describe(`${useWorkspaceDuplication.name}`, () => { + it("Will never be ready when there is no workspace passed in", async () => { + const { rootComponent, rerender } = renderInMemory(undefined); + const button = await screen.findByRole("button"); + expect(button).toBeDisabled(); + + for (let i = 0; i < 10; i++) { + rerender(rootComponent); + expect(button).toBeDisabled(); + } + }); + + it("Will become ready when workspace is provided and build params are successfully fetched", async () => { + renderInMemory(MockWorkspace); + const button = await screen.findByRole("button"); + + expect(button).toBeDisabled(); + await waitFor(() => expect(button).not.toBeDisabled()); + }); + + it("duplicateWorkspace navigates the user to the workspace creation page", async () => { + const { router } = renderInMemory(MockWorkspace); + const button = await screen.findByRole("button"); + await performNavigation(button, router); + }); + + test("Navigating populates the URL search params with the workspace's build params", async () => { + const { router } = renderInMemory(MockWorkspace); + const button = await screen.findByRole("button"); + await performNavigation(button, router); + + const parsedParams = new URLSearchParams(router.state.location.search); + const mockBuildParams = [ + M.MockWorkspaceBuildParameter1, + M.MockWorkspaceBuildParameter2, + M.MockWorkspaceBuildParameter3, + M.MockWorkspaceBuildParameter4, + M.MockWorkspaceBuildParameter5, + ]; + + for (const p of mockBuildParams) { + expect(parsedParams.get(`param.${p.name}`)).toEqual(p.value); + } + }); + + test("Navigating appends other necessary metadata to the search params", async () => { + const { router } = renderInMemory(MockWorkspace); + const button = await screen.findByRole("button"); + await performNavigation(button, router); + + const parsedParams = new URLSearchParams(router.state.location.search); + const expectedParamEntries = [ + ["mode", "duplicate"], + ["name", MockWorkspace.name], + ["version", MockWorkspace.template_active_version_id], + ] as const; + + for (const [key, value] of expectedParamEntries) { + expect(parsedParams.get(key)).toBe(value); + } + }); +}); From bf216569510c8e255a50b28160f36d192aabc225 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 1 Nov 2023 14:33:40 +0000 Subject: [PATCH 20/22] refactor: move stuff around for clarity --- .../CreateWorkspacePage/useWorkspaceDuplication.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx index 7ede996963e88..123755c6c9e6e 100644 --- a/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx +++ b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx @@ -41,9 +41,9 @@ async function performNavigation( router: ReturnType, ) { await waitFor(() => expect(button).not.toBeDisabled()); - await userEvent.click(button); - await waitFor(() => { + + return waitFor(() => { expect(router.state.location.pathname).toEqual( `/templates/${MockWorkspace.template_name}/workspace`, ); From 38ba3b2ca3d9b4e691c80d8653dfd2c9af96c213 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 1 Nov 2023 15:46:51 +0000 Subject: [PATCH 21/22] chore: finish tests --- .../CreateWorkspacePage.test.tsx | 22 +++++++++++++++++++ .../CreateWorkspacePageView.tsx | 8 +++++-- .../useWorkspaceDuplication.test.tsx | 9 ++++---- 3 files changed, 33 insertions(+), 6 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx index 37cfbd791ca6a..f4ffaecf5285c 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx @@ -20,6 +20,7 @@ import { waitForLoaderToBeRemoved, } from "testHelpers/renderHelpers"; import CreateWorkspacePage from "./CreateWorkspacePage"; +import { Language } from "./CreateWorkspacePageView"; const nameLabelText = "Workspace Name"; const createWorkspaceText = "Create Workspace"; @@ -270,4 +271,25 @@ describe("CreateWorkspacePage", () => { ); }); }); + + it("Detects when a workspace is being created with the 'duplicate' mode", async () => { + const params = new URLSearchParams({ + mode: "duplicate", + name: MockWorkspace.name, + version: MockWorkspace.template_active_version_id, + }); + + renderWithAuth(, { + path: "/templates/:template/workspace", + route: `/templates/${MockWorkspace.name}/workspace?${params.toString()}`, + }); + + const warningMessage = await screen.findByRole("alert"); + const nameInput = await screen.findByRole("textbox", { + name: "Workspace Name", + }); + + expect(warningMessage).toHaveTextContent(Language.duplicationWarning); + expect(nameInput).toHaveValue(`${MockWorkspace.name}-copy`); + }); }); diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx index 8077f838af77b..1c6010b0f332e 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx @@ -37,6 +37,11 @@ import { useSearchParams } from "react-router-dom"; import { CreateWSPermissions } from "./permissions"; import { Alert } from "components/Alert/Alert"; +export const Language = { + duplicationWarning: + "Duplicating a workspace only copies its parameters. No state from the old workspace is copied over.", +} as const; + export interface CreateWorkspacePageViewProps { mode: CreateWorkspaceMode; error: unknown; @@ -124,8 +129,7 @@ export const CreateWorkspacePageView: FC = ({ {mode === "duplicate" && ( - Duplicating a workspace only copies its parameters. No state from - the old workspace is copied over. + {Language.duplicationWarning} )} diff --git a/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx index 123755c6c9e6e..c54166a6f5ee4 100644 --- a/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx +++ b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx @@ -90,8 +90,9 @@ describe(`${useWorkspaceDuplication.name}`, () => { M.MockWorkspaceBuildParameter5, ]; - for (const p of mockBuildParams) { - expect(parsedParams.get(`param.${p.name}`)).toEqual(p.value); + for (const { name, value } of mockBuildParams) { + const key = `param.${name}`; + expect(parsedParams.get(key)).toEqual(value); } }); @@ -101,13 +102,13 @@ describe(`${useWorkspaceDuplication.name}`, () => { await performNavigation(button, router); const parsedParams = new URLSearchParams(router.state.location.search); - const expectedParamEntries = [ + const extraMetadataEntries = [ ["mode", "duplicate"], ["name", MockWorkspace.name], ["version", MockWorkspace.template_active_version_id], ] as const; - for (const [key, value] of expectedParamEntries) { + for (const [key, value] of extraMetadataEntries) { expect(parsedParams.get(key)).toBe(value); } }); From 923d080dc105a5adf68d95077e0039aec860546d Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 3 Nov 2023 14:46:42 +0000 Subject: [PATCH 22/22] chore: revamp tests --- .../useWorkspaceDuplication.test.tsx | 86 ++++++------- site/src/testHelpers/renderHelpers.tsx | 114 +++++++++++++++--- 2 files changed, 133 insertions(+), 67 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx index c54166a6f5ee4..44f09b8261ec8 100644 --- a/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx +++ b/site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx @@ -1,47 +1,36 @@ -import { waitFor, screen } from "@testing-library/react"; -import userEvent from "@testing-library/user-event"; -import { createMemoryRouter } from "react-router-dom"; -import { renderWithRouter } from "testHelpers/renderHelpers"; - +import { waitFor } from "@testing-library/react"; import * as M from "../../testHelpers/entities"; import { type Workspace } from "api/typesGenerated"; import { useWorkspaceDuplication } from "./useWorkspaceDuplication"; import { MockWorkspace } from "testHelpers/entities"; import CreateWorkspacePage from "./CreateWorkspacePage"; +import { renderHookWithAuth } from "testHelpers/renderHelpers"; -// Tried really hard to get these tests working with RTL's renderHook, but I -// kept running into weird function mismatches, mostly stemming from the fact -// that React Router's RouteProvider does not accept children, meaning that you -// can't inject values into it with renderHook's wrapper -function WorkspaceMock({ workspace }: { workspace?: Workspace }) { - const { duplicateWorkspace, isDuplicationReady } = - useWorkspaceDuplication(workspace); - - return ( - - ); -} - -function renderInMemory(workspace?: Workspace) { - const router = createMemoryRouter([ - { path: "/", element: }, +function render(workspace?: Workspace) { + return renderHookWithAuth( + ({ workspace }: { workspace?: Workspace }) => { + return useWorkspaceDuplication(workspace); + }, { - path: "/templates/:template/workspace", - element: , + initialProps: { workspace }, + extraRoutes: [ + { + path: "/templates/:template/workspace", + element: , + }, + ], }, - ]); - - return renderWithRouter(router); + ); } +type RenderResult = Awaited>; + async function performNavigation( - button: HTMLElement, - router: ReturnType, + result: RenderResult["result"], + router: RenderResult["router"], ) { - await waitFor(() => expect(button).not.toBeDisabled()); - await userEvent.click(button); + await waitFor(() => expect(result.current.isDuplicationReady).toBe(true)); + result.current.duplicateWorkspace(); return waitFor(() => { expect(router.state.location.pathname).toEqual( @@ -52,34 +41,30 @@ async function performNavigation( describe(`${useWorkspaceDuplication.name}`, () => { it("Will never be ready when there is no workspace passed in", async () => { - const { rootComponent, rerender } = renderInMemory(undefined); - const button = await screen.findByRole("button"); - expect(button).toBeDisabled(); + const { result, rerender } = await render(undefined); + expect(result.current.isDuplicationReady).toBe(false); for (let i = 0; i < 10; i++) { - rerender(rootComponent); - expect(button).toBeDisabled(); + rerender({ workspace: undefined }); + expect(result.current.isDuplicationReady).toBe(false); } }); it("Will become ready when workspace is provided and build params are successfully fetched", async () => { - renderInMemory(MockWorkspace); - const button = await screen.findByRole("button"); + const { result } = await render(MockWorkspace); - expect(button).toBeDisabled(); - await waitFor(() => expect(button).not.toBeDisabled()); + expect(result.current.isDuplicationReady).toBe(false); + await waitFor(() => expect(result.current.isDuplicationReady).toBe(true)); }); - it("duplicateWorkspace navigates the user to the workspace creation page", async () => { - const { router } = renderInMemory(MockWorkspace); - const button = await screen.findByRole("button"); - await performNavigation(button, router); + it("Is able to navigate the user to the workspace creation page", async () => { + const { result, router } = await render(MockWorkspace); + await performNavigation(result, router); }); test("Navigating populates the URL search params with the workspace's build params", async () => { - const { router } = renderInMemory(MockWorkspace); - const button = await screen.findByRole("button"); - await performNavigation(button, router); + const { result, router } = await render(MockWorkspace); + await performNavigation(result, router); const parsedParams = new URLSearchParams(router.state.location.search); const mockBuildParams = [ @@ -97,9 +82,8 @@ describe(`${useWorkspaceDuplication.name}`, () => { }); test("Navigating appends other necessary metadata to the search params", async () => { - const { router } = renderInMemory(MockWorkspace); - const button = await screen.findByRole("button"); - await performNavigation(button, router); + const { result, router } = await render(MockWorkspace); + await performNavigation(result, router); const parsedParams = new URLSearchParams(router.state.location.search); const extraMetadataEntries = [ diff --git a/site/src/testHelpers/renderHelpers.tsx b/site/src/testHelpers/renderHelpers.tsx index fb9fcf5c0ebfe..b07d4921bee0d 100644 --- a/site/src/testHelpers/renderHelpers.tsx +++ b/site/src/testHelpers/renderHelpers.tsx @@ -1,4 +1,9 @@ -import { render as tlRender, screen, waitFor } from "@testing-library/react"; +import { + render as tlRender, + screen, + waitFor, + renderHook, +} from "@testing-library/react"; import { AppProviders, ThemeProviders } from "App"; import { DashboardLayout } from "components/Dashboard/DashboardLayout"; import { TemplateSettingsLayout } from "pages/TemplateSettingsPage/TemplateSettingsLayout"; @@ -10,15 +15,13 @@ import { } from "react-router-dom"; import { RequireAuth } from "../components/RequireAuth/RequireAuth"; import { MockUser } from "./entities"; -import { ReactNode } from "react"; +import { ReactNode, useState } from "react"; import { QueryClient } from "react-query"; -export const renderWithRouter = ( - router: ReturnType, -) => { - // Create one query client for each render isolate it avoid other - // tests to be affected - const queryClient = new QueryClient({ +function createTestQueryClient() { + // Helps create one query client for each test case, to make sure that tests + // are isolated and can't affect each other + return new QueryClient({ defaultOptions: { queries: { retry: false, @@ -28,16 +31,19 @@ export const renderWithRouter = ( }, }, }); +} - const rootComponent = ( - - - - ); +export const renderWithRouter = ( + router: ReturnType, +) => { + const queryClient = createTestQueryClient(); return { - ...tlRender(rootComponent), - rootComponent, + ...tlRender( + + + , + ), router, }; }; @@ -56,7 +62,7 @@ export const render = (element: ReactNode) => { ); }; -type RenderWithAuthOptions = { +export type RenderWithAuthOptions = { // The current URL, /workspaces/123 route?: string; // The route path, /workspaces/:workspaceId @@ -98,6 +104,82 @@ export function renderWithAuth( }; } +type RenderHookWithAuthOptions = Partial< + Readonly< + Omit & { + initialProps: Props; + } + > +>; + +/** + * Custom version of renderHook that is aware of all our App providers. + * + * Had to do some nasty, cursed things in the implementation to make sure that + * the tests using this function remained simple. + * + * @see {@link https://github.com/coder/coder/pull/10362#discussion_r1380852725} + */ +export async function renderHookWithAuth( + render: (initialProps: Props) => Result, + { + initialProps, + path = "/", + extraRoutes = [], + }: RenderHookWithAuthOptions = {}, +) { + const queryClient = createTestQueryClient(); + + // Easy to miss – there's an evil definite assignment via the ! + let escapedRouter!: ReturnType; + + const { result, rerender, unmount } = renderHook(render, { + initialProps, + wrapper: ({ children }) => { + /** + * Unfortunately, there isn't a way to define the router outside the + * wrapper while keeping it aware of children, meaning that we need to + * define the router as readonly state in the component instance. This + * ensures the value remains stable across all re-renders + */ + // eslint-disable-next-line react-hooks/rules-of-hooks -- This is actually processed as a component; the linter just isn't aware of that + const [readonlyStatefulRouter] = useState(() => { + return createMemoryRouter([ + { path, element: <>{children} }, + ...extraRoutes, + ]); + }); + + /** + * Leaks the wrapper component's state outside React's render cycles. + */ + escapedRouter = readonlyStatefulRouter; + + return ( + + + + ); + }, + }); + + /** + * This is necessary to get around some providers in AppProviders having + * conditional rendering and not always rendering their children immediately. + * + * The hook result won't actually exist until the children defined via wrapper + * render in full. + */ + await waitFor(() => expect(result.current).not.toBe(null)); + + return { + result, + rerender, + unmount, + router: escapedRouter, + } as const; +} + export function renderWithTemplateSettingsLayout( element: JSX.Element, {