fix: update template with noop returned undefined template (#11688)

Emyrk · web-flow · commit ca48b8783b6c · 2024-01-19T18:54:25.000Z
* 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.
diff --git 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
@@ -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
@@ -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}`);
       },
