Revert "Make log_data required in mailer.send"

This reverts commit c33f557.

Author: mtomilov

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:
+    def send(self, email_data: EmailData, log_data: LogData | None = None) -> None:
         if self._request.debug:  # pragma: no cover
             logger.info("emailing in debug mode: check the `mail/` directory")
         try:
@@ -75,15 +75,16 @@ def send(self, email_data: EmailData, log_data: LogData) -> None:
         except smtplib.SMTPException:
             raise
 
-        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,
-        )
+        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,
+            )
 
 
 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],
+    log_data: dict[str, Any] | None = None,
 ) -> 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))
+    service.send(EmailData(**email_data), LogData(**log_data) if log_data else None)

tests/unit/h/services/email_test.py

@@ -7,10 +7,14 @@
 
 
 class TestEmailService:
-    def test_send_creates_email_message(
-        self, email_data, log_data, email_service, pyramid_mailer
-    ):
-        email_service.send(email_data, log_data)
+    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)
 
         pyramid_mailer.message.Message.assert_called_once_with(
             recipients=["[email protected]"],
@@ -21,7 +25,7 @@ def test_send_creates_email_message(
         )
 
     def test_send_creates_email_message_with_html_body(
-        self, log_data, email_service, pyramid_mailer
+        self, email_service, pyramid_mailer
     ):
         email = EmailData(
             recipients=["[email protected]"],
@@ -30,7 +34,7 @@ def test_send_creates_email_message_with_html_body(
             tag=EmailTag.TEST,
             html="<p>An HTML body</p>",
         )
-        email_service.send(email, log_data)
+        email_service.send(email)
 
         pyramid_mailer.message.Message.assert_called_once_with(
             recipients=["[email protected]"],
@@ -41,32 +45,60 @@ def test_send_creates_email_message_with_html_body(
         )
 
     def test_send_dispatches_email_using_request_mailer(
-        self, email_data, log_data, email_service, pyramid_mailer
+        self, email_service, pyramid_mailer
     ):
         request_mailer = pyramid_mailer.get_mailer.return_value
         message = pyramid_mailer.message.Message.return_value
 
-        email_service.send(email_data, log_data)
+        email = EmailData(
+            recipients=["[email protected]"],
+            subject="My email subject",
+            body="Some text body",
+            tag=EmailTag.TEST,
+        )
+        email_service.send(email)
 
         request_mailer.send_immediately.assert_called_once_with(message)
 
-    def test_raises_smtplib_exception(
-        self, email_data, log_data, email_service, pyramid_mailer
-    ):
+    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()
 
+        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_data, log_data)
+            email_service.send(email)
 
-    def test_send_logging(self, email_data, log_data, email_service, info_caplog):
+    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],
+        )
         email_service.send(email_data, log_data)
 
         assert info_caplog.messages == [
-            f"Sent email: tag={log_data.tag!r}, sender_id={log_data.sender_id}, recipient_ids={log_data.recipient_ids}"
+            f"Sent email: tag={log_data.tag!r}, sender_id={user_id}, recipient_ids={[user_id]}"
         ]
 
-    def test_send_logging_with_extra(self, email_data, email_service, info_caplog):
+    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,
+        )
         user_id = 123
         annotation_id = "annotation_id"
         log_data = LogData(
@@ -81,23 +113,6 @@ def test_send_logging_with_extra(self, email_data, 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,38 +6,52 @@
 from h.tasks import mailer
 
 
-def test_send(email_data, log_data, email_service):
+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"},
+    }
     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_data, log_data, email_service):
+def test_send_retries_if_mailing_fails(email_service):
     email_service.send.side_effect = Exception()
     mailer.send.retry = mock.Mock(wraps=mailer.send.retry)
 
-    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 {
+    email_data = {
         "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)
 
-
-@pytest.fixture
-def log_data():
-    return {"tag": EmailTag.TEST, "sender_id": 123, "recipient_ids": [123]}
+    assert mailer.send.retry.called
 
 
 @pytest.fixture