Apply Dannys review comments

coder · BrunoQuaresma · Jul 24, 2024 · Jul 10, 2024 · Jul 10, 2024 · Jul 11, 2024
commit 4d71c946c8ba5c8e5e41d8508b0dad9a78cb45cc
diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go
@@ -299,7 +299,7 @@ func (e *Executor) runOnce(t time.Time) Stats {
 						nextBuildReason = string(nextBuild.Reason)
 					}
 
-					if _, err := e.notificationsEnqueuer.Enqueue(e.ctx, ws.OwnerID, notifications.WorkspaceAutoUpdated,
+					if _, err := e.notificationsEnqueuer.Enqueue(e.ctx, ws.OwnerID, notifications.TemplateWorkspaceAutoUpdated,
 						map[string]string{
 							"name":                  ws.Name,
 							"initiator":             "autobuild",
@@ -334,8 +334,6 @@ func (e *Executor) runOnce(t time.Time) Stats {
 					if err != nil {
 						log.Warn(e.ctx, "failed to notify of workspace marked as dormant", slog.Error(err), slog.F("workspace_id", dormantNotification.Workspace.ID))
 					}
-				} else {
-					log.Warn(e.ctx, "no dormant notification to send", slog.F("workspace_id", wsID))
 				}
 				return nil
 			}()

diff --git a/coderd/autobuild/lifecycle_executor_test.go b/coderd/autobuild/lifecycle_executor_test.go
@@ -18,6 +18,7 @@ import (
 	"github.com/coder/coder/v2/coderd/coderdtest"
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/dbauthz"
+	"github.com/coder/coder/v2/coderd/notifications"
 	"github.com/coder/coder/v2/coderd/schedule"
 	"github.com/coder/coder/v2/coderd/schedule/cron"
 	"github.com/coder/coder/v2/coderd/util/ptr"
@@ -1107,15 +1108,16 @@ func TestNotifications(t *testing.T) {
 
 		// Wait for workspace to become dormant
 		ticker <- workspace.LastUsedAt.Add(timeTilDormant * 3)
-		<-statCh
+		_ = testutil.RequireRecvCtx(testutil.Context(t, testutil.WaitShort), t, statCh)
 
 		// Check that the workspace is dormant
 		workspace = coderdtest.MustWorkspace(t, client, workspace.ID)
-		require.NotNil(t, workspace.DormantAt)
+		require.Equal(t, workspace.DormantAt, workspace.LastUsedAt.Add(timeTilDormant))
 
 		// Check that a notification was enqueued
 		require.Len(t, notifyEnq.Sent, 1)
 		require.Equal(t, notifyEnq.Sent[0].UserID, workspace.OwnerID)
+		require.Equal(t, notifyEnq.Sent[0].TemplateID, notifications.TemplateWorkspaceDormant)
 		require.Contains(t, notifyEnq.Sent[0].Targets, template.ID)
 		require.Contains(t, notifyEnq.Sent[0].Targets, workspace.ID)
 		require.Contains(t, notifyEnq.Sent[0].Targets, workspace.OrganizationID)

diff --git a/coderd/database/migrations/000229_dormancy_notification_template.down.sql b/coderd/database/migrations/000229_dormancy_notification_template.down.sql
@@ -1 +1,7 @@
-DELETE FROM notification_templates WHERE id = '0ea69165-ec14-4314-91f1-69566ac3c5a0';
+DELETE FROM notification_templates
+WHERE
+    id = '0ea69165-ec14-4314-91f1-69566ac3c5a0';
+
+DELETE FROM notification_templates
+WHERE
+    id = '51ce2fdf-c9ca-4be1-8d70-628674f9bc42';
diff --git a/coderd/database/migrations/000229_dormancy_notification_template.up.sql b/coderd/database/migrations/000229_dormancy_notification_template.up.sql
@@ -18,5 +18,18 @@ VALUES (
 			"label": "View workspace",
 			"url": "{{ base_url }}/@{{.UserName}}/{{.Labels.name}}"
 		}
+    ]'::jsonb,
+    ),
+    (
+        '51ce2fdf-c9ca-4be1-8d70-628674f9bc42',
+        'Workspace Marked for Deletion',
+        E'Workspace "{{.Labels.name}}" marked for deletion',
+        E'Hi {{.UserName}}\n\n' || E'Your workspace **{{.Labels.name}}** has been marked for **deletion** after {{.Labels.dormancyHours}} hours of dormancy.\n' || E'The specified reason was "**{{.Labels.reason}}{{end}}**\n\n' || E'Dormancy refers to a workspace being unused for a defined length of time, and after it exceeds {{.Labels.dormancyHours}} hours of dormancy it will be deleted.\n' || E'To prevent your workspace from being deleted, simply use it as normal.',
+        'Workspace Events',
+        '[
+        {
+			"label": "View workspace",
+			"url": "{{ base_url }}/@{{.UserName}}/{{.Labels.name}}"
+		}
     ]'::jsonb
     );
diff --git a/coderd/database/migrations/000230_marked_for_deletion_notification_template.down.sql b/coderd/database/migrations/000230_marked_for_deletion_notification_template.down.sql
diff --git a/coderd/database/migrations/000230_marked_for_deletion_notification_template.up sql b/coderd/database/migrations/000230_marked_for_deletion_notification_template.up sql
diff --git a/coderd/dormancy/notifications.go b/coderd/dormancy/notifications.go
@@ -57,14 +57,13 @@ func NotifyWorkspaceMarkedForDeletion(
 	notification WorkspaceMarkedForDeletionNotification,
 ) (id *uuid.UUID, err error) {
 	labels := map[string]string{
-		"name":      notification.Workspace.Name,
-		"initiator": "autobuild",
-		"reason":    notification.Reason,
+		"name":   notification.Workspace.Name,
+		"reason": notification.Reason,
 	}
 	return enqueuer.Enqueue(
 		ctx,
 		notification.Workspace.OwnerID,
-		notifications.TemplateWorkspaceDormant,
+		notifications.TemplateWorkspaceMarkedForDeletion,
 		labels,
 		notification.CreatedBy,
 		// Associate this notification with all the related entities.

diff --git a/coderd/notifications/events.go b/coderd/notifications/events.go
@@ -8,8 +8,8 @@ import "github.com/google/uuid"
 // Workspace-related events.
 var (
 	TemplateWorkspaceDeleted           = uuid.MustParse("f517da0b-cdc9-410f-ab89-a86107c420ed")
-	WorkspaceAutobuildFailed           = uuid.MustParse("381df2a9-c0c0-4749-420f-80a9280c66f9")
+	TemplateWorkspaceAutobuildFailed   = uuid.MustParse("381df2a9-c0c0-4749-420f-80a9280c66f9")
 	TemplateWorkspaceDormant           = uuid.MustParse("0ea69165-ec14-4314-91f1-69566ac3c5a0")
-	WorkspaceAutoUpdated               = uuid.MustParse("c34a0c09-0704-4cac-bd1c-0c0146811c2b")
+	TemplateWorkspaceAutoUpdated       = uuid.MustParse("c34a0c09-0704-4cac-bd1c-0c0146811c2b")
 	TemplateWorkspaceMarkedForDeletion = uuid.MustParse("51ce2fdf-c9ca-4be1-8d70-628674f9bc42")
 )
diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go
@@ -1103,7 +1103,7 @@ func (s *server) notifyWorkspaceBuildFailed(ctx context.Context, workspace datab
 	reason = string(build.Reason)
 	initiator := "autobuild"
 
-	if _, err := s.NotificationsEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.WorkspaceAutobuildFailed,
+	if _, err := s.NotificationsEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.TemplateWorkspaceAutobuildFailed,
 		map[string]string{
 			"name":      workspace.Name,
 			"initiator": initiator,

diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go
@@ -30,6 +30,7 @@ import (
 	"github.com/coder/coder/v2/coderd/database/dbgen"
 	"github.com/coder/coder/v2/coderd/database/dbtestutil"
 	"github.com/coder/coder/v2/coderd/database/dbtime"
+	"github.com/coder/coder/v2/coderd/notifications"
 	"github.com/coder/coder/v2/coderd/rbac"
 	"github.com/coder/coder/v2/coderd/rbac/policy"
 	"github.com/coder/coder/v2/coderd/render"
@@ -3541,6 +3542,7 @@ func TestNotifications(t *testing.T) {
 			// Then
 			require.NoError(t, err, "mark workspace as dormant")
 			require.Len(t, notifyEnq.Sent, 1)
+			require.Equal(t, notifyEnq.Sent[0].TemplateID, notifications.TemplateWorkspaceDormant)
 			require.Equal(t, notifyEnq.Sent[0].UserID, workspace.OwnerID)
 			require.Contains(t, notifyEnq.Sent[0].Targets, template.ID)
 			require.Contains(t, notifyEnq.Sent[0].Targets, workspace.ID)

diff --git a/enterprise/coderd/schedule/template.go b/enterprise/coderd/schedule/template.go
@@ -216,7 +216,7 @@ func (s *EnterpriseTemplateScheduleStore) Set(ctx context.Context, db database.S
 			},
 		)
 		if err != nil {
-			s.logger.Warn(ctx, "failed to notify of workspace marked for deletion", slog.Error(err))
+			s.logger.Warn(ctx, "failed to notify of workspace marked for deletion", slog.Error(err), slog.F("workspace_id", workspace.ID))
 		}
 	}
 

diff --git a/enterprise/coderd/schedule/template_test.go b/enterprise/coderd/schedule/template_test.go
@@ -683,15 +683,24 @@ func TestNotifications(t *testing.T) {
 		templateScheduleStore := schedule.NewEnterpriseTemplateScheduleStore(userQuietHoursStorePtr, &notifyEnq, logger)
 		templateScheduleStore.TimeNowFn = time.Now
 
-		// Update dormancy TTL for a lower value
+		// Lower the dormancy TTL to ensure the schedule recalculates deadlines and
+		// triggers notifications.
 		_, err = templateScheduleStore.Set(ctx, db, template, agplschedule.TemplateScheduleOptions{
 			TimeTilDormant:           timeTilDormant / 2,
 			TimeTilDormantAutoDelete: timeTilDormant / 2,
 		})
 		require.NoError(t, err)
 
-		// We should expect two notifications. One for each dormant workspace.
+		// We should expect a notification for each dormant workspace.
 		require.Len(t, notifyEnq.Sent, len(dormantWorkspaces))
+		for i, dormantWs := range dormantWorkspaces {
+			require.Equal(t, notifyEnq.Sent[i].UserID, dormantWs.OwnerID)
+			require.Equal(t, notifyEnq.Sent[i].TemplateID, notifications.TemplateWorkspaceMarkedForDeletion)
+			require.Contains(t, notifyEnq.Sent[i].Targets, template.ID)
+			require.Contains(t, notifyEnq.Sent[i].Targets, dormantWs.ID)
+			require.Contains(t, notifyEnq.Sent[i].Targets, dormantWs.OrganizationID)
+			require.Contains(t, notifyEnq.Sent[i].Targets, dormantWs.OwnerID)
+		}
 	})
 }
-Original file line number
+Diff line change
@@ Expand Up @@
     			},
     		)
     		if err != nil {
-    			s.logger.Warn(ctx, "failed to notify of workspace marked for deletion", slog.Error(err))
+    			s.logger.Warn(ctx, "failed to notify of workspace marked for deletion", slog.Error(err), slog.F("workspace_id", workspace.ID))
     		}
     	}
@@ Expand Down @@