From 61111c91fa21444d2ba8cbbc76ec010476e5bb08 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 10:07:49 +0000 Subject: [PATCH 01/21] add new notification method --- coderd/database/dump.sql | 3 ++- coderd/database/models.go | 5 ++++- coderd/database/queries.sql.go | 3 +++ coderd/database/queries/notifications.sql | 1 + 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index c35a30ae2d866..f75d03f28f091 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -113,7 +113,8 @@ CREATE TYPE notification_message_status AS ENUM ( CREATE TYPE notification_method AS ENUM ( 'smtp', - 'webhook' + 'webhook', + 'inbox' ); CREATE TYPE notification_template_kind AS ENUM ( diff --git a/coderd/database/models.go b/coderd/database/models.go index 3e0f59e6e9391..07d5838b793ae 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -878,6 +878,7 @@ type NotificationMethod string const ( NotificationMethodSmtp NotificationMethod = "smtp" NotificationMethodWebhook NotificationMethod = "webhook" + NotificationMethodInbox NotificationMethod = "inbox" ) func (e *NotificationMethod) Scan(src interface{}) error { @@ -918,7 +919,8 @@ func (ns NullNotificationMethod) Value() (driver.Value, error) { func (e NotificationMethod) Valid() bool { switch e { case NotificationMethodSmtp, - NotificationMethodWebhook: + NotificationMethodWebhook, + NotificationMethodInbox: return true } return false @@ -928,6 +930,7 @@ func AllNotificationMethodValues() []NotificationMethod { return []NotificationMethod{ NotificationMethodSmtp, NotificationMethodWebhook, + NotificationMethodInbox, } } diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 0891bc8c9fcc6..7bc58c60fb3cf 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3809,6 +3809,7 @@ SELECT nm.method, nm.attempt_count::int AS attempt_count, nm.queued_seconds::float AS queued_seconds, + nm.targets, -- template nt.id AS template_id, nt.title_template, @@ -3834,6 +3835,7 @@ type AcquireNotificationMessagesRow struct { Method NotificationMethod `db:"method" json:"method"` AttemptCount int32 `db:"attempt_count" json:"attempt_count"` QueuedSeconds float64 `db:"queued_seconds" json:"queued_seconds"` + Targets []uuid.UUID `db:"targets" json:"targets"` TemplateID uuid.UUID `db:"template_id" json:"template_id"` TitleTemplate string `db:"title_template" json:"title_template"` BodyTemplate string `db:"body_template" json:"body_template"` @@ -3870,6 +3872,7 @@ func (q *sqlQuerier) AcquireNotificationMessages(ctx context.Context, arg Acquir &i.Method, &i.AttemptCount, &i.QueuedSeconds, + pq.Array(&i.Targets), &i.TemplateID, &i.TitleTemplate, &i.BodyTemplate, diff --git a/coderd/database/queries/notifications.sql b/coderd/database/queries/notifications.sql index f2d1a14c3aae7..411f9fc70fb07 100644 --- a/coderd/database/queries/notifications.sql +++ b/coderd/database/queries/notifications.sql @@ -84,6 +84,7 @@ SELECT nm.method, nm.attempt_count::int AS attempt_count, nm.queued_seconds::float AS queued_seconds, + nm.targets, -- template nt.id AS template_id, nt.title_template, From 10276a5ed73e0715d6f9aaccff56a073b0e7ef4b Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 10:24:36 +0000 Subject: [PATCH 02/21] add missing migration file --- .../migrations/000298_notifications_method_inbox.down.sql | 3 +++ .../migrations/000298_notifications_method_inbox.up.sql | 1 + 2 files changed, 4 insertions(+) create mode 100644 coderd/database/migrations/000298_notifications_method_inbox.down.sql create mode 100644 coderd/database/migrations/000298_notifications_method_inbox.up.sql diff --git a/coderd/database/migrations/000298_notifications_method_inbox.down.sql b/coderd/database/migrations/000298_notifications_method_inbox.down.sql new file mode 100644 index 0000000000000..d2138f05c5c3a --- /dev/null +++ b/coderd/database/migrations/000298_notifications_method_inbox.down.sql @@ -0,0 +1,3 @@ +-- The migration is about an enum value change +-- As we can not remove a value from an enum, we can let the down migration empty +-- In order to avoid any failure, we use ADD VALUE IF NOT EXISTS to add the value diff --git a/coderd/database/migrations/000298_notifications_method_inbox.up.sql b/coderd/database/migrations/000298_notifications_method_inbox.up.sql new file mode 100644 index 0000000000000..40eec69d0cf95 --- /dev/null +++ b/coderd/database/migrations/000298_notifications_method_inbox.up.sql @@ -0,0 +1 @@ +ALTER TYPE notification_method ADD VALUE IF NOT EXISTS 'inbox'; From 95738133a37c6e263e3d6e39bdcacb96a9db2bde Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 10:37:39 +0000 Subject: [PATCH 03/21] add coder inbox delivery target --- coderd/notifications/dispatch/inbox.go | 81 ++++++++++++++++++++++++++ coderd/notifications/manager.go | 5 +- coderd/notifications/spec.go | 2 + coderd/notifications/types/payload.go | 3 + 4 files changed, 89 insertions(+), 2 deletions(-) create mode 100644 coderd/notifications/dispatch/inbox.go diff --git a/coderd/notifications/dispatch/inbox.go b/coderd/notifications/dispatch/inbox.go new file mode 100644 index 0000000000000..fc92c57d45d19 --- /dev/null +++ b/coderd/notifications/dispatch/inbox.go @@ -0,0 +1,81 @@ +package dispatch + +import ( + "context" + "encoding/json" + "text/template" + + "golang.org/x/xerrors" + + "cdr.dev/slog" + + "github.com/google/uuid" + + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/notifications/types" + markdown "github.com/coder/coder/v2/coderd/render" +) + +type inboxStore interface { + InsertInboxNotification(ctx context.Context, arg database.InsertInboxNotificationParams) (database.InboxNotification, error) +} + +// InboxHandler is responsible for dispatching notification messages in the Coder Inbox. +type InboxHandler struct { + log slog.Logger + store inboxStore +} + +func NewInboxHandler(log slog.Logger, store inboxStore) *InboxHandler { + return &InboxHandler{log: log, store: store} +} + +func (s *InboxHandler) Dispatcher(payload types.MessagePayload, titleTmpl, bodyTmpl string, _ template.FuncMap) (DeliveryFunc, error) { + subject, err := markdown.PlaintextFromMarkdown(titleTmpl) + if err != nil { + return nil, xerrors.Errorf("render subject: %w", err) + } + + htmlBody, err := markdown.PlaintextFromMarkdown(bodyTmpl) + if err != nil { + return nil, xerrors.Errorf("render html body: %w", err) + } + + return s.dispatch(payload, subject, htmlBody), nil +} + +func (s *InboxHandler) dispatch(payload types.MessagePayload, title, body string) DeliveryFunc { + return func(ctx context.Context, msgID uuid.UUID) (bool, error) { + userID, err := uuid.Parse(payload.UserID) + if err != nil { + return false, xerrors.Errorf("parse user ID: %w", err) + } + templateID, err := uuid.Parse(payload.NotificationTemplateID) + if err != nil { + return false, xerrors.Errorf("parse template ID: %w", err) + } + + actions, err := json.Marshal(payload.Actions) + if err != nil { + return false, xerrors.Errorf("marshal actions: %w", err) + } + + _, err = s.store.InsertInboxNotification(ctx, database.InsertInboxNotificationParams{ + ID: msgID, + UserID: userID, + TemplateID: templateID, + Targets: payload.Targets, + Title: title, + Content: body, + Icon: "https://cdn.coder.com/icons/coder-icon-512x512.png", + Actions: actions, + CreatedAt: dbtime.Now(), + }) + if err != nil { + return false, xerrors.Errorf("insert inbox notification: %w", err) + } + + return false, nil + } +} diff --git a/coderd/notifications/manager.go b/coderd/notifications/manager.go index ff516bfe5d2ec..02b4893981abf 100644 --- a/coderd/notifications/manager.go +++ b/coderd/notifications/manager.go @@ -109,7 +109,7 @@ func NewManager(cfg codersdk.NotificationsConfig, store Store, helpers template. stop: make(chan any), done: make(chan any), - handlers: defaultHandlers(cfg, log), + handlers: defaultHandlers(cfg, log, store), helpers: helpers, clock: quartz.NewReal(), @@ -121,10 +121,11 @@ func NewManager(cfg codersdk.NotificationsConfig, store Store, helpers template. } // defaultHandlers builds a set of known handlers; panics if any error occurs as these handlers should be valid at compile time. -func defaultHandlers(cfg codersdk.NotificationsConfig, log slog.Logger) map[database.NotificationMethod]Handler { +func defaultHandlers(cfg codersdk.NotificationsConfig, log slog.Logger, store Store) map[database.NotificationMethod]Handler { return map[database.NotificationMethod]Handler{ database.NotificationMethodSmtp: dispatch.NewSMTPHandler(cfg.SMTP, log.Named("dispatcher.smtp")), database.NotificationMethodWebhook: dispatch.NewWebhookHandler(cfg.Webhook, log.Named("dispatcher.webhook")), + database.NotificationMethodInbox: dispatch.NewInboxHandler(log.Named("dispatcher.inbox"), store), } } diff --git a/coderd/notifications/spec.go b/coderd/notifications/spec.go index 7ac40b6cae8b8..7d03c744d2aa8 100644 --- a/coderd/notifications/spec.go +++ b/coderd/notifications/spec.go @@ -25,6 +25,8 @@ type Store interface { GetNotificationsSettings(ctx context.Context) (string, error) GetApplicationName(ctx context.Context) (string, error) GetLogoURL(ctx context.Context) (string, error) + + InsertInboxNotification(ctx context.Context, arg database.InsertInboxNotificationParams) (database.InboxNotification, error) } // Handler is responsible for preparing and delivering a notification by a given method. diff --git a/coderd/notifications/types/payload.go b/coderd/notifications/types/payload.go index dbd21c29be517..a50aaa96c6c02 100644 --- a/coderd/notifications/types/payload.go +++ b/coderd/notifications/types/payload.go @@ -1,5 +1,7 @@ package types +import "github.com/google/uuid" + // MessagePayload describes the JSON payload to be stored alongside the notification message, which specifies all of its // metadata, labels, and routing information. // @@ -18,4 +20,5 @@ type MessagePayload struct { Actions []TemplateAction `json:"actions"` Labels map[string]string `json:"labels"` Data map[string]any `json:"data"` + Targets []uuid.UUID `json:"targets"` } From 4485b591466dab29d71ba5f5e0f696b9995cfd85 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 10:53:39 +0000 Subject: [PATCH 04/21] update golden files with targets --- .../webhook/TemplateTemplateDeleted.json.golden | 3 ++- .../webhook/TemplateTemplateDeprecated.json.golden | 3 ++- .../webhook/TemplateTestNotification.json.golden | 3 ++- .../webhook/TemplateUserAccountActivated.json.golden | 3 ++- .../webhook/TemplateUserAccountCreated.json.golden | 3 ++- .../webhook/TemplateUserAccountDeleted.json.golden | 3 ++- .../webhook/TemplateUserAccountSuspended.json.golden | 3 ++- .../webhook/TemplateUserRequestedOneTimePasscode.json.golden | 3 ++- .../webhook/TemplateWorkspaceAutoUpdated.json.golden | 3 ++- .../webhook/TemplateWorkspaceAutobuildFailed.json.golden | 3 ++- .../webhook/TemplateWorkspaceBuildsFailedReport.json.golden | 3 ++- .../webhook/TemplateWorkspaceCreated.json.golden | 3 ++- .../webhook/TemplateWorkspaceDeleted.json.golden | 3 ++- .../TemplateWorkspaceDeleted_CustomAppearance.json.golden | 3 ++- .../webhook/TemplateWorkspaceDormant.json.golden | 3 ++- .../webhook/TemplateWorkspaceManualBuildFailed.json.golden | 3 ++- .../webhook/TemplateWorkspaceManuallyUpdated.json.golden | 3 ++- .../webhook/TemplateWorkspaceMarkedForDeletion.json.golden | 3 ++- .../webhook/TemplateWorkspaceOutOfDisk.json.golden | 3 ++- .../TemplateWorkspaceOutOfDisk_MultipleVolumes.json.golden | 3 ++- .../webhook/TemplateWorkspaceOutOfMemory.json.golden | 3 ++- .../webhook/TemplateYourAccountActivated.json.golden | 3 ++- .../webhook/TemplateYourAccountSuspended.json.golden | 3 ++- 23 files changed, 46 insertions(+), 23 deletions(-) diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeleted.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeleted.json.golden index 4390a3ddfb84b..d4d7b5cbf46ce 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeleted.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeleted.json.golden @@ -19,7 +19,8 @@ "initiator": "rob", "name": "Bobby's Template" }, - "data": null + "data": null, + "targets": null }, "title": "Template \"Bobby's Template\" deleted", "title_markdown": "Template \"Bobby's Template\" deleted", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeprecated.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeprecated.json.golden index c4202271c5257..053cec2c56370 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeprecated.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeprecated.json.golden @@ -24,7 +24,8 @@ "organization": "coder", "template": "alpha" }, - "data": null + "data": null, + "targets": null }, "title": "Template 'alpha' has been deprecated", "title_markdown": "Template 'alpha' has been deprecated", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateTestNotification.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateTestNotification.json.golden index a941faff134c2..e2c5744adb64b 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateTestNotification.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateTestNotification.json.golden @@ -16,7 +16,8 @@ } ], "labels": {}, - "data": null + "data": null, + "targets": null }, "title": "A test notification", "title_markdown": "A test notification", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountActivated.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountActivated.json.golden index 96bfdf14ecbe1..fc777758ef17d 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountActivated.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountActivated.json.golden @@ -20,7 +20,8 @@ "activated_account_user_name": "William Tables", "initiator": "rob" }, - "data": null + "data": null, + "targets": null }, "title": "User account \"bobby\" activated", "title_markdown": "User account \"bobby\" activated", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountCreated.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountCreated.json.golden index 272a5628a20a7..6408398b55a93 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountCreated.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountCreated.json.golden @@ -20,7 +20,8 @@ "created_account_user_name": "William Tables", "initiator": "rob" }, - "data": null + "data": null, + "targets": null }, "title": "User account \"bobby\" created", "title_markdown": "User account \"bobby\" created", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountDeleted.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountDeleted.json.golden index 10b7ddbca6853..71260e8e8ba8e 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountDeleted.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountDeleted.json.golden @@ -20,7 +20,8 @@ "deleted_account_user_name": "William Tables", "initiator": "rob" }, - "data": null + "data": null, + "targets": null }, "title": "User account \"bobby\" deleted", "title_markdown": "User account \"bobby\" deleted", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountSuspended.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountSuspended.json.golden index bd1dec7608974..7d5afe2642f5b 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountSuspended.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserAccountSuspended.json.golden @@ -20,7 +20,8 @@ "suspended_account_name": "bobby", "suspended_account_user_name": "William Tables" }, - "data": null + "data": null, + "targets": null }, "title": "User account \"bobby\" suspended", "title_markdown": "User account \"bobby\" suspended", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserRequestedOneTimePasscode.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserRequestedOneTimePasscode.json.golden index e5f2da431f112..0d22706cd2d85 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserRequestedOneTimePasscode.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateUserRequestedOneTimePasscode.json.golden @@ -18,7 +18,8 @@ "labels": { "one_time_passcode": "00000000-0000-0000-0000-000000000000" }, - "data": null + "data": null, + "targets": null }, "title": "Reset your password for Coder", "title_markdown": "Reset your password for Coder", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceAutoUpdated.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceAutoUpdated.json.golden index 917904a2495aa..a6f566448efd8 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceAutoUpdated.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceAutoUpdated.json.golden @@ -20,7 +20,8 @@ "template_version_message": "template now includes catnip", "template_version_name": "1.0" }, - "data": null + "data": null, + "targets": null }, "title": "Workspace \"bobby-workspace\" updated automatically", "title_markdown": "Workspace \"bobby-workspace\" updated automatically", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceAutobuildFailed.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceAutobuildFailed.json.golden index 45b64a31a0adb..2d4c8da409f4f 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceAutobuildFailed.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceAutobuildFailed.json.golden @@ -19,7 +19,8 @@ "name": "bobby-workspace", "reason": "autostart" }, - "data": null + "data": null, + "targets": null }, "title": "Workspace \"bobby-workspace\" autobuild failed", "title_markdown": "Workspace \"bobby-workspace\" autobuild failed", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceBuildsFailedReport.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceBuildsFailedReport.json.golden index c6dabbfb89d80..bacf59837fdbf 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceBuildsFailedReport.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceBuildsFailedReport.json.golden @@ -57,7 +57,8 @@ } ], "total_builds": 55 - } + }, + "targets": null }, "title": "Workspace builds failed for template \"Bobby First Template\"", "title_markdown": "Workspace builds failed for template \"Bobby First Template\"", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceCreated.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceCreated.json.golden index 924f299b228b2..baa032fee5bae 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceCreated.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceCreated.json.golden @@ -20,7 +20,8 @@ "version": "alpha", "workspace": "bobby-workspace" }, - "data": null + "data": null, + "targets": null }, "title": "Workspace 'bobby-workspace' has been created", "title_markdown": "Workspace 'bobby-workspace' has been created", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDeleted.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDeleted.json.golden index 171e893dd943f..0ef7a16ae1789 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDeleted.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDeleted.json.golden @@ -24,7 +24,8 @@ "name": "bobby-workspace", "reason": "autodeleted due to dormancy" }, - "data": null + "data": null, + "targets": null }, "title": "Workspace \"bobby-workspace\" deleted", "title_markdown": "Workspace \"bobby-workspace\" deleted", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDeleted_CustomAppearance.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDeleted_CustomAppearance.json.golden index 171e893dd943f..0ef7a16ae1789 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDeleted_CustomAppearance.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDeleted_CustomAppearance.json.golden @@ -24,7 +24,8 @@ "name": "bobby-workspace", "reason": "autodeleted due to dormancy" }, - "data": null + "data": null, + "targets": null }, "title": "Workspace \"bobby-workspace\" deleted", "title_markdown": "Workspace \"bobby-workspace\" deleted", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDormant.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDormant.json.golden index 00c591d9d15d3..5e672c16578d2 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDormant.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDormant.json.golden @@ -22,7 +22,8 @@ "reason": "breached the template's threshold for inactivity", "timeTilDormant": "24 hours" }, - "data": null + "data": null, + "targets": null }, "title": "Workspace \"bobby-workspace\" marked as dormant", "title_markdown": "Workspace \"bobby-workspace\" marked as dormant", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManualBuildFailed.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManualBuildFailed.json.golden index 6b406a1928a70..e06fdb36a24d0 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManualBuildFailed.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManualBuildFailed.json.golden @@ -23,7 +23,8 @@ "workspace_build_number": "3", "workspace_owner_username": "mrbobby" }, - "data": null + "data": null, + "targets": null }, "title": "Workspace \"bobby-workspace\" manual build failed", "title_markdown": "Workspace \"bobby-workspace\" manual build failed", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManuallyUpdated.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManuallyUpdated.json.golden index 7fbda32e194f4..af80db4cf73a0 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManuallyUpdated.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManuallyUpdated.json.golden @@ -26,7 +26,8 @@ "version": "alpha", "workspace": "bobby-workspace" }, - "data": null + "data": null, + "targets": null }, "title": "Workspace 'bobby-workspace' has been manually updated", "title_markdown": "Workspace 'bobby-workspace' has been manually updated", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceMarkedForDeletion.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceMarkedForDeletion.json.golden index 3cb1690b0b583..2701337b344d7 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceMarkedForDeletion.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceMarkedForDeletion.json.golden @@ -21,7 +21,8 @@ "reason": "template updated to new dormancy policy", "timeTilDormant": "24 hours" }, - "data": null + "data": null, + "targets": null }, "title": "Workspace \"bobby-workspace\" marked for deletion", "title_markdown": "Workspace \"bobby-workspace\" marked for deletion", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfDisk.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfDisk.json.golden index 1bc671f52b6f9..a87d32d4b3fd1 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfDisk.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfDisk.json.golden @@ -25,7 +25,8 @@ "threshold": "90%" } ] - } + }, + "targets": null }, "title": "Your workspace \"bobby-workspace\" is low on volume space", "title_markdown": "Your workspace \"bobby-workspace\" is low on volume space", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfDisk_MultipleVolumes.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfDisk_MultipleVolumes.json.golden index c876fb1754dd1..d2d666377bed8 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfDisk_MultipleVolumes.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfDisk_MultipleVolumes.json.golden @@ -33,7 +33,8 @@ "threshold": "95%" } ] - } + }, + "targets": null }, "title": "Your workspace \"bobby-workspace\" is low on volume space", "title_markdown": "Your workspace \"bobby-workspace\" is low on volume space", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfMemory.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfMemory.json.golden index a0fce437e3c56..4787c5c256334 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfMemory.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceOutOfMemory.json.golden @@ -19,7 +19,8 @@ "threshold": "90%", "workspace": "bobby-workspace" }, - "data": null + "data": null, + "targets": null }, "title": "Your workspace \"bobby-workspace\" is low on memory", "title_markdown": "Your workspace \"bobby-workspace\" is low on memory", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateYourAccountActivated.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateYourAccountActivated.json.golden index 2e01ab7c631dc..df0681c76e7cf 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateYourAccountActivated.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateYourAccountActivated.json.golden @@ -19,7 +19,8 @@ "activated_account_name": "bobby", "initiator": "rob" }, - "data": null + "data": null, + "targets": null }, "title": "Your account \"bobby\" has been activated", "title_markdown": "Your account \"bobby\" has been activated", diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateYourAccountSuspended.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateYourAccountSuspended.json.golden index 53516dbdab5ce..8bfeff26a387f 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateYourAccountSuspended.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateYourAccountSuspended.json.golden @@ -14,7 +14,8 @@ "initiator": "rob", "suspended_account_name": "bobby" }, - "data": null + "data": null, + "targets": null }, "title": "Your account \"bobby\" has been suspended", "title_markdown": "Your account \"bobby\" has been suspended", From 02113f64a5f104e5e3f34c5172165ab6294323d2 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 11:09:10 +0000 Subject: [PATCH 05/21] fix missing details in test --- enterprise/coderd/notifications_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/notifications_test.go b/enterprise/coderd/notifications_test.go index b71bde86a5736..77b057bf41657 100644 --- a/enterprise/coderd/notifications_test.go +++ b/enterprise/coderd/notifications_test.go @@ -114,7 +114,7 @@ func TestUpdateNotificationTemplateMethod(t *testing.T) { require.Equal(t, "Invalid request to update notification template method", sdkError.Response.Message) require.Len(t, sdkError.Response.Validations, 1) require.Equal(t, "method", sdkError.Response.Validations[0].Field) - require.Equal(t, fmt.Sprintf("%q is not a valid method; smtp, webhook are the available options", method), sdkError.Response.Validations[0].Detail) + require.Equal(t, fmt.Sprintf("%q is not a valid method; smtp, webhook, inbox are the available options", method), sdkError.Response.Validations[0].Detail) }) t.Run("Not modified", func(t *testing.T) { From 1c2f5d2a3fe15cfcf1fff5b71686543435196986 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 11:55:15 +0000 Subject: [PATCH 06/21] fix enqueue functions --- coderd/database/queries.sql.go | 2 +- coderd/database/queries/notifications.sql | 2 +- coderd/notifications/enqueuer.go | 10 +++++----- coderd/notifications/notifications_test.go | 16 ++++++++-------- .../notificationstest/fake_enqueuer.go | 8 ++++---- coderd/notifications/spec.go | 4 ++-- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 7bc58c60fb3cf..81457b821eb87 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3809,7 +3809,7 @@ SELECT nm.method, nm.attempt_count::int AS attempt_count, nm.queued_seconds::float AS queued_seconds, - nm.targets, + nm.targets, -- template nt.id AS template_id, nt.title_template, diff --git a/coderd/database/queries/notifications.sql b/coderd/database/queries/notifications.sql index 411f9fc70fb07..921a58379db39 100644 --- a/coderd/database/queries/notifications.sql +++ b/coderd/database/queries/notifications.sql @@ -84,7 +84,7 @@ SELECT nm.method, nm.attempt_count::int AS attempt_count, nm.queued_seconds::float AS queued_seconds, - nm.targets, + nm.targets, -- template nt.id AS template_id, nt.title_template, diff --git a/coderd/notifications/enqueuer.go b/coderd/notifications/enqueuer.go index df91efe31d003..919d70307127c 100644 --- a/coderd/notifications/enqueuer.go +++ b/coderd/notifications/enqueuer.go @@ -53,13 +53,13 @@ func NewStoreEnqueuer(cfg codersdk.NotificationsConfig, store Store, helpers tem } // Enqueue queues a notification message for later delivery, assumes no structured input data. -func (s *StoreEnqueuer) Enqueue(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, createdBy string, targets ...uuid.UUID) (*uuid.UUID, error) { +func (s *StoreEnqueuer) Enqueue(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, createdBy string, targets ...uuid.UUID) ([]uuid.UUID, error) { return s.EnqueueWithData(ctx, userID, templateID, labels, nil, createdBy, targets...) } // Enqueue queues a notification message for later delivery. // Messages will be dequeued by a notifier later and dispatched. -func (s *StoreEnqueuer) EnqueueWithData(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, data map[string]any, createdBy string, targets ...uuid.UUID) (*uuid.UUID, error) { +func (s *StoreEnqueuer) EnqueueWithData(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, data map[string]any, createdBy string, targets ...uuid.UUID) ([]uuid.UUID, error) { metadata, err := s.store.FetchNewMessageMetadata(ctx, database.FetchNewMessageMetadataParams{ UserID: userID, NotificationTemplateID: templateID, @@ -118,7 +118,7 @@ func (s *StoreEnqueuer) EnqueueWithData(ctx context.Context, userID, templateID } s.log.Debug(ctx, "enqueued notification", slog.F("msg_id", id)) - return &id, nil + return []uuid.UUID{id}, nil } // buildPayload creates the payload that the notification will for variable substitution and/or routing. @@ -165,12 +165,12 @@ func NewNoopEnqueuer() *NoopEnqueuer { return &NoopEnqueuer{} } -func (*NoopEnqueuer) Enqueue(context.Context, uuid.UUID, uuid.UUID, map[string]string, string, ...uuid.UUID) (*uuid.UUID, error) { +func (*NoopEnqueuer) Enqueue(context.Context, uuid.UUID, uuid.UUID, map[string]string, string, ...uuid.UUID) ([]uuid.UUID, error) { // nolint:nilnil // irrelevant. return nil, nil } -func (*NoopEnqueuer) EnqueueWithData(context.Context, uuid.UUID, uuid.UUID, map[string]string, map[string]any, string, ...uuid.UUID) (*uuid.UUID, error) { +func (*NoopEnqueuer) EnqueueWithData(context.Context, uuid.UUID, uuid.UUID, map[string]string, map[string]any, string, ...uuid.UUID) ([]uuid.UUID, error) { // nolint:nilnil // irrelevant. return nil, nil } diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index f6287993a3a91..1a4693e0d08b1 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -103,8 +103,8 @@ func TestBasicNotificationRoundtrip(t *testing.T) { require.Eventually(t, func() bool { handler.mu.RLock() defer handler.mu.RUnlock() - return slices.Contains(handler.succeeded, sid.String()) && - slices.Contains(handler.failed, fid.String()) + return slices.Contains(handler.succeeded, sid[0].String()) && + slices.Contains(handler.failed, fid[0].String()) }, testutil.WaitLong, testutil.IntervalFast) // THEN: we expect the store to be called with the updates of the earlier dispatches @@ -255,7 +255,7 @@ func TestWebhookDispatch(t *testing.T) { // THEN: the webhook is received by the mock server and has the expected contents payload := testutil.RequireRecvCtx(testutil.Context(t, testutil.WaitShort), t, sent) require.EqualValues(t, "1.1", payload.Version) - require.Equal(t, *msgID, payload.MsgID) + require.Equal(t, msgID[0], payload.MsgID) require.Equal(t, payload.Payload.Labels, input) require.Equal(t, payload.Payload.UserEmail, email) // UserName is coalesced from `name` and `username`; in this case `name` wins. @@ -536,7 +536,7 @@ func TestExpiredLeaseIsRequeued(t *testing.T) { id, err := enq.Enqueue(ctx, user.ID, notifications.TemplateWorkspaceDeleted, map[string]string{"type": "success", "index": fmt.Sprintf("%d", i)}, "test") require.NoError(t, err) - msgs = append(msgs, id.String()) + msgs = append(msgs, id[0].String()) } mgr.Run(mgrCtx) @@ -668,7 +668,7 @@ func TestNotifierPaused(t *testing.T) { }) require.NoError(t, err) require.Len(t, pendingMessages, 1) - require.Equal(t, pendingMessages[0].ID.String(), sid.String()) + require.Equal(t, pendingMessages[0].ID.String(), sid[0].String()) // Wait a few fetch intervals to be sure that no new notifications are being sent. // TODO: use quartz instead. @@ -691,7 +691,7 @@ func TestNotifierPaused(t *testing.T) { require.Eventually(t, func() bool { handler.mu.RLock() defer handler.mu.RUnlock() - return slices.Contains(handler.succeeded, sid.String()) + return slices.Contains(handler.succeeded, sid[0].String()) }, fetchInterval*5, testutil.IntervalFast) } @@ -1621,7 +1621,7 @@ func TestDisabledAfterEnqueue(t *testing.T) { }) assert.NoError(ct, err) if assert.Equal(ct, len(m), 1) { - assert.Equal(ct, m[0].ID.String(), msgID.String()) + assert.Equal(ct, m[0].ID.String(), msgID[0].String()) assert.Contains(ct, m[0].StatusReason.String, "disabled by user") } }, testutil.WaitLong, testutil.IntervalFast, "did not find the expected inhibited message") @@ -1713,7 +1713,7 @@ func TestCustomNotificationMethod(t *testing.T) { mgr.Run(ctx) receivedMsgID := testutil.RequireRecvCtx(ctx, t, received) - require.Equal(t, msgID.String(), receivedMsgID.String()) + require.Equal(t, msgID[0].String(), receivedMsgID.String()) // Ensure no messages received by default method (SMTP): msgs := mockSMTPSrv.MessagesAndPurge() diff --git a/coderd/notifications/notificationstest/fake_enqueuer.go b/coderd/notifications/notificationstest/fake_enqueuer.go index b26501cf492eb..8fbc2cee25806 100644 --- a/coderd/notifications/notificationstest/fake_enqueuer.go +++ b/coderd/notifications/notificationstest/fake_enqueuer.go @@ -59,15 +59,15 @@ func (f *FakeEnqueuer) assertRBACNoLock(ctx context.Context) { } } -func (f *FakeEnqueuer) Enqueue(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, createdBy string, targets ...uuid.UUID) (*uuid.UUID, error) { +func (f *FakeEnqueuer) Enqueue(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, createdBy string, targets ...uuid.UUID) ([]uuid.UUID, error) { return f.EnqueueWithData(ctx, userID, templateID, labels, nil, createdBy, targets...) } -func (f *FakeEnqueuer) EnqueueWithData(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, data map[string]any, createdBy string, targets ...uuid.UUID) (*uuid.UUID, error) { +func (f *FakeEnqueuer) EnqueueWithData(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, data map[string]any, createdBy string, targets ...uuid.UUID) ([]uuid.UUID, error) { return f.enqueueWithDataLock(ctx, userID, templateID, labels, data, createdBy, targets...) } -func (f *FakeEnqueuer) enqueueWithDataLock(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, data map[string]any, createdBy string, targets ...uuid.UUID) (*uuid.UUID, error) { +func (f *FakeEnqueuer) enqueueWithDataLock(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, data map[string]any, createdBy string, targets ...uuid.UUID) ([]uuid.UUID, error) { f.mu.Lock() defer f.mu.Unlock() f.assertRBACNoLock(ctx) @@ -82,7 +82,7 @@ func (f *FakeEnqueuer) enqueueWithDataLock(ctx context.Context, userID, template }) id := uuid.New() - return &id, nil + return []uuid.UUID{id}, nil } func (f *FakeEnqueuer) Clear() { diff --git a/coderd/notifications/spec.go b/coderd/notifications/spec.go index 7d03c744d2aa8..4fc3c513c4b7b 100644 --- a/coderd/notifications/spec.go +++ b/coderd/notifications/spec.go @@ -37,6 +37,6 @@ type Handler interface { // Enqueuer enqueues a new notification message in the store and returns its ID, should it enqueue without failure. type Enqueuer interface { - Enqueue(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, createdBy string, targets ...uuid.UUID) (*uuid.UUID, error) - EnqueueWithData(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, data map[string]any, createdBy string, targets ...uuid.UUID) (*uuid.UUID, error) + Enqueue(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, createdBy string, targets ...uuid.UUID) ([]uuid.UUID, error) + EnqueueWithData(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string, data map[string]any, createdBy string, targets ...uuid.UUID) ([]uuid.UUID, error) } From 9134ea202d9cb00c1ac1285d679471fcfc9c530f Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 12:36:00 +0000 Subject: [PATCH 07/21] fix tests on notifications --- coderd/notifications/notifications_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 1a4693e0d08b1..3031346bd4bd3 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -187,7 +187,7 @@ func TestSMTPDispatch(t *testing.T) { require.Len(t, msgs, 1) require.Contains(t, msgs[0].MsgRequest(), fmt.Sprintf("From: %s", from)) require.Contains(t, msgs[0].MsgRequest(), fmt.Sprintf("To: %s", user.Email)) - require.Contains(t, msgs[0].MsgRequest(), fmt.Sprintf("Message-Id: %s", msgID)) + require.Contains(t, msgs[0].MsgRequest(), fmt.Sprintf("Message-Id: %s", msgID[0])) } func TestWebhookDispatch(t *testing.T) { From 9a58bf70566f748fd1613d6db72143c8925884f2 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 14:12:26 +0000 Subject: [PATCH 08/21] fix missing index --- coderd/notifications/notifications_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 3031346bd4bd3..21d7639cae772 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -1725,7 +1725,7 @@ func TestCustomNotificationMethod(t *testing.T) { require.EventuallyWithT(t, func(ct *assert.CollectT) { msgs := mockSMTPSrv.MessagesAndPurge() if assert.Len(ct, msgs, 1) { - assert.Contains(ct, msgs[0].MsgRequest(), fmt.Sprintf("Message-Id: %s", msgID)) + assert.Contains(ct, msgs[0].MsgRequest(), fmt.Sprintf("Message-Id: %s", msgID[0])) } }, testutil.WaitLong, testutil.IntervalFast) } From 7e088ac4cb8ee2ae69a9cfffc57309ce19dd1be3 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 15:07:07 +0000 Subject: [PATCH 09/21] improve EnqueueWithData function to have multiple methods --- coderd/notifications/enqueuer.go | 65 +++++++++++++++++--------------- 1 file changed, 34 insertions(+), 31 deletions(-) diff --git a/coderd/notifications/enqueuer.go b/coderd/notifications/enqueuer.go index 919d70307127c..32d716faf5582 100644 --- a/coderd/notifications/enqueuer.go +++ b/coderd/notifications/enqueuer.go @@ -85,40 +85,43 @@ func (s *StoreEnqueuer) EnqueueWithData(ctx context.Context, userID, templateID return nil, xerrors.Errorf("failed encoding input labels: %w", err) } - id := uuid.New() - err = s.store.EnqueueNotificationMessage(ctx, database.EnqueueNotificationMessageParams{ - ID: id, - UserID: userID, - NotificationTemplateID: templateID, - Method: dispatchMethod, - Payload: input, - Targets: targets, - CreatedBy: createdBy, - CreatedAt: dbtime.Time(s.clock.Now().UTC()), - }) - if err != nil { - // We have a trigger on the notification_messages table named `inhibit_enqueue_if_disabled` which prevents messages - // from being enqueued if the user has disabled them via notification_preferences. The trigger will fail the insertion - // with the message "cannot enqueue message: user has disabled this notification". - // - // This is more efficient than fetching the user's preferences for each enqueue, and centralizes the business logic. - if strings.Contains(err.Error(), ErrCannotEnqueueDisabledNotification.Error()) { - return nil, ErrCannotEnqueueDisabledNotification + uuids := make([]uuid.UUID, 0, 2) + for _, method := range []database.NotificationMethod{dispatchMethod, database.NotificationMethodInbox} { + id := uuid.New() + err = s.store.EnqueueNotificationMessage(ctx, database.EnqueueNotificationMessageParams{ + ID: id, + UserID: userID, + NotificationTemplateID: templateID, + Method: method, + Payload: input, + Targets: targets, + CreatedBy: createdBy, + CreatedAt: dbtime.Time(s.clock.Now().UTC()), + }) + if err != nil { + // We have a trigger on the notification_messages table named `inhibit_enqueue_if_disabled` which prevents messages + // from being enqueued if the user has disabled them via notification_preferences. The trigger will fail the insertion + // with the message "cannot enqueue message: user has disabled this notification". + // + // This is more efficient than fetching the user's preferences for each enqueue, and centralizes the business logic. + if strings.Contains(err.Error(), ErrCannotEnqueueDisabledNotification.Error()) { + return nil, ErrCannotEnqueueDisabledNotification + } + + // If the enqueue fails due to a dedupe hash conflict, this means that a notification has already been enqueued + // today with identical properties. It's far simpler to prevent duplicate sends in this central manner, rather than + // having each notification enqueue handle its own logic. + if database.IsUniqueViolation(err, database.UniqueNotificationMessagesDedupeHashIndex) { + return nil, ErrDuplicate + } + + s.log.Warn(ctx, "failed to enqueue notification", slog.F("template_id", templateID), slog.F("input", input), slog.Error(err)) + return nil, xerrors.Errorf("enqueue notification: %w", err) } - - // If the enqueue fails due to a dedupe hash conflict, this means that a notification has already been enqueued - // today with identical properties. It's far simpler to prevent duplicate sends in this central manner, rather than - // having each notification enqueue handle its own logic. - if database.IsUniqueViolation(err, database.UniqueNotificationMessagesDedupeHashIndex) { - return nil, ErrDuplicate - } - - s.log.Warn(ctx, "failed to enqueue notification", slog.F("template_id", templateID), slog.F("input", input), slog.Error(err)) - return nil, xerrors.Errorf("enqueue notification: %w", err) } - s.log.Debug(ctx, "enqueued notification", slog.F("msg_id", id)) - return []uuid.UUID{id}, nil + s.log.Debug(ctx, "enqueued notification", slog.F("msg_ids", uuids)) + return uuids, nil } // buildPayload creates the payload that the notification will for variable substitution and/or routing. From b995d9dfbf80ebbaf667a7eee78456f2a77df917 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 15:12:41 +0000 Subject: [PATCH 10/21] rename migration --- ..._inbox.down.sql => 000299_notifications_method_inbox.down.sql} | 0 ...thod_inbox.up.sql => 000299_notifications_method_inbox.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000298_notifications_method_inbox.down.sql => 000299_notifications_method_inbox.down.sql} (100%) rename coderd/database/migrations/{000298_notifications_method_inbox.up.sql => 000299_notifications_method_inbox.up.sql} (100%) diff --git a/coderd/database/migrations/000298_notifications_method_inbox.down.sql b/coderd/database/migrations/000299_notifications_method_inbox.down.sql similarity index 100% rename from coderd/database/migrations/000298_notifications_method_inbox.down.sql rename to coderd/database/migrations/000299_notifications_method_inbox.down.sql diff --git a/coderd/database/migrations/000298_notifications_method_inbox.up.sql b/coderd/database/migrations/000299_notifications_method_inbox.up.sql similarity index 100% rename from coderd/database/migrations/000298_notifications_method_inbox.up.sql rename to coderd/database/migrations/000299_notifications_method_inbox.up.sql From ab1e9ed08c576470e536bf95722d879f8e24999d Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 17:05:51 +0000 Subject: [PATCH 11/21] work on tests --- coderd/notifications/enqueuer.go | 2 ++ coderd/notifications/manager_test.go | 10 ++++++---- coderd/notifications/metrics_test.go | 13 ++++++++----- coderd/notifications/notifications_test.go | 8 ++++++-- 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/coderd/notifications/enqueuer.go b/coderd/notifications/enqueuer.go index 32d716faf5582..6a54e9862b385 100644 --- a/coderd/notifications/enqueuer.go +++ b/coderd/notifications/enqueuer.go @@ -118,6 +118,8 @@ func (s *StoreEnqueuer) EnqueueWithData(ctx context.Context, userID, templateID s.log.Warn(ctx, "failed to enqueue notification", slog.F("template_id", templateID), slog.F("input", input), slog.Error(err)) return nil, xerrors.Errorf("enqueue notification: %w", err) } + + uuids = append(uuids, id) } s.log.Debug(ctx, "enqueued notification", slog.F("msg_ids", uuids)) diff --git a/coderd/notifications/manager_test.go b/coderd/notifications/manager_test.go index 1897213efda70..816a4f3fa7e08 100644 --- a/coderd/notifications/manager_test.go +++ b/coderd/notifications/manager_test.go @@ -38,6 +38,7 @@ func TestBufferedUpdates(t *testing.T) { interceptor := &syncInterceptor{Store: store} santa := &santaHandler{} + santaInbox := &santaHandler{} cfg := defaultNotificationsConfig(database.NotificationMethodSmtp) cfg.StoreSyncInterval = serpent.Duration(time.Hour) // Ensure we don't sync the store automatically. @@ -46,7 +47,8 @@ func TestBufferedUpdates(t *testing.T) { mgr, err := notifications.NewManager(cfg, interceptor, defaultHelpers(), createMetrics(), logger.Named("notifications-manager")) require.NoError(t, err) mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ - database.NotificationMethodSmtp: santa, + database.NotificationMethodSmtp: santa, + database.NotificationMethodInbox: santaInbox, }) enq, err := notifications.NewStoreEnqueuer(cfg, interceptor, defaultHelpers(), logger.Named("notifications-enqueuer"), quartz.NewReal()) require.NoError(t, err) @@ -79,7 +81,7 @@ func TestBufferedUpdates(t *testing.T) { // Wait for the expected number of buffered updates to be accumulated. require.Eventually(t, func() bool { success, failure := mgr.BufferedUpdatesCount() - return success == expectedSuccess && failure == expectedFailure + return success == 4 && failure == 2 }, testutil.WaitShort, testutil.IntervalFast) // Stop the manager which forces an update of buffered updates. @@ -93,8 +95,8 @@ func TestBufferedUpdates(t *testing.T) { ct.FailNow() } - assert.EqualValues(ct, expectedFailure, interceptor.failed.Load()) - assert.EqualValues(ct, expectedSuccess, interceptor.sent.Load()) + assert.EqualValues(ct, 2, interceptor.failed.Load()) + assert.EqualValues(ct, 4, interceptor.sent.Load()) }, testutil.WaitMedium, testutil.IntervalFast) } diff --git a/coderd/notifications/metrics_test.go b/coderd/notifications/metrics_test.go index a1937add18b47..c3d30e9db37e7 100644 --- a/coderd/notifications/metrics_test.go +++ b/coderd/notifications/metrics_test.go @@ -252,8 +252,11 @@ func TestPendingUpdatesMetric(t *testing.T) { assert.NoError(t, mgr.Stop(ctx)) }) handler := &fakeHandler{} + inboxHandler := &fakeHandler{} + mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ - method: handler, + method: handler, + database.NotificationMethodInbox: inboxHandler, }) enq, err := notifications.NewStoreEnqueuer(cfg, store, defaultHelpers(), logger.Named("enqueuer"), quartz.NewReal()) @@ -285,7 +288,7 @@ func TestPendingUpdatesMetric(t *testing.T) { }() // Both handler calls should be pending in the metrics. - require.EqualValues(t, 2, promtest.ToFloat64(metrics.PendingUpdates)) + require.EqualValues(t, 4, promtest.ToFloat64(metrics.PendingUpdates)) // THEN: // Trigger syncing updates @@ -293,13 +296,13 @@ func TestPendingUpdatesMetric(t *testing.T) { // Wait until we intercept the calls to sync the pending updates to the store. success := testutil.RequireRecvCtx(testutil.Context(t, testutil.WaitShort), t, interceptor.updateSuccess) - require.EqualValues(t, 1, success) + require.EqualValues(t, 2, success) failure := testutil.RequireRecvCtx(testutil.Context(t, testutil.WaitShort), t, interceptor.updateFailure) - require.EqualValues(t, 1, failure) + require.EqualValues(t, 2, failure) // Validate that the store synced the expected number of updates. require.Eventually(t, func() bool { - return syncer.sent.Load() == 1 && syncer.failed.Load() == 1 + return syncer.sent.Load() == 2 && syncer.failed.Load() == 2 }, testutil.WaitShort, testutil.IntervalFast) // Wait for the updates to be synced and the metric to reflect that. diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 21d7639cae772..a766b99a1a9a7 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -639,7 +639,10 @@ func TestNotifierPaused(t *testing.T) { cfg.FetchInterval = serpent.Duration(fetchInterval) mgr, err := notifications.NewManager(cfg, store, defaultHelpers(), createMetrics(), logger.Named("manager")) require.NoError(t, err) - mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{method: handler}) + mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ + method: handler, + database.NotificationMethodInbox: &fakeHandler{}, + }) t.Cleanup(func() { assert.NoError(t, mgr.Stop(ctx)) }) @@ -667,8 +670,9 @@ func TestNotifierPaused(t *testing.T) { Limit: 10, }) require.NoError(t, err) - require.Len(t, pendingMessages, 1) + require.Len(t, pendingMessages, 2) require.Equal(t, pendingMessages[0].ID.String(), sid[0].String()) + require.Equal(t, pendingMessages[1].ID.String(), sid[1].String()) // Wait a few fetch intervals to be sure that no new notifications are being sent. // TODO: use quartz instead. From 61746c85832e1c3af9eacffaef655f7a48efcd4d Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 18:05:02 +0000 Subject: [PATCH 12/21] finishing up tests --- coderd/notifications/manager_test.go | 2 +- coderd/notifications/metrics_test.go | 5 +++-- coderd/notifications/notifications_test.go | 5 ++++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/coderd/notifications/manager_test.go b/coderd/notifications/manager_test.go index 816a4f3fa7e08..68877ed7f4b44 100644 --- a/coderd/notifications/manager_test.go +++ b/coderd/notifications/manager_test.go @@ -231,7 +231,7 @@ type enqueueInterceptor struct { } func newEnqueueInterceptor(db notifications.Store, metadataFn func() database.FetchNewMessageMetadataRow) *enqueueInterceptor { - return &enqueueInterceptor{Store: db, payload: make(chan types.MessagePayload, 1), metadataFn: metadataFn} + return &enqueueInterceptor{Store: db, payload: make(chan types.MessagePayload, 2), metadataFn: metadataFn} } func (e *enqueueInterceptor) EnqueueNotificationMessage(_ context.Context, arg database.EnqueueNotificationMessageParams) error { diff --git a/coderd/notifications/metrics_test.go b/coderd/notifications/metrics_test.go index c3d30e9db37e7..6ef3a0612c9c3 100644 --- a/coderd/notifications/metrics_test.go +++ b/coderd/notifications/metrics_test.go @@ -345,7 +345,8 @@ func TestInflightDispatchesMetric(t *testing.T) { // Barrier handler will wait until all notification messages are in-flight. barrier := newBarrierHandler(msgCount, handler) mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ - method: barrier, + method: barrier, + database.NotificationMethodInbox: &fakeHandler{}, }) enq, err := notifications.NewStoreEnqueuer(cfg, store, defaultHelpers(), logger.Named("enqueuer"), quartz.NewReal()) @@ -381,7 +382,7 @@ func TestInflightDispatchesMetric(t *testing.T) { // Wait for the updates to be synced and the metric to reflect that. require.Eventually(t, func() bool { - return promtest.ToFloat64(metrics.InflightDispatches) == 0 + return promtest.ToFloat64(metrics.InflightDispatches.WithLabelValues(string(method), tmpl.String())) == 0 }, testutil.WaitShort, testutil.IntervalFast) } diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index a766b99a1a9a7..00292b34eadd8 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -160,7 +160,10 @@ func TestSMTPDispatch(t *testing.T) { handler := newDispatchInterceptor(dispatch.NewSMTPHandler(cfg.SMTP, logger.Named("smtp"))) mgr, err := notifications.NewManager(cfg, store, defaultHelpers(), createMetrics(), logger.Named("manager")) require.NoError(t, err) - mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{method: handler}) + mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ + method: handler, + database.NotificationMethodInbox: &fakeHandler{}, + }) t.Cleanup(func() { assert.NoError(t, mgr.Stop(ctx)) }) From e265ea07841135113a207a8f9c31f9d5ea4f1590 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 21:03:32 +0000 Subject: [PATCH 13/21] fix tests --- coderd/notifications/metrics_test.go | 27 +++++++++---- coderd/notifications/notifications_test.go | 44 ++++++++++++++-------- 2 files changed, 47 insertions(+), 24 deletions(-) diff --git a/coderd/notifications/metrics_test.go b/coderd/notifications/metrics_test.go index 6ef3a0612c9c3..cfe1615bcbbfc 100644 --- a/coderd/notifications/metrics_test.go +++ b/coderd/notifications/metrics_test.go @@ -67,7 +67,8 @@ func TestMetrics(t *testing.T) { }) handler := &fakeHandler{} mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ - method: handler, + method: handler, + database.NotificationMethodInbox: &fakeHandler{}, }) enq, err := notifications.NewStoreEnqueuer(cfg, store, defaultHelpers(), logger.Named("enqueuer"), quartz.NewReal()) @@ -77,7 +78,10 @@ func TestMetrics(t *testing.T) { // Build fingerprints for the two different series we expect. methodTemplateFP := fingerprintLabels(notifications.LabelMethod, string(method), notifications.LabelTemplateID, tmpl.String()) + methodTemplateFPWithInbox := fingerprintLabels(notifications.LabelMethod, string(database.NotificationMethodInbox), notifications.LabelTemplateID, tmpl.String()) + methodFP := fingerprintLabels(notifications.LabelMethod, string(method)) + methodFPWithInbox := fingerprintLabels(notifications.LabelMethod, string(database.NotificationMethodInbox)) expected := map[string]func(metric *dto.Metric, series string) bool{ "coderd_notifications_dispatch_attempts_total": func(metric *dto.Metric, series string) bool { @@ -91,7 +95,8 @@ func TestMetrics(t *testing.T) { var match string for result, val := range results { seriesFP := fingerprintLabels(notifications.LabelMethod, string(method), notifications.LabelTemplateID, tmpl.String(), notifications.LabelResult, result) - if !hasMatchingFingerprint(metric, seriesFP) { + seriesFPWithInbox := fingerprintLabels(notifications.LabelMethod, string(database.NotificationMethodInbox), notifications.LabelTemplateID, tmpl.String(), notifications.LabelResult, result) + if !hasMatchingFingerprint(metric, seriesFP) && !hasMatchingFingerprint(metric, seriesFPWithInbox) { continue } @@ -115,7 +120,7 @@ func TestMetrics(t *testing.T) { return metric.Counter.GetValue() == target }, "coderd_notifications_retry_count": func(metric *dto.Metric, series string) bool { - assert.Truef(t, hasMatchingFingerprint(metric, methodTemplateFP), "found unexpected series %q", series) + assert.Truef(t, hasMatchingFingerprint(metric, methodTemplateFP) || hasMatchingFingerprint(metric, methodTemplateFPWithInbox), "found unexpected series %q", series) if debug { t.Logf("coderd_notifications_retry_count == %v: %v", maxAttempts-1, metric.Counter.GetValue()) @@ -125,7 +130,7 @@ func TestMetrics(t *testing.T) { return metric.Counter.GetValue() == maxAttempts-1 }, "coderd_notifications_queued_seconds": func(metric *dto.Metric, series string) bool { - assert.Truef(t, hasMatchingFingerprint(metric, methodFP), "found unexpected series %q", series) + assert.Truef(t, hasMatchingFingerprint(metric, methodFP) || hasMatchingFingerprint(metric, methodFPWithInbox), "found unexpected series %q", series) if debug { t.Logf("coderd_notifications_queued_seconds > 0: %v", metric.Histogram.GetSampleSum()) @@ -140,7 +145,7 @@ func TestMetrics(t *testing.T) { return metric.Histogram.GetSampleSum() > 0 }, "coderd_notifications_dispatcher_send_seconds": func(metric *dto.Metric, series string) bool { - assert.Truef(t, hasMatchingFingerprint(metric, methodFP), "found unexpected series %q", series) + assert.Truef(t, hasMatchingFingerprint(metric, methodFP) || hasMatchingFingerprint(metric, methodFPWithInbox), "found unexpected series %q", series) if debug { t.Logf("coderd_notifications_dispatcher_send_seconds > 0: %v", metric.Histogram.GetSampleSum()) @@ -170,7 +175,9 @@ func TestMetrics(t *testing.T) { } // 1 message will exceed its maxAttempts, 1 will succeed on the first try. - return metric.Counter.GetValue() == maxAttempts+1 + t.Logf("values : %v", metric.Counter.GetValue()) + t.Logf("max Attempt : %v", maxAttempts+1) + return metric.Counter.GetValue() == (maxAttempts+1)*2 // *2 because we have 2 enqueuers. }, } @@ -206,6 +213,9 @@ func TestMetrics(t *testing.T) { } for _, metric := range family.Metric { + t.Logf("metric ----> %q", metric) + t.Logf("metric(string) ----> %q", metric.String()) + t.Logf("family(GetName) ----> %q", family.GetName()) assert.True(ct, hasExpectedValue(metric, metric.String())) } } @@ -431,8 +441,9 @@ func TestCustomMethodMetricCollection(t *testing.T) { smtpHandler := &fakeHandler{} webhookHandler := &fakeHandler{} mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ - defaultMethod: smtpHandler, - customMethod: webhookHandler, + defaultMethod: smtpHandler, + customMethod: webhookHandler, + database.NotificationMethodInbox: &fakeHandler{}, }) enq, err := notifications.NewStoreEnqueuer(cfg, store, defaultHelpers(), logger.Named("enqueuer"), quartz.NewReal()) diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 00292b34eadd8..7c87cbc19cd2c 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -82,7 +82,10 @@ func TestBasicNotificationRoundtrip(t *testing.T) { cfg.RetryInterval = serpent.Duration(time.Hour) // Ensure retries don't interfere with the test mgr, err := notifications.NewManager(cfg, interceptor, defaultHelpers(), createMetrics(), logger.Named("manager")) require.NoError(t, err) - mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{method: handler}) + mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ + method: handler, + database.NotificationMethodInbox: &fakeHandler{}, + }) t.Cleanup(func() { assert.NoError(t, mgr.Stop(ctx)) }) @@ -109,8 +112,8 @@ func TestBasicNotificationRoundtrip(t *testing.T) { // THEN: we expect the store to be called with the updates of the earlier dispatches require.Eventually(t, func() bool { - return interceptor.sent.Load() == 1 && - interceptor.failed.Load() == 1 + return interceptor.sent.Load() == 2 && + interceptor.failed.Load() == 2 }, testutil.WaitLong, testutil.IntervalFast) // THEN: we verify that the store contains notifications in their expected state @@ -119,13 +122,13 @@ func TestBasicNotificationRoundtrip(t *testing.T) { Limit: 10, }) require.NoError(t, err) - require.Len(t, success, 1) + require.Len(t, success, 2) failed, err := store.GetNotificationMessagesByStatus(ctx, database.GetNotificationMessagesByStatusParams{ Status: database.NotificationMessageStatusTemporaryFailure, Limit: 10, }) require.NoError(t, err) - require.Len(t, failed, 1) + require.Len(t, failed, 2) } func TestSMTPDispatch(t *testing.T) { @@ -318,7 +321,10 @@ func TestBackpressure(t *testing.T) { mgr, err := notifications.NewManager(cfg, storeInterceptor, defaultHelpers(), createMetrics(), logger.Named("manager"), notifications.WithTestClock(mClock)) require.NoError(t, err) - mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{method: handler}) + mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ + method: handler, + database.NotificationMethodInbox: handler, + }) enq, err := notifications.NewStoreEnqueuer(cfg, store, defaultHelpers(), logger.Named("enqueuer"), mClock) require.NoError(t, err) @@ -466,7 +472,10 @@ func TestRetries(t *testing.T) { t.Cleanup(func() { assert.NoError(t, mgr.Stop(ctx)) }) - mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{method: handler}) + mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ + method: handler, + database.NotificationMethodInbox: &fakeHandler{}, + }) enq, err := notifications.NewStoreEnqueuer(cfg, store, defaultHelpers(), logger.Named("enqueuer"), quartz.NewReal()) require.NoError(t, err) @@ -484,9 +493,9 @@ func TestRetries(t *testing.T) { // THEN: we expect to see all but the final attempts failing require.Eventually(t, func() bool { // We expect all messages to fail all attempts but the final; - return storeInterceptor.failed.Load() == msgCount*(maxAttempts-1) && + return storeInterceptor.failed.Load() == 0 && // ...and succeed on the final attempt. - storeInterceptor.sent.Load() == msgCount + storeInterceptor.sent.Load() == 0 }, testutil.WaitLong, testutil.IntervalFast) } @@ -536,10 +545,10 @@ func TestExpiredLeaseIsRequeued(t *testing.T) { // WHEN: a few notifications are enqueued which will all succeed var msgs []string for i := 0; i < msgCount; i++ { - id, err := enq.Enqueue(ctx, user.ID, notifications.TemplateWorkspaceDeleted, + ids, err := enq.Enqueue(ctx, user.ID, notifications.TemplateWorkspaceDeleted, map[string]string{"type": "success", "index": fmt.Sprintf("%d", i)}, "test") require.NoError(t, err) - msgs = append(msgs, id[0].String()) + msgs = append(msgs, ids[0].String(), ids[1].String()) } mgr.Run(mgrCtx) @@ -554,7 +563,7 @@ func TestExpiredLeaseIsRequeued(t *testing.T) { // Fetch any messages currently in "leased" status, and verify that they're exactly the ones we enqueued. leased, err := store.GetNotificationMessagesByStatus(ctx, database.GetNotificationMessagesByStatusParams{ Status: database.NotificationMessageStatusLeased, - Limit: msgCount, + Limit: msgCount * 2, }) require.NoError(t, err) @@ -576,7 +585,10 @@ func TestExpiredLeaseIsRequeued(t *testing.T) { handler := newDispatchInterceptor(&fakeHandler{}) mgr, err = notifications.NewManager(cfg, storeInterceptor, defaultHelpers(), createMetrics(), logger.Named("manager")) require.NoError(t, err) - mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{method: handler}) + mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ + method: handler, + database.NotificationMethodInbox: &fakeHandler{}, + }) // Use regular context now. t.Cleanup(func() { @@ -587,7 +599,7 @@ func TestExpiredLeaseIsRequeued(t *testing.T) { // Wait until all messages are sent & updates flushed to the database. require.Eventually(t, func() bool { return handler.sent.Load() == msgCount && - storeInterceptor.sent.Load() == msgCount + storeInterceptor.sent.Load() == msgCount*2 }, testutil.WaitLong, testutil.IntervalFast) // Validate that no more messages are in "leased" status. @@ -1627,8 +1639,8 @@ func TestDisabledAfterEnqueue(t *testing.T) { Limit: 10, }) assert.NoError(ct, err) - if assert.Equal(ct, len(m), 1) { - assert.Equal(ct, m[0].ID.String(), msgID[0].String()) + if assert.Equal(ct, len(m), 2) { + assert.Contains(ct, []string{m[0].ID.String(), m[1].ID.String()}, msgID[0].String()) assert.Contains(ct, m[0].StatusReason.String, "disabled by user") } }, testutil.WaitLong, testutil.IntervalFast, "did not find the expected inhibited message") From e56c155c734a18c273d746997abfae1aaa98dd16 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Mon, 3 Mar 2025 21:32:31 +0000 Subject: [PATCH 14/21] cleanup testing variables --- coderd/notifications/manager_test.go | 6 +++--- coderd/notifications/metrics_test.go | 5 ----- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/coderd/notifications/manager_test.go b/coderd/notifications/manager_test.go index 68877ed7f4b44..261ca5d28dff0 100644 --- a/coderd/notifications/manager_test.go +++ b/coderd/notifications/manager_test.go @@ -81,7 +81,7 @@ func TestBufferedUpdates(t *testing.T) { // Wait for the expected number of buffered updates to be accumulated. require.Eventually(t, func() bool { success, failure := mgr.BufferedUpdatesCount() - return success == 4 && failure == 2 + return success == expectedSuccess*2 && failure == expectedFailure*2 // Each message is enqueued twice. }, testutil.WaitShort, testutil.IntervalFast) // Stop the manager which forces an update of buffered updates. @@ -95,8 +95,8 @@ func TestBufferedUpdates(t *testing.T) { ct.FailNow() } - assert.EqualValues(ct, 2, interceptor.failed.Load()) - assert.EqualValues(ct, 4, interceptor.sent.Load()) + assert.EqualValues(ct, expectedFailure*2, interceptor.failed.Load()) + assert.EqualValues(ct, expectedSuccess*2, interceptor.sent.Load()) }, testutil.WaitMedium, testutil.IntervalFast) } diff --git a/coderd/notifications/metrics_test.go b/coderd/notifications/metrics_test.go index cfe1615bcbbfc..2780596fb2c66 100644 --- a/coderd/notifications/metrics_test.go +++ b/coderd/notifications/metrics_test.go @@ -175,8 +175,6 @@ func TestMetrics(t *testing.T) { } // 1 message will exceed its maxAttempts, 1 will succeed on the first try. - t.Logf("values : %v", metric.Counter.GetValue()) - t.Logf("max Attempt : %v", maxAttempts+1) return metric.Counter.GetValue() == (maxAttempts+1)*2 // *2 because we have 2 enqueuers. }, } @@ -213,9 +211,6 @@ func TestMetrics(t *testing.T) { } for _, metric := range family.Metric { - t.Logf("metric ----> %q", metric) - t.Logf("metric(string) ----> %q", metric.String()) - t.Logf("family(GetName) ----> %q", family.GetName()) assert.True(ct, hasExpectedValue(metric, metric.String())) } } From ac360857ce3da2c2dedf8f7aeaf173c80fbf450b Mon Sep 17 00:00:00 2001 From: defelmnq Date: Tue, 4 Mar 2025 10:32:13 +0000 Subject: [PATCH 15/21] add tests for notifications inbox --- coderd/notifications/dispatch/inbox.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/notifications/dispatch/inbox.go b/coderd/notifications/dispatch/inbox.go index fc92c57d45d19..42c3c27000be9 100644 --- a/coderd/notifications/dispatch/inbox.go +++ b/coderd/notifications/dispatch/inbox.go @@ -61,6 +61,7 @@ func (s *InboxHandler) dispatch(payload types.MessagePayload, title, body string return false, xerrors.Errorf("marshal actions: %w", err) } + // nolint:exhaustruct _, err = s.store.InsertInboxNotification(ctx, database.InsertInboxNotificationParams{ ID: msgID, UserID: userID, @@ -68,7 +69,6 @@ func (s *InboxHandler) dispatch(payload types.MessagePayload, title, body string Targets: payload.Targets, Title: title, Content: body, - Icon: "https://cdn.coder.com/icons/coder-icon-512x512.png", Actions: actions, CreatedAt: dbtime.Now(), }) From ad59e8b938fb49914a08f63d090863fd292e9c83 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Tue, 4 Mar 2025 11:15:06 +0000 Subject: [PATCH 16/21] filter coder inbox from UI --- coderd/notifications.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/coderd/notifications.go b/coderd/notifications.go index 812d8cd3e450b..670f3625f41bc 100644 --- a/coderd/notifications.go +++ b/coderd/notifications.go @@ -157,6 +157,11 @@ func (api *API) systemNotificationTemplates(rw http.ResponseWriter, r *http.Requ func (api *API) notificationDispatchMethods(rw http.ResponseWriter, r *http.Request) { var methods []string for _, nm := range database.AllNotificationMethodValues() { + // Skip inbox method as for now this is an implicit delivery target and should not appear + // anywhere in the Web UI. + if nm == database.NotificationMethodInbox { + continue + } methods = append(methods, string(nm)) } From a11f505f4aea903bd37e5f8ae0de4c5a70563664 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Tue, 4 Mar 2025 11:24:22 +0000 Subject: [PATCH 17/21] add missing inbox tests --- coderd/notifications/dispatch/inbox_test.go | 104 ++++++++++++++++++++ coderd/notifications_test.go | 3 + 2 files changed, 107 insertions(+) create mode 100644 coderd/notifications/dispatch/inbox_test.go diff --git a/coderd/notifications/dispatch/inbox_test.go b/coderd/notifications/dispatch/inbox_test.go new file mode 100644 index 0000000000000..7c7c1e8fa84b3 --- /dev/null +++ b/coderd/notifications/dispatch/inbox_test.go @@ -0,0 +1,104 @@ +package dispatch_test + +import ( + "context" + "testing" + + "cdr.dev/slog" + "cdr.dev/slog/sloggers/slogtest" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/notifications" + "github.com/coder/coder/v2/coderd/notifications/dispatch" + "github.com/coder/coder/v2/coderd/notifications/types" +) + +func TestInbox(t *testing.T) { + t.Parallel() + + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true, IgnoredErrorIs: []error{}}).Leveled(slog.LevelDebug) + tests := []struct { + name string + msgID uuid.UUID + payload types.MessagePayload + expectedErr string + expectedRetry bool + }{ + { + name: "OK", + msgID: uuid.New(), + payload: types.MessagePayload{ + NotificationName: "test", + NotificationTemplateID: notifications.TemplateWorkspaceDeleted.String(), + UserID: "1e965b51-9465-43d8-ac20-c5f689f9c788", + Actions: []types.TemplateAction{ + { + Label: "View my workspace", + URL: "https://coder.com/workspaces/1", + }, + }, + }, + }, + { + name: "InvalidUserID", + payload: types.MessagePayload{ + NotificationName: "test", + NotificationTemplateID: notifications.TemplateWorkspaceDeleted.String(), + UserID: "invalid", + Actions: []types.TemplateAction{}, + }, + expectedErr: "parse user ID", + expectedRetry: false, + }, + { + name: "InvalidTemplateID", + payload: types.MessagePayload{ + NotificationName: "test", + NotificationTemplateID: "invalid", + UserID: "1e965b51-9465-43d8-ac20-c5f689f9c788", + Actions: []types.TemplateAction{}, + }, + expectedErr: "parse template ID", + expectedRetry: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + ctx := context.Background() + + handler := dispatch.NewInboxHandler(logger.Named("smtp"), db) + dispatcherFunc, err := handler.Dispatcher(tc.payload, "", "", nil) + require.NoError(t, err) + + retryable, err := dispatcherFunc(ctx, tc.msgID) + + if tc.expectedErr != "" { + require.ErrorContains(t, err, tc.expectedErr) + if tc.expectedRetry { + require.True(t, retryable) + } else { + require.False(t, retryable) + } + } else { + require.NoError(t, err) + require.False(t, retryable) + uid := uuid.MustParse(tc.payload.UserID) + notifs, err := db.GetInboxNotificationsByUserID(ctx, database.GetInboxNotificationsByUserIDParams{ + UserID: uid, + }) + + require.NoError(t, err) + require.Len(t, notifs, 1) + require.Equal(t, tc.msgID, notifs[0].ID) + } + }) + } +} diff --git a/coderd/notifications_test.go b/coderd/notifications_test.go index d50464869298b..bae8b8827fe79 100644 --- a/coderd/notifications_test.go +++ b/coderd/notifications_test.go @@ -296,6 +296,9 @@ func TestNotificationDispatchMethods(t *testing.T) { var allMethods []string for _, nm := range database.AllNotificationMethodValues() { + if nm == database.NotificationMethodInbox { + continue + } allMethods = append(allMethods, string(nm)) } slices.Sort(allMethods) From fe58472d4ad479e820cd011b6cdf315293fb0a0c Mon Sep 17 00:00:00 2001 From: defelmnq Date: Tue, 4 Mar 2025 11:30:55 +0000 Subject: [PATCH 18/21] fix tests --- coderd/notifications/dispatch/inbox_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/notifications/dispatch/inbox_test.go b/coderd/notifications/dispatch/inbox_test.go index 7c7c1e8fa84b3..d8b97d7548bc3 100644 --- a/coderd/notifications/dispatch/inbox_test.go +++ b/coderd/notifications/dispatch/inbox_test.go @@ -68,6 +68,7 @@ func TestInbox(t *testing.T) { } for _, tc := range tests { + tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() From 15933fe988d915a261182d04eac020281d2ea695 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Tue, 4 Mar 2025 15:06:59 +0000 Subject: [PATCH 19/21] fixes from review --- coderd/notifications/dispatch/inbox.go | 8 ++++---- coderd/notifications/dispatch/inbox_test.go | 13 ++++++------- coderd/notifications/enqueuer.go | 3 +++ coderd/notifications/notifications_test.go | 9 ++++++++- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/coderd/notifications/dispatch/inbox.go b/coderd/notifications/dispatch/inbox.go index 42c3c27000be9..036424decf3c7 100644 --- a/coderd/notifications/dispatch/inbox.go +++ b/coderd/notifications/dispatch/inbox.go @@ -17,17 +17,17 @@ import ( markdown "github.com/coder/coder/v2/coderd/render" ) -type inboxStore interface { +type InboxStore interface { InsertInboxNotification(ctx context.Context, arg database.InsertInboxNotificationParams) (database.InboxNotification, error) } -// InboxHandler is responsible for dispatching notification messages in the Coder Inbox. +// InboxHandler is responsible for dispatching notification messages to the Coder Inbox. type InboxHandler struct { log slog.Logger - store inboxStore + store InboxStore } -func NewInboxHandler(log slog.Logger, store inboxStore) *InboxHandler { +func NewInboxHandler(log slog.Logger, store InboxStore) *InboxHandler { return &InboxHandler{log: log, store: store} } diff --git a/coderd/notifications/dispatch/inbox_test.go b/coderd/notifications/dispatch/inbox_test.go index d8b97d7548bc3..d997b2be3ef3d 100644 --- a/coderd/notifications/dispatch/inbox_test.go +++ b/coderd/notifications/dispatch/inbox_test.go @@ -20,7 +20,7 @@ import ( func TestInbox(t *testing.T) { t.Parallel() - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true, IgnoredErrorIs: []error{}}).Leveled(slog.LevelDebug) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) tests := []struct { name string msgID uuid.UUID @@ -73,6 +73,8 @@ func TestInbox(t *testing.T) { t.Parallel() db, _ := dbtestutil.NewDB(t) + dbtestutil.DisableForeignKeysAndTriggers(t, db) + ctx := context.Background() handler := dispatch.NewInboxHandler(logger.Named("smtp"), db) @@ -83,17 +85,14 @@ func TestInbox(t *testing.T) { if tc.expectedErr != "" { require.ErrorContains(t, err, tc.expectedErr) - if tc.expectedRetry { - require.True(t, retryable) - } else { - require.False(t, retryable) - } + require.Equal(t, tc.expectedRetry, retryable) } else { require.NoError(t, err) require.False(t, retryable) uid := uuid.MustParse(tc.payload.UserID) notifs, err := db.GetInboxNotificationsByUserID(ctx, database.GetInboxNotificationsByUserIDParams{ - UserID: uid, + UserID: uid, + ReadStatus: database.InboxNotificationReadStatusAll, }) require.NoError(t, err) diff --git a/coderd/notifications/enqueuer.go b/coderd/notifications/enqueuer.go index 6a54e9862b385..dbcc67d1c5e70 100644 --- a/coderd/notifications/enqueuer.go +++ b/coderd/notifications/enqueuer.go @@ -86,6 +86,9 @@ func (s *StoreEnqueuer) EnqueueWithData(ctx context.Context, userID, templateID } uuids := make([]uuid.UUID, 0, 2) + // All the enqueued messages are enqueued both on the dispatch method set by the user (or default one) and the inbox. + // As the inbox is not configurable per the user and is always enabled, we always enqueue the message on the inbox. + // The logic is done here in order to have two completely separated processing and retries are handled separately. for _, method := range []database.NotificationMethod{dispatchMethod, database.NotificationMethodInbox} { id := uuid.New() err = s.store.EnqueueNotificationMessage(ctx, database.EnqueueNotificationMessageParams{ diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 7c87cbc19cd2c..0ff1a3923b8ac 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -178,6 +178,7 @@ func TestSMTPDispatch(t *testing.T) { // WHEN: a message is enqueued msgID, err := enq.Enqueue(ctx, user.ID, notifications.TemplateWorkspaceDeleted, map[string]string{}, "test") require.NoError(t, err) + require.Len(t, msgID, 2) mgr.Run(ctx) @@ -548,6 +549,7 @@ func TestExpiredLeaseIsRequeued(t *testing.T) { ids, err := enq.Enqueue(ctx, user.ID, notifications.TemplateWorkspaceDeleted, map[string]string{"type": "success", "index": fmt.Sprintf("%d", i)}, "test") require.NoError(t, err) + require.Len(t, ids, 2) msgs = append(msgs, ids[0].String(), ids[1].String()) } @@ -786,6 +788,10 @@ func TestNotificationTemplates_Golden(t *testing.T) { "reason": "autodeleted due to dormancy", "initiator": "autobuild", }, + Targets: []uuid.UUID{ + uuid.MustParse("5c6ea841-ca63-46cc-9c37-78734c7a788b"), + uuid.MustParse("b8355e3a-f3c5-4dd1-b382-7eb1fae7db52"), + }, }, }, { @@ -1317,6 +1323,7 @@ func TestNotificationTemplates_Golden(t *testing.T) { ) require.NoError(t, err) + tc.payload.Targets = append(tc.payload.Targets, user.ID) _, err = smtpEnqueuer.EnqueueWithData( ctx, user.ID, @@ -1324,7 +1331,7 @@ func TestNotificationTemplates_Golden(t *testing.T) { tc.payload.Labels, tc.payload.Data, user.Username, - user.ID, + tc.payload.Targets..., ) require.NoError(t, err) From aae0f985768d49de2798a9b90fcc91468a76a9b4 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Tue, 4 Mar 2025 16:02:55 +0000 Subject: [PATCH 20/21] improve testing logic --- coderd/notifications/manager_test.go | 13 ++++++++----- coderd/notifications/notifications_test.go | 13 ++++++++----- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/coderd/notifications/manager_test.go b/coderd/notifications/manager_test.go index 261ca5d28dff0..f9f8920143e3c 100644 --- a/coderd/notifications/manager_test.go +++ b/coderd/notifications/manager_test.go @@ -46,10 +46,13 @@ func TestBufferedUpdates(t *testing.T) { // GIVEN: a manager which will pass or fail notifications based on their "nice" labels mgr, err := notifications.NewManager(cfg, interceptor, defaultHelpers(), createMetrics(), logger.Named("notifications-manager")) require.NoError(t, err) - mgr.WithHandlers(map[database.NotificationMethod]notifications.Handler{ + + handlers := map[database.NotificationMethod]notifications.Handler{ database.NotificationMethodSmtp: santa, database.NotificationMethodInbox: santaInbox, - }) + } + + mgr.WithHandlers(handlers) enq, err := notifications.NewStoreEnqueuer(cfg, interceptor, defaultHelpers(), logger.Named("notifications-enqueuer"), quartz.NewReal()) require.NoError(t, err) @@ -81,7 +84,7 @@ func TestBufferedUpdates(t *testing.T) { // Wait for the expected number of buffered updates to be accumulated. require.Eventually(t, func() bool { success, failure := mgr.BufferedUpdatesCount() - return success == expectedSuccess*2 && failure == expectedFailure*2 // Each message is enqueued twice. + return success == expectedSuccess*len(handlers) && failure == expectedFailure*len(handlers) }, testutil.WaitShort, testutil.IntervalFast) // Stop the manager which forces an update of buffered updates. @@ -95,8 +98,8 @@ func TestBufferedUpdates(t *testing.T) { ct.FailNow() } - assert.EqualValues(ct, expectedFailure*2, interceptor.failed.Load()) - assert.EqualValues(ct, expectedSuccess*2, interceptor.sent.Load()) + assert.EqualValues(ct, expectedFailure*len(handlers), interceptor.failed.Load()) + assert.EqualValues(ct, expectedSuccess*len(handlers), interceptor.sent.Load()) }, testutil.WaitMedium, testutil.IntervalFast) } diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 0ff1a3923b8ac..b8c59e7682f14 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -491,12 +491,15 @@ func TestRetries(t *testing.T) { mgr.Run(ctx) - // THEN: we expect to see all but the final attempts failing + // the number of tries is equal to the number of messages times the number of attempts + // times 2 as the Enqueue method pushes into both the defined dispatch method and inbox + nbTries := msgCount * maxAttempts * 2 + + // THEN: we expect to see all but the final attempts failing on webhook, and all messages to fail on inbox require.Eventually(t, func() bool { - // We expect all messages to fail all attempts but the final; - return storeInterceptor.failed.Load() == 0 && - // ...and succeed on the final attempt. - storeInterceptor.sent.Load() == 0 + // nolint:gosec + return storeInterceptor.failed.Load() == int32(nbTries-msgCount) && + storeInterceptor.sent.Load() == msgCount }, testutil.WaitLong, testutil.IntervalFast) } From 5a5f2d68fe6a67fc0f6e703aa80ce52e376e51c9 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Wed, 5 Mar 2025 14:38:05 +0000 Subject: [PATCH 21/21] change tests for user id --- coderd/notifications/dispatch/inbox_test.go | 11 ++++++++--- coderd/notifications/notifications_test.go | 4 ++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/coderd/notifications/dispatch/inbox_test.go b/coderd/notifications/dispatch/inbox_test.go index d997b2be3ef3d..72547122b2e01 100644 --- a/coderd/notifications/dispatch/inbox_test.go +++ b/coderd/notifications/dispatch/inbox_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/notifications/dispatch" @@ -34,7 +35,7 @@ func TestInbox(t *testing.T) { payload: types.MessagePayload{ NotificationName: "test", NotificationTemplateID: notifications.TemplateWorkspaceDeleted.String(), - UserID: "1e965b51-9465-43d8-ac20-c5f689f9c788", + UserID: "valid", Actions: []types.TemplateAction{ { Label: "View my workspace", @@ -59,7 +60,7 @@ func TestInbox(t *testing.T) { payload: types.MessagePayload{ NotificationName: "test", NotificationTemplateID: "invalid", - UserID: "1e965b51-9465-43d8-ac20-c5f689f9c788", + UserID: "valid", Actions: []types.TemplateAction{}, }, expectedErr: "parse template ID", @@ -73,7 +74,11 @@ func TestInbox(t *testing.T) { t.Parallel() db, _ := dbtestutil.NewDB(t) - dbtestutil.DisableForeignKeysAndTriggers(t, db) + + if tc.payload.UserID == "valid" { + user := dbgen.User(t, db, database.User{}) + tc.payload.UserID = user.ID.String() + } ctx := context.Background() diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index b8c59e7682f14..3ef8f59228093 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -808,6 +808,10 @@ func TestNotificationTemplates_Golden(t *testing.T) { "name": "bobby-workspace", "reason": "autostart", }, + Targets: []uuid.UUID{ + uuid.MustParse("5c6ea841-ca63-46cc-9c37-78734c7a788b"), + uuid.MustParse("b8355e3a-f3c5-4dd1-b382-7eb1fae7db52"), + }, }, }, {