Backend/emails: Separate email content generation from email queuing#7813
Backend/emails: Separate email content generation from email queuing#7813tristanlabelle wants to merge 7 commits intodevelopfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
What is the reason for wanting to separate out the textual content? Why include the list unsubscribe info? The email content should be basically all the stuff that gets passed into the enqueue function. |
|
@aapeliv There are two sides to the email code: generating emails and sending emails. To send emails you need all the info, but to generate emails the code that generates the semantic content of the email can be decoupled from the code that decides who to send it to and from. We have 3 different code paths that generate email content (system, userless and logged in), they all don't need to know who it comes from or who it goes to, they only need to be able to format the textual content of the email. The list unsubscribe info is an SMTP header, but the same code that generates the email body will know about this information since it'll also need to add the unsubscribe link in the body. That code doesn't need to know about sender/recipients addresses though. I'm aiming to get to something like this shape: # Something happened, tell the user
content = get_email_content(template="user_warning", args) # Doesn't need to deal with email addresses, only produces semantic content (title, body, and the list unsubscribe stuff)
email = Email(sender=..., recipient=user.email, content=content) # This function knows who it comes from/goes to
queue_email(email)Now that I think about it, I might also be able to treat the list unsubscribe stuff separately: content = get_email_content(template="user_warning", args)
email = Email(sender=..., recipient=user.email, content=content)
if url := get_list_unsubscribe_url(event="user_warning"):
email.list_unsubscribe_url = url
queue_email(email)So mainly, I want to be able to implement a |
|
My feeling is that the unit of "email" is the email + all its headers. In the future we may need additional headers or other information, if the |
|
@aapeliv Your feedback made me think it'll be better with two types:
I'll try to define two clearer types.
That header breaks the abstraction I'd like to create, where the knowledge of the template is encapsulated in the Notification -> EmailContent function. I think the header is just for metrics purpose. Do you know if I could change it to use the topic_action instead? |
Our email content generation is coupled with email queuing code, which means that email content generation needs to know about database
Sessionobjects.I'm working towards having all email generating functions (system, userless/security and notification emails) return a common type that includes the title and plaintext+html body, separately from sender/recipient headers. This PR introduces the
EmailContentto this effect, similar toPushNotificationContent, migrates email generating functions to produce it and email queuing code to consume it.Notifications emails are trickier. There's already a
RenderedEmailNotificationbut it's a misnomer because it still requires rendering the string template afterwards. The logic is also more coupled with the model objects. My goal is to removeRenderedEmailNotificationin favor ofEmailContent, but this PR only gets partway there, and currently involves the awkward_get_notification_email_content_and_template_name, because the knowledge of template name leaks to provide the emailsource_data. I'll consider this problem separately.Testing
Ran tests, leaving to CI.
Backend checklist
developif necessary for linear migration historyFor maintainers