From f69cf07d3153c65f30e335b1e3142b5a9849e06c Mon Sep 17 00:00:00 2001 From: Misha Tomilov Date: Thu, 6 Feb 2025 12:46:10 +0100 Subject: [PATCH] Move email-sending code into EmailService --- h/services/__init__.py | 2 + h/services/email.py | 41 +++++++++++++ h/tasks/mailer.py | 22 ++----- tests/common/fixtures/services.py | 7 +++ tests/unit/h/services/email_test.py | 94 +++++++++++++++++++++++++++++ tests/unit/h/tasks/mailer_test.py | 75 +++-------------------- 6 files changed, 157 insertions(+), 84 deletions(-) create mode 100644 h/services/email.py create mode 100644 tests/unit/h/services/email_test.py diff --git a/h/services/__init__.py b/h/services/__init__.py index 3ed27124858..a48d0f253a1 100644 --- a/h/services/__init__.py +++ b/h/services/__init__.py @@ -10,6 +10,7 @@ BulkGroupService, BulkLMSStatsService, ) +from h.services.email import EmailService from h.services.job_queue import JobQueueService from h.services.subscription import SubscriptionService @@ -161,3 +162,4 @@ def includeme(config): # pragma: no cover config.register_service_factory( "h.services.analytics.analytics_service_factory", name="analytics" ) + config.register_service_factory("h.services.email.factory", iface=EmailService) diff --git a/h/services/email.py b/h/services/email.py new file mode 100644 index 00000000000..f51c6930ed6 --- /dev/null +++ b/h/services/email.py @@ -0,0 +1,41 @@ +import smtplib + +import pyramid_mailer +import pyramid_mailer.message +from pyramid.request import Request +from pyramid_mailer import IMailer + +from h.tasks.celery import get_task_logger + +logger = get_task_logger(__name__) + + +class EmailService: + """A service for sending emails.""" + + def __init__(self, request: Request, mailer: IMailer) -> None: + self._request = request + self._mailer = mailer + + def send( + self, recipients: list[str], subject: str, body: str, html: str | None = None + ): + email = pyramid_mailer.message.Message( + subject=subject, recipients=recipients, body=body, html=html + ) + if self._request.debug: # pragma: no cover + logger.info("emailing in debug mode: check the `mail/` directory") + try: + self._mailer.send_immediately(email) + except smtplib.SMTPRecipientsRefused as exc: # pragma: no cover + logger.warning( + "Recipient was refused when trying to send an email. Does the user have an invalid email address?", + exc_info=exc, + ) + except smtplib.SMTPException as exc: + raise exc + + +def factory(_context, request: Request) -> EmailService: + mailer = pyramid_mailer.get_mailer(request) + return EmailService(request, mailer) diff --git a/h/tasks/mailer.py b/h/tasks/mailer.py index 55ace8871f2..bf4ee51af2e 100644 --- a/h/tasks/mailer.py +++ b/h/tasks/mailer.py @@ -6,15 +6,11 @@ import smtplib -import pyramid_mailer -import pyramid_mailer.message - -from h.tasks.celery import celery, get_task_logger +from h.services.email import EmailService +from h.tasks.celery import celery __all__ = ("send",) -log = get_task_logger(__name__) - @celery.task(bind=True, max_retries=3, acks_late=True) def send(self, recipients, subject, body, html=None): @@ -30,19 +26,9 @@ def send(self, recipients, subject, body, html=None): :param body: the body of the email :type body: unicode """ - email = pyramid_mailer.message.Message( - subject=subject, recipients=recipients, body=body, html=html - ) - mailer = pyramid_mailer.get_mailer(celery.request) - if celery.request.debug: # pragma: no cover - log.info("emailing in debug mode: check the `mail/' directory") + service = celery.request.find_service(EmailService) try: - mailer.send_immediately(email) - except smtplib.SMTPRecipientsRefused as exc: # pragma: no cover - log.warning( - "Recipient was refused when trying to send an email. Does the user have an invalid email address?", - exc_info=exc, - ) + service.send(recipients=recipients, subject=subject, body=body, html=html) except smtplib.socket.error as exc: # Exponential backoff in case the SMTP service is having problems. countdown = self.default_retry_delay * 2**self.request.retries diff --git a/tests/common/fixtures/services.py b/tests/common/fixtures/services.py index 6d008002ff7..adaf2283aed 100644 --- a/tests/common/fixtures/services.py +++ b/tests/common/fixtures/services.py @@ -19,6 +19,7 @@ BulkLMSStatsService, ) from h.services.developer_token import DeveloperTokenService +from h.services.email import EmailService from h.services.flag import FlagService from h.services.group import GroupService from h.services.group_create import GroupCreateService @@ -84,6 +85,7 @@ "user_signup_service", "user_unique_service", "user_update_service", + "email_service", ) @@ -306,3 +308,8 @@ def user_unique_service(mock_service): @pytest.fixture def user_update_service(mock_service): return mock_service(UserUpdateService, name="user_update") + + +@pytest.fixture +def email_service(mock_service): + return mock_service(EmailService) diff --git a/tests/unit/h/services/email_test.py b/tests/unit/h/services/email_test.py new file mode 100644 index 00000000000..ab5465ba3a0 --- /dev/null +++ b/tests/unit/h/services/email_test.py @@ -0,0 +1,94 @@ +import smtplib +from unittest.mock import sentinel + +import pytest + +from h.services.email import EmailService, factory + + +class TestEmailService: + def test_send_creates_email_message(self, email_service, pyramid_mailer): + email_service.send( + recipients=["foo@example.com"], + subject="My email subject", + body="Some text body", + ) + + pyramid_mailer.message.Message.assert_called_once_with( + recipients=["foo@example.com"], + subject="My email subject", + body="Some text body", + html=None, + ) + + def test_send_creates_email_message_with_html_body( + self, email_service, pyramid_mailer + ): + email_service.send( + recipients=["foo@example.com"], + subject="My email subject", + body="Some text body", + html="

An HTML body

", + ) + + pyramid_mailer.message.Message.assert_called_once_with( + recipients=["foo@example.com"], + subject="My email subject", + body="Some text body", + html="

An HTML body

", + ) + + def test_send_dispatches_email_using_request_mailer( + self, email_service, pyramid_mailer, pyramid_request + ): + request_mailer = pyramid_mailer.get_mailer.return_value + message = pyramid_mailer.message.Message.return_value + + email_service.send( + recipients=["foo@example.com"], + subject="My email subject", + body="Some text body", + ) + + request_mailer.send_immediately.assert_called_once_with(message) + + def test_raises_smtplib_exception(self, email_service, pyramid_mailer): + request_mailer = pyramid_mailer.get_mailer.return_value + request_mailer.send_immediately.side_effect = smtplib.SMTPException() + + with pytest.raises(smtplib.SMTPException): + email_service.send( + recipients=["foo@example.com"], + subject="My email subject", + body="Some text body", + ) + + @pytest.fixture + def pyramid_request(self, pyramid_request): + pyramid_request.debug = False + return pyramid_request + + @pytest.fixture + def email_service(self, pyramid_request, pyramid_mailer): + request_mailer = pyramid_mailer.get_mailer.return_value + return EmailService(pyramid_request, request_mailer) + + +class TestFactory: + def test_it(self, pyramid_request, pyramid_mailer, EmailService): + service = factory(sentinel.context, pyramid_request) + + EmailService.assert_called_once_with( + request=pyramid_request, mailer=pyramid_mailer.get_mailer.return_value + ) + + assert service == EmailService.return_value + + @pytest.fixture(autouse=True) + def EmailService(self, patch): + return patch("h.services.email.EmailService") + + +@pytest.fixture(autouse=True) +def pyramid_mailer(patch): + return patch("h.services.email.pyramid_mailer", autospec=True) diff --git a/tests/unit/h/tasks/mailer_test.py b/tests/unit/h/tasks/mailer_test.py index 2d4fdd0cefd..4ecc444b770 100644 --- a/tests/unit/h/tasks/mailer_test.py +++ b/tests/unit/h/tasks/mailer_test.py @@ -6,72 +6,8 @@ from h.tasks import mailer -@mock.patch("h.tasks.mailer.celery", autospec=True) -@mock.patch("h.tasks.mailer.pyramid_mailer", autospec=True) -def test_send_creates_email_message(pyramid_mailer, celery, pyramid_request): - celery.request = pyramid_request - - mailer.send( - recipients=["foo@example.com"], - subject="My email subject", - body="Some text body", - ) - - pyramid_mailer.message.Message.assert_called_once_with( - subject="My email subject", - recipients=["foo@example.com"], - body="Some text body", - html=None, - ) - - -@mock.patch("h.tasks.mailer.celery", autospec=True) -@mock.patch("h.tasks.mailer.pyramid_mailer", autospec=True) -def test_send_creates_email_message_with_html_body( - pyramid_mailer, celery, pyramid_request -): - celery.request = pyramid_request - - mailer.send( - recipients=["foo@example.com"], - subject="My email subject", - body="Some text body", - html="

An HTML body

", - ) - - pyramid_mailer.message.Message.assert_called_once_with( - subject="My email subject", - recipients=["foo@example.com"], - body="Some text body", - html="

An HTML body

", - ) - - -@mock.patch("h.tasks.mailer.celery", autospec=True) -@mock.patch("h.tasks.mailer.pyramid_mailer", autospec=True) -def test_send_dispatches_email_using_request_mailer( - pyramid_mailer, celery, pyramid_request -): - celery.request = pyramid_request - request_mailer = pyramid_mailer.get_mailer.return_value - message = pyramid_mailer.message.Message.return_value - - mailer.send( - recipients=["foo@example.com"], - subject="My email subject", - body="Some text body", - ) - - pyramid_mailer.get_mailer.assert_called_once_with(pyramid_request) - request_mailer.send_immediately.assert_called_once_with(message) - - -@mock.patch("h.tasks.mailer.celery", autospec=True) -@mock.patch("h.tasks.mailer.pyramid_mailer", autospec=True) -def test_send_retries_if_mailing_fails(pyramid_mailer, celery, pyramid_request): - celery.request = pyramid_request - request_mailer = pyramid_mailer.get_mailer.return_value - request_mailer.send_immediately.side_effect = SMTPServerDisconnected() +def test_send_retries_if_mailing_fails(email_service): + email_service.send.side_effect = SMTPServerDisconnected() mailer.send.retry = mock.Mock(spec_set=[]) mailer.send( @@ -87,3 +23,10 @@ def test_send_retries_if_mailing_fails(pyramid_mailer, celery, pyramid_request): def pyramid_request(pyramid_request): pyramid_request.debug = False return pyramid_request + + +@pytest.fixture(autouse=True) +def celery(patch, pyramid_request): + cel = patch("h.tasks.mailer.celery") + cel.request = pyramid_request + return cel