Make log_data required in mailer.send

h/services/email.py

@@ -62,7 +62,7 @@ def __init__(self, request: Request, mailer: IMailer) -> None:
         self._request = request
         self._mailer = mailer
 
-    def send(self, email_data: EmailData, log_data: LogData | None = None) -> None:
+    def send(self, email_data: EmailData, log_data: LogData) -> None:
         if self._request.debug:  # pragma: no cover
             logger.info("emailing in debug mode: check the `mail/` directory")
         try:
@@ -75,16 +75,15 @@ def send(self, email_data: EmailData, log_data: LogData | None = None) -> None:
         except smtplib.SMTPException:
             raise
 
-        if log_data:
-            separator = ", " if log_data.extra_msg else ""
-            logger.info(
-                "Sent email: tag=%r, sender_id=%s, recipient_ids=%s%s%s",
-                log_data.tag,
-                log_data.sender_id,
-                log_data.recipient_ids,
-                separator,
-                log_data.extra_msg,
-            )
+        separator = ", " if log_data.extra_msg else ""
+        logger.info(
+            "Sent email: tag=%r, sender_id=%s, recipient_ids=%s%s%s",
+            log_data.tag,
+            log_data.sender_id,
+            log_data.recipient_ids,
+            separator,
+            log_data.extra_msg,
+        )
 
 
 def factory(_context, request: Request) -> EmailService:

h/services/user_signup.py

@@ -7,6 +7,7 @@
 from h.emails import signup
 from h.models import Activation, User, UserIdentity
 from h.services import SubscriptionService
+from h.services.email import EmailTag, LogData
 from h.services.exceptions import ConflictError
 from h.services.user_password import UserPasswordService
 from h.tasks import mailer as tasks_mailer
@@ -130,7 +131,12 @@ def _require_activation(self, user):
             email=user.email,
             activation_code=user.activation.code,
         )
-        tasks_mailer.send.delay(asdict(email))
+        log_data = LogData(
+            tag=EmailTag.ACTIVATION,
+            sender_id=user.id,
+            recipient_ids=[user.id],
+        )
+        tasks_mailer.send.delay(asdict(email), asdict(log_data))
 
 
 def user_signup_service_factory(_context, request):

h/tasks/mailer.py

@@ -24,11 +24,12 @@
 def send(
     self,  # noqa: ARG001
     email_data: dict[str, Any],
-    log_data: dict[str, Any] | None = None,
+    log_data: dict[str, Any],
 ) -> None:
     """Send an email.
 
     :param email_data: A dictionary containing email data compatible with EmailData class.
+    :param log_data: A dictionary containing log data compatible with LogData class.
     """
     service: EmailService = celery.request.find_service(EmailService)
-    service.send(EmailData(**email_data), LogData(**log_data) if log_data else None)
+    service.send(EmailData(**email_data), LogData(**log_data))

tests/unit/h/services/email_test.py

@@ -7,14 +7,10 @@
 
 
 class TestEmailService:
-    def test_send_creates_email_message(self, email_service, pyramid_mailer):
-        email = EmailData(
-            recipients=["[email protected]"],
-            subject="My email subject",
-            body="Some text body",
-            tag=EmailTag.TEST,
-        )
-        email_service.send(email)
+    def test_send_creates_email_message(
+        self, email_data, log_data, email_service, pyramid_mailer
+    ):
+        email_service.send(email_data, log_data)
 
         pyramid_mailer.message.Message.assert_called_once_with(
             recipients=["[email protected]"],
@@ -25,7 +21,7 @@ def test_send_creates_email_message(self, email_service, pyramid_mailer):
         )
 
     def test_send_creates_email_message_with_html_body(
-        self, email_service, pyramid_mailer
+        self, log_data, email_service, pyramid_mailer
     ):
         email = EmailData(
             recipients=["[email protected]"],
@@ -34,7 +30,7 @@ def test_send_creates_email_message_with_html_body(
             tag=EmailTag.TEST,
             html="<p>An HTML body</p>",
         )
-        email_service.send(email)
+        email_service.send(email, log_data)
 
         pyramid_mailer.message.Message.assert_called_once_with(
             recipients=["[email protected]"],
@@ -45,60 +41,32 @@ def test_send_creates_email_message_with_html_body(
         )
 
     def test_send_dispatches_email_using_request_mailer(
-        self, email_service, pyramid_mailer
+        self, email_data, log_data, email_service, pyramid_mailer
     ):
         request_mailer = pyramid_mailer.get_mailer.return_value
         message = pyramid_mailer.message.Message.return_value
 
-        email = EmailData(
-            recipients=["[email protected]"],
-            subject="My email subject",
-            body="Some text body",
-            tag=EmailTag.TEST,
-        )
-        email_service.send(email)
+        email_service.send(email_data, log_data)
 
         request_mailer.send_immediately.assert_called_once_with(message)
 
-    def test_raises_smtplib_exception(self, email_service, pyramid_mailer):
+    def test_raises_smtplib_exception(
+        self, email_data, log_data, email_service, pyramid_mailer
+    ):
         request_mailer = pyramid_mailer.get_mailer.return_value
         request_mailer.send_immediately.side_effect = smtplib.SMTPException()
 
-        email = EmailData(
-            recipients=["[email protected]"],
-            subject="My email subject",
-            body="Some text body",
-            tag=EmailTag.TEST,
-        )
         with pytest.raises(smtplib.SMTPException):
-            email_service.send(email)
+            email_service.send(email_data, log_data)
 
-    def test_send_logging(self, email_service, info_caplog):
-        email_data = EmailData(
-            recipients=["[email protected]"],
-            subject="My email subject",
-            body="Some text body",
-            tag=EmailTag.TEST,
-        )
-        user_id = 123
-        log_data = LogData(
-            tag=email_data.tag,
-            sender_id=user_id,
-            recipient_ids=[user_id],
-        )
+    def test_send_logging(self, email_data, log_data, email_service, info_caplog):
         email_service.send(email_data, log_data)
 
         assert info_caplog.messages == [
-            f"Sent email: tag={log_data.tag!r}, sender_id={user_id}, recipient_ids={[user_id]}"
+            f"Sent email: tag={log_data.tag!r}, sender_id={log_data.sender_id}, recipient_ids={log_data.recipient_ids}"
         ]
 
-    def test_send_logging_with_extra(self, email_service, info_caplog):
-        email_data = EmailData(
-            recipients=["[email protected]"],
-            subject="My email subject",
-            body="Some text body",
-            tag=EmailTag.TEST,
-        )
+    def test_send_logging_with_extra(self, email_data, email_service, info_caplog):
         user_id = 123
         annotation_id = "annotation_id"
         log_data = LogData(
@@ -113,6 +81,23 @@ def test_send_logging_with_extra(self, email_service, info_caplog):
             f"Sent email: tag={log_data.tag!r}, sender_id={user_id}, recipient_ids={[user_id]}, annotation_id={annotation_id!r}"
         ]
 
+    @pytest.fixture
+    def email_data(self):
+        return EmailData(
+            recipients=["[email protected]"],
+            subject="My email subject",
+            body="Some text body",
+            tag=EmailTag.TEST,
+        )
+
+    @pytest.fixture
+    def log_data(self):
+        return LogData(
+            tag=EmailTag.TEST,
+            sender_id=123,
+            recipient_ids=[123],
+        )
+
     @pytest.fixture
     def pyramid_request(self, pyramid_request):
         pyramid_request.debug = False

tests/unit/h/services/user_signup_test.py

@@ -1,12 +1,13 @@
 import datetime
-from unittest.mock import sentinel
+from unittest.mock import call, create_autospec, sentinel
 
 import pytest
 from sqlalchemy.exc import IntegrityError
 
 from h.models import Activation, User
 from h.services.exceptions import ConflictError
 from h.services.user_signup import UserSignupService, user_signup_service_factory
+from h.tasks import mailer
 
 
 class TestUserSignupService:
@@ -22,7 +23,6 @@ def test_signup_creates_user_in_db(self, db_session, svc):
         db_session.close()
 
         user = db_session.query(User).filter_by(username="foo").one_or_none()
-
         assert user is not None
 
     def test_signup_creates_activation_for_user(self, svc):
@@ -94,10 +94,9 @@ def test_signup_sets_password_using_password_service(
         user_password_service.update_password.assert_called_once_with(user, "wibble")
 
     def test_signup_sends_email(
-        self, svc, signup, tasks_mailer, pyramid_request, asdict
+        self, svc, signup, tasks_mailer, pyramid_request, asdict, LogData
     ):
         signup.generate.return_value = sentinel.email
-        asdict.return_value = sentinel.email_data
 
         user = svc.signup(username="foo", email="[email protected]")
 
@@ -108,8 +107,12 @@ def test_signup_sends_email(
             activation_code=user.activation.code,
         )
 
-        asdict.assert_called_once_with(signup.generate.return_value)
-        tasks_mailer.send.delay.assert_called_once_with(asdict.return_value)
+        asdict.assert_has_calls(
+            [call(signup.generate.return_value), call(LogData.return_value)]
+        )
+        tasks_mailer.send.delay.assert_called_once_with(
+            sentinel.email_data, sentinel.log_data
+        )
 
     def test_signup_does_not_send_email_when_activation_not_required(
         self, svc, signup, tasks_mailer
@@ -179,15 +182,20 @@ def svc(self, pyramid_request, user_password_service, subscription_service):
 
     @pytest.fixture(autouse=True)
     def tasks_mailer(self, patch):
-        return patch("h.services.user_signup.tasks_mailer")
+        mock = patch("h.services.user_signup.tasks_mailer")
+        mock.send.delay = create_autospec(mailer.send.run)
+        return mock
 
     @pytest.fixture(autouse=True)
     def signup(self, patch):
         return patch("h.services.user_signup.signup")
 
     @pytest.fixture(autouse=True)
     def asdict(self, patch):
-        return patch("h.services.user_signup.asdict")
+        return patch(
+            "h.services.user_signup.asdict",
+            side_effect=[sentinel.email_data, sentinel.log_data],
+        )
 
 
 @pytest.mark.usefixtures("user_password_service")
@@ -212,3 +220,8 @@ def test_it(
     @pytest.fixture
     def UserSignupService(self, patch):
         return patch("h.services.user_signup.UserSignupService")
+
+
+@pytest.fixture(autouse=True)
+def LogData(patch):
+    return patch("h.services.user_signup.LogData")

tests/unit/h/tasks/mailer_test.py

@@ -6,52 +6,38 @@
 from h.tasks import mailer
 
 
-def test_send_without_log_data(email_service):
-    email_data = {
-        "recipients": ["[email protected]"],
-        "subject": "My email subject",
-        "body": "Some text body",
-        "tag": EmailTag.TEST,
-    }
-    mailer.send(email_data)
-
-    email_service.send.assert_called_once_with(EmailData(**email_data), None)
-
-
-def test_send_with_log_data(email_service):
-    email_data = {
-        "recipients": ["[email protected]"],
-        "subject": "My email subject",
-        "body": "Some text body",
-        "tag": EmailTag.TEST,
-    }
-    log_data = {
-        "sender_id": 123,
-        "recipient_ids": [456],
-        "tag": EmailTag.TEST,
-        "extra": {"annotation_id": "annotation_id"},
-    }
+def test_send(email_data, log_data, email_service):
     mailer.send(email_data, log_data)
 
     email_service.send.assert_called_once_with(
         EmailData(**email_data), LogData(**log_data)
     )
 
 
-def test_send_retries_if_mailing_fails(email_service):
+def test_send_retries_if_mailing_fails(email_data, log_data, email_service):
     email_service.send.side_effect = Exception()
     mailer.send.retry = mock.Mock(wraps=mailer.send.retry)
 
-    email_data = {
+    with pytest.raises(Exception) as exc_info:  # noqa: PT011
+        mailer.send(email_data, log_data)
+    assert exc_info.type is Exception
+
+    assert mailer.send.retry.called
+
+
+@pytest.fixture
+def email_data():
+    return {
         "recipients": ["[email protected]"],
         "subject": "My email subject",
         "body": "Some text body",
         "tag": EmailTag.TEST,
     }
-    with pytest.raises(Exception):  # noqa: B017, PT011
-        mailer.send(email_data)
 
-    assert mailer.send.retry.called
+
+@pytest.fixture
+def log_data():
+    return {"tag": EmailTag.TEST, "sender_id": 123, "recipient_ids": [123]}
 
 
 @pytest.fixture