Skip to content

feat(coderd/notifications): improve notification format consistency #14967

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.

Sign up for GitHub

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 20 commits into from
Oct 9, 2024
Merged

feat(coderd/notifications): improve notification format consistency #14967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
70f3486
feat(notifications): Improve notification format consistency
SasSwart Oct 3, 2024
7e31a34
chore(coderd/notifications): regenerate notification testdata from th…
SasSwart Oct 5, 2024
cf3afd4
Merge remote-tracking branch 'origin/main' into jjs/consistent-notifi…
SasSwart Oct 5, 2024
4b85f2b
chore(coderd/database): renumber migration
SasSwart Oct 5, 2024
e8ad3ac
chore(coderd/notifications): regenerate testdata
SasSwart Oct 5, 2024
2a4d740
fix(coderd/notifications): remove duplicate function signature
SasSwart Oct 5, 2024
e741c43
chore: remove redundant escaping in migration
SasSwart Oct 5, 2024
adffe60
chore(coderd/notifications): improve failed test feedback
SasSwart Oct 5, 2024
41ed54a
feat(coderd/database): add new information to the account activated n…
SasSwart Oct 7, 2024
5541331
Merge remote-tracking branch 'origin/main' into jjs/additional-notifi…
SasSwart Oct 8, 2024
fe94f0d
chore(coderd/database): rework migration for legibility
SasSwart Oct 8, 2024
98e7501
feat(coderd): send newly required information to notification templates
SasSwart Oct 8, 2024
d8e00c2
feat(coderd/notifications): provide additional context to workspace n…
SasSwart Oct 8, 2024
d6a339f
fix(coderd/notifications): add a missing call to fmt.Sprintf
SasSwart Oct 8, 2024
920ad31
fix(coderd/notifications): fix oversights in template migration
SasSwart Oct 9, 2024
9e938e5
chore(coderd/provisionerdserver): set the displayname in TestNotifica…
SasSwart Oct 9, 2024
59e57ac
chore(coderd): add more robust testing assertions to TestNotifyDelete…
SasSwart Oct 9, 2024
c907238
chore(coderd/notifications): fix migration indentation
SasSwart Oct 9, 2024
2493556
chore(coderd/notifications): regenerate golden files
SasSwart Oct 9, 2024
19dccc8
Merge remote-tracking branch 'origin/main' into jjs/consistent-notifi…
SasSwart Oct 9, 2024
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
5 changes: 5 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,7 @@ update-golden-files: \
enterprise/tailnet/testdata/.gen-golden \
tailnet/testdata/.gen-golden \
coderd/.gen-golden \
coderd/notifications/.gen-golden \
provisioner/terraform/testdata/.gen-golden
.PHONY: update-golden-files

Expand Down Expand Up @@ -698,6 +699,10 @@ coderd/.gen-golden: $(wildcard coderd/testdata/*/*.golden) $(GO_SRC_FILES) $(wil
go test ./coderd -run="Test.*Golden$$" -update
touch "$@"

coderd/notifications/.gen-golden: $(wildcard coderd/notifications/testdata/*/*.golden) $(GO_SRC_FILES) $(wildcard coderd/notifications/*_test.go)
go test ./coderd/notifications -run="Test.*Golden$$" -update
touch "$@"

provisioner/terraform/testdata/.gen-golden: $(wildcard provisioner/terraform/testdata/*/*.golden) $(GO_SRC_FILES) $(wildcard provisioner/terraform/*_test.go)
go test ./provisioner/terraform -run="Test.*Golden$$" -update
touch "$@"
Expand Down
84 changes: 84 additions & 0 deletions coderd/database/migrations/000262_improve_notification_templates.down.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\nUser account **{{.Labels.suspended_account_name}}** has been suspended.'
WHERE
id = 'b02ddd82-4733-4d02-a2d7-c36f3598997d';

UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\nYour account **{{.Labels.suspended_account_name}}** has been suspended.'
WHERE
id = '6a2f0609-9b69-4d36-a989-9f5925b6cbff';

UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\nUser account **{{.Labels.activated_account_name}}** has been activated.'
WHERE
id = '9f5af851-8408-4e73-a7a1-c6502ba46689';

UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\nYour account **{{.Labels.activated_account_name}}** has been activated.'
WHERE
id = '1a6a6bea-ee0a-43e2-9e7c-eabdb53730e4';

UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\New user account **{{.Labels.created_account_name}}** has been created.'
WHERE
id = '4e19c0ac-94e1-4532-9515-d1801aa283b2';

UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\nUser account **{{.Labels.deleted_account_name}}** has been deleted.'
WHERE
id = 'f44d9314-ad03-4bc8-95d0-5cad491da6b6';

UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}}\n\n' ||
E'The template **{{.Labels.name}}** was deleted by **{{ .Labels.initiator }}**.'
WHERE
id = '29a09665-2a4c-403f-9648-54301670e7be';

UPDATE notification_templates
SET body_template = E'Hi {{.UserName}}\n' ||
E'Your workspace **{{.Labels.name}}** has been updated automatically to the latest template version ({{.Labels.template_version_name}}).\n' ||
E'Reason for update: **{{.Labels.template_version_message}}**'
WHERE
id = 'c34a0c09-0704-4cac-bd1c-0c0146811c2b';

UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}}\n\nYour workspace **{{.Labels.name}}** was deleted.\nThe specified reason was "**{{.Labels.reason}}{{ if .Labels.initiator }} ({{ .Labels.initiator }}){{end}}**".'
WHERE
id = '381df2a9-c0c0-4749-420f-80a9280c66f9';

UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}}\n\nYour workspace **{{.Labels.name}}** was deleted.\nThe specified reason was "**{{.Labels.reason}}{{ if .Labels.initiator }} ({{ .Labels.initiator }}){{end}}**".'
WHERE
id = 'f517da0b-cdc9-410f-ab89-a86107c420ed';

UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}}\n\n' ||
E'Your workspace **{{.Labels.name}}** has been marked as [**dormant**](https://coder.com/docs/templates/schedule#dormancy-threshold-enterprise) because of {{.Labels.reason}}.\n' ||
E'Dormant workspaces are [automatically deleted](https://coder.com/docs/templates/schedule#dormancy-auto-deletion-enterprise) after {{.Labels.timeTilDormant}} of inactivity.\n' ||
E'To prevent deletion, use your workspace with the link below.'
WHERE
id = '0ea69165-ec14-4314-91f1-69566ac3c5a0';

UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}}\n\n' ||
E'Your workspace **{{.Labels.name}}** has been marked for **deletion** after {{.Labels.timeTilDormant}} of [dormancy](https://coder.com/docs/templates/schedule#dormancy-auto-deletion-enterprise) because of {{.Labels.reason}}.\n' ||
E'To prevent deletion, use your workspace with the link below.'
WHERE
id = '51ce2fdf-c9ca-4be1-8d70-628674f9bc42';

UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\nA manual build of the workspace **{{.Labels.name}}** using the template **{{.Labels.template_name}}** failed (version: **{{.Labels.template_version_name}}**).\nThe workspace build was initiated by **{{.Labels.initiator}}**.'
WHERE
id = '2faeee0f-26cb-4e96-821c-85ccb9f71513';
128 changes: 128 additions & 0 deletions coderd/database/migrations/000262_improve_notification_templates.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
-- https://github.com/coder/coder/issues/14893

-- UserAccountSuspended
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
E'User account **{{.Labels.suspended_account_name}}** has been suspended.\n\n' ||
-- Mention the real name of the user who suspended the account:
E'The newly suspended account belongs to **{{.Labels.suspended_account_user_name}}** and was suspended by **{{.Labels.account_suspender_user_name}}**.'
WHERE
id = 'b02ddd82-4733-4d02-a2d7-c36f3598997d';

-- YourAccountSuspended
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
-- Mention who suspended the account:
E'Your account **{{.Labels.suspended_account_name}}** has been suspended by **{{.Labels.account_suspender_user_name}}**.'
WHERE
id = '6a2f0609-9b69-4d36-a989-9f5925b6cbff';

-- UserAccountActivated
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
E'User account **{{.Labels.activated_account_name}}** has been activated.\n\n' ||
-- Mention the real name of the user who activated the account:
E'The newly activated account belongs to **{{.Labels.activated_account_user_name}}** and was activated by **{{.Labels.account_activator_user_name}}**.'
WHERE
id = '9f5af851-8408-4e73-a7a1-c6502ba46689';

-- YourAccountActivated
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
-- Mention who activated the account:
E'Your account **{{.Labels.activated_account_name}}** has been activated by **{{.Labels.account_activator_user_name}}**.'
WHERE
id = '1a6a6bea-ee0a-43e2-9e7c-eabdb53730e4';

-- UserAccountCreated
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' ||
E'New user account **{{.Labels.created_account_name}}** has been created.\n\n' ||
-- Mention the real name of the user who created the account:
E'This new user account was created for **{{.Labels.created_account_user_name}}** by **{{.Labels.account_creator}}**.'
WHERE
id = '4e19c0ac-94e1-4532-9515-d1801aa283b2';

-- UserAccountDeleted
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' ||
E'User account **{{.Labels.deleted_account_name}}** has been deleted.\n\n' ||
-- Mention the real name of the user who deleted the account:
E'The deleted account belonged to **{{.Labels.deleted_account_user_name}}** and was deleted by **{{.Labels.account_deleter_user_name}}**.'
WHERE
id = 'f44d9314-ad03-4bc8-95d0-5cad491da6b6';

-- TemplateDeleted
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a comma
E'The template **{{.Labels.name}}** was deleted by **{{ .Labels.initiator }}**.\n\n' ||
-- Mention template display name:
E'The template''s display name was **{{.Labels.display_name}}**.'
WHERE
id = '29a09665-2a4c-403f-9648-54301670e7be';

-- WorkspaceAutoUpdated
UPDATE notification_templates
SET body_template = E'Hi {{.UserName}},\n\n' || -- Add a comma and a \n
-- Add a \n:
E'Your workspace **{{.Labels.name}}** has been updated automatically to the latest template version ({{.Labels.template_version_name}}).\n\n' ||
E'Reason for update: **{{.Labels.template_version_message}}**.'
WHERE
id = 'c34a0c09-0704-4cac-bd1c-0c0146811c2b';

-- WorkspaceAutoBuildFailed
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a comma
-- Add a \n after:
E'Automatic build of your workspace **{{.Labels.name}}** failed.\n\n' ||
E'The specified reason was "**{{.Labels.reason}}**".'
WHERE
id = '381df2a9-c0c0-4749-420f-80a9280c66f9';

-- WorkspaceDeleted
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a comma
-- Add a \n after:
E'Your workspace **{{.Labels.name}}** was deleted.\n\n' ||
E'The specified reason was "**{{.Labels.reason}}{{ if .Labels.initiator }} ({{ .Labels.initiator }}){{end}}**".'
WHERE
id = 'f517da0b-cdc9-410f-ab89-a86107c420ed';

-- WorkspaceDormant
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- add comma
E'Your workspace **{{.Labels.name}}** has been marked as [**dormant**](https://coder.com/docs/templates/schedule#dormancy-threshold-enterprise) because of {{.Labels.reason}}.\n' ||
E'Dormant workspaces are [automatically deleted](https://coder.com/docs/templates/schedule#dormancy-auto-deletion-enterprise) after {{.Labels.timeTilDormant}} of inactivity.\n' ||
E'To prevent deletion, use your workspace with the link below.'
WHERE
id = '0ea69165-ec14-4314-91f1-69566ac3c5a0';

-- WorkspaceMarkedForDeletion
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- add comma
E'Your workspace **{{.Labels.name}}** has been marked for **deletion** after {{.Labels.timeTilDormant}} of [dormancy](https://coder.com/docs/templates/schedule#dormancy-auto-deletion-enterprise) because of {{.Labels.reason}}.\n' ||
E'To prevent deletion, use your workspace with the link below.'
WHERE
id = '51ce2fdf-c9ca-4be1-8d70-628674f9bc42';

-- WorkspaceManualBuildFailed
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' ||
E'A manual build of the workspace **{{.Labels.name}}** using the template **{{.Labels.template_name}}** failed (version: **{{.Labels.template_version_name}}**).\n\n' ||
-- Mention template display name:
E'The template''s display name was **{{.Labels.template_display_name}}**. ' ||
E'The workspace build was initiated by **{{.Labels.initiator}}**.'
WHERE
id = '2faeee0f-26cb-4e96-821c-85ccb9f71513';
40 changes: 27 additions & 13 deletions coderd/notifications/notifications_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,7 @@ func enumerateAllTemplates(t *testing.T) ([]string, error) {
return out, nil
}

func TestNotificationTemplatesCanRender(t *testing.T) {
func TestNotificationTemplates_Golden(t *testing.T) {
t.Parallel()

if !dbtestutil.WillUsePostgres() {
Expand Down Expand Up @@ -764,7 +764,9 @@ func TestNotificationTemplatesCanRender(t *testing.T) {
payload: types.MessagePayload{
UserName: "Bobby",
Labels: map[string]string{
"created_account_name": "bobby",
"created_account_name": "bobby",
"created_account_user_name": "William Tables",
"account_creator": "rob",
},
},
},
Expand All @@ -774,7 +776,9 @@ func TestNotificationTemplatesCanRender(t *testing.T) {
payload: types.MessagePayload{
UserName: "Bobby",
Labels: map[string]string{
"deleted_account_name": "bobby",
"deleted_account_name": "bobby",
"deleted_account_user_name": "william tables",
"account_deleter_user_name": "rob",
},
},
},
Expand All @@ -784,7 +788,9 @@ func TestNotificationTemplatesCanRender(t *testing.T) {
payload: types.MessagePayload{
UserName: "Bobby",
Labels: map[string]string{
"suspended_account_name": "bobby",
"suspended_account_name": "bobby",
"suspended_account_user_name": "william tables",
"account_suspender_user_name": "rob",
},
},
},
Expand All @@ -794,7 +800,9 @@ func TestNotificationTemplatesCanRender(t *testing.T) {
payload: types.MessagePayload{
UserName: "Bobby",
Labels: map[string]string{
"activated_account_name": "bobby",
"activated_account_name": "bobby",
"activated_account_user_name": "william tables",
"account_activator_user_name": "rob",
},
},
},
Expand All @@ -804,7 +812,8 @@ func TestNotificationTemplatesCanRender(t *testing.T) {
payload: types.MessagePayload{
UserName: "Bobby",
Labels: map[string]string{
"suspended_account_name": "bobby",
"suspended_account_name": "bobby",
"account_suspender_user_name": "rob",
},
},
},
Expand All @@ -814,7 +823,8 @@ func TestNotificationTemplatesCanRender(t *testing.T) {
payload: types.MessagePayload{
UserName: "Bobby",
Labels: map[string]string{
"activated_account_name": "bobby",
"activated_account_name": "bobby",
"account_activator_user_name": "rob",
},
},
},
Expand All @@ -824,8 +834,9 @@ func TestNotificationTemplatesCanRender(t *testing.T) {
payload: types.MessagePayload{
UserName: "Bobby",
Labels: map[string]string{
"name": "bobby-template",
"initiator": "rob",
"name": "bobby-template",
"display_name": "Bobby's Template",
"initiator": "rob",
},
},
},
Expand All @@ -837,6 +848,7 @@ func TestNotificationTemplatesCanRender(t *testing.T) {
Labels: map[string]string{
"name": "bobby-workspace",
"template_name": "bobby-template",
"template_display_name": "William's Template",
"template_version_name": "bobby-template-version",
"initiator": "joe",
"workspace_owner_username": "mrbobby",
Expand Down Expand Up @@ -960,13 +972,15 @@ func TestNotificationTemplatesCanRender(t *testing.T) {
return
}

const hint = "run \"DB=ci make update-golden-files\" and commit the changes"

wantBody, err := os.ReadFile(bodyGoldenFile)
require.NoError(t, err, "open golden file, run \"DB=ci make update-golden-files\" and commit the changes")
require.NoError(t, err, fmt.Sprintf("missing golden notification body file. %s", hint))
wantTitle, err := os.ReadFile(titleGoldenFile)
require.NoError(t, err, "open golden file, run \"DB=ci make update-golden-files\" and commit the changes")
require.NoError(t, err, fmt.Sprintf("missing golden notification title file. %s", hint))

require.Equal(t, string(wantBody), body, "body should be equal")
require.Equal(t, string(wantTitle), title, "title should be equal")
require.Equal(t, string(wantBody), body, fmt.Sprintf("rendered template body does not match golden file. If this is expected, %s", hint))
require.Equal(t, string(wantTitle), title, fmt.Sprintf("rendered template title does not match golden file. If this is expected, %s", hint))
})
}
}
Expand Down
6 changes: 4 additions & 2 deletions coderd/notifications/testdata/rendered-templates/TemplateTemplateDeleted-body.md.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
Hi Bobby
Hi Bobby,

The template **bobby-template** was deleted by **rob**.
The template **bobby-template** was deleted by **rob**.

The template's display name was **Bobby's Template**.
Copy link
Member

Choose a reason for hiding this comment

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

The display name is just a human-readable representation of the template name, so in this case, you don't need to indicate that it is not the real template name.

Just go with:

The template Bobby's Template was deleted by rob.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered about this, but didn't want to assume I could overwrite what was specified.
Will update.

5 changes: 4 additions & 1 deletion coderd/notifications/testdata/rendered-templates/TemplateUserAccountActivated-body.md.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
Hi Bobby,
User account **bobby** has been activated.

User account **bobby** has been activated.

The newly activated account belongs to **william tables** and was activated by **rob**.
Comment on lines +3 to +5
Copy link
Member

Choose a reason for hiding this comment

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

Have we considered a bit different phrase?

The account for bobby has been activated by Rob. This account belongs to William Tables.

4 changes: 3 additions & 1 deletion coderd/notifications/testdata/rendered-templates/TemplateUserAccountCreated-body.md.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
Hi Bobby,

New user account **bobby** has been created.
New user account **bobby** has been created.

This new user account was created for **William Tables** by **rob**.
4 changes: 3 additions & 1 deletion coderd/notifications/testdata/rendered-templates/TemplateUserAccountDeleted-body.md.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
Hi Bobby,

User account **bobby** has been deleted.
User account **bobby** has been deleted.

The deleted account belonged to **william tables** and was deleted by **rob**.
Copy link
Member

Choose a reason for hiding this comment

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

Curious, if we can make notifications TemplateUserAccountDeleted, TemplateUserAccountCreated, TemplateUserAccountActivated and TemplateUserAccountSuspended sound more natural?

Any ideas, @dannykopping? Should we involve Christin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be keen for a professional Wordsmith to have a look if we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, we should involve @EdwardAngert.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can do this in a follow-up. Let's get this merged.

5 changes: 4 additions & 1 deletion coderd/notifications/testdata/rendered-templates/TemplateUserAccountSuspended-body.md.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
Hi Bobby,
User account **bobby** has been suspended.

User account **bobby** has been suspended.

The newly suspended account belongs to **william tables** and was suspended by **rob**.
Loading
Loading