Skip to content
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

Implementation of the custom email backend to handle SES events #1994

Merged

Conversation

albertisfu
Copy link
Contributor

This is a custom email backend to handle sending an email with some extra functions before the email is sent.

  • Verifies if the recipient's email address is not banned before sending or storing the message.
  • Stores a message in DB, generates a unique message_id.
  • Verifies if the recipient's email address is under a backoff event waiting period.
  • Verifies if the recipient's email address is small_email_only flagged or if attachments exceed MAX_ATTACHMENT_SIZE, if so send the small email version.
  • Compose messages according to available content types

It's necessary to set the following settings:

  • BASE_BACKEND: The base email backend to use to send emails, in our case: django_ses.SESBackend, for testing, is used: django.core.mail.backends.locmem.EmailBackend
  • MAX_ATTACHMENT_SIZE: The maximum file size in bytes that an attachment can have to be sent.

The backend supports storing only one email address for the recipient, bcc, and cc fields.
The verifications that the backend performs (email banned and backoff events) are not supported for messages with one recipient.

I saw that in general CL messages are sent to only one recipient, however, I found this command:
cl.alerts.management.commands.monitor_pacer

That seems to send emails to multiple recipients in the same message, using this email backend messages will be sent, however since we only store one recipient, verifications for email banned and backoff events are not supported.

One approach to solve this problem could be to change this command to send one message for each recipient, like this one:
cl.users.management.commands.cl_welcome_new_users

cl/users/tests.py Outdated Show resolved Hide resolved
cl/users/models.py Outdated Show resolved Hide resolved
cl/users/models.py Outdated Show resolved Hide resolved
cl/users/models.py Outdated Show resolved Hide resolved
@albertisfu albertisfu changed the base branch from main to 1942_ses_sending_email_notifications March 31, 2022 19:59
Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only did a partial review for now, but a couple high level comments and a few detailed once as well:

  1. I think the backend applies beyond just users, so I'd put it in cl.lib.email_backends.py instead. (Note that I made it plural too, in case someday we have another backend for some reason.)

  2. Should we have an admin page for the Email table?

  3. Wow, tests!

Thanks for this. Looking good.

cl/settings/10-public.py Outdated Show resolved Hide resolved
cl/users/models.py Outdated Show resolved Hide resolved
cl/users/models.py Outdated Show resolved Hide resolved
cl/users/email_handlers.py Outdated Show resolved Hide resolved
cl/users/email_handlers.py Outdated Show resolved Hide resolved
cl/users/email_backend.py Outdated Show resolved Hide resolved
cl/users/email_backend.py Outdated Show resolved Hide resolved
cl/users/email_backend.py Outdated Show resolved Hide resolved
cl/users/email_backend.py Outdated Show resolved Hide resolved
cl/users/email_backend.py Outdated Show resolved Hide resolved
@albertisfu
Copy link
Contributor Author

Thank you so much for your comments!
I've followed your recommendations and applied some improvements.
Now we are using the last release of django-ses 3.0.1!

Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of little stuff. I still don't love some of the deep nesting and long functions you have here. I think we may just decide it's fine, but one opportunity I did see was to simplify the store_message and compose_message functions.

Anyhow, let me know what you think.

cl/users/email_handlers.py Outdated Show resolved Hide resolved
cl/users/email_handlers.py Outdated Show resolved Hide resolved
cl/users/email_handlers.py Outdated Show resolved Hide resolved
cl/lib/email_backends.py Outdated Show resolved Hide resolved
cl/lib/email_backends.py Outdated Show resolved Hide resolved
cl/lib/email_backends.py Outdated Show resolved Hide resolved
cl/users/models.py Outdated Show resolved Hide resolved
cl/users/models.py Outdated Show resolved Hide resolved
cl/users/tests.py Outdated Show resolved Hide resolved
cl/users/tests.py Outdated Show resolved Hide resolved
@albertisfu
Copy link
Contributor Author

Thank you for your comments!

I think these changes are ready, I left some comments above, let me know if there is something more to change.

There's a problem with our black version to parse the new parenthesized context managers #1994 (comment)

albertisfu and others added 4 commits April 6, 2022 16:50
In doing this, MyPy complained that the super class expected an int to
be returned, so I implemented that.
@mlissner
Copy link
Member

mlissner commented Apr 6, 2022

Tests are still going, but this isn't a merge into main, so I'm going to merge. Thanks for the good work here.

@mlissner mlissner merged this pull request into 1942_ses_sending_email_notifications Apr 6, 2022
@mlissner mlissner deleted the ses_custom_email_backend branch April 6, 2022 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants