From ca48b8783b6c4a1fa4c929fa074a98d210cd67cb Mon Sep 17 00:00:00 2001
From: Steven Masley <Emyrk@users.noreply.github.com>
Date: Fri, 19 Jan 2024 12:54:25 -0600
Subject: [PATCH] fix: update template with noop returned undefined template
 (#11688)

* fix: doing a noop patch to templates resulted in 404

The patch response did not include the template. The UI required the
template to be returned to form the new page path

null is more explicit, and harder to make occur by mistake.
---
 enterprise/coderd/templates_test.go           | 38 +++++++++++++++++++
 site/src/api/api.ts                           |  7 +++-
 .../TemplateSettingsPage.tsx                  | 17 +++++++--
 3 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go
index b340f90ece..ca70113744 100644
--- a/enterprise/coderd/templates_test.go
+++ b/enterprise/coderd/templates_test.go
@@ -687,6 +687,44 @@ func TestTemplates(t *testing.T) {
 		require.Empty(t, template.DeprecationMessage)
 		require.False(t, template.Deprecated)
 	})
+
+	// Create a template, remove the group, see if an owner can
+	// still fetch the template.
+	t.Run("GetOnEveryoneRemove", func(t *testing.T) {
+		t.Parallel()
+		owner, first := coderdenttest.New(t, &coderdenttest.Options{
+			Options: &coderdtest.Options{
+				IncludeProvisionerDaemon: true,
+				TemplateScheduleStore:    schedule.NewEnterpriseTemplateScheduleStore(agplUserQuietHoursScheduleStore()),
+			},
+			LicenseOptions: &coderdenttest.LicenseOptions{
+				Features: license.Features{
+					codersdk.FeatureAccessControl: 1,
+					codersdk.FeatureTemplateRBAC:  1,
+				},
+			},
+		})
+
+		client, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleTemplateAdmin())
+		version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil)
+		template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID)
+
+		ctx := testutil.Context(t, testutil.WaitMedium)
+		err := client.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{
+			UserPerms: nil,
+			GroupPerms: map[string]codersdk.TemplateRole{
+				// OrgID is the everyone ID
+				first.OrganizationID.String(): codersdk.TemplateRoleDeleted,
+			},
+		})
+		require.NoError(t, err)
+
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		_, err = owner.Template(ctx, template.ID)
+		require.NoError(t, err)
+	})
 }
 
 func TestTemplateACL(t *testing.T) {
diff --git a/site/src/api/api.ts b/site/src/api/api.ts
index 0f13aa4249..6814ad1b62 100644
--- a/site/src/api/api.ts
+++ b/site/src/api/api.ts
@@ -414,11 +414,16 @@ export const unarchiveTemplateVersion = async (templateVersionId: string) => {
 export const updateTemplateMeta = async (
   templateId: string,
   data: TypesGen.UpdateTemplateMeta,
-): Promise<TypesGen.Template> => {
+): Promise<TypesGen.Template | null> => {
   const response = await axios.patch<TypesGen.Template>(
     `/api/v2/templates/${templateId}`,
     data,
   );
+  // On 304 response there is no data payload.
+  if (response.status === 304) {
+    return null;
+  }
+
   return response.data;
 };
 
diff --git a/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsPage.tsx b/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsPage.tsx
index b9fd383f63..e2c3d03c9f 100644
--- a/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsPage.tsx
+++ b/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsPage.tsx
@@ -29,10 +29,19 @@ export const TemplateSettingsPage: FC = () => {
     (data: UpdateTemplateMeta) => updateTemplateMeta(template.id, data),
     {
       onSuccess: async (data) => {
-        // we use data.name because an admin may have updated templateName to something new
-        await queryClient.invalidateQueries(
-          templateByNameKey(orgId, data.name),
-        );
+        // This update has a chance to return a 304 which means nothing was updated.
+        // In this case, the return payload will be empty and we should use the
+        // original template data.
+        if (!data) {
+          data = template;
+        } else {
+          // Only invalid the query if data is returned, indicating at least one field was updated.
+          //
+          // we use data.name because an admin may have updated templateName to something new
+          await queryClient.invalidateQueries(
+            templateByNameKey(orgId, data.name),
+          );
+        }
         displaySuccess("Template updated successfully");
         navigate(`/templates/${data.name}`);
       },