fix(site): fix parameters' request upon template variables update (#11898)

Fix https://github.com/coder/coder/issues/11870

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),
   };
 };

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<ProvisionerTagsPopoverProps> = ({
                     return key !== "owner";
                   })
                   .map((k) => (
-                    <>
+                    <Fragment key={k}>
                       {k === "scope" ? (
-                        <ProvisionerTag key={k} k={k} v={tags[k]} />
+                        <ProvisionerTag k={k} v={tags[k]} />
                       ) : (
-                        <ProvisionerTag
-                          key={k}
-                          k={k}
-                          v={tags[k]}
-                          onDelete={onDelete}
-                        />
+                        <ProvisionerTag k={k} v={tags[k]} onDelete={onDelete} />
                       )}
-                    </>
+                    </Fragment>
                   ))}
               </Stack>
 

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,
       },
 

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",
+    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 () => {
+      jest.resetAllMocks();
+      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(
+        <AppProviders queryClient={queryClient}>
+          <RouterProvider
+            router={createMemoryRouter(
+              [
+                {
+                  element: <RequireAuth />,
+                  children: [
+                    {
+                      element: <TemplateVersionEditorPage />,
+                      path: "/templates/:template/versions/:version/edit",
+                    },
+                  ],
+                },
+              ],
+              {
+                initialEntries: [
+                  `/templates/${MockTemplate.name}/versions/${MockTemplateVersion.name}/edit`,
+                ],
+              },
+            )}
+          />
+        </AppProviders>,
+      );
+      await waitForLoaderToBeRemoved();
+
+      const dialogSelector = /template variables/i;
+      if (askForVariables) {
+        await screen.findByText(dialogSelector);
+      } else {
+        expect(screen.queryByText(dialogSelector)).not.toBeInTheDocument();
+      }
+    });
+  },
+);

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,
   };

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));
   }),