fix: fix template edit overriding with flag defaults (#11564)

coder · Emyrk · Jan 11, 2024 · Jan 11, 2024 · Jan 11, 2024 · Jan 11, 2024
commit a074f2832dd8c051316fe97e67be4a825ce8f3b8
diff --git a/cli/templateedit.go b/cli/templateedit.go
@@ -87,48 +87,86 @@ func (r *RootCmd) templateEdit() *clibase.Cmd {
 				return xerrors.Errorf("get workspace template: %w", err)
 			}
 
-			// Copy the default value if the list is empty, or if the user
-			// specified the "none" value clear the list.
-			if len(autostopRequirementDaysOfWeek) == 0 {
-				autostopRequirementDaysOfWeek = template.AutostopRequirement.DaysOfWeek
+			// Default values
+			if !userSetOption(inv, "description") {
+				description = template.Description
 			}
-			if len(autostartRequirementDaysOfWeek) == 1 && autostartRequirementDaysOfWeek[0] == "all" {
-				// Set it to every day of the week
-				autostartRequirementDaysOfWeek = []string{"monday", "tuesday", "wednesday", "thursday", "friday", "saturday", "sunday"}
-			} else if len(autostartRequirementDaysOfWeek) == 0 {
-				autostartRequirementDaysOfWeek = template.AutostartRequirement.DaysOfWeek
+
+			if !userSetOption(inv, "icon") {
+				icon = template.Icon
 			}
-			if unsetAutostopRequirementDaysOfWeek {
-				autostopRequirementDaysOfWeek = []string{}
+
+			if !userSetOption(inv, "display-name") {
+				displayName = template.DisplayName
 			}
-			if failureTTL == 0 {
+
+			if !userSetOption(inv, "max-ttl") {
+				maxTTL = time.Duration(template.MaxTTLMillis) * time.Millisecond
+			}
+
+			if !userSetOption(inv, "default-ttl") {
+				defaultTTL = time.Duration(template.DefaultTTLMillis) * time.Millisecond
+			}
+
+			if !userSetOption(inv, "allow-user-autostop") {
+				allowUserAutostop = template.AllowUserAutostop
+			}
+
+			if !userSetOption(inv, "allow-user-autostart") {
+				allowUserAutostart = template.AllowUserAutostart
+			}
+
+			if !userSetOption(inv, "allow-user-cancel-workspace-jobs") {
+				allowUserCancelWorkspaceJobs = template.AllowUserCancelWorkspaceJobs
+			}
+
+			if !userSetOption(inv, "failure-ttl") {
 				failureTTL = time.Duration(template.FailureTTLMillis) * time.Millisecond
 			}
-			if dormancyThreshold == 0 {
+
+			if !userSetOption(inv, "dormancy-threshold") {
 				dormancyThreshold = time.Duration(template.TimeTilDormantMillis) * time.Millisecond
 			}
-			if dormancyAutoDeletion == 0 {
+
+			if !userSetOption(inv, "dormancy-auto-deletion") {
 				dormancyAutoDeletion = time.Duration(template.TimeTilDormantAutoDeleteMillis) * time.Millisecond
 			}
 
-			// Default values
-			if !userSetOption(inv, "description") {
-				description = template.Description
+			if !userSetOption(inv, "require-active-version") {
+				requireActiveVersion = template.RequireActiveVersion
 			}
 
-			if !userSetOption(inv, "icon") {
-				icon = template.Icon
+			if !userSetOption(inv, "autostop-requirement-weekdays") {
+				autostopRequirementDaysOfWeek = template.AutostopRequirement.DaysOfWeek
 			}
 
-			if !userSetOption(inv, "display-name") {
-				displayName = template.DisplayName
+			if unsetAutostopRequirementDaysOfWeek {
+				autostopRequirementDaysOfWeek = []string{}
+			}
+
+			if !userSetOption(inv, "autostop-requirement-weeks") {
+				autostopRequirementWeeks = template.AutostopRequirement.Weeks
+			}
+
+			if len(autostartRequirementDaysOfWeek) == 1 && autostartRequirementDaysOfWeek[0] == "all" {
+				// Set it to every day of the week
+				autostartRequirementDaysOfWeek = []string{"monday", "tuesday", "wednesday", "thursday", "friday", "saturday", "sunday"}
+			} else if !userSetOption(inv, "autostart-requirement-weekdays") {
+				autostartRequirementDaysOfWeek = template.AutostartRequirement.DaysOfWeek
+			} else if len(autostartRequirementDaysOfWeek) == 0 {
+				autostartRequirementDaysOfWeek = []string{}
 			}
 
 			var deprecated *string
-			if !userSetOption(inv, "deprecated") {
+			if userSetOption(inv, "deprecated") {
 				deprecated = &deprecationMessage
 			}
 
+			var disableEveryoneGroup bool
+			if userSetOption(inv, "private") {
+				disableEveryoneGroup = disableEveryone
+			}
+
 			req := codersdk.UpdateTemplateMeta{
 				Name:             name,
 				DisplayName:      displayName,
@@ -151,7 +189,7 @@ func (r *RootCmd) templateEdit() *clibase.Cmd {
 				AllowUserAutostop:              allowUserAutostop,
 				RequireActiveVersion:           requireActiveVersion,
 				DeprecationMessage:             deprecated,
-				DisableEveryoneGroupAccess:     disableEveryone,
+				DisableEveryoneGroupAccess:     disableEveryoneGroup,
 			}
 
 			_, err = client.UpdateTemplateMeta(inv.Context(), template.ID, req)

diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go
@@ -72,6 +72,9 @@ func Template(t testing.TB, db database.Store, seed database.Template) database.
 			seed.OrganizationID.String(): []rbac.Action{rbac.ActionRead},
 		}
 	}
+	if seed.UserACL == nil {
+		seed.UserACL = database.TemplateACL{}
+	}
 	err := db.InsertTemplate(genCtx, database.InsertTemplateParams{
 		ID:                           id,
 		CreatedAt:                    takeFirst(seed.CreatedAt, dbtime.Now()),

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -6374,6 +6374,7 @@ func (q *FakeQuerier) UpdateTemplateMetaByID(_ context.Context, arg database.Upd
 		tpl.Description = arg.Description
 		tpl.Icon = arg.Icon
 		tpl.GroupACL = arg.GroupACL
+		tpl.AllowUserCancelWorkspaceJobs = arg.AllowUserCancelWorkspaceJobs
 		q.templates[idx] = tpl
 		return nil
 	}

diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go
@@ -325,7 +325,7 @@ func (c *DeviceAuth) AuthorizeDevice(ctx context.Context) (*codersdk.ExternalAut
 		// return a better error.
 		switch resp.StatusCode {
 		case http.StatusTooManyRequests:
-			return nil, fmt.Errorf("rate limit hit, unable to authorize device. please try again later")
+			return nil, xerrors.New("rate limit hit, unable to authorize device. please try again later")
 		default:
 			return nil, err
 		}

diff --git a/coderd/promoauth/github.go b/coderd/promoauth/github.go
@@ -1,10 +1,11 @@
 package promoauth
 
 import (
-	"fmt"
 	"net/http"
 	"strconv"
 	"time"
+
+	"golang.org/x/xerrors"
 )
 
 type rateLimits struct {
@@ -81,7 +82,7 @@ func (p *headerParser) string(key string) string {
 
 	v := p.header.Get(key)
 	if v == "" {
-		p.errors[key] = fmt.Errorf("missing header %q", key)
+		p.errors[key] = xerrors.Errorf("missing header %q", key)
 	}
 	return v
 }

diff --git a/coderd/templates.go b/coderd/templates.go
@@ -669,7 +669,7 @@ func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) {
 
 		groupACL := template.GroupACL
 		if req.DisableEveryoneGroupAccess {
-			groupACL = database.TemplateACL{}
+			delete(groupACL, template.OrganizationID.String())
 		}
 
 		var err error

diff --git a/enterprise/cli/templateedit_test.go b/enterprise/cli/templateedit_test.go
@@ -4,11 +4,15 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/coder/coder/v2/cli/clitest"
 	"github.com/coder/coder/v2/coderd/coderdtest"
+	"github.com/coder/coder/v2/coderd/database"
+	"github.com/coder/coder/v2/coderd/database/dbfake"
 	"github.com/coder/coder/v2/coderd/rbac"
+	"github.com/coder/coder/v2/coderd/util/ptr"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/enterprise/coderd/coderdenttest"
 	"github.com/coder/coder/v2/enterprise/coderd/license"
@@ -105,7 +109,6 @@ func TestTemplateEdit(t *testing.T) {
 		_ = coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID)
 		template := coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID)
 		require.False(t, template.RequireActiveVersion)
-
 		const (
 			expectedFailureTTL           = time.Hour * 3
 			expectedDormancyThreshold    = time.Hour * 4
@@ -150,4 +153,168 @@ func TestTemplateEdit(t *testing.T) {
 		require.Equal(t, expectedDormancyThreshold.Milliseconds(), template.TimeTilDormantMillis)
 		require.Equal(t, expectedDormancyAutoDeletion.Milliseconds(), template.TimeTilDormantAutoDeleteMillis)
 	})
+
+	// Test that omitting a flag does not update a template with the
+	// default for a flag.
+	t.Run("DefaultsDontOverride", func(t *testing.T) {
+		t.Parallel()
+
+		ctx := testutil.Context(t, testutil.WaitMedium)
+		ownerClient, db, owner := coderdenttest.NewWithDatabase(t, &coderdenttest.Options{
+			LicenseOptions: &coderdenttest.LicenseOptions{
+				Features: license.Features{
+					codersdk.FeatureAdvancedTemplateScheduling: 1,
+					codersdk.FeatureAccessControl:              1,
+					codersdk.FeatureTemplateRBAC:               1,
+				},
+			},
+		})
+
+		dbtemplate := dbfake.TemplateVersion(t, db).Seed(database.TemplateVersion{
+			CreatedBy:      owner.UserID,
+			OrganizationID: owner.OrganizationID,
+		}).Do().Template
+
+		var (
+			expectedName                 = "template"
+			expectedDisplayName          = "template_display"
+			expectedDescription          = "My description"
+			expectedIcon                 = "icon.pjg"
+			expectedDefaultTTLMillis     = time.Hour.Milliseconds()
+			expectedMaxTTLMillis         = (time.Hour * 24).Milliseconds()
+			expectedAllowAutostart       = false
+			expectedAllowAutostop        = false
+			expectedFailureTTLMillis     = time.Minute.Milliseconds()
+			expectedDormancyMillis       = 2 * time.Minute.Milliseconds()
+			expectedAutoDeleteMillis     = 3 * time.Minute.Milliseconds()
+			expectedRequireActiveVersion = true
+			expectedAllowCancelJobs      = false
+			deprecationMessage           = "Deprecate me"
+			expectedDisableEveryone      = true
+			expectedAutostartDaysOfWeek  = []string{}
+			expectedAutoStopDaysOfWeek   = []string{}
+			expectedAutoStopWeeks        = 1
+		)
+
+		assertFieldsFn := func(t *testing.T, tpl codersdk.Template, acl codersdk.TemplateACL) {
+			t.Helper()
+
+			assert.Equal(t, expectedName, tpl.Name)
+			assert.Equal(t, expectedDisplayName, tpl.DisplayName)
+			assert.Equal(t, expectedDescription, tpl.Description)
+			assert.Equal(t, expectedIcon, tpl.Icon)
+			assert.Equal(t, expectedDefaultTTLMillis, tpl.DefaultTTLMillis)
+			assert.Equal(t, expectedMaxTTLMillis, tpl.MaxTTLMillis)
+			assert.Equal(t, expectedAllowAutostart, tpl.AllowUserAutostart)
+			assert.Equal(t, expectedAllowAutostop, tpl.AllowUserAutostop)
+			assert.Equal(t, expectedFailureTTLMillis, tpl.FailureTTLMillis)
+			assert.Equal(t, expectedDormancyMillis, tpl.TimeTilDormantMillis)
+			assert.Equal(t, expectedAutoDeleteMillis, tpl.TimeTilDormantAutoDeleteMillis)
+			assert.Equal(t, expectedRequireActiveVersion, tpl.RequireActiveVersion)
+			assert.Equal(t, deprecationMessage, tpl.DeprecationMessage)
+			assert.Equal(t, expectedAllowCancelJobs, tpl.AllowUserCancelWorkspaceJobs)
+			assert.Equal(t, len(acl.Groups) == 0, expectedDisableEveryone)
+			assert.Equal(t, expectedAutostartDaysOfWeek, tpl.AutostartRequirement.DaysOfWeek)
+			assert.Equal(t, expectedAutoStopDaysOfWeek, tpl.AutostopRequirement.DaysOfWeek)
+			assert.Equal(t, int64(expectedAutoStopWeeks), tpl.AutostopRequirement.Weeks)
+		}
+
+		template, err := ownerClient.UpdateTemplateMeta(ctx, dbtemplate.ID, codersdk.UpdateTemplateMeta{
+			Name:                           expectedName,
+			DisplayName:                    expectedDisplayName,
+			Description:                    expectedDescription,
+			Icon:                           expectedIcon,
+			DefaultTTLMillis:               expectedDefaultTTLMillis,
+			MaxTTLMillis:                   expectedMaxTTLMillis,
+			AllowUserAutostop:              expectedAllowAutostop,
+			AllowUserAutostart:             expectedAllowAutostart,
+			FailureTTLMillis:               expectedFailureTTLMillis,
+			TimeTilDormantMillis:           expectedDormancyMillis,
+			TimeTilDormantAutoDeleteMillis: expectedAutoDeleteMillis,
+			RequireActiveVersion:           expectedRequireActiveVersion,
+			DeprecationMessage:             ptr.Ref(deprecationMessage),
+			DisableEveryoneGroupAccess:     expectedDisableEveryone,
+			AllowUserCancelWorkspaceJobs:   expectedAllowCancelJobs,
+			AutostartRequirement: &codersdk.TemplateAutostartRequirement{
+				DaysOfWeek: expectedAutostartDaysOfWeek,
+			},
+		})
+		require.NoError(t, err)
+
+		templateACL, err := ownerClient.TemplateACL(ctx, template.ID)
+		require.NoError(t, err)
+
+		assertFieldsFn(t, template, templateACL)
+
+		expectedName = "newName"
+		inv, conf := newCLI(t, "templates",
+			"edit", template.Name,
+			"--name=newName",
+			"-y",
+		)
+
+		clitest.SetupConfig(t, ownerClient, conf)
+
+		err = inv.Run()
+		require.NoError(t, err)
+
+		template, err = ownerClient.Template(ctx, template.ID)
+		require.NoError(t, err)
+		templateACL, err = ownerClient.TemplateACL(ctx, template.ID)
+		require.NoError(t, err)
+		assertFieldsFn(t, template, templateACL)
+
+		expectedAutostartDaysOfWeek = []string{"monday", "wednesday", "friday"}
+		expectedAutoStopDaysOfWeek = []string{"tuesday", "thursday"}
+		expectedAutoStopWeeks = 2
+		expectedMaxTTLMillis = 0
+
+		template, err = ownerClient.UpdateTemplateMeta(ctx, dbtemplate.ID, codersdk.UpdateTemplateMeta{
+			Name:                           expectedName,
+			DisplayName:                    expectedDisplayName,
+			Description:                    expectedDescription,
+			Icon:                           expectedIcon,
+			DefaultTTLMillis:               expectedDefaultTTLMillis,
+			AllowUserAutostop:              expectedAllowAutostop,
+			AllowUserAutostart:             expectedAllowAutostart,
+			FailureTTLMillis:               expectedFailureTTLMillis,
+			TimeTilDormantMillis:           expectedDormancyMillis,
+			TimeTilDormantAutoDeleteMillis: expectedAutoDeleteMillis,
+			RequireActiveVersion:           expectedRequireActiveVersion,
+			DeprecationMessage:             ptr.Ref(deprecationMessage),
+			DisableEveryoneGroupAccess:     expectedDisableEveryone,
+			AllowUserCancelWorkspaceJobs:   expectedAllowCancelJobs,
+			AutostartRequirement: &codersdk.TemplateAutostartRequirement{
+				DaysOfWeek: expectedAutostartDaysOfWeek,
+			},
+
+			AutostopRequirement: &codersdk.TemplateAutostopRequirement{
+				DaysOfWeek: expectedAutoStopDaysOfWeek,
+				Weeks:      int64(expectedAutoStopWeeks),
+			},
+		})
+		require.NoError(t, err)
+		assertFieldsFn(t, template, templateACL)
+
+		// Rerun the update so we can assert that autostop days aren't
+		// mucked with.
+		expectedName = "newName2"
+		inv, conf = newCLI(t, "templates",
+			"edit", template.Name,
+			"--name=newName2",
+			"-y",
+		)
+
+		clitest.SetupConfig(t, ownerClient, conf)
+
+		err = inv.Run()
+		require.NoError(t, err)
+
+		template, err = ownerClient.Template(ctx, template.ID)
+		require.NoError(t, err)
+
+		templateACL, err = ownerClient.TemplateACL(ctx, template.ID)
+		require.NoError(t, err)
+		assertFieldsFn(t, template, templateACL)
+	})
 }