fix: ensure reason present for workspace autoupdated notification #17935
Conversation
…autoupdated Fixes #17930 Update the `WorkspaceAutoUpdated` notification to only display the reason if it is present.
There was a problem hiding this comment.
Pull Request Overview
This PR updates the WorkspaceAutoUpdated notification to suppress the “Reason for update” block when no reason is provided.
- Adds golden fixtures for both webhook and SMTP outputs when
.Labels.template_version_messageis empty - Introduces a conditional in the SQL migration to append the reason only if present
- Adds a new test case to verify rendering when no message is provided
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceAutoUpdated_NoMessage.json.golden | New JSON fixture showing no reason block |
| coderd/notifications/testdata/rendered-templates/smtp/TemplateWorkspaceAutoUpdated_NoMessage.html.golden | New HTML fixture showing no reason block |
| coderd/notifications/notifications_test.go | Added TemplateWorkspaceAutoUpdated_NoMessage test case |
| coderd/database/migrations/000328_fix_workspace_update_reason.up.sql | Updated body_template to conditionally include the reason block |
| coderd/database/migrations/000328_fix_workspace_update_reason.down.sql | Restores original template with unconditional reason block |
Comments suppressed due to low confidence (1)
coderd/notifications/notifications_test.go:849
- Consider adding a complementary test case where
template_version_messageis non-empty to verify that the reason block renders correctly under the conditional.
name: "TemplateWorkspaceAutoUpdated_NoMessage",
coderd/database/migrations/000328_fix_workspace_update_reason.up.sql
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,4 @@ | |||
| UPDATE notification_templates | |||
| SET body_template = E'Your workspace **{{.Labels.name}}** has been updated automatically to the latest template version ({{.Labels.template_version_name}}).' || | |||
| E'{{if .Labels.template_version_message}}\n\nReason for update: **{{.Labels.template_version_message}}**.{{end}}' | |||
There was a problem hiding this comment.
Would it not be easier to inject a default non-empty value for the reason instead of excluding it?
As a receiver of this notification, if I see this reason sometimes but not other times then it'll be confusing.
There was a problem hiding this comment.
Something like "No reason given"?
There was a problem hiding this comment.
That could work, although I think it'd be good to find out why we don't specify a reason.
There was a problem hiding this comment.
maybe just None / None provided?
There was a problem hiding this comment.
That could work, although I think it'd be good to find out why we don't specify a reason.
When making a new template version, if there is no message specified in the optional field, then this field will end up empty on a workspace auto update.
I'll go with @ethanndickson's suggestion of None provided.
There was a problem hiding this comment.
If that's the case, "template version updated" would make more sense than "none provided", no?
There was a problem hiding this comment.
I'd assume "template version updated" is implied by the prior part of the notification, so I feel like we don't need to repeat that.
Your workspace {{.Labels.name}} has been updated automatically to the latest template version ({{.Labels.template_version_name}}).
|
I've updated the PR to conditionally check the argument provided to the template instead of making a change to the template itself. |
Fixes #17930
Update the
WorkspaceAutoUpdatednotification to only display the reason if it is present.