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/tasks/mailer.py

@@ -24,11 +24,11 @@
 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.
     """
     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/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