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

Replace django-mailer with django-yubin #1231

Merged
merged 2 commits into from
May 31, 2024
Merged

Replace django-mailer with django-yubin #1231

merged 2 commits into from
May 31, 2024

Conversation

pi-sigma
Copy link
Contributor

@pi-sigma pi-sigma commented May 30, 2024

Reverting the previous change in #1090

@pi-sigma pi-sigma marked this pull request as ready for review May 30, 2024 14:17
@pi-sigma pi-sigma requested a review from swrichards May 30, 2024 14:19
class DailyFailingEmailDigestTestCase(TestCase):
def test_no_recipients_configured(self):
config = SiteConfiguration.get_solo()
config.recipients_email_digest = []
config.save()

with freeze_time("2024-01-01T12:00:00"):
make_message_log(["[email protected]"], "failed message", RESULT_FAILURE)
message = Message.objects.create(
Copy link
Contributor

Choose a reason for hiding this comment

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

My sense is that it would make more sense to use Django's send_mail rather than manually creating the artifacts we expect django-yubin to create as a consequence of that call.

We should also double-check we've configured the correct testing backends where yubin expects them. The Django docs say that EMAIL_BACKEND is automatically set to "django.core.mail.backends.locmem.EmailBackend" during testing , but I believe what we want here is for the EMAIL_BACKEND to be explicitly set to "django_yubin.backends.QueuedEmailBackend" and for MAILSER_USE_BACKEND to be set to the in-memory variety.

Copy link
Member

@alextreme alextreme left a comment

Choose a reason for hiding this comment

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

This seems okay but isn't fully complete. Note that only the send_mail task seems to be added automatically (this will need to be verified after merging and deploying to the testenvironment).

The management commands are gone with Yubin 2.x so afaik we need to add all yubin tasks we need to CELERY_BEAT_SCHEDULE (we don't want to manually add celery beat tasks individually per install)

https://django-yubin.readthedocs.io/en/latest/queue.html#tasks

For retry_emails (1x per hour) and delete_old_emails (1x per day) we also have (less frequent) cronjobs

You don’t usually need to create a send_email task, Yubin email backend does it automatically. For retry_emails and delete_old_emails, you can use Celery Beat to schedule periodic task.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.22%. Comparing base (0c300a4) to head (66da7d2).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1231      +/-   ##
===========================================
- Coverage    95.22%   95.22%   -0.01%     
===========================================
  Files          968      968              
  Lines        35254    35245       -9     
===========================================
- Hits         33571    33562       -9     
  Misses        1683     1683              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pi-sigma pi-sigma marked this pull request as draft May 31, 2024 10:32
@alextreme
Copy link
Member

Can be merged in develop, however PR is still in draft

@pi-sigma pi-sigma marked this pull request as ready for review May 31, 2024 12:04
@pi-sigma pi-sigma marked this pull request as draft May 31, 2024 12:12
@pi-sigma pi-sigma marked this pull request as ready for review May 31, 2024 12:13
@pi-sigma pi-sigma merged commit 52816c1 into develop May 31, 2024
18 checks passed
@pi-sigma pi-sigma deleted the django-yubin branch May 31, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants