From 257eeda9667118391c0a6f7de028d34da53c8e27 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Mon, 29 Jan 2024 17:29:02 +0000 Subject: [PATCH 1/4] fix(site): fix parameters getting asked on template variables update --- site/src/api/queries/templates.ts | 8 +- .../ProvisionerTagsPopover.tsx | 15 +-- .../TemplateVersionEditor.tsx | 4 +- .../TemplateVersionEditorPage.test.tsx | 124 +++++++++++++++++- .../TemplateVersionEditorPage.tsx | 12 +- site/src/testHelpers/renderHelpers.tsx | 9 +- 6 files changed, 150 insertions(+), 22 deletions(-) diff --git a/site/src/api/queries/templates.ts b/site/src/api/queries/templates.ts index 3be881f0d5b03..9eddafc7800fe 100644 --- a/site/src/api/queries/templates.ts +++ b/site/src/api/queries/templates.ts @@ -127,9 +127,15 @@ export const templateVersions = (templateId: string) => { }; }; +export const templateVersionVariablesKey = (versionId: string) => [ + "templateVersion", + versionId, + "variables", +]; + export const templateVersionVariables = (versionId: string) => { return { - queryKey: ["templateVersion", versionId, "variables"], + queryKey: templateVersionVariablesKey(versionId), queryFn: () => API.getTemplateVersionVariables(versionId), }; }; diff --git a/site/src/pages/TemplateVersionEditorPage/ProvisionerTagsPopover.tsx b/site/src/pages/TemplateVersionEditorPage/ProvisionerTagsPopover.tsx index 9d65021dc6b77..07c5f7df0796d 100644 --- a/site/src/pages/TemplateVersionEditorPage/ProvisionerTagsPopover.tsx +++ b/site/src/pages/TemplateVersionEditorPage/ProvisionerTagsPopover.tsx @@ -6,7 +6,7 @@ import { PopoverTrigger, } from "components/Popover/Popover"; import { ProvisionerTag } from "pages/HealthPage/ProvisionerDaemonsPage"; -import { type FC } from "react"; +import { Fragment, type FC } from "react"; import useTheme from "@mui/system/useTheme"; import { useFormik } from "formik"; import * as Yup from "yup"; @@ -111,18 +111,13 @@ export const ProvisionerTagsPopover: FC = ({ return key !== "owner"; }) .map((k) => ( - <> + {k === "scope" ? ( - + ) : ( - + )} - + ))} diff --git a/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditor.tsx b/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditor.tsx index 995c4267ca586..96d9f8c0d20b0 100644 --- a/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditor.tsx +++ b/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditor.tsx @@ -689,7 +689,7 @@ const styles = { padding: "0 16px", fontFamily: MONOSPACE_FONT_FAMILY, - "&:first-child": { + "&:first-of-type": { paddingTop: 16, }, @@ -715,7 +715,7 @@ const styles = { borderLeft: 0, borderRight: 0, - "&:first-child": { + "&:first-of-type": { borderTop: 0, }, diff --git a/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx b/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx index 67ec4c92b1e21..feb7fd7ee04a4 100644 --- a/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx +++ b/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx @@ -1,14 +1,26 @@ -import { renderWithAuth } from "testHelpers/renderHelpers"; +import { + renderWithAuth, + waitForLoaderToBeRemoved, +} from "testHelpers/renderHelpers"; import TemplateVersionEditorPage from "./TemplateVersionEditorPage"; -import { screen, waitFor, within } from "@testing-library/react"; +import { render, screen, waitFor, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import * as api from "api/api"; import { MockTemplate, MockTemplateVersion, + MockTemplateVersionVariable1, + MockTemplateVersionVariable2, MockWorkspaceBuildLogs, } from "testHelpers/entities"; import { Language } from "./PublishTemplateVersionDialog"; +import { QueryClient } from "react-query"; +import { templateVersionVariablesKey } from "api/queries/templates"; +import { RouterProvider, createMemoryRouter } from "react-router-dom"; +import { RequireAuth } from "contexts/auth/RequireAuth"; +import { server } from "testHelpers/server"; +import { rest } from "msw"; +import { AppProviders } from "App"; // For some reason this component in Jest is throwing a MUI style warning so, // since we don't need it for this test, we can mock it out @@ -208,3 +220,111 @@ test("Patch request is not send when there are no changes", async () => { ); expect(patchTemplateVersion).toBeCalledTimes(0); }); + +describe.each([ + { + testName: + "Do not ask when template version has no errors and no initial variables", + initialVariables: undefined, + loadedVariables: undefined, + templateVersion: MockTemplateVersion, + askForVariables: false, + }, + { + testName: + "Do not ask when template version has no errors even when having previously loaded variables", + initialVariables: [ + MockTemplateVersionVariable1, + MockTemplateVersionVariable2, + ], + loadedVariables: undefined, + templateVersion: MockTemplateVersion, + askForVariables: false, + }, + { + testName: "Ask when template version has errors", + initialVariables: undefined, + templateVersion: { + ...MockTemplateVersion, + job: { + ...MockTemplateVersion.job, + error_code: "REQUIRED_TEMPLATE_VARIABLES", + }, + }, + loadedVariables: [ + MockTemplateVersionVariable1, + MockTemplateVersionVariable2, + ], + askForVariables: true, + }, +])( + "Missing template variables", + ({ + testName, + initialVariables, + loadedVariables, + templateVersion, + askForVariables, + }) => { + it(testName, async () => { + const queryClient = new QueryClient(); + queryClient.setQueryData( + templateVersionVariablesKey(MockTemplateVersion.id), + initialVariables, + ); + + server.use( + rest.get( + "/api/v2/organizations/:org/templates/:template/versions/:version", + (req, res, ctx) => { + return res(ctx.json(templateVersion)); + }, + ), + ); + + if (loadedVariables) { + server.use( + rest.get( + "/api/v2/templateversions/:version/variables", + (req, res, ctx) => { + return res(ctx.json(loadedVariables)); + }, + ), + ); + } + + render( + + , + children: [ + { + element: , + path: "/templates/:template/versions/:version/edit", + }, + ], + }, + ], + { + initialEntries: [ + `/templates/${MockTemplate.name}/versions/${MockTemplateVersion.name}/edit`, + ], + }, + )} + /> + , + ); + await waitForLoaderToBeRemoved(); + + const dialogSelector = /template variables/i; + if (askForVariables) { + await screen.findByText(dialogSelector); + } else { + expect(screen.queryByText(dialogSelector)).not.toBeInTheDocument(); + } + }); + }, +); diff --git a/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.tsx b/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.tsx index 742a10316d378..143002096ddc7 100644 --- a/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.tsx +++ b/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.tsx @@ -302,21 +302,23 @@ const useVersionLogs = ( }; const useMissingVariables = (templateVersion: TemplateVersion | undefined) => { - const { data: missingVariables } = useQuery({ + const isRequiringVariables = + templateVersion?.job.error_code === "REQUIRED_TEMPLATE_VARIABLES"; + const { data: variables } = useQuery({ ...templateVersionVariables(templateVersion?.id ?? ""), - enabled: templateVersion?.job.error_code === "REQUIRED_TEMPLATE_VARIABLES", + enabled: isRequiringVariables, }); const [isMissingVariablesDialogOpen, setIsMissingVariablesDialogOpen] = useState(false); useEffect(() => { - if (missingVariables) { + if (isRequiringVariables) { setIsMissingVariablesDialogOpen(true); } - }, [missingVariables]); + }, [isRequiringVariables]); return { - missingVariables, + missingVariables: isRequiringVariables ? variables : undefined, isMissingVariablesDialogOpen, setIsMissingVariablesDialogOpen, }; diff --git a/site/src/testHelpers/renderHelpers.tsx b/site/src/testHelpers/renderHelpers.tsx index 3a58557acaf00..19e6415767b97 100644 --- a/site/src/testHelpers/renderHelpers.tsx +++ b/site/src/testHelpers/renderHelpers.tsx @@ -5,7 +5,7 @@ import { renderHook, } from "@testing-library/react"; import { type ReactNode, useState } from "react"; -import { QueryClient } from "react-query"; +import { QueryClient, QueryClientConfig } from "react-query"; import { AppProviders } from "App"; import { RequireAuth } from "contexts/auth/RequireAuth"; import { ThemeProvider } from "contexts/ThemeProvider"; @@ -19,16 +19,21 @@ import { } from "react-router-dom"; import { MockUser } from "./entities"; -function createTestQueryClient() { +export function createTestQueryClient( + overrideConfig?: Partial, +) { // 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({ + ...overrideConfig, defaultOptions: { + ...overrideConfig?.defaultOptions, queries: { retry: false, cacheTime: 0, refetchOnWindowFocus: false, networkMode: "offlineFirst", + ...overrideConfig?.defaultOptions?.queries, }, }, }); From 5b2b84b24cac4e7d8df08b90eb4a8bc57af1269c Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Mon, 29 Jan 2024 17:32:49 +0000 Subject: [PATCH 2/4] Rollback custom test query client config --- site/src/testHelpers/renderHelpers.tsx | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/site/src/testHelpers/renderHelpers.tsx b/site/src/testHelpers/renderHelpers.tsx index 19e6415767b97..3a58557acaf00 100644 --- a/site/src/testHelpers/renderHelpers.tsx +++ b/site/src/testHelpers/renderHelpers.tsx @@ -5,7 +5,7 @@ import { renderHook, } from "@testing-library/react"; import { type ReactNode, useState } from "react"; -import { QueryClient, QueryClientConfig } from "react-query"; +import { QueryClient } from "react-query"; import { AppProviders } from "App"; import { RequireAuth } from "contexts/auth/RequireAuth"; import { ThemeProvider } from "contexts/ThemeProvider"; @@ -19,21 +19,16 @@ import { } from "react-router-dom"; import { MockUser } from "./entities"; -export function createTestQueryClient( - overrideConfig?: Partial, -) { +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({ - ...overrideConfig, defaultOptions: { - ...overrideConfig?.defaultOptions, queries: { retry: false, cacheTime: 0, refetchOnWindowFocus: false, networkMode: "offlineFirst", - ...overrideConfig?.defaultOptions?.queries, }, }, }); From bf2575171f8215a50b9b5cfb9f9eedeef1a02645 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Mon, 29 Jan 2024 17:34:18 +0000 Subject: [PATCH 3/4] Add better name --- .../TemplateVersionEditorPage.test.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx b/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx index feb7fd7ee04a4..795b3a612a391 100644 --- a/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx +++ b/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx @@ -223,8 +223,7 @@ test("Patch request is not send when there are no changes", async () => { describe.each([ { - testName: - "Do not ask when template version has no errors and no initial variables", + testName: "Do not ask when template version has no errors", initialVariables: undefined, loadedVariables: undefined, templateVersion: MockTemplateVersion, From b486ff1ceba2e568ecd300a385b5ef89ae4e4a43 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Mon, 29 Jan 2024 19:34:15 +0000 Subject: [PATCH 4/4] Fix tests --- .../TemplateVersionEditorPage.test.tsx | 1 + site/src/testHelpers/handlers.ts | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx b/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx index 795b3a612a391..5a94169f7b2d2 100644 --- a/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx +++ b/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.test.tsx @@ -266,6 +266,7 @@ describe.each([ askForVariables, }) => { it(testName, async () => { + jest.resetAllMocks(); const queryClient = new QueryClient(); queryClient.setQueryData( templateVersionVariablesKey(MockTemplateVersion.id), diff --git a/site/src/testHelpers/handlers.ts b/site/src/testHelpers/handlers.ts index 416f10ef8acdc..61f33d5cc1ac2 100644 --- a/site/src/testHelpers/handlers.ts +++ b/site/src/testHelpers/handlers.ts @@ -82,6 +82,12 @@ export const handlers = [ ctx.json([M.MockTemplateVersion2, M.MockTemplateVersion]), ); }), + rest.patch( + "/api/v2/templates/:templateId/versions", + async (req, res, ctx) => { + return res(ctx.status(200), ctx.json({})); + }, + ), rest.patch("/api/v2/templates/:templateId", async (req, res, ctx) => { return res(ctx.status(200), ctx.json(M.MockTemplate)); }),