From c4755f428543742cd97897ad93632c252b1c9c94 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 10 Jan 2025 13:49:06 +0000 Subject: [PATCH 1/7] feat: allow notification templates to be disabled by default --- coderd/apidoc/docs.go | 3 ++ coderd/apidoc/swagger.json | 3 ++ coderd/database/dump.sql | 14 ++++++++- ...notification_templates_by_default.down.sql | 18 +++++++++++ ...g_notification_templates_by_default.up.sql | 31 +++++++++++++++++++ coderd/database/models.go | 5 +-- coderd/database/queries.sql.go | 9 ++++-- coderd/notifications.go | 17 +++++----- coderd/notifications/notifications_test.go | 28 +++++++++++++++++ codersdk/notifications.go | 17 +++++----- docs/admin/security/audit-logs.md | 2 +- docs/reference/api/notifications.md | 24 +++++++------- docs/reference/api/schemas.md | 22 +++++++------ enterprise/audit/table.go | 17 +++++----- site/src/api/typesGenerated.ts | 1 + .../NotificationsPage/NotificationsPage.tsx | 19 ++++++++++-- site/src/testHelpers/entities.ts | 7 +++++ 17 files changed, 183 insertions(+), 54 deletions(-) create mode 100644 coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.down.sql create mode 100644 coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index a8bfcb2af3b19..4c553eba482cc 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -11664,6 +11664,9 @@ const docTemplate = `{ "body_template": { "type": "string" }, + "enabled_by_default": { + "type": "boolean" + }, "group": { "type": "string" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index d7c32d8a33a52..58e9db9196e7e 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -10462,6 +10462,9 @@ "body_template": { "type": "string" }, + "enabled_by_default": { + "type": "boolean" + }, "group": { "type": "string" }, diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 50519485dc505..701319801a263 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -355,6 +355,17 @@ BEGIN RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; END IF; + IF (NOT EXISTS (SELECT 1 + FROM notification_preferences + WHERE user_id = NEW.user_id + AND notification_template_id = NEW.notification_template_id)) + AND (EXISTS (SELECT 1 + FROM notification_templates + WHERE id = NEW.notification_template_id + AND enabled_by_default = FALSE)) THEN + RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; + END IF; + RETURN NEW; END; $$; @@ -844,7 +855,8 @@ CREATE TABLE notification_templates ( actions jsonb, "group" text, method notification_method, - kind notification_template_kind DEFAULT 'system'::notification_template_kind NOT NULL + kind notification_template_kind DEFAULT 'system'::notification_template_kind NOT NULL, + enabled_by_default boolean DEFAULT true NOT NULL ); COMMENT ON TABLE notification_templates IS 'Templates from which to create notification messages.'; diff --git a/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.down.sql b/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.down.sql new file mode 100644 index 0000000000000..cdcaff6553f52 --- /dev/null +++ b/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.down.sql @@ -0,0 +1,18 @@ +ALTER TABLE notification_templates DROP COLUMN enabled_by_default; + +CREATE OR REPLACE FUNCTION inhibit_enqueue_if_disabled() + RETURNS TRIGGER AS +$$ +BEGIN + -- Fail the insertion if the user has disabled this notification. + IF EXISTS (SELECT 1 + FROM notification_preferences + WHERE disabled = TRUE + AND user_id = NEW.user_id + AND notification_template_id = NEW.notification_template_id) THEN + RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; + END IF; + + RETURN NEW; +END; +$$ LANGUAGE plpgsql; diff --git a/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql b/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql new file mode 100644 index 0000000000000..b2ff7b4ed2e87 --- /dev/null +++ b/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql @@ -0,0 +1,31 @@ +ALTER TABLE notification_templates ADD COLUMN enabled_by_default boolean DEFAULT TRUE NOT NULL; + +CREATE OR REPLACE FUNCTION inhibit_enqueue_if_disabled() + RETURNS TRIGGER AS +$$ +BEGIN + -- Fail the insertion if the user has disabled this notification. + IF EXISTS (SELECT 1 + FROM notification_preferences + WHERE disabled = TRUE + AND user_id = NEW.user_id + AND notification_template_id = NEW.notification_template_id) THEN + RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; + END IF; + + -- Fails if the notification template is disabled by default and the + -- user hasn't explicitly enabled it. + IF (NOT EXISTS (SELECT 1 + FROM notification_preferences + WHERE user_id = NEW.user_id + AND notification_template_id = NEW.notification_template_id)) + AND (EXISTS (SELECT 1 + FROM notification_templates + WHERE id = NEW.notification_template_id + AND enabled_by_default = FALSE)) THEN + RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; + END IF; + + RETURN NEW; +END; +$$ LANGUAGE plpgsql; diff --git a/coderd/database/models.go b/coderd/database/models.go index 9ca80d119a502..cc3c49b715616 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -2480,8 +2480,9 @@ type NotificationTemplate struct { Actions []byte `db:"actions" json:"actions"` Group sql.NullString `db:"group" json:"group"` // NULL defers to the deployment-level method - Method NullNotificationMethod `db:"method" json:"method"` - Kind NotificationTemplateKind `db:"kind" json:"kind"` + Method NullNotificationMethod `db:"method" json:"method"` + Kind NotificationTemplateKind `db:"kind" json:"kind"` + EnabledByDefault bool `db:"enabled_by_default" json:"enabled_by_default"` } // A table used to configure apps that can use Coder as an OAuth2 provider, the reverse of what we are calling external authentication. diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 8fbb7c0b5be6c..c5624670754ab 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3969,7 +3969,7 @@ func (q *sqlQuerier) GetNotificationReportGeneratorLogByTemplate(ctx context.Con } const getNotificationTemplateByID = `-- name: GetNotificationTemplateByID :one -SELECT id, name, title_template, body_template, actions, "group", method, kind +SELECT id, name, title_template, body_template, actions, "group", method, kind, enabled_by_default FROM notification_templates WHERE id = $1::uuid ` @@ -3986,12 +3986,13 @@ func (q *sqlQuerier) GetNotificationTemplateByID(ctx context.Context, id uuid.UU &i.Group, &i.Method, &i.Kind, + &i.EnabledByDefault, ) return i, err } const getNotificationTemplatesByKind = `-- name: GetNotificationTemplatesByKind :many -SELECT id, name, title_template, body_template, actions, "group", method, kind +SELECT id, name, title_template, body_template, actions, "group", method, kind, enabled_by_default FROM notification_templates WHERE kind = $1::notification_template_kind ORDER BY name ASC @@ -4015,6 +4016,7 @@ func (q *sqlQuerier) GetNotificationTemplatesByKind(ctx context.Context, kind No &i.Group, &i.Method, &i.Kind, + &i.EnabledByDefault, ); err != nil { return nil, err } @@ -4068,7 +4070,7 @@ const updateNotificationTemplateMethodByID = `-- name: UpdateNotificationTemplat UPDATE notification_templates SET method = $1::notification_method WHERE id = $2::uuid -RETURNING id, name, title_template, body_template, actions, "group", method, kind +RETURNING id, name, title_template, body_template, actions, "group", method, kind, enabled_by_default ` type UpdateNotificationTemplateMethodByIDParams struct { @@ -4088,6 +4090,7 @@ func (q *sqlQuerier) UpdateNotificationTemplateMethodByID(ctx context.Context, a &i.Group, &i.Method, &i.Kind, + &i.EnabledByDefault, ) return i, err } diff --git a/coderd/notifications.go b/coderd/notifications.go index bdf71f99cab98..32f035a076b43 100644 --- a/coderd/notifications.go +++ b/coderd/notifications.go @@ -271,14 +271,15 @@ func (api *API) putUserNotificationPreferences(rw http.ResponseWriter, r *http.R func convertNotificationTemplates(in []database.NotificationTemplate) (out []codersdk.NotificationTemplate) { for _, tmpl := range in { out = append(out, codersdk.NotificationTemplate{ - ID: tmpl.ID, - Name: tmpl.Name, - TitleTemplate: tmpl.TitleTemplate, - BodyTemplate: tmpl.BodyTemplate, - Actions: string(tmpl.Actions), - Group: tmpl.Group.String, - Method: string(tmpl.Method.NotificationMethod), - Kind: string(tmpl.Kind), + ID: tmpl.ID, + Name: tmpl.Name, + TitleTemplate: tmpl.TitleTemplate, + BodyTemplate: tmpl.BodyTemplate, + Actions: string(tmpl.Actions), + Group: tmpl.Group.String, + Method: string(tmpl.Method.NotificationMethod), + Kind: string(tmpl.Kind), + EnabledByDefault: tmpl.EnabledByDefault, }) } diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index e404f4afb3c19..15f3d90a8a89f 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -1106,6 +1106,20 @@ func TestNotificationTemplates_Golden(t *testing.T) { r.Name = tc.payload.UserName }, ) + + // With the introduction of notifications that can be disabled + // by default, we want to make sure the user preferences have + // the notification enabled. + _, err := adminClient.UpdateUserNotificationPreferences( + context.Background(), + user.ID, + codersdk.UpdateUserNotificationPreferences{ + TemplateDisabledMap: map[string]bool{ + tc.id.String(): false, + }, + }) + require.NoError(t, err) + return &db, &api.Logger, &user }() @@ -1275,6 +1289,20 @@ func TestNotificationTemplates_Golden(t *testing.T) { r.Name = tc.payload.UserName }, ) + + // With the introduction of notifications that can be disabled + // by default, we want to make sure the user preferences have + // the notification enabled. + _, err := adminClient.UpdateUserNotificationPreferences( + context.Background(), + user.ID, + codersdk.UpdateUserNotificationPreferences{ + TemplateDisabledMap: map[string]bool{ + tc.id.String(): false, + }, + }) + require.NoError(t, err) + return &db, &api.Logger, &user }() diff --git a/codersdk/notifications.go b/codersdk/notifications.go index 92870b4dd2b95..c1602c19f4260 100644 --- a/codersdk/notifications.go +++ b/codersdk/notifications.go @@ -17,14 +17,15 @@ type NotificationsSettings struct { } type NotificationTemplate struct { - ID uuid.UUID `json:"id" format:"uuid"` - Name string `json:"name"` - TitleTemplate string `json:"title_template"` - BodyTemplate string `json:"body_template"` - Actions string `json:"actions" format:""` - Group string `json:"group"` - Method string `json:"method"` - Kind string `json:"kind"` + ID uuid.UUID `json:"id" format:"uuid"` + Name string `json:"name"` + TitleTemplate string `json:"title_template"` + BodyTemplate string `json:"body_template"` + Actions string `json:"actions" format:""` + Group string `json:"group"` + Method string `json:"method"` + Kind string `json:"kind"` + EnabledByDefault bool `json:"enabled_by_default"` } type NotificationMethodsResponse struct { diff --git a/docs/admin/security/audit-logs.md b/docs/admin/security/audit-logs.md index 430d03adb0667..85e3a17e34665 100644 --- a/docs/admin/security/audit-logs.md +++ b/docs/admin/security/audit-logs.md @@ -19,7 +19,7 @@ We track the following resources: | GroupSyncSettings
| |
FieldTracked
auto_create_missing_groupstrue
fieldtrue
legacy_group_name_mappingfalse
mappingtrue
regex_filtertrue
| | HealthSettings
| |
FieldTracked
dismissed_healthcheckstrue
idfalse
| | License
create, delete | |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| -| NotificationTemplate
| |
FieldTracked
actionstrue
body_templatetrue
grouptrue
idfalse
kindtrue
methodtrue
nametrue
title_templatetrue
| +| NotificationTemplate
| |
FieldTracked
actionstrue
body_templatetrue
enabled_by_defaulttrue
grouptrue
idfalse
kindtrue
methodtrue
nametrue
title_templatetrue
| | NotificationsSettings
| |
FieldTracked
idfalse
notifier_pausedtrue
| | OAuth2ProviderApp
| |
FieldTracked
callback_urltrue
created_atfalse
icontrue
idfalse
nametrue
updated_atfalse
| | OAuth2ProviderAppSecret
| |
FieldTracked
app_idfalse
created_atfalse
display_secretfalse
hashed_secretfalse
idfalse
last_used_atfalse
secret_prefixfalse
| diff --git a/docs/reference/api/notifications.md b/docs/reference/api/notifications.md index 21b91182d78fa..0d9b07b3ffce2 100644 --- a/docs/reference/api/notifications.md +++ b/docs/reference/api/notifications.md @@ -146,6 +146,7 @@ curl -X GET http://coder-server:8080/api/v2/notifications/templates/system \ { "actions": "string", "body_template": "string", + "enabled_by_default": true, "group": "string", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", "kind": "string", @@ -166,17 +167,18 @@ curl -X GET http://coder-server:8080/api/v2/notifications/templates/system \ Status Code **200** -| Name | Type | Required | Restrictions | Description | -|--------------------|--------------|----------|--------------|-------------| -| `[array item]` | array | false | | | -| `» actions` | string | false | | | -| `» body_template` | string | false | | | -| `» group` | string | false | | | -| `» id` | string(uuid) | false | | | -| `» kind` | string | false | | | -| `» method` | string | false | | | -| `» name` | string | false | | | -| `» title_template` | string | false | | | +| Name | Type | Required | Restrictions | Description | +|------------------------|--------------|----------|--------------|-------------| +| `[array item]` | array | false | | | +| `» actions` | string | false | | | +| `» body_template` | string | false | | | +| `» enabled_by_default` | boolean | false | | | +| `» group` | string | false | | | +| `» id` | string(uuid) | false | | | +| `» kind` | string | false | | | +| `» method` | string | false | | | +| `» name` | string | false | | | +| `» title_template` | string | false | | | To perform this operation, you must be authenticated. [Learn more](authentication.md). diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index b6874bc5b1bc9..1704ce9d29ec9 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -3522,6 +3522,7 @@ Git clone makes use of this by parsing the URL from: 'Username for "https://gith { "actions": "string", "body_template": "string", + "enabled_by_default": true, "group": "string", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", "kind": "string", @@ -3533,16 +3534,17 @@ Git clone makes use of this by parsing the URL from: 'Username for "https://gith ### Properties -| Name | Type | Required | Restrictions | Description | -|------------------|--------|----------|--------------|-------------| -| `actions` | string | false | | | -| `body_template` | string | false | | | -| `group` | string | false | | | -| `id` | string | false | | | -| `kind` | string | false | | | -| `method` | string | false | | | -| `name` | string | false | | | -| `title_template` | string | false | | | +| Name | Type | Required | Restrictions | Description | +|----------------------|---------|----------|--------------|-------------| +| `actions` | string | false | | | +| `body_template` | string | false | | | +| `enabled_by_default` | boolean | false | | | +| `group` | string | false | | | +| `id` | string | false | | | +| `kind` | string | false | | | +| `method` | string | false | | | +| `name` | string | false | | | +| `title_template` | string | false | | | ## codersdk.NotificationsConfig diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 4bbeefdf01e09..b72a64c2eeae4 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -279,14 +279,15 @@ var auditableResourcesTypes = map[any]map[string]Action{ "icon": ActionTrack, }, &database.NotificationTemplate{}: { - "id": ActionIgnore, - "name": ActionTrack, - "title_template": ActionTrack, - "body_template": ActionTrack, - "actions": ActionTrack, - "group": ActionTrack, - "method": ActionTrack, - "kind": ActionTrack, + "id": ActionIgnore, + "name": ActionTrack, + "title_template": ActionTrack, + "body_template": ActionTrack, + "actions": ActionTrack, + "group": ActionTrack, + "method": ActionTrack, + "kind": ActionTrack, + "enabled_by_default": ActionTrack, }, &idpsync.OrganizationSyncSettings{}: { "field": ActionTrack, diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 4956de8691ed7..8c4ad595ed487 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1218,6 +1218,7 @@ export interface NotificationTemplate { readonly group: string; readonly method: string; readonly kind: string; + readonly enabled_by_default: boolean; } // From codersdk/deployment.go diff --git a/site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx b/site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx index 6a89714edf877..d10a5c853e56a 100644 --- a/site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx +++ b/site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx @@ -105,7 +105,7 @@ export const NotificationsPage: FC = () => { {Object.entries(templatesByGroup.data).map(([group, templates]) => { const allDisabled = templates.some((tpl) => { - return disabledPreferences.data[tpl.id] === true; + return notificationIsDisabled(disabledPreferences.data, tpl); }); return ( @@ -150,6 +150,11 @@ export const NotificationsPage: FC = () => { const label = methodLabels[method]; const isLastItem = i === templates.length - 1; + const disabled = notificationIsDisabled( + disabledPreferences.data, + tmpl, + ); + return ( @@ -157,7 +162,7 @@ export const NotificationsPage: FC = () => { { await updatePreferences.mutateAsync({ template_disabled_map: { @@ -207,6 +212,16 @@ export const NotificationsPage: FC = () => { export default NotificationsPage; +function notificationIsDisabled( + disabledPreferences: Record, + tmpl: NotificationTemplate, +): boolean { + return ( + (!tmpl.enabled_by_default && disabledPreferences[tmpl.id] === undefined) || + !!disabledPreferences[tmpl.id] + ); +} + function selectDisabledPreferences(data: NotificationPreference[]) { return data.reduce( (acc, pref) => { diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 33d04cb23e60c..e15377de05430 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -4052,6 +4052,7 @@ export const MockNotificationTemplates: TypesGen.NotificationTemplate[] = [ group: "Workspace Events", method: "webhook", kind: "system", + enabled_by_default: true, }, { id: "f517da0b-cdc9-410f-ab89-a86107c420ed", @@ -4064,6 +4065,7 @@ export const MockNotificationTemplates: TypesGen.NotificationTemplate[] = [ group: "Workspace Events", method: "smtp", kind: "system", + enabled_by_default: true, }, { id: "f44d9314-ad03-4bc8-95d0-5cad491da6b6", @@ -4076,6 +4078,7 @@ export const MockNotificationTemplates: TypesGen.NotificationTemplate[] = [ group: "User Events", method: "", kind: "system", + enabled_by_default: true, }, { id: "4e19c0ac-94e1-4532-9515-d1801aa283b2", @@ -4088,6 +4091,7 @@ export const MockNotificationTemplates: TypesGen.NotificationTemplate[] = [ group: "User Events", method: "", kind: "system", + enabled_by_default: true, }, { id: "0ea69165-ec14-4314-91f1-69566ac3c5a0", @@ -4100,6 +4104,7 @@ export const MockNotificationTemplates: TypesGen.NotificationTemplate[] = [ group: "Workspace Events", method: "smtp", kind: "system", + enabled_by_default: true, }, { id: "c34a0c09-0704-4cac-bd1c-0c0146811c2b", @@ -4112,6 +4117,7 @@ export const MockNotificationTemplates: TypesGen.NotificationTemplate[] = [ group: "Workspace Events", method: "smtp", kind: "system", + enabled_by_default: true, }, { id: "51ce2fdf-c9ca-4be1-8d70-628674f9bc42", @@ -4124,6 +4130,7 @@ export const MockNotificationTemplates: TypesGen.NotificationTemplate[] = [ group: "Workspace Events", method: "webhook", kind: "system", + enabled_by_default: true, }, ]; From 5c3ac59d7bd4ae2fdd725ddd96c7c0ca5e9b29dc Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 10 Jan 2025 15:14:39 +0000 Subject: [PATCH 2/7] chore: run 'make gen' and change error message --- coderd/database/dump.sql | 4 +++- ...3_allow_disabling_notification_templates_by_default.up.sql | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 701319801a263..ebf793c47d731 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -355,6 +355,8 @@ BEGIN RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; END IF; + -- Fails if the notification template is disabled by default and the + -- user hasn't explicitly enabled it. IF (NOT EXISTS (SELECT 1 FROM notification_preferences WHERE user_id = NEW.user_id @@ -363,7 +365,7 @@ BEGIN FROM notification_templates WHERE id = NEW.notification_template_id AND enabled_by_default = FALSE)) THEN - RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; + RAISE EXCEPTION 'cannot enqueue message: user has not enabled this notification'; END IF; RETURN NEW; diff --git a/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql b/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql index b2ff7b4ed2e87..a6b3f39dbfa4d 100644 --- a/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql +++ b/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql @@ -23,7 +23,7 @@ BEGIN FROM notification_templates WHERE id = NEW.notification_template_id AND enabled_by_default = FALSE)) THEN - RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; + RAISE EXCEPTION 'cannot enqueue message: user has not enabled this notification'; END IF; RETURN NEW; From 013a626fafbff848c5806119edbbcb3c6c16f87c Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 10 Jan 2025 16:42:59 +0000 Subject: [PATCH 3/7] chore: add test --- coderd/database/dump.sql | 5 ++-- ...g_notification_templates_by_default.up.sql | 15 ++++++++++-- coderd/notifications/notifications_test.go | 24 +++++++++++++++++++ 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index ebf793c47d731..58f45012dffdd 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -359,13 +359,14 @@ BEGIN -- user hasn't explicitly enabled it. IF (NOT EXISTS (SELECT 1 FROM notification_preferences - WHERE user_id = NEW.user_id + WHERE disabled = FALSE + AND user_id = NEW.user_id AND notification_template_id = NEW.notification_template_id)) AND (EXISTS (SELECT 1 FROM notification_templates WHERE id = NEW.notification_template_id AND enabled_by_default = FALSE)) THEN - RAISE EXCEPTION 'cannot enqueue message: user has not enabled this notification'; + RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; END IF; RETURN NEW; diff --git a/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql b/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql index a6b3f39dbfa4d..e66d939533996 100644 --- a/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql +++ b/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql @@ -1,5 +1,15 @@ ALTER TABLE notification_templates ADD COLUMN enabled_by_default boolean DEFAULT TRUE NOT NULL; +-- Disable 'workspace created' notification by default +UPDATE notification_templates +SET enabled_by_default = FALSE +WHERE id = '281fdf73-c6d6-4cbb-8ff5-888baf8a2fff'; + +-- Disable 'workspace manually updated' notification by default +UPDATE notification_templates +SET enabled_by_default = FALSE +WHERE id = 'd089fe7b-d5c5-4c0c-aaf5-689859f7d392'; + CREATE OR REPLACE FUNCTION inhibit_enqueue_if_disabled() RETURNS TRIGGER AS $$ @@ -17,13 +27,14 @@ BEGIN -- user hasn't explicitly enabled it. IF (NOT EXISTS (SELECT 1 FROM notification_preferences - WHERE user_id = NEW.user_id + WHERE disabled = FALSE + AND user_id = NEW.user_id AND notification_template_id = NEW.notification_template_id)) AND (EXISTS (SELECT 1 FROM notification_templates WHERE id = NEW.notification_template_id AND enabled_by_default = FALSE)) THEN - RAISE EXCEPTION 'cannot enqueue message: user has not enabled this notification'; + RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; END IF; RETURN NEW; diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 15f3d90a8a89f..62fa50f453cfa 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -1438,6 +1438,30 @@ func normalizeGoldenWebhook(content []byte) []byte { return content } +func TestDisabledByDefaultBeforeEnqueue(t *testing.T) { + t.Parallel() + + if !dbtestutil.WillUsePostgres() { + t.Skip("This test requires postgres; it is testing business-logic implemented in the database") + } + + // nolint:gocritic // Unit test. + ctx := dbauthz.AsNotifier(testutil.Context(t, testutil.WaitSuperLong)) + store, _ := dbtestutil.NewDB(t) + logger := testutil.Logger(t) + + cfg := defaultNotificationsConfig(database.NotificationMethodSmtp) + enq, err := notifications.NewStoreEnqueuer(cfg, store, defaultHelpers(), logger.Named("enqueuer"), quartz.NewReal()) + require.NoError(t, err) + user := createSampleUser(t, store) + + // We want to try enqueuing a notification on a template that is disabled + // by default. We expect this to fail. + templateID := notifications.TemplateWorkspaceManuallyUpdated + _, err = enq.Enqueue(ctx, user.ID, templateID, map[string]string{}, "test") + require.ErrorIs(t, err, notifications.ErrCannotEnqueueDisabledNotification, "enqueuing did not fail with expected error") +} + // TestDisabledBeforeEnqueue ensures that notifications cannot be enqueued once a user has disabled that notification template func TestDisabledBeforeEnqueue(t *testing.T) { t.Parallel() From fd7ed60e065d1e9000d7a35d11e6c153d83ee3ca Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 13 Jan 2025 12:56:56 +0000 Subject: [PATCH 4/7] chore: create new migration, refactor inhibit logic, change error message --- ...g_notification_templates_by_default.up.sql | 42 +++++++------------ ..._updated_notifications_by_default.down.sql | 9 ++++ ...ly_updated_notifications_by_default.up.sql | 9 ++++ coderd/notifications/enqueuer.go | 2 +- 4 files changed, 34 insertions(+), 28 deletions(-) create mode 100644 coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.down.sql create mode 100644 coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.up.sql diff --git a/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql b/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql index e66d939533996..c215a66068779 100644 --- a/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql +++ b/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql @@ -1,40 +1,28 @@ ALTER TABLE notification_templates ADD COLUMN enabled_by_default boolean DEFAULT TRUE NOT NULL; --- Disable 'workspace created' notification by default -UPDATE notification_templates -SET enabled_by_default = FALSE -WHERE id = '281fdf73-c6d6-4cbb-8ff5-888baf8a2fff'; - --- Disable 'workspace manually updated' notification by default -UPDATE notification_templates -SET enabled_by_default = FALSE -WHERE id = 'd089fe7b-d5c5-4c0c-aaf5-689859f7d392'; - CREATE OR REPLACE FUNCTION inhibit_enqueue_if_disabled() RETURNS TRIGGER AS $$ BEGIN - -- Fail the insertion if the user has disabled this notification. + -- Fail the insertion if one of the following: + -- * the user has disabled this notification. + -- * the notification template is disabled by default and hasn't + -- been explicitly enabled by the user. IF EXISTS (SELECT 1 FROM notification_preferences WHERE disabled = TRUE AND user_id = NEW.user_id - AND notification_template_id = NEW.notification_template_id) THEN - RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; - END IF; - - -- Fails if the notification template is disabled by default and the - -- user hasn't explicitly enabled it. - IF (NOT EXISTS (SELECT 1 - FROM notification_preferences - WHERE disabled = FALSE - AND user_id = NEW.user_id - AND notification_template_id = NEW.notification_template_id)) - AND (EXISTS (SELECT 1 - FROM notification_templates - WHERE id = NEW.notification_template_id - AND enabled_by_default = FALSE)) THEN - RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; + AND notification_template_id = NEW.notification_template_id) + OR (NOT EXISTS (SELECT 1 + FROM notification_preferences + WHERE disabled = FALSE + AND user_id = NEW.user_id + AND notification_template_id = NEW.notification_template_id)) + AND (EXISTS (SELECT 1 + FROM notification_templates + WHERE id = NEW.notification_template_id + AND enabled_by_default = FALSE) ) THEN + RAISE EXCEPTION 'cannot enqueue message: notification is not enabled'; END IF; RETURN NEW; diff --git a/coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.down.sql b/coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.down.sql new file mode 100644 index 0000000000000..4d4910480f0ce --- /dev/null +++ b/coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.down.sql @@ -0,0 +1,9 @@ +-- Enable 'workspace created' notification by default +UPDATE notification_templates +SET enabled_by_default = TRUE +WHERE id = '281fdf73-c6d6-4cbb-8ff5-888baf8a2fff'; + +-- Enable 'workspace manually updated' notification by default +UPDATE notification_templates +SET enabled_by_default = TRUE +WHERE id = 'd089fe7b-d5c5-4c0c-aaf5-689859f7d392'; diff --git a/coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.up.sql b/coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.up.sql new file mode 100644 index 0000000000000..118b1dee0f700 --- /dev/null +++ b/coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.up.sql @@ -0,0 +1,9 @@ +-- Disable 'workspace created' notification by default +UPDATE notification_templates +SET enabled_by_default = FALSE +WHERE id = '281fdf73-c6d6-4cbb-8ff5-888baf8a2fff'; + +-- Disable 'workspace manually updated' notification by default +UPDATE notification_templates +SET enabled_by_default = FALSE +WHERE id = 'd089fe7b-d5c5-4c0c-aaf5-689859f7d392'; diff --git a/coderd/notifications/enqueuer.go b/coderd/notifications/enqueuer.go index 260fcd2675278..df91efe31d003 100644 --- a/coderd/notifications/enqueuer.go +++ b/coderd/notifications/enqueuer.go @@ -20,7 +20,7 @@ import ( ) var ( - ErrCannotEnqueueDisabledNotification = xerrors.New("user has disabled this notification") + ErrCannotEnqueueDisabledNotification = xerrors.New("notification is not enabled") ErrDuplicate = xerrors.New("duplicate notification") ) From 73bc02e6432d4a4405b774dca147c5a91444077b Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 13 Jan 2025 13:04:53 +0000 Subject: [PATCH 5/7] chore: update migration numbers --- ...84_allow_disabling_notification_templates_by_default.down.sql} | 0 ...0284_allow_disabling_notification_templates_by_default.up.sql} | 0 ...reated_and_manually_updated_notifications_by_default.down.sql} | 0 ..._created_and_manually_updated_notifications_by_default.up.sql} | 0 4 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000283_allow_disabling_notification_templates_by_default.down.sql => 000284_allow_disabling_notification_templates_by_default.down.sql} (100%) rename coderd/database/migrations/{000283_allow_disabling_notification_templates_by_default.up.sql => 000284_allow_disabling_notification_templates_by_default.up.sql} (100%) rename coderd/database/migrations/{000284_disable_workspace_created_and_manually_updated_notifications_by_default.down.sql => 000285_disable_workspace_created_and_manually_updated_notifications_by_default.down.sql} (100%) rename coderd/database/migrations/{000284_disable_workspace_created_and_manually_updated_notifications_by_default.up.sql => 000285_disable_workspace_created_and_manually_updated_notifications_by_default.up.sql} (100%) diff --git a/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.down.sql b/coderd/database/migrations/000284_allow_disabling_notification_templates_by_default.down.sql similarity index 100% rename from coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.down.sql rename to coderd/database/migrations/000284_allow_disabling_notification_templates_by_default.down.sql diff --git a/coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql b/coderd/database/migrations/000284_allow_disabling_notification_templates_by_default.up.sql similarity index 100% rename from coderd/database/migrations/000283_allow_disabling_notification_templates_by_default.up.sql rename to coderd/database/migrations/000284_allow_disabling_notification_templates_by_default.up.sql diff --git a/coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.down.sql b/coderd/database/migrations/000285_disable_workspace_created_and_manually_updated_notifications_by_default.down.sql similarity index 100% rename from coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.down.sql rename to coderd/database/migrations/000285_disable_workspace_created_and_manually_updated_notifications_by_default.down.sql diff --git a/coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.up.sql b/coderd/database/migrations/000285_disable_workspace_created_and_manually_updated_notifications_by_default.up.sql similarity index 100% rename from coderd/database/migrations/000284_disable_workspace_created_and_manually_updated_notifications_by_default.up.sql rename to coderd/database/migrations/000285_disable_workspace_created_and_manually_updated_notifications_by_default.up.sql From de41561f80346f08d795e5bbfdab8aff676ed9aa Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 13 Jan 2025 13:19:47 +0000 Subject: [PATCH 6/7] chore: run 'make gen' --- coderd/database/dump.sql | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 407c11d160f08..ea210e3ec0849 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -346,27 +346,25 @@ CREATE FUNCTION inhibit_enqueue_if_disabled() RETURNS trigger LANGUAGE plpgsql AS $$ BEGIN - -- Fail the insertion if the user has disabled this notification. + -- Fail the insertion if one of the following: + -- * the user has disabled this notification. + -- * the notification template is disabled by default and hasn't + -- been explicitly enabled by the user. IF EXISTS (SELECT 1 FROM notification_preferences WHERE disabled = TRUE AND user_id = NEW.user_id - AND notification_template_id = NEW.notification_template_id) THEN - RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; - END IF; - - -- Fails if the notification template is disabled by default and the - -- user hasn't explicitly enabled it. - IF (NOT EXISTS (SELECT 1 - FROM notification_preferences - WHERE disabled = FALSE - AND user_id = NEW.user_id - AND notification_template_id = NEW.notification_template_id)) - AND (EXISTS (SELECT 1 - FROM notification_templates - WHERE id = NEW.notification_template_id - AND enabled_by_default = FALSE)) THEN - RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; + AND notification_template_id = NEW.notification_template_id) + OR (NOT EXISTS (SELECT 1 + FROM notification_preferences + WHERE disabled = FALSE + AND user_id = NEW.user_id + AND notification_template_id = NEW.notification_template_id)) + AND (EXISTS (SELECT 1 + FROM notification_templates + WHERE id = NEW.notification_template_id + AND enabled_by_default = FALSE) ) THEN + RAISE EXCEPTION 'cannot enqueue message: notification is not enabled'; END IF; RETURN NEW; @@ -2509,4 +2507,3 @@ ALTER TABLE ONLY workspaces ALTER TABLE ONLY workspaces ADD CONSTRAINT workspaces_template_id_fkey FOREIGN KEY (template_id) REFERENCES templates(id) ON DELETE RESTRICT; - From dd51bd7eb338c6841ef75cfdd2bbb4322f8212d7 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 13 Jan 2025 13:45:02 +0000 Subject: [PATCH 7/7] chore: refactor sql --- coderd/database/dump.sql | 28 +++++++++---------- ...g_notification_templates_by_default.up.sql | 27 +++++++++--------- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index ea210e3ec0849..37e2cd4d764bf 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -350,20 +350,19 @@ BEGIN -- * the user has disabled this notification. -- * the notification template is disabled by default and hasn't -- been explicitly enabled by the user. - IF EXISTS (SELECT 1 - FROM notification_preferences - WHERE disabled = TRUE - AND user_id = NEW.user_id - AND notification_template_id = NEW.notification_template_id) - OR (NOT EXISTS (SELECT 1 - FROM notification_preferences - WHERE disabled = FALSE - AND user_id = NEW.user_id - AND notification_template_id = NEW.notification_template_id)) - AND (EXISTS (SELECT 1 - FROM notification_templates - WHERE id = NEW.notification_template_id - AND enabled_by_default = FALSE) ) THEN + IF EXISTS ( + SELECT 1 FROM notification_templates + LEFT JOIN notification_preferences + ON notification_preferences.notification_template_id = notification_templates.id + AND notification_preferences.user_id = NEW.user_id + WHERE notification_templates.id = NEW.notification_template_id AND ( + -- Case 1: The user has explicitly disabled this template + notification_preferences.disabled = TRUE + OR + -- Case 2: The template is disabled by default AND the user hasn't enabled it + (notification_templates.enabled_by_default = FALSE AND notification_preferences.notification_template_id IS NULL) + ) + ) THEN RAISE EXCEPTION 'cannot enqueue message: notification is not enabled'; END IF; @@ -2507,3 +2506,4 @@ ALTER TABLE ONLY workspaces ALTER TABLE ONLY workspaces ADD CONSTRAINT workspaces_template_id_fkey FOREIGN KEY (template_id) REFERENCES templates(id) ON DELETE RESTRICT; + diff --git a/coderd/database/migrations/000284_allow_disabling_notification_templates_by_default.up.sql b/coderd/database/migrations/000284_allow_disabling_notification_templates_by_default.up.sql index c215a66068779..462d859d95be3 100644 --- a/coderd/database/migrations/000284_allow_disabling_notification_templates_by_default.up.sql +++ b/coderd/database/migrations/000284_allow_disabling_notification_templates_by_default.up.sql @@ -8,20 +8,19 @@ BEGIN -- * the user has disabled this notification. -- * the notification template is disabled by default and hasn't -- been explicitly enabled by the user. - IF EXISTS (SELECT 1 - FROM notification_preferences - WHERE disabled = TRUE - AND user_id = NEW.user_id - AND notification_template_id = NEW.notification_template_id) - OR (NOT EXISTS (SELECT 1 - FROM notification_preferences - WHERE disabled = FALSE - AND user_id = NEW.user_id - AND notification_template_id = NEW.notification_template_id)) - AND (EXISTS (SELECT 1 - FROM notification_templates - WHERE id = NEW.notification_template_id - AND enabled_by_default = FALSE) ) THEN + IF EXISTS ( + SELECT 1 FROM notification_templates + LEFT JOIN notification_preferences + ON notification_preferences.notification_template_id = notification_templates.id + AND notification_preferences.user_id = NEW.user_id + WHERE notification_templates.id = NEW.notification_template_id AND ( + -- Case 1: The user has explicitly disabled this template + notification_preferences.disabled = TRUE + OR + -- Case 2: The template is disabled by default AND the user hasn't enabled it + (notification_templates.enabled_by_default = FALSE AND notification_preferences.notification_template_id IS NULL) + ) + ) THEN RAISE EXCEPTION 'cannot enqueue message: notification is not enabled'; END IF;