- Notifications
You must be signed in to change notification settings - Fork928
fix: correct typo in notification template#14108
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
| @@ -0,0 +1,5 @@ | |||
| UPDATE notification_templates | |||
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 don't really see value in restoring the typo in the down migration.
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.
just consistency?
| @@ -0,0 +1,5 @@ | |||
| UPDATE notification_templates | |||
| SET | |||
| body_template = E'Hi {{.UserName}},\n\nNew user account **{{.Labels.created_account_name}}** has been created.' | |||
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.
This was incorrectly set in theinitial migration, let's sneak it in here if you don't mind
| body_template= E'Hi {{.UserName}},\n\nNew user account **{{.Labels.created_account_name}}** has been created.' | |
| body_template= E'Hi {{.UserName}},\n\nNew user account **{{.Labels.created_account_name}}** has been created.', | |
| group='User Events' |
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 would rather keep this idempotent. It does not seem to be an urgent fix that requires modifying existing files, and if there are some scripts checking the content of migration files, and I wouldn't like to make them suffer.
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 you missed the content of the change? The event is a user event, but its group is incorrectly set toWorkspace Events currently.
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.
Oh crap, let me redo it.
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.
76ce460 intomainUh oh!
There was an error while loading.Please reload this page.
Related:#14010
There is a typo in the notification template, unescaped character.