Skip to content

feat: allow notification templates to be disabled by default #16093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions coderd/apidoc/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions coderd/apidoc/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 16 additions & 1 deletion coderd/database/dump.sql

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -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;
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this might've been better suited in its own migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do! 👍


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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pinging @mafredri to review SQL changes

-- 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: could this not be an OR on the other branch?
If we're gonna keep this separate branch I'd rather we raised a more specific error since this one is no longer accurate.

Perhaps notification is not enabled covers both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do!

So make this an OR to the other branch and rename the error message to notification is not enabled?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Yes please, I think that'd be a more precise error message

RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification';
END IF;

RETURN NEW;
END;
$$ LANGUAGE plpgsql;
5 changes: 3 additions & 2 deletions coderd/database/models.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 6 additions & 3 deletions coderd/database/queries.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 9 additions & 8 deletions coderd/notifications.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
}

Expand Down
52 changes: 52 additions & 0 deletions coderd/notifications/notifications_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}()

Expand Down Expand Up @@ -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
}()

Expand Down Expand Up @@ -1410,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()
Expand Down
17 changes: 9 additions & 8 deletions codersdk/notifications.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading
Loading