- Notifications
You must be signed in to change notification settings - Fork905
fix: ensure reason present for workspace autoupdated notification#17935
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…autoupdatedFixes#17930Update the `WorkspaceAutoUpdated` notification to only display thereason if it is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 | AddedTemplateWorkspaceAutoUpdated_NoMessage test case |
| coderd/database/migrations/000328_fix_workspace_update_reason.up.sql | Updatedbody_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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Something like "No reason given"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That could work, although I think it'd be good to find out why we don't specify a reason.
ethanndicksonMay 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
maybe justNone /None provided?
DanielleMaywoodMay 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ofNone provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If that's the case, "template version updated" would make more sense than "none provided", no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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}}).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK 👍
I've updated the PR to conditionally check the argument provided to the template instead of making a change to the template itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
1267c9c intomainUh oh!
There was an error while loading.Please reload this page.
Fixes#17930
Update the
WorkspaceAutoUpdatednotification to only display the reason if it is present.