From fac77f956ec28ad04c0c90750e381e69465d2ae1 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 15 Oct 2024 21:02:02 +0200 Subject: [PATCH] fix(coderd/notifications): simplify TemplateWorkspaceManualBuildFailed (#15067) This PR closes #15065. As advised by @mtojek, a template's display name may be set to "", which is not useful in an email notification. We'd like to provide a friendly name for the template, but it also needs to be identifiable. As such, we fall back to template.Name in the case that the template's display name is empty. --- ...uild_failed_notification_template.down.sql | 18 +++++++++++++++ ..._build_failed_notification_template.up.sql | 16 ++++++++++++++ coderd/notifications/notifications_test.go | 6 ++--- .../smtp/TemplateTemplateDeleted.html.golden | 17 +++++--------- ...lateWorkspaceManualBuildFailed.html.golden | 6 ++--- .../TemplateTemplateDeleted.json.golden | 11 +++++----- ...lateWorkspaceManualBuildFailed.json.golden | 5 ++--- .../provisionerdserver/provisionerdserver.go | 22 +++++++++++-------- .../provisionerdserver_test.go | 3 +-- coderd/templates.go | 10 ++++++--- coderd/templates_test.go | 3 +-- 11 files changed, 73 insertions(+), 44 deletions(-) create mode 100644 coderd/database/migrations/000264_manual_build_failed_notification_template.down.sql create mode 100644 coderd/database/migrations/000264_manual_build_failed_notification_template.up.sql diff --git a/coderd/database/migrations/000264_manual_build_failed_notification_template.down.sql b/coderd/database/migrations/000264_manual_build_failed_notification_template.down.sql new file mode 100644 index 0000000000..9a9d5b9c5c --- /dev/null +++ b/coderd/database/migrations/000264_manual_build_failed_notification_template.down.sql @@ -0,0 +1,18 @@ +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'; + +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'; diff --git a/coderd/database/migrations/000264_manual_build_failed_notification_template.up.sql b/coderd/database/migrations/000264_manual_build_failed_notification_template.up.sql new file mode 100644 index 0000000000..b5deebe303 --- /dev/null +++ b/coderd/database/migrations/000264_manual_build_failed_notification_template.up.sql @@ -0,0 +1,16 @@ +UPDATE notification_templates +SET + body_template = E'Hi {{.UserName}},\n\n' || + -- Revert to a single label for the template name: + E'A manual build of the workspace **{{.Labels.name}}** using the template **{{.Labels.template_name}}** failed (version: **{{.Labels.template_version_name}}**).\n\n' || + E'The workspace build was initiated by **{{.Labels.initiator}}**.' +WHERE + id = '2faeee0f-26cb-4e96-821c-85ccb9f71513'; + +UPDATE notification_templates +SET + body_template = E'Hi {{.UserName}},\n\n' || + -- Revert to a single label for the template name: + E'The template **{{.Labels.name}}** was deleted by **{{ .Labels.initiator }}**.\n\n' +WHERE + id = '29a09665-2a4c-403f-9648-54301670e7be'; diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index d6eb480f75..61862826ac 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -870,9 +870,8 @@ func TestNotificationTemplates_Golden(t *testing.T) { UserEmail: "bobby@coder.com", UserUsername: "bobby", Labels: map[string]string{ - "name": "bobby-template", - "display_name": "Bobby's Template", - "initiator": "rob", + "name": "Bobby's Template", + "initiator": "rob", }, }, }, @@ -886,7 +885,6 @@ func TestNotificationTemplates_Golden(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", diff --git a/coderd/notifications/testdata/rendered-templates/smtp/TemplateTemplateDeleted.html.golden b/coderd/notifications/testdata/rendered-templates/smtp/TemplateTemplateDeleted.html.golden index ec7c3cfdce..2ae9ac8e61 100644 --- a/coderd/notifications/testdata/rendered-templates/smtp/TemplateTemplateDeleted.html.golden +++ b/coderd/notifications/testdata/rendered-templates/smtp/TemplateTemplateDeleted.html.golden @@ -1,6 +1,6 @@ From: system@coder.com To: bobby@coder.com -Subject: Template "bobby-template" deleted +Subject: Template "Bobby's Template" deleted Message-Id: 02ee4935-73be-4fa1-a290-ff9999026b13@blush-whale-48 Date: Fri, 11 Oct 2024 09:03:06 +0000 Content-Type: multipart/alternative; boundary=bbe61b741255b6098bb6b3c1f41b885773df633cb18d2a3002b68e4bc9c4 @@ -12,9 +12,7 @@ Content-Type: text/plain; charset=UTF-8 Hi Bobby, -The template bobby-template was deleted by rob. - -The template's display name was Bobby's Template. +The template Bobby's Template was deleted by rob. View templates: http://test.com/templates @@ -29,7 +27,7 @@ Content-Type: text/html; charset=UTF-8 - Template "bobby-template" deleted + Template "Bobby's Template" deleted

- Template "bobby-template" deleted + Template "Bobby's Template" deleted

Hi Bobby,

-

The template bobby-template was deleted by rob<= -/strong>.

- -

The template’s display name was Bobby’s Template.

+

The template Bobby’s Template was deleted by rob.

=20 diff --git a/coderd/notifications/testdata/rendered-templates/smtp/TemplateWorkspaceManualBuildFailed.html.golden b/coderd/notifications/testdata/rendered-templates/smtp/TemplateWorkspaceManualBuildFailed.html.golden index a1711b70ea..1f456a72f4 100644 --- a/coderd/notifications/testdata/rendered-templates/smtp/TemplateWorkspaceManualBuildFailed.html.golden +++ b/coderd/notifications/testdata/rendered-templates/smtp/TemplateWorkspaceManualBuildFailed.html.golden @@ -15,8 +15,7 @@ Hi Bobby, A manual build of the workspace bobby-workspace using the template bobby-te= mplate failed (version: bobby-template-version). -The template's display name was William's Template. The workspace build was= - initiated by joe. +The workspace build was initiated by joe. View build: http://test.com/@mrbobby/bobby-workspace/builds/3 @@ -55,8 +54,7 @@ argin: 8px 0 32px; line-height: 1.5;"> he template bobby-template failed (version: bobby-= template-version).

-

The template’s display name was William’s Template. The workspace build was initiated by joe.

+

The workspace build was initiated by joe.

=20 diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeleted.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeleted.json.golden index 4a97b3a14f..4390a3ddfb 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeleted.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateTemplateDeleted.json.golden @@ -16,14 +16,13 @@ } ], "labels": { - "display_name": "Bobby's Template", "initiator": "rob", - "name": "bobby-template" + "name": "Bobby's Template" }, "data": null }, - "title": "Template \"bobby-template\" deleted", - "title_markdown": "Template \"bobby-template\" deleted", - "body": "Hi Bobby,\n\nThe template bobby-template was deleted by rob.\n\nThe template's display name was Bobby's Template.", - "body_markdown": "Hi Bobby,\n\nThe template **bobby-template** was deleted by **rob**.\n\nThe template's display name was **Bobby's Template**." + "title": "Template \"Bobby's Template\" deleted", + "title_markdown": "Template \"Bobby's Template\" deleted", + "body": "Hi Bobby,\n\nThe template Bobby's Template was deleted by rob.", + "body_markdown": "Hi Bobby,\n\nThe template **Bobby's Template** was deleted by **rob**.\n\n" } \ No newline at end of file diff --git a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManualBuildFailed.json.golden b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManualBuildFailed.json.golden index 8fe2b8b86f..6b406a1928 100644 --- a/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManualBuildFailed.json.golden +++ b/coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceManualBuildFailed.json.golden @@ -18,7 +18,6 @@ "labels": { "initiator": "joe", "name": "bobby-workspace", - "template_display_name": "William's Template", "template_name": "bobby-template", "template_version_name": "bobby-template-version", "workspace_build_number": "3", @@ -28,6 +27,6 @@ }, "title": "Workspace \"bobby-workspace\" manual build failed", "title_markdown": "Workspace \"bobby-workspace\" manual build failed", - "body": "Hi Bobby,\n\nA manual build of the workspace bobby-workspace using the template bobby-template failed (version: bobby-template-version).\n\nThe template's display name was William's Template. The workspace build was initiated by joe.", - "body_markdown": "Hi Bobby,\n\nA manual build of the workspace **bobby-workspace** using the template **bobby-template** failed (version: **bobby-template-version**).\n\nThe template's display name was **William's Template**. The workspace build was initiated by **joe**." + "body": "Hi Bobby,\n\nA manual build of the workspace bobby-workspace using the template bobby-template failed (version: bobby-template-version).\n\nThe workspace build was initiated by joe.", + "body_markdown": "Hi Bobby,\n\nA manual build of the workspace **bobby-workspace** using the template **bobby-template** failed (version: **bobby-template-version**).\n\nThe workspace build was initiated by **joe**." } \ No newline at end of file diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 0fc89b239a..e81aa02f0c 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1124,16 +1124,20 @@ func (s *server) notifyWorkspaceManualBuildFailed(ctx context.Context, workspace } for _, templateAdmin := range templateAdmins { + templateNameLabel := template.DisplayName + if templateNameLabel == "" { + templateNameLabel = template.Name + } + labels := map[string]string{ + "name": workspace.Name, + "template_name": templateNameLabel, + "template_version_name": templateVersion.Name, + "initiator": build.InitiatorByUsername, + "workspace_owner_username": workspaceOwner.Username, + "workspace_build_number": strconv.Itoa(int(build.BuildNumber)), + } if _, err := s.NotificationsEnqueuer.Enqueue(ctx, templateAdmin.ID, notifications.TemplateWorkspaceManualBuildFailed, - map[string]string{ - "name": workspace.Name, - "template_name": template.Name, - "template_display_name": template.DisplayName, - "template_version_name": templateVersion.Name, - "initiator": build.InitiatorByUsername, - "workspace_owner_username": workspaceOwner.Username, - "workspace_build_number": strconv.Itoa(int(build.BuildNumber)), - }, "provisionerdserver", + labels, "provisionerdserver", // Associate this notification with all the related entities. workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, ); err != nil { diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 32ae2efa64..c6c5613f97 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1859,8 +1859,7 @@ func TestNotifications(t *testing.T) { assert.Contains(t, notifEnq.Sent[0].Targets, workspace.OrganizationID) assert.Contains(t, notifEnq.Sent[0].Targets, user.ID) assert.Equal(t, workspace.Name, notifEnq.Sent[0].Labels["name"]) - assert.Equal(t, template.Name, notifEnq.Sent[0].Labels["template_name"]) - assert.Equal(t, template.DisplayName, notifEnq.Sent[0].Labels["template_display_name"]) + assert.Equal(t, template.DisplayName, notifEnq.Sent[0].Labels["template_name"]) assert.Equal(t, version.Name, notifEnq.Sent[0].Labels["template_version_name"]) assert.Equal(t, user.Username, notifEnq.Sent[0].Labels["initiator"]) assert.Equal(t, user.Username, notifEnq.Sent[0].Labels["workspace_owner_username"]) diff --git a/coderd/templates.go b/coderd/templates.go index 01c2a056bb..907a4d1265 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -135,11 +135,15 @@ func (api *API) notifyTemplateDeleted(ctx context.Context, template database.Tem return } + templateNameLabel := template.DisplayName + if templateNameLabel == "" { + templateNameLabel = template.Name + } + if _, err := api.NotificationsEnqueuer.Enqueue(ctx, receiverID, notifications.TemplateTemplateDeleted, map[string]string{ - "name": template.Name, - "display_name": template.DisplayName, - "initiator": initiator.Username, + "name": templateNameLabel, + "initiator": initiator.Username, }, "api-templates-delete", // Associate this notification with all the related entities. template.ID, template.OrganizationID, diff --git a/coderd/templates_test.go b/coderd/templates_test.go index 4273169d4c..c1f1f8f1bb 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -1456,8 +1456,7 @@ func TestTemplateNotifications(t *testing.T) { require.Contains(t, notifiedUsers, n.UserID) require.Contains(t, n.Targets, template.ID) require.Contains(t, n.Targets, template.OrganizationID) - require.Equal(t, n.Labels["name"], template.Name) - require.Equal(t, n.Labels["display_name"], template.DisplayName) + require.Equal(t, n.Labels["name"], template.DisplayName) require.Equal(t, n.Labels["initiator"], coderdtest.FirstUserParams.Username) } })