From 5915daa286eb0c571208080f4fc40210775a0513 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Mon, 12 Aug 2024 18:54:54 +0000 Subject: [PATCH 01/10] feat: add template delete notification --- ...242_notifications_delete_template.down.sql | 0 ...00242_notifications_delete_template.up.sql | 22 +++++++ coderd/notifications/events.go | 1 + coderd/templates.go | 31 +++++++++ coderd/templates_test.go | 64 +++++++++++++++++++ coderd/workspaces_test.go | 2 +- 6 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 coderd/database/migrations/000242_notifications_delete_template.down.sql create mode 100644 coderd/database/migrations/000242_notifications_delete_template.up.sql diff --git a/coderd/database/migrations/000242_notifications_delete_template.down.sql b/coderd/database/migrations/000242_notifications_delete_template.down.sql new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/coderd/database/migrations/000242_notifications_delete_template.up.sql b/coderd/database/migrations/000242_notifications_delete_template.up.sql new file mode 100644 index 0000000000000..1dbc985f52566 --- /dev/null +++ b/coderd/database/migrations/000242_notifications_delete_template.up.sql @@ -0,0 +1,22 @@ +INSERT INTO + notification_templates ( + id, + name, + title_template, + body_template, + "group", + actions + ) +VALUES ( + '29a09665-2a4c-403f-9648-54301670e7be', + 'Template Deleted', + E'Template "{{.Labels.name}}" deleted', + E'Hi {{.UserName}}\n\nThe template **{{.Labels.name}}** was deleted by **{{ .Labels.initiator }}**.', + 'Template Events', + '[ + { + "label": "View templates", + "url": "{{ base_url }}/templates" + } + ]'::jsonb + ); diff --git a/coderd/notifications/events.go b/coderd/notifications/events.go index 94260317e20c9..0cc2bbb837a06 100644 --- a/coderd/notifications/events.go +++ b/coderd/notifications/events.go @@ -12,6 +12,7 @@ var ( TemplateWorkspaceDormant = uuid.MustParse("0ea69165-ec14-4314-91f1-69566ac3c5a0") TemplateWorkspaceAutoUpdated = uuid.MustParse("c34a0c09-0704-4cac-bd1c-0c0146811c2b") TemplateWorkspaceMarkedForDeletion = uuid.MustParse("51ce2fdf-c9ca-4be1-8d70-628674f9bc42") + TemplateTemplateDeleted = uuid.MustParse("29a09665-2a4c-403f-9648-54301670e7be") ) // Account-related events. diff --git a/coderd/templates.go b/coderd/templates.go index 93a5943b40193..7cc2269bb7466 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -1,6 +1,7 @@ package coderd import ( + "context" "database/sql" "errors" "fmt" @@ -12,12 +13,15 @@ import ( "github.com/google/uuid" "golang.org/x/xerrors" + "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" + "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/schedule" @@ -56,6 +60,7 @@ func (api *API) template(rw http.ResponseWriter, r *http.Request) { // @Router /templates/{template} [delete] func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) { var ( + apiKey = httpmw.APIKey(r) ctx = r.Context() template = httpmw.TemplateParam(r) auditor = *api.Auditor.Load() @@ -101,11 +106,37 @@ func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) { }) return } + + api.notifyTemplateDeleted(ctx, template, apiKey.UserID) + httpapi.Write(ctx, rw, http.StatusOK, codersdk.Response{ Message: "Template has been deleted!", }) } +func (api *API) notifyTemplateDeleted(ctx context.Context, template database.Template, initiatorID uuid.UUID) { + if template.CreatedBy == initiatorID { + return + } + + initiator, err := api.Database.GetUserByID(ctx, initiatorID) + if err != nil { + api.Logger.Warn(ctx, "failed to fetch initiator for template deletion notification", slog.F("initiator_id", initiatorID), slog.Error(err)) + return + } + + if _, err := api.NotificationsEnqueuer.Enqueue(ctx, template.CreatedBy, notifications.TemplateTemplateDeleted, + map[string]string{ + "name": template.Name, + "initiator": initiator.Username, + }, "api-templates-delete", + // Associate this notification with all the related entities. + template.ID, template.OrganizationID, + ); err != nil { + api.Logger.Warn(ctx, "failed to notify of template deletion", slog.F("deleted_template", template.ID), slog.Error(err)) + } +} + // Create a new template in an organization. // Returns a single template. // diff --git a/coderd/templates_test.go b/coderd/templates_test.go index 4d6073d6ab835..49867834c1655 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -19,6 +19,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "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/schedule" "github.com/coder/coder/v2/coderd/util/ptr" @@ -1326,3 +1327,66 @@ func TestTemplateMetrics(t *testing.T) { dbtime.Now(), res.Workspaces[0].LastUsedAt, time.Minute, ) } + +func TestTemplateNotifications(t *testing.T) { + t.Parallel() + + t.Run("Delete", func(t *testing.T) { + t.Parallel() + + t.Run("InitiatorNotTemplateAdmin", func(t *testing.T) { + t.Parallel() + + var ( + notifyEnq = &testutil.FakeNotificationsEnqueuer{} + client = coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + NotificationsEnqueuer: notifyEnq, + }) + user = coderdtest.CreateFirstUser(t, client) + version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + initiatorClient, _ = coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleOwner()) + ctx = testutil.Context(t, testutil.WaitLong) + ) + + // When: initiator is not the template admin + err := initiatorClient.DeleteTemplate(ctx, template.ID) + require.NoError(t, err) + + // Then: the template admin should receive a notification + require.Len(t, notifyEnq.Sent, 1) + require.Equal(t, notifyEnq.Sent[0].TemplateID, notifications.TemplateTemplateDeleted) + require.Equal(t, notifyEnq.Sent[0].UserID, template.CreatedByID) + require.Contains(t, notifyEnq.Sent[0].Targets, template.ID) + require.Contains(t, notifyEnq.Sent[0].Targets, template.OrganizationID) + require.Contains(t, notifyEnq.Sent[0].Labels["name"], template.Name) + require.Contains(t, notifyEnq.Sent[0].Labels["initiator"], coderdtest.FirstUserParams.Username) + }) + + t.Run("InitiatorIsTemplateAdmin", func(t *testing.T) { + t.Parallel() + + var ( + notifyEnq = &testutil.FakeNotificationsEnqueuer{} + client = coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + NotificationsEnqueuer: notifyEnq, + }) + user = coderdtest.CreateFirstUser(t, client) + version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + ctx = testutil.Context(t, testutil.WaitLong) + ) + + // When: initiator is the template admin + err := client.DeleteTemplate(ctx, template.ID) + require.NoError(t, err) + + // Then: the template admin should not receive a notification + require.Len(t, notifyEnq.Sent, 0) + }) + }) +} diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 2bbbf171eab61..ec7c03dd53013 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -3441,7 +3441,7 @@ func TestWorkspaceUsageTracking(t *testing.T) { }) } -func TestNotifications(t *testing.T) { +func TestWorkspaceNotifications(t *testing.T) { t.Parallel() t.Run("Dormant", func(t *testing.T) { From 1f7046cdd8d5d0ac7c84dea5d77aae2a5dda1a2d Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 13 Aug 2024 14:06:59 +0000 Subject: [PATCH 02/10] Notify all template admins --- coderd/notifications/events.go | 6 ++- coderd/templates.go | 40 ++++++++++++-- coderd/templates_test.go | 97 +++++++++++++++------------------- 3 files changed, 85 insertions(+), 58 deletions(-) diff --git a/coderd/notifications/events.go b/coderd/notifications/events.go index 0cc2bbb837a06..b340b281e0757 100644 --- a/coderd/notifications/events.go +++ b/coderd/notifications/events.go @@ -12,7 +12,6 @@ var ( TemplateWorkspaceDormant = uuid.MustParse("0ea69165-ec14-4314-91f1-69566ac3c5a0") TemplateWorkspaceAutoUpdated = uuid.MustParse("c34a0c09-0704-4cac-bd1c-0c0146811c2b") TemplateWorkspaceMarkedForDeletion = uuid.MustParse("51ce2fdf-c9ca-4be1-8d70-628674f9bc42") - TemplateTemplateDeleted = uuid.MustParse("29a09665-2a4c-403f-9648-54301670e7be") ) // Account-related events. @@ -20,3 +19,8 @@ var ( TemplateUserAccountCreated = uuid.MustParse("4e19c0ac-94e1-4532-9515-d1801aa283b2") TemplateUserAccountDeleted = uuid.MustParse("f44d9314-ad03-4bc8-95d0-5cad491da6b6") ) + +// Template-related events. +var ( + TemplateTemplateDeleted = uuid.MustParse("29a09665-2a4c-403f-9648-54301670e7be") +) diff --git a/coderd/templates.go b/coderd/templates.go index 7cc2269bb7466..50ed835fff524 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -107,15 +107,28 @@ func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) { return } - api.notifyTemplateDeleted(ctx, template, apiKey.UserID) + admins, err := findTemplateAdmins(ctx, api.Database) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching template admins.", + Detail: err.Error(), + }) + return + } + for _, admin := range admins { + if admin.ID == apiKey.UserID { + continue + } + api.notifyTemplateDeleted(ctx, template, apiKey.UserID, admin.ID) + } httpapi.Write(ctx, rw, http.StatusOK, codersdk.Response{ Message: "Template has been deleted!", }) } -func (api *API) notifyTemplateDeleted(ctx context.Context, template database.Template, initiatorID uuid.UUID) { - if template.CreatedBy == initiatorID { +func (api *API) notifyTemplateDeleted(ctx context.Context, template database.Template, initiatorID uuid.UUID, receiverID uuid.UUID) { + if initiatorID == receiverID { return } @@ -125,7 +138,7 @@ func (api *API) notifyTemplateDeleted(ctx context.Context, template database.Tem return } - if _, err := api.NotificationsEnqueuer.Enqueue(ctx, template.CreatedBy, notifications.TemplateTemplateDeleted, + if _, err := api.NotificationsEnqueuer.Enqueue(ctx, receiverID, notifications.TemplateTemplateDeleted, map[string]string{ "name": template.Name, "initiator": initiator.Username, @@ -979,3 +992,22 @@ func (api *API) convertTemplate( MaxPortShareLevel: maxPortShareLevel, } } + +// findTemplateAdmins fetches all users with template admin permission including owners. +func findTemplateAdmins(ctx context.Context, store database.Store) ([]database.GetUsersRow, error) { + // Notice: we can't scrape the user information in parallel as pq + // fails with: unexpected describe rows response: 'D' + owners, err := store.GetUsers(ctx, database.GetUsersParams{ + RbacRole: []string{codersdk.RoleOwner}, + }) + if err != nil { + return nil, xerrors.Errorf("get owners: %w", err) + } + templateAdmins, err := store.GetUsers(ctx, database.GetUsersParams{ + RbacRole: []string{codersdk.RoleTemplateAdmin}, + }) + if err != nil { + return nil, xerrors.Errorf("get template admins: %w", err) + } + return append(owners, templateAdmins...), nil +} diff --git a/coderd/templates_test.go b/coderd/templates_test.go index 49867834c1655..1022e0369cb3e 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -1328,65 +1328,56 @@ func TestTemplateMetrics(t *testing.T) { ) } -func TestTemplateNotifications(t *testing.T) { +func TestNotifyDeletedTemplate(t *testing.T) { t.Parallel() - t.Run("Delete", func(t *testing.T) { + t.Run("OnlyNotifyOwnersAndTemplateAdmins", func(t *testing.T) { t.Parallel() - t.Run("InitiatorNotTemplateAdmin", func(t *testing.T) { - t.Parallel() - - var ( - notifyEnq = &testutil.FakeNotificationsEnqueuer{} - client = coderdtest.New(t, &coderdtest.Options{ - IncludeProvisionerDaemon: true, - NotificationsEnqueuer: notifyEnq, - }) - user = coderdtest.CreateFirstUser(t, client) - version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - template = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - initiatorClient, _ = coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleOwner()) - ctx = testutil.Context(t, testutil.WaitLong) - ) - - // When: initiator is not the template admin - err := initiatorClient.DeleteTemplate(ctx, template.ID) - require.NoError(t, err) - - // Then: the template admin should receive a notification - require.Len(t, notifyEnq.Sent, 1) - require.Equal(t, notifyEnq.Sent[0].TemplateID, notifications.TemplateTemplateDeleted) - require.Equal(t, notifyEnq.Sent[0].UserID, template.CreatedByID) - require.Contains(t, notifyEnq.Sent[0].Targets, template.ID) - require.Contains(t, notifyEnq.Sent[0].Targets, template.OrganizationID) - require.Contains(t, notifyEnq.Sent[0].Labels["name"], template.Name) - require.Contains(t, notifyEnq.Sent[0].Labels["initiator"], coderdtest.FirstUserParams.Username) - }) + var ( + notifyEnq = &testutil.FakeNotificationsEnqueuer{} + client = coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + NotificationsEnqueuer: notifyEnq, + }) + firstUser = coderdtest.CreateFirstUser(t, client) + version = coderdtest.CreateTemplateVersion(t, client, firstUser.OrganizationID, nil) + _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template = coderdtest.CreateTemplate(t, client, firstUser.OrganizationID, version.ID) + ctx = testutil.Context(t, testutil.WaitLong) + ) - t.Run("InitiatorIsTemplateAdmin", func(t *testing.T) { - t.Parallel() + _, anotherOwner := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleOwner()) + _, tmplAdmin := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleTemplateAdmin()) + coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleMember()) - var ( - notifyEnq = &testutil.FakeNotificationsEnqueuer{} - client = coderdtest.New(t, &coderdtest.Options{ - IncludeProvisionerDaemon: true, - NotificationsEnqueuer: notifyEnq, - }) - user = coderdtest.CreateFirstUser(t, client) - version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - template = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - ctx = testutil.Context(t, testutil.WaitLong) - ) - - // When: initiator is the template admin - err := client.DeleteTemplate(ctx, template.ID) - require.NoError(t, err) + // When: template is deleted + err := client.DeleteTemplate(ctx, template.ID) + require.NoError(t, err) - // Then: the template admin should not receive a notification - require.Len(t, notifyEnq.Sent, 0) - }) + // Then: the template owners and template admins should receive the notification + shouldBeNotified := []uuid.UUID{anotherOwner.ID, tmplAdmin.ID} + var deleteTemplateNotifications []*testutil.Notification + for _, n := range notifyEnq.Sent { + if n.TemplateID == notifications.TemplateTemplateDeleted { + deleteTemplateNotifications = append(deleteTemplateNotifications, n) + } + } + require.Len(t, deleteTemplateNotifications, len(shouldBeNotified)) + + for _, userID := range shouldBeNotified { + var notification *testutil.Notification + for _, n := range deleteTemplateNotifications { + if n.UserID == userID { + notification = n + break + } + } + require.NotNil(t, notification) + require.Contains(t, notification.Targets, template.ID) + require.Contains(t, notification.Targets, template.OrganizationID) + require.Equal(t, notification.Labels["name"], template.Name) + require.Equal(t, notification.Labels["initiator"], coderdtest.FirstUserParams.Username) + } }) } From 30faf4fb18f14f9938136979796456b6895f1f9e Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 13 Aug 2024 16:08:43 +0000 Subject: [PATCH 03/10] Apply Dannys suggestions --- coderd/templates.go | 5 +-- coderd/templates_test.go | 84 +++++++++++++++++++++------------------- 2 files changed, 45 insertions(+), 44 deletions(-) diff --git a/coderd/templates.go b/coderd/templates.go index 50ed835fff524..323e017b6e7e4 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -116,6 +116,7 @@ func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) { return } for _, admin := range admins { + // Don't send notification to user which initiated the event. if admin.ID == apiKey.UserID { continue } @@ -128,10 +129,6 @@ func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) { } func (api *API) notifyTemplateDeleted(ctx context.Context, template database.Template, initiatorID uuid.UUID, receiverID uuid.UUID) { - if initiatorID == receiverID { - return - } - initiator, err := api.Database.GetUserByID(ctx, initiatorID) if err != nil { api.Logger.Warn(ctx, "failed to fetch initiator for template deletion notification", slog.F("initiator_id", initiatorID), slog.Error(err)) diff --git a/coderd/templates_test.go b/coderd/templates_test.go index 1022e0369cb3e..5126aaca97cae 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -1328,56 +1328,60 @@ func TestTemplateMetrics(t *testing.T) { ) } -func TestNotifyDeletedTemplate(t *testing.T) { +func TestTemplateNotifications(t *testing.T) { t.Parallel() - t.Run("OnlyNotifyOwnersAndTemplateAdmins", func(t *testing.T) { + t.Run("Delete", func(t *testing.T) { t.Parallel() - var ( - notifyEnq = &testutil.FakeNotificationsEnqueuer{} - client = coderdtest.New(t, &coderdtest.Options{ - IncludeProvisionerDaemon: true, - NotificationsEnqueuer: notifyEnq, - }) - firstUser = coderdtest.CreateFirstUser(t, client) - version = coderdtest.CreateTemplateVersion(t, client, firstUser.OrganizationID, nil) - _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - template = coderdtest.CreateTemplate(t, client, firstUser.OrganizationID, version.ID) - ctx = testutil.Context(t, testutil.WaitLong) - ) + t.Run("OnlyNotifyOwnersAndTemplateAdmins", func(t *testing.T) { + t.Parallel() + + var ( + notifyEnq = &testutil.FakeNotificationsEnqueuer{} + client = coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + NotificationsEnqueuer: notifyEnq, + }) + firstUser = coderdtest.CreateFirstUser(t, client) + version = coderdtest.CreateTemplateVersion(t, client, firstUser.OrganizationID, nil) + _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template = coderdtest.CreateTemplate(t, client, firstUser.OrganizationID, version.ID) + ctx = testutil.Context(t, testutil.WaitLong) + ) - _, anotherOwner := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleOwner()) - _, tmplAdmin := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleTemplateAdmin()) - coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleMember()) + _, anotherOwner := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleOwner()) + _, tmplAdmin := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleTemplateAdmin()) + coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleMember()) + coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleUserAdmin()) - // When: template is deleted - err := client.DeleteTemplate(ctx, template.ID) - require.NoError(t, err) + // When: template is deleted + err := client.DeleteTemplate(ctx, template.ID) + require.NoError(t, err) - // Then: the template owners and template admins should receive the notification - shouldBeNotified := []uuid.UUID{anotherOwner.ID, tmplAdmin.ID} - var deleteTemplateNotifications []*testutil.Notification - for _, n := range notifyEnq.Sent { - if n.TemplateID == notifications.TemplateTemplateDeleted { - deleteTemplateNotifications = append(deleteTemplateNotifications, n) + // Then: the template owners and template admins should receive the + // notification. Since the first user is the initiator, it should not + // receive the notification. + shouldBeNotified := []uuid.UUID{anotherOwner.ID, tmplAdmin.ID} + var deleteTemplateNotifications []*testutil.Notification + for _, n := range notifyEnq.Sent { + if n.TemplateID == notifications.TemplateTemplateDeleted { + deleteTemplateNotifications = append(deleteTemplateNotifications, n) + } } - } - require.Len(t, deleteTemplateNotifications, len(shouldBeNotified)) + notifiedUsers := make([]uuid.UUID, 0, len(deleteTemplateNotifications)) + for _, n := range deleteTemplateNotifications { + notifiedUsers = append(notifiedUsers, n.UserID) + } + require.ElementsMatch(t, shouldBeNotified, notifiedUsers) - for _, userID := range shouldBeNotified { - var notification *testutil.Notification + // Check whether notifications are correctly enqueued for _, n := range deleteTemplateNotifications { - if n.UserID == userID { - notification = n - break - } + require.Contains(t, n.Targets, template.ID) + require.Contains(t, n.Targets, template.OrganizationID) + require.Equal(t, n.Labels["name"], template.Name) + require.Equal(t, n.Labels["initiator"], coderdtest.FirstUserParams.Username) } - require.NotNil(t, notification) - require.Contains(t, notification.Targets, template.ID) - require.Contains(t, notification.Targets, template.OrganizationID) - require.Equal(t, notification.Labels["name"], template.Name) - require.Equal(t, notification.Labels["initiator"], coderdtest.FirstUserParams.Username) - } + }) }) } From 1e11cb7309db2a7108c900d84c8cb720a07e9dd2 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 13 Aug 2024 18:54:56 +0000 Subject: [PATCH 04/10] Fix lint --- coderd/templates.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/templates.go b/coderd/templates.go index 323e017b6e7e4..ade849ba99c9c 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -143,7 +143,7 @@ func (api *API) notifyTemplateDeleted(ctx context.Context, template database.Tem // Associate this notification with all the related entities. template.ID, template.OrganizationID, ); err != nil { - api.Logger.Warn(ctx, "failed to notify of template deletion", slog.F("deleted_template", template.ID), slog.Error(err)) + api.Logger.Warn(ctx, "failed to notify of template deletion", slog.F("deleted_template_id", template.ID), slog.Error(err)) } } From be31ed8e6a6a53dd0a1444caf2794d0ca2a99e5a Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 13 Aug 2024 18:55:35 +0000 Subject: [PATCH 05/10] Fix migration --- ...ate.down.sql => 000244_notifications_delete_template.down.sql} | 0 ...emplate.up.sql => 000244_notifications_delete_template.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000242_notifications_delete_template.down.sql => 000244_notifications_delete_template.down.sql} (100%) rename coderd/database/migrations/{000242_notifications_delete_template.up.sql => 000244_notifications_delete_template.up.sql} (100%) diff --git a/coderd/database/migrations/000242_notifications_delete_template.down.sql b/coderd/database/migrations/000244_notifications_delete_template.down.sql similarity index 100% rename from coderd/database/migrations/000242_notifications_delete_template.down.sql rename to coderd/database/migrations/000244_notifications_delete_template.down.sql diff --git a/coderd/database/migrations/000242_notifications_delete_template.up.sql b/coderd/database/migrations/000244_notifications_delete_template.up.sql similarity index 100% rename from coderd/database/migrations/000242_notifications_delete_template.up.sql rename to coderd/database/migrations/000244_notifications_delete_template.up.sql From 7fb0fa20845b642a9c9d016773c24cb929472f49 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 Aug 2024 13:37:03 +0000 Subject: [PATCH 06/10] Improve tests --- coderd/templates_test.go | 48 +++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/coderd/templates_test.go b/coderd/templates_test.go index 5126aaca97cae..dbeb95b11bb72 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -1334,9 +1334,44 @@ func TestTemplateNotifications(t *testing.T) { t.Run("Delete", func(t *testing.T) { t.Parallel() + t.Run("SendNotification", func(t *testing.T) { + t.Parallel() + + // Given: an owner and a template admin + var ( + notifyEnq = &testutil.FakeNotificationsEnqueuer{} + client = coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + NotificationsEnqueuer: notifyEnq, + }) + owner = coderdtest.CreateFirstUser(t, client) + version = coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil) + _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template = coderdtest.CreateTemplate(t, client, owner.OrganizationID, version.ID) + ctx = testutil.Context(t, testutil.WaitLong) + _, templateAdmin = coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) + ) + + // When: the template is deleted + err := client.DeleteTemplate(ctx, template.ID) + require.NoError(t, err) + + // Then: verify that the notification is sent to the correct user + // (template admin) and targets, using the appropriate labels. Note that + // the owner, being the initiator, will not receive the notification. + require.Len(t, notifyEnq.Sent, 1) + require.Contains(t, notifyEnq.Sent[0].TemplateID, notifications.TemplateTemplateDeleted) + require.Contains(t, notifyEnq.Sent[0].UserID, templateAdmin.ID) + require.Contains(t, notifyEnq.Sent[0].Targets, template.ID) + require.Contains(t, notifyEnq.Sent[0].Targets, template.OrganizationID) + require.Equal(t, notifyEnq.Sent[0].Labels["name"], template.Name) + require.Equal(t, notifyEnq.Sent[0].Labels["initiator"], coderdtest.FirstUserParams.Username) + }) + t.Run("OnlyNotifyOwnersAndTemplateAdmins", func(t *testing.T) { t.Parallel() + // Given: multiple users with different roles var ( notifyEnq = &testutil.FakeNotificationsEnqueuer{} client = coderdtest.New(t, &coderdtest.Options{ @@ -1349,17 +1384,16 @@ func TestTemplateNotifications(t *testing.T) { template = coderdtest.CreateTemplate(t, client, firstUser.OrganizationID, version.ID) ctx = testutil.Context(t, testutil.WaitLong) ) - _, anotherOwner := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleOwner()) _, tmplAdmin := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleTemplateAdmin()) coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleMember()) coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleUserAdmin()) - // When: template is deleted + // When: the template is deleted by the owner err := client.DeleteTemplate(ctx, template.ID) require.NoError(t, err) - // Then: the template owners and template admins should receive the + // Then: only the template owners and template admins should receive the // notification. Since the first user is the initiator, it should not // receive the notification. shouldBeNotified := []uuid.UUID{anotherOwner.ID, tmplAdmin.ID} @@ -1374,14 +1408,6 @@ func TestTemplateNotifications(t *testing.T) { notifiedUsers = append(notifiedUsers, n.UserID) } require.ElementsMatch(t, shouldBeNotified, notifiedUsers) - - // Check whether notifications are correctly enqueued - for _, n := range deleteTemplateNotifications { - require.Contains(t, n.Targets, template.ID) - require.Contains(t, n.Targets, template.OrganizationID) - require.Equal(t, n.Labels["name"], template.Name) - require.Equal(t, n.Labels["initiator"], coderdtest.FirstUserParams.Username) - } }) }) } From 92c11deb5e777557e15577c48b54e5bb57343294 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 Aug 2024 13:47:37 +0000 Subject: [PATCH 07/10] Filter delete notifications --- coderd/templates_test.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/coderd/templates_test.go b/coderd/templates_test.go index dbeb95b11bb72..6d0bfd6bf4d8f 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -1359,13 +1359,19 @@ func TestTemplateNotifications(t *testing.T) { // Then: verify that the notification is sent to the correct user // (template admin) and targets, using the appropriate labels. Note that // the owner, being the initiator, will not receive the notification. - require.Len(t, notifyEnq.Sent, 1) - require.Contains(t, notifyEnq.Sent[0].TemplateID, notifications.TemplateTemplateDeleted) - require.Contains(t, notifyEnq.Sent[0].UserID, templateAdmin.ID) - require.Contains(t, notifyEnq.Sent[0].Targets, template.ID) - require.Contains(t, notifyEnq.Sent[0].Targets, template.OrganizationID) - require.Equal(t, notifyEnq.Sent[0].Labels["name"], template.Name) - require.Equal(t, notifyEnq.Sent[0].Labels["initiator"], coderdtest.FirstUserParams.Username) + deleteNotifications := make([]*testutil.Notification, 0) + for _, n := range notifyEnq.Sent { + if n.TemplateID == notifications.TemplateTemplateDeleted { + deleteNotifications = append(deleteNotifications, n) + } + } + require.Len(t, deleteNotifications, 1) + require.Contains(t, deleteNotifications[0].TemplateID, notifications.TemplateTemplateDeleted) + require.Contains(t, deleteNotifications[0].UserID, templateAdmin.ID) + require.Contains(t, deleteNotifications[0].Targets, template.ID) + require.Contains(t, deleteNotifications[0].Targets, template.OrganizationID) + require.Equal(t, deleteNotifications[0].Labels["name"], template.Name) + require.Equal(t, deleteNotifications[0].Labels["initiator"], coderdtest.FirstUserParams.Username) }) t.Run("OnlyNotifyOwnersAndTemplateAdmins", func(t *testing.T) { From 450235edac4eb981991431713dee40fdd625789d Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 Aug 2024 16:37:25 +0000 Subject: [PATCH 08/10] Improve test --- coderd/templates_test.go | 71 +++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 33 deletions(-) diff --git a/coderd/templates_test.go b/coderd/templates_test.go index 6d0bfd6bf4d8f..caf5a42997cd0 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -1334,47 +1334,38 @@ func TestTemplateNotifications(t *testing.T) { t.Run("Delete", func(t *testing.T) { t.Parallel() - t.Run("SendNotification", func(t *testing.T) { + t.Run("InitiatorIsNotNotified", func(t *testing.T) { t.Parallel() - // Given: an owner and a template admin + // Given: an initiator var ( notifyEnq = &testutil.FakeNotificationsEnqueuer{} client = coderdtest.New(t, &coderdtest.Options{ IncludeProvisionerDaemon: true, NotificationsEnqueuer: notifyEnq, }) - owner = coderdtest.CreateFirstUser(t, client) - version = coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil) - _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - template = coderdtest.CreateTemplate(t, client, owner.OrganizationID, version.ID) - ctx = testutil.Context(t, testutil.WaitLong) - _, templateAdmin = coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) + initiator = coderdtest.CreateFirstUser(t, client) + version = coderdtest.CreateTemplateVersion(t, client, initiator.OrganizationID, nil) + _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template = coderdtest.CreateTemplate(t, client, initiator.OrganizationID, version.ID) + ctx = testutil.Context(t, testutil.WaitLong) ) - // When: the template is deleted + // When: the template is deleted by the initiator err := client.DeleteTemplate(ctx, template.ID) require.NoError(t, err) - // Then: verify that the notification is sent to the correct user - // (template admin) and targets, using the appropriate labels. Note that - // the owner, being the initiator, will not receive the notification. + // Then: the delete notification is not sent to the initiator. deleteNotifications := make([]*testutil.Notification, 0) for _, n := range notifyEnq.Sent { if n.TemplateID == notifications.TemplateTemplateDeleted { deleteNotifications = append(deleteNotifications, n) } } - require.Len(t, deleteNotifications, 1) - require.Contains(t, deleteNotifications[0].TemplateID, notifications.TemplateTemplateDeleted) - require.Contains(t, deleteNotifications[0].UserID, templateAdmin.ID) - require.Contains(t, deleteNotifications[0].Targets, template.ID) - require.Contains(t, deleteNotifications[0].Targets, template.OrganizationID) - require.Equal(t, deleteNotifications[0].Labels["name"], template.Name) - require.Equal(t, deleteNotifications[0].Labels["initiator"], coderdtest.FirstUserParams.Username) + require.Len(t, deleteNotifications, 0) }) - t.Run("OnlyNotifyOwnersAndTemplateAdmins", func(t *testing.T) { + t.Run("OnlyOwnersAndAdminsAreNotified", func(t *testing.T) { t.Parallel() // Given: multiple users with different roles @@ -1384,25 +1375,29 @@ func TestTemplateNotifications(t *testing.T) { IncludeProvisionerDaemon: true, NotificationsEnqueuer: notifyEnq, }) - firstUser = coderdtest.CreateFirstUser(t, client) - version = coderdtest.CreateTemplateVersion(t, client, firstUser.OrganizationID, nil) - _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - template = coderdtest.CreateTemplate(t, client, firstUser.OrganizationID, version.ID) + initiator = coderdtest.CreateFirstUser(t, client) ctx = testutil.Context(t, testutil.WaitLong) + + // Setup template + version = coderdtest.CreateTemplateVersion(t, client, initiator.OrganizationID, nil) + _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template = coderdtest.CreateTemplate(t, client, initiator.OrganizationID, version.ID) ) - _, anotherOwner := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleOwner()) - _, tmplAdmin := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleTemplateAdmin()) - coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleMember()) - coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleUserAdmin()) - // When: the template is deleted by the owner + // Setup users with different roles + _, owner := coderdtest.CreateAnotherUser(t, client, initiator.OrganizationID, rbac.RoleOwner()) + _, tmplAdmin := coderdtest.CreateAnotherUser(t, client, initiator.OrganizationID, rbac.RoleTemplateAdmin()) + coderdtest.CreateAnotherUser(t, client, initiator.OrganizationID, rbac.RoleMember()) + coderdtest.CreateAnotherUser(t, client, initiator.OrganizationID, rbac.RoleUserAdmin()) + coderdtest.CreateAnotherUser(t, client, initiator.OrganizationID, rbac.RoleAuditor()) + + // When: the template is deleted by the initiator err := client.DeleteTemplate(ctx, template.ID) require.NoError(t, err) - // Then: only the template owners and template admins should receive the - // notification. Since the first user is the initiator, it should not - // receive the notification. - shouldBeNotified := []uuid.UUID{anotherOwner.ID, tmplAdmin.ID} + // Then: only owners and template admins should receive the + // notification. + shouldBeNotified := []uuid.UUID{owner.ID, tmplAdmin.ID} var deleteTemplateNotifications []*testutil.Notification for _, n := range notifyEnq.Sent { if n.TemplateID == notifications.TemplateTemplateDeleted { @@ -1414,6 +1409,16 @@ func TestTemplateNotifications(t *testing.T) { notifiedUsers = append(notifiedUsers, n.UserID) } require.ElementsMatch(t, shouldBeNotified, notifiedUsers) + + // Validate the notification content + for _, n := range deleteTemplateNotifications { + require.Contains(t, n.TemplateID, notifications.TemplateTemplateDeleted) + require.Contains(t, notifiedUsers, n.UserID) + require.Contains(t, n.Targets, template.ID) + require.Contains(t, n.Targets, template.OrganizationID) + require.Equal(t, n.Labels["name"], template.Name) + require.Equal(t, n.Labels["initiator"], coderdtest.FirstUserParams.Username) + } }) }) } From eaf87c556c8829bebac96ad785b507ab6de339a6 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 Aug 2024 16:51:25 +0000 Subject: [PATCH 09/10] Fix test --- coderd/templates_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/templates_test.go b/coderd/templates_test.go index caf5a42997cd0..5efd127681bbd 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -1412,7 +1412,7 @@ func TestTemplateNotifications(t *testing.T) { // Validate the notification content for _, n := range deleteTemplateNotifications { - require.Contains(t, n.TemplateID, notifications.TemplateTemplateDeleted) + require.Equal(t, n.TemplateID, notifications.TemplateTemplateDeleted) require.Contains(t, notifiedUsers, n.UserID) require.Contains(t, n.Targets, template.ID) require.Contains(t, n.Targets, template.OrganizationID) From 4688765f4ac776e25cebd03c8382d461128bdb06 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 Aug 2024 17:13:41 +0000 Subject: [PATCH 10/10] Add missing can render test --- coderd/notifications/notifications_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 077018b32581c..e21dce4246f20 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -740,6 +740,17 @@ func TestNotificationTemplatesCanRender(t *testing.T) { }, }, }, + { + name: "TemplateTemplateDeleted", + id: notifications.TemplateTemplateDeleted, + payload: types.MessagePayload{ + UserName: "bobby", + Labels: map[string]string{ + "name": "bobby-template", + "initiator": "rob", + }, + }, + }, } allTemplates, err := enumerateAllTemplates(t)