diff --git a/conf/celery.py b/conf/celery.py index 11f3e1ef..c2065444 100644 --- a/conf/celery.py +++ b/conf/celery.py @@ -19,6 +19,8 @@ app.autodiscover_tasks(related_name="celery_tasks") app.autodiscover_tasks(["core"], related_name="celery_tasks") +# Also allow messages that are serialized/deserialized using pickle +app.conf.accept_content = ["json", "pickle"] # Define any regular scheduled tasks diff --git a/conf/settings.py b/conf/settings.py index 6cd7dfa2..bd400db0 100644 --- a/conf/settings.py +++ b/conf/settings.py @@ -280,6 +280,14 @@ def _build_redis_url(base_url, db_number, **query_args): CELERY_BROKER_URL = _build_redis_url(REDIS_BASE_URL, REDIS_CELERY_DB, **url_args) CELERY_RESULT_BACKEND = CELERY_BROKER_URL + +CACHES = { + "default": { + "BACKEND": "django.core.cache.backends.redis.RedisCache", + "LOCATION": REDIS_BASE_URL, + } +} + CELERY_TASK_ALWAYS_EAGER = env.bool("CELERY_TASK_ALWAYS_EAGER", False) CELERY_TASK_STORE_EAGER_RESULT = env.bool("CELERY_TASK_STORE_EAGER_RESULT", False) CELERY_TASK_SEND_SENT_EVENT = env.bool("CELERY_TASK_SEND_SENT_EVENT", True) diff --git a/mail/celery_tasks.py b/mail/celery_tasks.py index 3d82d571..d964acfd 100644 --- a/mail/celery_tasks.py +++ b/mail/celery_tasks.py @@ -1,27 +1,32 @@ +import time import urllib.parse -from email.mime.multipart import MIMEMultipart -from email.mime.text import MIMEText + from smtplib import SMTPException from typing import List, MutableMapping, Tuple from celery import Task, shared_task from celery.utils.log import get_task_logger +from contextlib import contextmanager from django.conf import settings +from django.core.cache import cache from django.db import transaction from django.utils import timezone from rest_framework.status import HTTP_207_MULTI_STATUS, HTTP_208_ALREADY_REPORTED from mail import requests as mail_requests from mail.enums import ReceptionStatusEnum, SourceEnum -from mail.libraries.builders import build_licence_data_mail +from mail.libraries.builders import build_email_message, build_licence_data_mail, build_licence_rejected_email_message from mail.libraries.data_processors import build_request_mail_message_dto -from mail.libraries.routing_controller import check_and_route_emails, send, update_mail +from mail.libraries.email_message_dto import EmailMessageDto +from mail.libraries.lite_to_edifact_converter import EdifactValidationError +from mail.libraries.routing_controller import check_and_route_emails, update_mail from mail.libraries.usage_data_decomposition import build_json_payload_from_data_blocks, split_edi_data_by_id from mail.models import LicenceIdMapping, LicencePayload, Mail, UsageData from mail.servers import smtp_send logger = get_task_logger(__name__) + # Send Usage Figures to LITE API def get_lite_api_url(): """The URL for the licence usage callback, from the LITE_API_URL setting. @@ -75,38 +80,106 @@ def _log_error(message, lite_usage_data_id): MAX_ATTEMPTS = 3 RETRY_BACKOFF = 180 +LOCK_EXPIRE = 60 * 10 # secs (10 min) CELERY_SEND_LICENCE_UPDATES_TASK_NAME = "mail.celery_tasks.send_licence_details_to_hmrc" CELERY_MANAGE_INBOX_TASK_NAME = "mail.celery_tasks.manage_inbox" -# Notify Users of Rejected Mail +class SMTPConnectionBusy(SMTPException): + pass + + +@contextmanager +def cache_lock(lock_id): + timeout_at = time.monotonic() + LOCK_EXPIRE - 3 + # cache.add fails if the key already exists. + # return True if lock is acquired, False otherwise + status = cache.add(lock_id, "lock_acquired", LOCK_EXPIRE) + try: + yield status + finally: + if time.monotonic() < timeout_at and status: + cache.delete(lock_id) + + +class SendEmailBaseTask(Task): + def on_failure(self, exc, task_id, args, kwargs, einfo): + message = """ + Maximum attempts for send_email_task exceeded - the task has failed and needs manual inspection. + Args: %s + """ % ( + args, + ) + + # Log the final failure message + logger.critical(message) + + @shared_task( - autoretry_for=(SMTPException,), + autoretry_for=(SMTPConnectionBusy, SMTPException), max_retries=MAX_ATTEMPTS, retry_backoff=RETRY_BACKOFF, + base=SendEmailBaseTask, ) +def send_email_task(message): + """ + Main purpose of this task is to send email. + + We use SMTP to send emails. As we process messages we have a requirement to + send emails from multiple places, because of this we may open multiple SMTP connections. + This results in error if the number of concurrent connections exceed maximum allowed value. + + To manage this reliably we are restricting access to this shared resource using a lock. + This is achieved by deferring all email sending functionality to this task. + Before sending email it first tries to acquire a lock. + - If there are no active connections then it acquires lock and sends email. + In some cases we need to update state which is handled in subtask linked to this task. + - If there is active connection (lock acquisition fails) then it raises an exception + which triggers a retry. + If all retries fail then manual intervention may be required (unlikely) + """ + + global_lock_id = "global_send_email_lock" + + with cache_lock(global_lock_id) as lock_acquired: + if not lock_acquired: + logger.exception("Another SMTP connection is active, will be retried after backing off") + raise SMTPConnectionBusy() + + logger.info("Lock acquired, proceeding to send email") + + try: + smtp_send(message) + except SMTPException: + logger.exception("An unexpected error occurred when sending email -> %s") + raise + + logger.info("Email sent successfully") + + +# Notify Users of Rejected Mail +@shared_task def notify_users_of_rejected_licences(mail_id, mail_response_subject): """If a reply is received with rejected licences this task notifies users of the rejection""" logger.info("Notifying users of rejected licences found in mail with subject %s", mail_response_subject) - try: - multipart_msg = MIMEMultipart() - multipart_msg["From"] = settings.EMAIL_USER - multipart_msg["To"] = ",".join(settings.NOTIFY_USERS) - multipart_msg["Subject"] = "Licence rejected by HMRC" - body = MIMEText(f"Mail (Id: {mail_id}) with subject {mail_response_subject} has rejected licences") - multipart_msg.attach(body) - - smtp_send(multipart_msg) - - except SMTPException: - logger.exception( - "An unexpected error occurred when notifying users of rejected licences, Mail Id: %s, subject: %s", - mail_id, - mail_response_subject, - ) - raise + message_dto = EmailMessageDto( + run_number=None, + sender=settings.EMAIL_USER, + receiver=",".join(settings.NOTIFY_USERS), + date=timezone.now(), + subject="Licence rejected by HMRC", + body=f"Mail (Id: {mail_id}) with subject {mail_response_subject} has rejected licences", + attachment=None, + raw_data=None, + ) + message = build_licence_rejected_email_message(message_dto) + + send_email_task.apply_async( + args=(message,), + serializer="pickle", + ) logger.info("Successfully notified users of rejected licences found in mail with subject %s", mail_response_subject) @@ -125,10 +198,52 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): logger.error(message) +@shared_task +def finalise_sending_spire_licence_details(mail_id, message_dto): + """Subtask that performs follow-up tasks after completing the primary purpose + of sending an email""" + + mail = Mail.objects.get(id=mail_id) + message_dto = EmailMessageDto(*message_dto) + + update_mail(mail, message_dto) + + +@shared_task +def finalise_sending_lite_licence_details(mail_id, message_dto, licence_payload_ids): + """Subtask that performs follow-up tasks after completing the primary purpose + of sending an email""" + + mail = Mail.objects.get(id=mail_id) + message_dto = EmailMessageDto(*message_dto) + + update_mail(mail, message_dto) + + licence_payloads = LicencePayload.objects.filter(id__in=licence_payload_ids, is_processed=False) + references = [item.reference for item in licence_payloads] + + licence_payloads.update(is_processed=True) + + logger.info("Licence payloads with references %s marked as processed", references) + + +class SendLicenceDetailsBaseTask(Task): + def on_failure(self, exc, task_id, args, kwargs, einfo): + message = """ + Maximum attempts for send_licence_details_to_hmrc task exceeded - the task has failed and needs manual inspection. + + Args: %s + """ % ( + args, + ) + logger.error(message) + + @shared_task( - autoretry_for=(SMTPException,), + autoretry_for=(EdifactValidationError,), max_retries=MAX_ATTEMPTS, retry_backoff=RETRY_BACKOFF, + base=SendLicenceDetailsBaseTask, ) def send_licence_details_to_hmrc(): """Sends LITE issued licence details to HMRC""" @@ -162,14 +277,15 @@ def send_licence_details_to_hmrc(): "Created licenceData mail with subject %s for licences [%s]", mail_dto.subject, licence_references ) - send(mail_dto) - update_mail(mail, mail_dto) - - # Mark the payloads as processed - licences.update(is_processed=True) - logger.info("Licence references [%s] marked as processed", licence_references) + message = build_email_message(mail_dto) + licence_payload_ids = [str(licence.id) for licence in licences] + send_email_task.apply_async( + args=(message,), + serializer="pickle", + link=finalise_sending_lite_licence_details.si(mail.id, mail_dto, licence_payload_ids), + ) - except SMTPException: + except EdifactValidationError: logger.exception("An unexpected error occurred when sending LITE licence updates to HMRC -> %s") raise diff --git a/mail/libraries/builders.py b/mail/libraries/builders.py index e103d76b..e0aaf2f0 100644 --- a/mail/libraries/builders.py +++ b/mail/libraries/builders.py @@ -1,6 +1,7 @@ import datetime import json import logging + from email.mime.application import MIMEApplication from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText @@ -274,6 +275,19 @@ def build_email_message(email_message_dto: EmailMessageDto) -> MIMEMultipart: return multipart_msg +def build_licence_rejected_email_message(email_message_dto: EmailMessageDto) -> MIMEMultipart: + logger.info("Building licences rejected notification email message") + multipart_msg = MIMEMultipart() + multipart_msg["From"] = settings.EMAIL_USER + multipart_msg["To"] = ",".join(settings.NOTIFY_USERS) + multipart_msg["Subject"] = email_message_dto.subject + multipart_msg["name"] = email_message_dto.subject + body = MIMEText(email_message_dto.body) + multipart_msg.attach(body) + logger.info("Message headers: %s", multipart_msg.items()) + return multipart_msg + + def _validate_dto(email_message_dto): if email_message_dto is None: raise TypeError("None email_message_dto received!") diff --git a/mail/libraries/routing_controller.py b/mail/libraries/routing_controller.py index 1e415386..303fdf20 100644 --- a/mail/libraries/routing_controller.py +++ b/mail/libraries/routing_controller.py @@ -175,6 +175,8 @@ def send(email_message_dto: EmailMessageDto): def _collect_and_send(mail: Mail): + from mail.celery_tasks import send_email_task, finalise_sending_spire_licence_details + logger.info("Sending Mail [%s] of extract type %s", mail.id, mail.extract_type) message_to_send_dto = to_email_message_dto_from(mail) @@ -185,11 +187,16 @@ def _collect_and_send(mail: Mail): if message_to_send_dto: if message_to_send_dto.receiver != SourceEnum.LITE and message_to_send_dto.subject: - send(message_to_send_dto) - update_mail(mail, message_to_send_dto) + message = build_email_message(message_to_send_dto) + # Schedule a task to send email + send_email_task.apply_async( + args=(message,), + serializer="pickle", + link=finalise_sending_spire_licence_details.si(mail.id, message_to_send_dto), + ) logger.info( - "Mail [%s] routed from [%s] to [%s] with subject %s", + "Scheduled sending of mail [%s] from [%s] to [%s] with subject %s", mail.id, message_to_send_dto.sender, message_to_send_dto.receiver, diff --git a/mail/tests/test_celery_task.py b/mail/tests/test_celery_task.py index d4a3ee53..9ba65974 100644 --- a/mail/tests/test_celery_task.py +++ b/mail/tests/test_celery_task.py @@ -1,22 +1,40 @@ import email.mime.multipart -from unittest import mock - import pytest + +from datetime import datetime, timezone +from parameterized import parameterized +from unittest import mock from django.test import TestCase, override_settings from mail.celery_tasks import manage_inbox, notify_users_of_rejected_licences +from mail.enums import ExtractTypeEnum, ReceptionStatusEnum, SourceEnum +from mail.libraries.routing_controller import check_and_route_emails +from mail.models import LicenceData, Mail +from mail.tests.libraries.client import LiteHMRCTestClient class NotifyUsersOfRejectedMailTests(TestCase): + @parameterized.expand( + [ + # lock_acquired + ([True],), + ([False, True],), + ] + ) @override_settings(EMAIL_USER="test@example.com", NOTIFY_USERS=["notify@example.com"]) # /PS-IGNORE @mock.patch("mail.celery_tasks.smtp_send") - def test_send_success(self, mock_send): - notify_users_of_rejected_licences("123", "CHIEF_SPIRE_licenceReply_202401180900_42557") + @mock.patch("mail.celery_tasks.cache") + def test_send_rejected_notification_email_success(self, lock_acquired, mock_cache, mock_smtp_send): + """Test sending of licence rejected emails without and with retry scenario""" + mock_cache.add.side_effect = lock_acquired + + notify_users_of_rejected_licences.delay("123", "CHIEF_SPIRE_licenceReply_202401180900_42557") - mock_send.assert_called_once() + assert mock_cache.add.call_count == len(lock_acquired) + mock_smtp_send.assert_called_once() - self.assertEqual(len(mock_send.call_args_list), 1) - message = mock_send.call_args[0][0] + self.assertEqual(len(mock_smtp_send.call_args_list), 1) + message = mock_smtp_send.call_args[0][0] self.assertIsInstance(message, email.mime.multipart.MIMEMultipart) expected_headers = { @@ -25,6 +43,7 @@ def test_send_success(self, mock_send): "From": "test@example.com", # /PS-IGNORE "To": "notify@example.com", # /PS-IGNORE "Subject": "Licence rejected by HMRC", + "name": "Licence rejected by HMRC", } self.assertDictEqual(dict(message), expected_headers) @@ -33,7 +52,7 @@ def test_send_success(self, mock_send): self.assertEqual(text_payload.get_payload(), expected_body) -class ManageInboxTests(TestCase): +class ManageInboxTests(LiteHMRCTestClient): @mock.patch("mail.celery_tasks.check_and_route_emails") def test_manage_inbox(self, mock_function): manage_inbox() @@ -45,3 +64,53 @@ def test_error_manage_inbox(self, mock_function): with pytest.raises(Exception) as excinfo: manage_inbox() assert str(excinfo.value) == "Test Error" + + @parameterized.expand( + [ + # lock_acquired + ([True],), + ([False, True],), + ] + ) + @mock.patch("mail.libraries.routing_controller.get_spire_to_dit_mailserver") + @mock.patch("mail.libraries.routing_controller.get_hmrc_to_dit_mailserver") + @mock.patch("mail.celery_tasks.smtp_send") + @mock.patch("mail.celery_tasks.cache") + @mock.patch("mail.libraries.routing_controller.get_email_message_dtos") + def test_sending_of_new_message_from_spire_success( + self, + lock_acquired, + email_dtos, + mock_cache, + mock_smtp_send, + mock_get_hmrc_to_dit_mailserver, + mock_get_spire_to_dit_mailserver, + ): + """Test sending of email when a message from SPIRE is processed without and with retry scenario""" + email_dtos.return_value = [] + mock_cache.add.side_effect = lock_acquired + + # When a new message is processed from inbox it will be created with 'pending' status + pending_mail = Mail.objects.create( + extract_type=ExtractTypeEnum.LICENCE_DATA, + edi_filename=self.licence_data_file_name, + edi_data=self.licence_data_file_body.decode("utf-8"), + status=ReceptionStatusEnum.PENDING, + sent_at=datetime.now(timezone.utc), + ) + LicenceData.objects.create( + mail=pending_mail, + source_run_number=78120, + hmrc_run_number=78120, + source=SourceEnum.SPIRE, + licence_ids=f"{78120}", + ) + + check_and_route_emails() + + # assert that the pending mail is sent and status updated + mail = Mail.objects.get(id=pending_mail.id) + self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_PENDING) + + assert mock_cache.add.call_count == len(lock_acquired) + mock_smtp_send.assert_called_once() diff --git a/mail/tests/test_end_to_end.py b/mail/tests/test_end_to_end.py index 6a6b742c..66955085 100644 --- a/mail/tests/test_end_to_end.py +++ b/mail/tests/test_end_to_end.py @@ -1,4 +1,5 @@ from pathlib import Path +from unittest import mock from urllib.parse import quote import requests @@ -25,7 +26,9 @@ def get_smtp_body(): class EndToEndTests(LiteHMRCTestClient): - def test_send_email_to_hmrc_e2e(self): + @mock.patch("mail.celery_tasks.cache") + def test_send_email_to_hmrc_e2e(self, mock_cache): + mock_cache.add.return_value = True clear_stmp_mailbox() self.client.get(reverse("mail:set_all_to_reply_sent")) self.client.post( @@ -52,7 +55,9 @@ def test_send_email_to_hmrc_e2e(self): @override_settings(CHIEF_SOURCE_SYSTEM=ChiefSystemEnum.ICMS) class ICMSEndToEndTests(testcases.TestCase): - def test_icms_send_email_to_hmrc_fa_oil_e2e(self): + @mock.patch("mail.celery_tasks.cache") + def test_icms_send_email_to_hmrc_fa_oil_e2e(self, mock_cache): + mock_cache.add.return_value = True clear_stmp_mailbox() self.client.get(reverse("mail:set_all_to_reply_sent")) @@ -113,7 +118,9 @@ def test_icms_send_email_to_hmrc_fa_oil_e2e(self): licence_payload: LicencePayload = ld.licence_payloads.first() assert licence_payload.reference == "IMA/2022/00001" - def test_icms_send_email_to_hmrc_fa_dfl_e2e(self): + @mock.patch("mail.celery_tasks.cache") + def test_icms_send_email_to_hmrc_fa_dfl_e2e(self, mock_cache): + mock_cache.add.return_value = True clear_stmp_mailbox() self.client.get(reverse("mail:set_all_to_reply_sent")) @@ -177,7 +184,9 @@ def test_icms_send_email_to_hmrc_fa_dfl_e2e(self): response = self.client.get(f"{reverse('mail:licence')}?id={encoded_reference_code}") self.assertEqual(response.json()["status"], "reply_pending", f"{ref} has incorrect status") - def test_icms_send_email_to_hmrc_fa_sil_e2e(self): + @mock.patch("mail.celery_tasks.cache") + def test_icms_send_email_to_hmrc_fa_sil_e2e(self, mock_cache): + mock_cache.add.return_value = True clear_stmp_mailbox() self.client.get(reverse("mail:set_all_to_reply_sent")) @@ -198,7 +207,9 @@ def test_icms_send_email_to_hmrc_fa_sil_e2e(self): response = self.client.get(f"{reverse('mail:licence')}?id={encoded_reference_code}") self.assertEqual(response.json()["status"], "reply_pending") - def test_icms_send_email_to_hmrc_sanctions_e2e(self): + @mock.patch("mail.celery_tasks.cache") + def test_icms_send_email_to_hmrc_sanctions_e2e(self, mock_cache): + mock_cache.add.return_value = True clear_stmp_mailbox() self.client.get(reverse("mail:set_all_to_reply_sent")) diff --git a/mail/tests/test_licence_to_edifact.py b/mail/tests/test_licence_to_edifact.py index 6f321c54..72ba6559 100644 --- a/mail/tests/test_licence_to_edifact.py +++ b/mail/tests/test_licence_to_edifact.py @@ -53,9 +53,11 @@ def test_single_siel(self): self.assertEqual(result, expected) - @mock.patch("mail.celery_tasks.send") - def test_licence_is_marked_as_processed_after_sending(self, send): - send.return_value = None + @mock.patch("mail.celery_tasks.smtp_send") + @mock.patch("mail.celery_tasks.cache") + def test_licence_is_marked_as_processed_after_sending(self, mock_cache, mock_smtp_send): + mock_cache.add.return_value = True + mock_smtp_send.return_value = None send_licence_details_to_hmrc.delay() self.assertEqual(Mail.objects.count(), 1) diff --git a/mail/tests/test_resend_email.py b/mail/tests/test_resend_email.py index 1ee22a00..8bc9a7f5 100644 --- a/mail/tests/test_resend_email.py +++ b/mail/tests/test_resend_email.py @@ -3,6 +3,7 @@ from django.conf import settings from django.core.management import call_command +from django.test import override_settings from mail.enums import ExtractTypeEnum, ReceptionStatusEnum, SourceEnum from mail.libraries.email_message_dto import EmailMessageDto @@ -14,12 +15,16 @@ class LITEHMRCResendEmailTests(LiteHMRCTestClient): @mock.patch("mail.libraries.routing_controller.get_spire_to_dit_mailserver") @mock.patch("mail.libraries.routing_controller.get_hmrc_to_dit_mailserver") - @mock.patch("mail.libraries.routing_controller.send") + @mock.patch("mail.management.commands.resend_email.send") + @mock.patch("mail.celery_tasks.smtp_send") + @mock.patch("mail.celery_tasks.cache") @mock.patch("mail.libraries.routing_controller.get_email_message_dtos") def test_resend_licence_data_mail_to_hmrc( self, email_dtos, - send_mail, + mock_cache, + mock_smtp_send, + mock_send, mock_get_hmrc_to_dit_mailserver, mock_get_spire_to_dit_mailserver, ): @@ -28,6 +33,8 @@ def test_resend_licence_data_mail_to_hmrc( Initially we setup an email and send it to HMRC and this sets the mail is in the required status. Now the test executes the management command which resends the email """ + mock_cache.add.return_value = True + mock_smtp_send.wraps = lambda x: x source_run_number = 49530 hmrc_run_number = 49543 filename = self.licence_data_file_name @@ -51,7 +58,7 @@ def test_resend_licence_data_mail_to_hmrc( # assert that the pending mail is sent and status updated mail = Mail.objects.get(id=pending_mail.id) - send_mail.assert_called_once() + mock_smtp_send.assert_called_once() self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_PENDING) call_command("resend_email", "--hmrc_run_number", 49543) @@ -62,19 +69,26 @@ def test_resend_licence_data_mail_to_hmrc( self.assertEqual(mail.id, pending_mail.id) self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_PENDING) self.assertEqual(mail.extract_type, ExtractTypeEnum.LICENCE_DATA) - self.assertEqual(send_mail.call_count, 2) + self.assertEqual(mock_send.call_count, 1) + @override_settings(SEND_REJECTED_EMAIL=False) @mock.patch("mail.libraries.routing_controller.get_spire_to_dit_mailserver") @mock.patch("mail.libraries.routing_controller.get_hmrc_to_dit_mailserver") - @mock.patch("mail.libraries.routing_controller.send") + @mock.patch("mail.management.commands.resend_email.send") + @mock.patch("mail.celery_tasks.smtp_send") + @mock.patch("mail.celery_tasks.cache") @mock.patch("mail.libraries.routing_controller.get_email_message_dtos") def test_resend_licence_reply_mail_to_spire( self, email_dtos, - send_mail, + mock_cache, + mock_smtp_send, + mock_send, mock_get_hmrc_to_dit_mailserver, mock_get_spire_to_dit_mailserver, ): + mock_cache.add.return_value = True + mock_smtp_send.wraps = lambda x: x source_run_number = 49530 hmrc_run_number = 49543 filename = self.licence_reply_file_name @@ -118,7 +132,7 @@ def test_resend_licence_reply_mail_to_spire( # assert that the pending mail is sent and status updated mail = Mail.objects.get(id=pending_mail.id) - send_mail.assert_called_once() + mock_smtp_send.assert_called_once() self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_SENT) call_command("resend_email", "--hmrc_run_number", 49543) @@ -129,19 +143,25 @@ def test_resend_licence_reply_mail_to_spire( self.assertEqual(mail.id, pending_mail.id) self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_SENT) self.assertEqual(mail.extract_type, ExtractTypeEnum.LICENCE_REPLY) - send_mail.assert_called_once() + mock_send.assert_called_once() @mock.patch("mail.libraries.routing_controller.get_spire_to_dit_mailserver") @mock.patch("mail.libraries.routing_controller.get_hmrc_to_dit_mailserver") - @mock.patch("mail.libraries.routing_controller.send") + @mock.patch("mail.management.commands.resend_email.send") + @mock.patch("mail.celery_tasks.smtp_send") + @mock.patch("mail.celery_tasks.cache") @mock.patch("mail.libraries.routing_controller.get_email_message_dtos") def test_resend_usage_data_mail_to_spire( self, email_dtos, - send_mail, + mock_cache, + mock_smtp_send, + mock_send, mock_get_hmrc_to_dit_mailserver, mock_get_spire_to_dit_mailserver, ): + mock_cache.add.return_value = True + mock_smtp_send.wraps = lambda x: x source_run_number = 49530 hmrc_run_number = 49543 filename = self.licence_usage_file_name @@ -184,7 +204,7 @@ def test_resend_usage_data_mail_to_spire( # assert that the pending mail is sent and status updated mail = Mail.objects.get(id=pending_mail.id) - send_mail.assert_called_once() + mock_smtp_send.assert_called_once() self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_SENT) call_command("resend_email", "--hmrc_run_number", 49543) @@ -195,4 +215,37 @@ def test_resend_usage_data_mail_to_spire( self.assertEqual(mail.id, pending_mail.id) self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_SENT) self.assertEqual(mail.extract_type, ExtractTypeEnum.USAGE_DATA) - send_mail.assert_called_once() + mock_send.assert_called_once() + + @mock.patch("mail.libraries.routing_controller.smtp_send") + def test_resend_licence_data_mail_success(self, mock_smtp_send): + source_run_number = 49530 + hmrc_run_number = 49543 + filename = self.licence_data_file_name + mail_body = self.licence_data_file_body.decode("utf-8") + reply_pending_mail = Mail.objects.create( + extract_type=ExtractTypeEnum.LICENCE_DATA, + edi_filename=filename, + edi_data=mail_body, + status=ReceptionStatusEnum.REPLY_PENDING, + sent_at=datetime.now(timezone.utc), + sent_filename=filename, + sent_data=mail_body, + ) + LicenceData.objects.create( + mail=reply_pending_mail, + source_run_number=source_run_number, + hmrc_run_number=hmrc_run_number, + source=SourceEnum.SPIRE, + licence_ids=f"{source_run_number},{hmrc_run_number}", + ) + + call_command("resend_email", "--hmrc_run_number", 49543) + + mail_qs = Mail.objects.filter(status=ReceptionStatusEnum.REPLY_PENDING) + self.assertEqual(mail_qs.count(), 1) + mail = mail_qs.first() + self.assertEqual(mail.id, reply_pending_mail.id) + self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_PENDING) + self.assertEqual(mail.extract_type, ExtractTypeEnum.LICENCE_DATA) + self.assertEqual(mock_smtp_send.call_count, 1) diff --git a/mail/tests/test_select_email_for_sending.py b/mail/tests/test_select_email_for_sending.py index 3c09711e..0052e0bf 100644 --- a/mail/tests/test_select_email_for_sending.py +++ b/mail/tests/test_select_email_for_sending.py @@ -104,12 +104,14 @@ def test_email_selected_if_no_spire_data(self): @mock.patch("mail.libraries.routing_controller.get_spire_to_dit_mailserver") @mock.patch("mail.libraries.routing_controller.get_hmrc_to_dit_mailserver") - @mock.patch("mail.libraries.routing_controller.send") + @mock.patch("mail.celery_tasks.smtp_send") + @mock.patch("mail.celery_tasks.cache") @mock.patch("mail.libraries.routing_controller.get_email_message_dtos") def test_case1_sending_of_pending_licencedata_mails( self, email_dtos, - send_mail, + mock_cache, + mock_smtp_send, mock_get_hmrc_to_dit_mailserver, mock_get_spire_to_dit_mailserver, ): @@ -127,7 +129,8 @@ def test_case1_sending_of_pending_licencedata_mails( num_sent_mails = 3 start_run_number = 78120 email_dtos.return_value = [] - send_mail.wraps = lambda x: x + mock_cache.add.return_value = True + mock_smtp_send.wraps = lambda x: x for i in range(num_sent_mails): mail = self.get_mail(extract_type=ExtractTypeEnum.LICENCE_DATA, status=ReceptionStatusEnum.REPLY_SENT) LicenceData.objects.create( @@ -159,17 +162,19 @@ def test_case1_sending_of_pending_licencedata_mails( # assert that the pending mail is sent and status updated mail = Mail.objects.get(id=pending_mail.id) - send_mail.assert_called_once() + mock_smtp_send.assert_called_once() self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_PENDING) @mock.patch("mail.libraries.routing_controller.get_spire_to_dit_mailserver") @mock.patch("mail.libraries.routing_controller.get_hmrc_to_dit_mailserver") - @mock.patch("mail.libraries.routing_controller.send") + @mock.patch("mail.celery_tasks.smtp_send") + @mock.patch("mail.celery_tasks.cache") @mock.patch("mail.libraries.routing_controller.get_email_message_dtos") def test_case2_sending_of_pending_usagedata_mails( self, email_dtos, - send_mail, + mock_cache, + mock_smtp_send, mock_get_hmrc_to_dit_mailserver, mock_get_spire_to_dit_mailserver, ): @@ -188,7 +193,8 @@ def test_case2_sending_of_pending_usagedata_mails( num_sent_mails = 3 start_run_number = 78120 email_dtos.return_value = [] - send_mail.wraps = lambda x: x + mock_cache.add.return_value = True + mock_smtp_send.wraps = lambda x: x for i in range(num_sent_mails): mail = self.get_mail(extract_type=ExtractTypeEnum.LICENCE_DATA, status=ReceptionStatusEnum.REPLY_SENT) LicenceData.objects.create( @@ -223,18 +229,20 @@ def test_case2_sending_of_pending_usagedata_mails( # assert that the pending mail is sent and status updated mail = Mail.objects.get(id=pending_mails[i].id) - send_mail.assert_called() - self.assertEqual(send_mail.call_count, int(i + 1)) + mock_smtp_send.assert_called() + self.assertEqual(mock_smtp_send.call_count, int(i + 1)) self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_SENT) @mock.patch("mail.libraries.routing_controller.get_spire_to_dit_mailserver") @mock.patch("mail.libraries.routing_controller.get_hmrc_to_dit_mailserver") - @mock.patch("mail.libraries.routing_controller.send") + @mock.patch("mail.celery_tasks.smtp_send") + @mock.patch("mail.celery_tasks.cache") @mock.patch("mail.libraries.routing_controller.get_email_message_dtos") def test_case3_sending_of_pending_licencedata_and_usagedata_mails_1( self, email_dtos, - send_mail, + mock_cache, + mock_smtp_send, mock_get_hmrc_to_dit_mailserver, mock_get_spire_to_dit_mailserver, ): @@ -255,7 +263,8 @@ def test_case3_sending_of_pending_licencedata_and_usagedata_mails_1( num_sent_mails = 3 start_run_number = 78120 email_dtos.return_value = [] - send_mail.wraps = lambda x: x + mock_cache.add.return_value = True + mock_smtp_send.wraps = lambda x: x for i in range(num_sent_mails): mail = self.get_mail(extract_type=ExtractTypeEnum.LICENCE_DATA, status=ReceptionStatusEnum.REPLY_SENT) LicenceData.objects.create( @@ -309,8 +318,8 @@ def test_case3_sending_of_pending_licencedata_and_usagedata_mails_1( # assert that the pending mail is sent and status updated mail = Mail.objects.get(id=pending_mails[i].id) - send_mail.assert_called() - self.assertEqual(send_mail.call_count, int(i + 1)) + mock_smtp_send.assert_called() + self.assertEqual(mock_smtp_send.call_count, int(i + 1)) if mail.extract_type == ExtractTypeEnum.LICENCE_DATA: self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_PENDING) @@ -319,12 +328,14 @@ def test_case3_sending_of_pending_licencedata_and_usagedata_mails_1( @mock.patch("mail.libraries.routing_controller.get_spire_to_dit_mailserver") @mock.patch("mail.libraries.routing_controller.get_hmrc_to_dit_mailserver") - @mock.patch("mail.libraries.routing_controller.send") + @mock.patch("mail.celery_tasks.smtp_send") + @mock.patch("mail.celery_tasks.cache") @mock.patch("mail.libraries.routing_controller.get_email_message_dtos") def test_case3_sending_of_pending_licencedata_and_usagedata_mails_2( self, email_dtos, - send_mail, + mock_cache, + mock_smtp_send, mock_get_hmrc_to_dit_mailserver, mock_get_spire_to_dit_mailserver, ): @@ -342,7 +353,8 @@ def test_case3_sending_of_pending_licencedata_and_usagedata_mails_2( start_run_number = 78120 usage_run_number = 5050 email_dtos.return_value = [] - send_mail.wraps = lambda x: x + mock_cache.add.return_value = True + mock_smtp_send.wraps = lambda x: x for i in range(num_sent_mails): mail = self.get_mail(extract_type=ExtractTypeEnum.LICENCE_DATA, status=ReceptionStatusEnum.REPLY_SENT) LicenceData.objects.create( @@ -390,7 +402,7 @@ def test_case3_sending_of_pending_licencedata_and_usagedata_mails_2( # assert that the pending mail is sent and status updated mail = Mail.objects.get(id=pending_mail.id) - send_mail.assert_called_once() + mock_smtp_send.assert_called_once() self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_PENDING) @mock.patch("mail.libraries.routing_controller.get_spire_to_dit_mailserver") diff --git a/mail/tests/test_send_lite_licence_updates_task.py b/mail/tests/test_send_lite_licence_updates_task.py index 27f81d2b..b653af8d 100644 --- a/mail/tests/test_send_lite_licence_updates_task.py +++ b/mail/tests/test_send_lite_licence_updates_task.py @@ -1,31 +1,61 @@ -from smtplib import SMTPException from unittest import mock -from django.test import override_settings from parameterized import parameterized from mail.celery_tasks import send_licence_details_to_hmrc +from mail.libraries.lite_to_edifact_converter import EdifactValidationError from mail.enums import ReceptionStatusEnum from mail.models import LicencePayload, Mail from mail.tests.libraries.client import LiteHMRCTestClient -class TaskTests(LiteHMRCTestClient): +class SendLiteLicenceDetailsTaskTests(LiteHMRCTestClient): def setUp(self): super().setUp() self.mail = Mail.objects.create(edi_filename="filename", edi_data="1\\fileHeader\\CHIEF\\SPIRE\\") @parameterized.expand( [ - (ReceptionStatusEnum.PENDING, 1, 1, 0), - (ReceptionStatusEnum.REPLY_PENDING, 1, 1, 0), - (ReceptionStatusEnum.REPLY_RECEIVED, 1, 1, 0), - (ReceptionStatusEnum.REPLY_SENT, 2, 1, 1), + ([True],), + ([False, True],), ] ) - @mock.patch("mail.celery_tasks.send") + @mock.patch("mail.celery_tasks.cache") + @mock.patch("mail.celery_tasks.smtp_send") + def test_send_licence_details_success(self, lock_acquired, mock_smtp_send, mock_cache): + """Test sending of LITE licence details without and with retry scenario""" + mock_cache.add.side_effect = lock_acquired + self.mail.status = ReceptionStatusEnum.REPLY_SENT + self.mail.save() + + self.assertEqual(LicencePayload.objects.count(), 1) + send_licence_details_to_hmrc.delay() + + self.assertEqual(Mail.objects.count(), 2) + self.assertEqual(LicencePayload.objects.filter(is_processed=True).count(), 1) + + assert mock_cache.add.call_count == len(lock_acquired) + assert mock_smtp_send.call_count == 1 + + @parameterized.expand( + [ + (ReceptionStatusEnum.PENDING, 1, 1, 0, 0), + (ReceptionStatusEnum.REPLY_PENDING, 1, 1, 0, 0), + (ReceptionStatusEnum.REPLY_RECEIVED, 1, 1, 0, 0), + (ReceptionStatusEnum.REPLY_SENT, 2, 1, 1, 1), + ] + ) + @mock.patch("mail.celery_tasks.cache") + @mock.patch("mail.celery_tasks.smtp_send") def test_send_licence_details_with_active_mail_status( - self, mail_status, expected_mail_count, payload_count, processed_payload_count, mock_send + self, + mail_status, + expected_mail_count, + payload_count, + processed_payload_count, + num_emails_sent, + mock_smtp_send, + mock_cache, ): """ We can only send one message at a time to HMRC so before we send next message the @@ -33,17 +63,20 @@ def test_send_licence_details_with_active_mail_status( details we need to check if there is an active email. In this test we ensure payload is only processed where there are no active emails. """ + mock_cache.add.return_value = True self.mail.status = mail_status self.mail.save() self.assertEqual(LicencePayload.objects.count(), payload_count) - send_licence_details_to_hmrc.delay() + self.assertEqual(Mail.objects.count(), expected_mail_count) self.assertEqual(LicencePayload.objects.filter(is_processed=True).count(), processed_payload_count) - @mock.patch("mail.celery_tasks.send") - def test_send_licence_details_not_sent_when_there_are_no_payloads(self, mock_send): + assert mock_smtp_send.call_count == num_emails_sent + + @mock.patch("mail.celery_tasks.smtp_send") + def test_send_licence_details_not_sent_when_there_are_no_payloads(self, mock_smtp_send): """Test to ensure no details are sent if there are no payloads to process""" self.mail.status = ReceptionStatusEnum.REPLY_SENT self.mail.save() @@ -56,10 +89,10 @@ def test_send_licence_details_not_sent_when_there_are_no_payloads(self, mock_sen send_licence_details_to_hmrc.delay() self.assertEqual(Mail.objects.count(), 1) - mock_send.assert_not_called() + mock_smtp_send.assert_not_called() - @mock.patch("mail.celery_tasks.send") - def test_send_licence_details_task_payload_not_processed_if_validation_error(self, mock_send): + @mock.patch("mail.celery_tasks.smtp_send") + def test_send_licence_details_task_payload_not_processed_if_validation_error(self, mock_smtp_send): """Test to ensure payload is not processed if there is a validation error""" self.mail.status = ReceptionStatusEnum.REPLY_SENT self.mail.save() @@ -74,12 +107,12 @@ def test_send_licence_details_task_payload_not_processed_if_validation_error(sel send_licence_details_to_hmrc.delay() self.assertEqual(LicencePayload.objects.filter(is_processed=False).count(), 1) - mock_send.assert_not_called() + mock_smtp_send.assert_not_called() - @mock.patch("mail.celery_tasks.send") - def test_send_licence_details_raises_exception(self, mock_send): - mock_send.side_effect = SMTPException() - with self.assertRaises(SMTPException): + @mock.patch("mail.libraries.builders.licences_to_edifact") + def test_send_licence_details_raises_exception(self, mock_licences_to_edifact): + mock_licences_to_edifact.side_effect = EdifactValidationError() + with self.assertRaises(EdifactValidationError): self.mail.status = ReceptionStatusEnum.REPLY_SENT self.mail.save() send_licence_details_to_hmrc()