-
Notifications
You must be signed in to change notification settings - Fork 648
Use minijinja
templates for emails
#11420
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
9ec4a0d
to
0f87495
Compare
rebased to include the new email type from #11419 |
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.
A quick glance, and overall LGTM, thanks! I've also left a few nitpicks more related to unified coding style.
…age::from_template()`
…e::from_template()`
…essage::from_template()`
Thanks for the review! All of the feedback should be addressed now :) |
let email = EmailMessage::from_template("new_token", context); | ||
let email = email.context("Failed to render email template")?; | ||
let result = emails.send(recipient, email).await; | ||
result.context("Failed to send email") |
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.
Nit: use chaining, as is done with others.
let email = EmailMessage::from_template("new_token", context); | |
let email = email.context("Failed to render email template")?; | |
let result = emails.send(recipient, email).await; | |
result.context("Failed to send email") | |
let email = EmailMessage::from_template("new_token", context) | |
.context("Failed to render email template")?; | |
emails | |
.send(recipient, email) | |
.await | |
.context("Failed to send email") |
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.
let email = EmailMessage::from_template("new_token", context); | |
let email = email.context("Failed to render email template")?; | |
let result = emails.send(recipient, email).await; | |
result.context("Failed to send email") | |
let email = EmailMessage::from_template("new_token", context) | |
.context("Failed to render email template")?; | |
emails.send(recipient, email).await | |
.context("Failed to send email") |
I'd be okay with this formatting, but rustfmt seems very keen on giving .await
its own line, which makes it look a bit ugly IMHO 😅
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 change other chained statements in other files to separate statements? I'm fine if they all follow the same style.
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.
These are all just nitpicks, not blockers, so feel free to merge if you want! :D
This PR refactors our email sending code to use
minijinja
templates instead offormat!()
and string concatenation. In the long term this even enables us to add dedicated templates for HTML emails too, or use template inheritance to use consistent email footers, etc.Before:
After (with templates):