diff --git a/h/services/email.py b/h/services/email.py index cc5a4737c83..043a170b779 100644 --- a/h/services/email.py +++ b/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: diff --git a/h/services/user_signup.py b/h/services/user_signup.py index 1550663b9de..fc0ad0a3ee5 100644 --- a/h/services/user_signup.py +++ b/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): diff --git a/h/tasks/mailer.py b/h/tasks/mailer.py index 8c0a2e0a55f..877767f0b3e 100644 --- a/h/tasks/mailer.py +++ b/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)) diff --git a/tests/unit/h/services/email_test.py b/tests/unit/h/services/email_test.py index 3d96f80e822..f73e5e293e2 100644 --- a/tests/unit/h/services/email_test.py +++ b/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=["foo@example.com"], - 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=["foo@example.com"], @@ -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=["foo@example.com"], @@ -34,7 +30,7 @@ def test_send_creates_email_message_with_html_body( tag=EmailTag.TEST, html="

An HTML body

", ) - email_service.send(email) + email_service.send(email, log_data) pyramid_mailer.message.Message.assert_called_once_with( recipients=["foo@example.com"], @@ -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=["foo@example.com"], - 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=["foo@example.com"], - 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=["foo@example.com"], - 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=["foo@example.com"], - 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=["foo@example.com"], + 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 diff --git a/tests/unit/h/services/user_signup_test.py b/tests/unit/h/services/user_signup_test.py index d065b7496b7..afc0b269b76 100644 --- a/tests/unit/h/services/user_signup_test.py +++ b/tests/unit/h/services/user_signup_test.py @@ -1,5 +1,5 @@ import datetime -from unittest.mock import sentinel +from unittest.mock import call, create_autospec, sentinel import pytest from sqlalchemy.exc import IntegrityError @@ -7,6 +7,7 @@ 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="foo@bar.com") @@ -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,7 +182,9 @@ 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): @@ -187,7 +192,10 @@ def signup(self, patch): @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") diff --git a/tests/unit/h/tasks/mailer_test.py b/tests/unit/h/tasks/mailer_test.py index f7e869b708e..2dd2132c57b 100644 --- a/tests/unit/h/tasks/mailer_test.py +++ b/tests/unit/h/tasks/mailer_test.py @@ -6,31 +6,7 @@ from h.tasks import mailer -def test_send_without_log_data(email_service): - email_data = { - "recipients": ["foo@example.com"], - "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": ["foo@example.com"], - "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( @@ -38,20 +14,30 @@ def test_send_with_log_data(email_service): ) -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": ["foo@example.com"], "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