Merge pull request #239 from uktrade/LTD-4793-debug

LTD-4793: Fix failure when sending licence rejected notifications email

mail/celery_tasks.py

@@ -120,6 +120,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo):
     max_retries=MAX_ATTEMPTS,
     retry_backoff=RETRY_BACKOFF,
     base=SendEmailBaseTask,
+    serializer="pickle",
 )
 def send_email_task(message):
     """
@@ -146,19 +147,18 @@ def send_email_task(message):
             logger.exception("Another SMTP connection is active, will be retried after backing off")
             raise SMTPConnectionBusy()
 
-        logger.info("Lock acquired, proceeding to send email")
+        logger.info("Lock acquired, proceeding to send email from %s to %s", message["From"], message["To"])
 
         try:
             smtp_send(message)
         except SMTPException:
             logger.exception("An unexpected error occurred when sending email -> %s")
             raise
 
-        logger.info("Email sent successfully")
+        logger.info("Email sent successfully to %s", message["To"])
 
 
 # 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"""
 
@@ -176,10 +176,7 @@ def notify_users_of_rejected_licences(mail_id, mail_response_subject):
     )
     message = build_licence_rejected_email_message(message_dto)
 
-    send_email_task.apply_async(
-        args=(message,),
-        serializer="pickle",
-    )
+    send_email_task.apply_async(args=(message,))
 
     logger.info("Successfully notified users of rejected licences found in mail with subject %s", mail_response_subject)
 
@@ -281,7 +278,6 @@ def send_licence_details_to_hmrc():
             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),
             )
 

mail/libraries/routing_controller.py

@@ -7,7 +7,7 @@
 from rest_framework.exceptions import ValidationError
 
 from mail.auth import BasicAuthentication, ModernAuthentication
-from mail.enums import ExtractTypeEnum, MailReadStatuses, ReceptionStatusEnum, SourceEnum
+from mail.enums import ExtractTypeEnum, MailReadStatuses, ReceptionStatusEnum, ReplyStatusEnum, SourceEnum
 from mail.libraries.builders import build_email_message
 from mail.libraries.data_processors import (
     lock_db_for_sending_transaction,
@@ -138,6 +138,8 @@ def check_and_route_emails():
         )
         _collect_and_send(mail)
 
+        check_and_notify_rejected_licences(mail)
+
     publish_queue_status()
 
 
@@ -191,7 +193,6 @@ def _collect_and_send(mail: Mail):
             # 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),
             )
 
@@ -216,3 +217,13 @@ def get_email_message_dtos(server: MailServer, number: Optional[int] = 3) -> Lis
     # emails = read_last_three_emails(pop3_connection)
     server.quit_pop3_connection()
     return emails
+
+
+def check_and_notify_rejected_licences(mail):
+    from mail.celery_tasks import notify_users_of_rejected_licences
+
+    if not settings.SEND_REJECTED_EMAIL:
+        return
+
+    if mail.response_data and ReplyStatusEnum.REJECTED in mail.response_data:
+        notify_users_of_rejected_licences(str(mail.id), mail.response_subject)

mail/models.py

@@ -15,7 +15,6 @@
     LicenceActionEnum,
     MailReadStatuses,
     ReceptionStatusEnum,
-    ReplyStatusEnum,
     SourceEnum,
 )
 
@@ -76,10 +75,6 @@ def save(self, *args, **kwargs):
 
         super(Mail, self).save(*args, **kwargs)
 
-        if settings.SEND_REJECTED_EMAIL:
-            if self.response_data and ReplyStatusEnum.REJECTED in self.response_data:
-                self.notify_users(self.id, self.response_subject)
-
     def set_locking_time(self, offset: int = 0):
         self.currently_processing_at = timezone.now() + timedelta(seconds=offset)
         self.save()
@@ -92,12 +87,6 @@ def set_response_date_time(self, offset: int = 0):
         self.response_date = timezone.now() + timedelta(seconds=offset)
         self.save()
 
-    @staticmethod
-    def notify_users(id, response_subject):
-        from mail.celery_tasks import notify_users_of_rejected_licences
-
-        notify_users_of_rejected_licences.delay(str(id), response_subject)
-
 
 class LicenceData(models.Model):
     licence_ids = models.TextField()

mail/tests/factories.py

@@ -2,8 +2,8 @@
 
 from uuid import uuid4
 
-from mail.enums import LicenceActionEnum
-from mail.models import LicencePayload, Mail
+from mail.enums import LicenceActionEnum, SourceEnum
+from mail.models import LicenceData, LicencePayload, Mail
 
 
 class MailFactory(factory.django.DjangoModelFactory):
@@ -16,6 +16,15 @@ class Meta:
         model = Mail
 
 
+class LicenceDataFactory(factory.django.DjangoModelFactory):
+    source = SourceEnum.SPIRE
+    mail = factory.SubFactory(Mail)
+    licence_ids = ""
+
+    class Meta:
+        model = LicenceData
+
+
 class LicencePayloadFactory(factory.django.DjangoModelFactory):
     lite_id = uuid4()
     reference = factory.Faker("word")

mail/tests/test_celery_task.py

@@ -3,13 +3,17 @@
 
 from datetime import datetime, timezone
 from parameterized import parameterized
+from email.mime.multipart import MIMEMultipart
 from unittest import mock
 from django.test import TestCase, override_settings
+from unittest.mock import MagicMock
 
 from mail.celery_tasks import manage_inbox, notify_users_of_rejected_licences
 from mail.enums import ExtractTypeEnum, ReceptionStatusEnum, SourceEnum
+from mail.libraries.email_message_dto import EmailMessageDto
 from mail.libraries.routing_controller import check_and_route_emails
 from mail.models import LicenceData, Mail
+from mail.tests.factories import LicenceDataFactory, MailFactory
 from mail.tests.libraries.client import LiteHMRCTestClient
 
 
@@ -28,7 +32,7 @@ def test_send_rejected_notification_email_success(self, lock_acquired, mock_cach
         """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")
+        notify_users_of_rejected_licences("123", "CHIEF_SPIRE_licenceReply_202401180900_42557")
 
         assert mock_cache.add.call_count == len(lock_acquired)
         mock_smtp_send.assert_called_once()
@@ -114,3 +118,112 @@ def test_sending_of_new_message_from_spire_success(
 
         assert mock_cache.add.call_count == len(lock_acquired)
         mock_smtp_send.assert_called_once()
+
+    @parameterized.expand(
+        [
+            # SEND_REJECTED_EMAIL state, mail sender and recipients
+            (
+                True,
+                [
+                    {
+                        "sender": "[email protected]",
+                        "recipients": "[email protected]",
+                        "subject": "ILBDOTI_live_CHIEF_licenceReply_78120_202403060300",
+                    },
+                    {
+                        "sender": "[email protected]",
+                        "recipients": "[email protected]",
+                        "subject": "Licence rejected by HMRC",
+                    },
+                ],
+            ),
+            (
+                False,
+                [
+                    {
+                        "sender": "[email protected]",
+                        "recipients": "[email protected]",
+                        "subject": "ILBDOTI_live_CHIEF_licenceReply_78120_202403060300",
+                    }
+                ],
+            ),
+        ]
+    )
+    @override_settings(
+        EMAIL_USER="[email protected]",
+        NOTIFY_USERS=["[email protected]"],
+        SPIRE_ADDRESS="[email protected]",
+    )
+    @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_processing_of_licence_reply_with_rejected_licences(
+        self,
+        send_rejected_email_flag,
+        emails_data,
+        email_dtos,
+        mock_cache,
+        mock_smtp_send,
+        mock_get_hmrc_to_dit_mailserver,
+        mock_get_spire_to_dit_mailserver,
+    ):
+        """
+        Test processing of licence reply from HMRC with rejected licences.
+        If SEND_REJECTED_EMAIL=True then we send email notifications to users if any licences are rejected.
+        """
+        obj = MagicMock()
+        mock_get_hmrc_to_dit_mailserver.return_value = obj
+        mock_get_spire_to_dit_mailserver.return_value = obj
+        mock_cache.add.return_value = True
+
+        run_number = 78120
+        mail = MailFactory(
+            extract_type=ExtractTypeEnum.LICENCE_DATA,
+            edi_filename=self.licence_data_file_name,
+            edi_data=self.licence_data_file_body.decode("utf-8"),
+            status=ReceptionStatusEnum.REPLY_PENDING,
+        )
+        LicenceDataFactory(mail=mail, source_run_number=run_number, hmrc_run_number=run_number)
+
+        licence_reply_filename = f"ILBDOTI_live_CHIEF_licenceReply_{run_number}_202403060300"
+        file_lines = "\n".join(
+            [
+                f"1\\fileHeader\\CHIEF\SPIRE\\licenceReply\\202403061600\\{run_number}",
+                "2\\accepted\\340631",
+                "3\\rejected\\340632",
+                "4\\end\\rejected\\3",
+                "5\\fileTrailer\\1\\1\\0",
+            ]
+        )
+
+        email_message_dto = EmailMessageDto(
+            run_number=f"{run_number}",
+            sender="[email protected]",
+            receiver="[email protected]",
+            date="Mon, 17 May 2021 14:20:18 +0100",
+            body="licence rejected",
+            subject=licence_reply_filename,
+            attachment=[licence_reply_filename, file_lines.encode("utf-8")],
+            raw_data="qwerty",
+        )
+        email_dtos.return_value = [
+            (email_message_dto, lambda x: x),
+        ]
+
+        with override_settings(SEND_REJECTED_EMAIL=send_rejected_email_flag):
+            check_and_route_emails()
+
+            mail.refresh_from_db()
+            self.assertEqual(mail.status, ReceptionStatusEnum.REPLY_SENT)
+
+            assert mock_cache.add.call_count == len(emails_data)
+            mock_smtp_send.call_count == len(emails_data)
+
+            for index, item in enumerate(mock_smtp_send.call_args_list):
+                message = item.args[0]
+                self.assertTrue(isinstance(message, MIMEMultipart))
+                self.assertEqual(message["From"], emails_data[index]["sender"])
+                self.assertEqual(message["To"], emails_data[index]["recipients"])
+                self.assertEqual(message["Subject"], emails_data[index]["subject"])

mail/tests/test_models.py

@@ -1,54 +1,6 @@
 import uuid
-from unittest.mock import create_autospec
 
-import pytest
-from django.test import override_settings
-from django.utils import timezone
-
-from mail.enums import ExtractTypeEnum, ReceptionStatusEnum, ReplyStatusEnum
-from mail.models import LicencePayload, Mail
-
-
-class TestSendNotifyEmail:
-    @pytest.fixture(autouse=True)
-    def _setup(self, db, monkeypatch):
-        self.mock_notify_users = create_autospec(spec=Mail.notify_users)
-
-        # # Create a mail object that is waiting for a licence reply from HMRC
-        self.mail = Mail.objects.create(
-            status=ReceptionStatusEnum.REPLY_PENDING,
-            extract_type=ExtractTypeEnum.LICENCE_DATA,
-            edi_filename="the_licence_data_file",
-            edi_data="lovely data",
-            sent_filename="the_licence_data_file",
-            sent_data="lovely data",
-        )
-
-        monkeypatch.setattr(self.mail, "notify_users", self.mock_notify_users)
-
-    @override_settings(SEND_REJECTED_EMAIL=True)
-    def test_notify_users_called_when_setting_enabled(self):
-        self.mail.response_data = f"test {ReplyStatusEnum.REJECTED}"
-        self.mail.response_date = timezone.now()
-        self.mail.save()
-
-        self.mock_notify_users.assert_called_with(self.mail.id, self.mail.response_filename)
-
-    @override_settings(SEND_REJECTED_EMAIL=True)
-    def test_notify_users_not_called_when_setting_enabled_but_response_accepted(self):
-        self.mail.response_data = f"test {ReplyStatusEnum.ACCEPTED}"
-        self.mail.response_date = timezone.now()
-        self.mail.save()
-
-        self.mock_notify_users.assert_not_called()
-
-    @override_settings(SEND_REJECTED_EMAIL=False)
-    def test_notify_users_not_called_when_setting_disabled(self):
-        self.mail.response_data = f"test {ReplyStatusEnum.REJECTED}"
-        self.mail.response_date = timezone.now()
-        self.mail.save()
-
-        self.mock_notify_users.assert_not_called()
+from mail.models import LicencePayload
 
 
 def test_licence_payload_model__str__():