Refactor to catch errors and not update TaskOrder.pdf_last_sent_at unless the email has been sent. Add tests for failure cases

atst/domain/task_orders.py

@@ -91,9 +91,3 @@ class TaskOrders(BaseDomainClass):
             )
             .all()
         )
-
-    @classmethod
-    def update_pdf_last_sent_at(cls, task_order):
-        task_order.pdf_last_sent_at = datetime.now()
-        db.session.add(task_order)
-        db.session.commit()

atst/jobs.py

@@ -1,5 +1,7 @@
 import pendulum
 from flask import current_app as app
+from smtplib import SMTPException
+from azure.core.exceptions import AzureError
 
 from atst.database import db
 from atst.domain.application_roles import ApplicationRoles
@@ -45,8 +47,8 @@ class RecordFailure(celery.Task):
 
 
 @celery.task(ignore_result=True)
-def send_mail(recipients, subject, body):
+def send_mail(recipients, subject, body, attachments=[]):
-    app.mailer.send(recipients, subject, body)
+    app.mailer.send(recipients, subject, body, attachments)
 
 
 @celery.task(ignore_result=True)
@@ -146,14 +148,6 @@ def do_provision_portfolio(csp: CloudProviderInterface, portfolio_id=None):
     fsm.trigger_next_transition()
 
 
-def do_send_with_attachment(
-    csp: CloudProviderInterface, recipients, subject, body, attachments
-):
-    app.mailer.send(
-        recipients=recipients, subject=subject, body=body, attachments=attachments
-    )
-
-
 @celery.task(bind=True, base=RecordFailure)
 def provision_portfolio(self, portfolio_id=None):
     do_work(do_provision_portfolio, self, app.csp.cloud, portfolio_id=portfolio_id)
@@ -176,32 +170,6 @@ def create_environment(self, environment_id=None):
     do_work(do_create_environment, self, app.csp.cloud, environment_id=environment_id)
 
 
-@celery.task(bind=True)
-def send_with_attachment(self, recipients, subject, body, attachments):
-    do_work(
-        do_send_with_attachment,
-        self,
-        app.csp.cloud,
-        recipients=recipients,
-        subject=subject,
-        body=body,
-        attachments=attachments,
-    )
-
-
-@celery.task(bind=True)
-def send_with_attachment(self, recipients, subject, body, attachments):
-    do_work(
-        do_send_with_attachment,
-        self,
-        app.csp.cloud,
-        recipients=recipients,
-        subject=subject,
-        body=body,
-        attachments=attachments,
-    )
-
-
 @celery.task(bind=True)
 def dispatch_provision_portfolio(self):
     """
@@ -250,11 +218,17 @@ def dispatch_send_task_order_files(self):
         )
         body = translate("email.task_order_sent.body", {"to_number": task_order.number})
 
+        try:
             file = app.csp.files.download_task_order(task_order.pdf.object_name)
             file["maintype"] = "application"
             file["subtype"] = "pdf"
-
+            send_mail(
-        send_with_attachment.delay(
                 recipients=recipients, subject=subject, body=body, attachments=[file]
             )
-        TaskOrders.update_pdf_last_sent_at(task_order)
+        except (AzureError, SMTPException):
+            continue
+
+        task_order.pdf_last_sent_at = pendulum.now()
+        db.session.add(task_order)
+
+    db.session.commit()

atst/utils/mailer.py

@@ -72,7 +72,7 @@ class Mailer(object):
         msg = EmailMessage()
         msg.set_content(body)
         msg["From"] = self.sender
-        msg["To"] = ", ".join(recipients)
+        msg["To"] = ", ".join(recipients) if type(recipients) == list else recipients
         msg["Subject"] = subject
 
         return msg

tests/test_jobs.py

@@ -2,6 +2,8 @@ import pendulum
 import pytest
 from uuid import uuid4
 from unittest.mock import Mock
+from smtplib import SMTPException
+from azure.core.exceptions import AzureError
 
 from atst.domain.csp.cloud import MockCloudProvider
 from atst.domain.portfolios import Portfolios
@@ -292,11 +294,9 @@ def test_provision_portfolio_create_tenant(
     # mock.delay.assert_called_once_with(portfolio_id=portfolio.id)
 
 
-def test_dispatch_send_task_order_files(
+def test_dispatch_send_task_order_files(monkeypatch, app):
-    csp, session, celery_app, celery_worker, monkeypatch, app
-):
     mock = Mock()
-    monkeypatch.setattr("atst.jobs.send_with_attachment", mock)
+    monkeypatch.setattr("atst.jobs.send_mail", mock)
 
     def _download_task_order(MockFileService, object_name):
         return {"name": object_name}
@@ -313,7 +313,7 @@ def test_dispatch_send_task_order_files(
     dispatch_send_task_order_files.run()
 
     # Check that send_with_attachment was called once for each task order
-    assert mock.delay.call_count == 3
+    assert mock.call_count == 3
     mock.reset_mock()
 
     # Create new TO
@@ -323,7 +323,7 @@ def test_dispatch_send_task_order_files(
     dispatch_send_task_order_files.run()
 
     # Check that send_with_attachment was called with correct kwargs
-    mock.delay.assert_called_once_with(
+    mock.assert_called_once_with(
         recipients=app.config.get("MICROSOFT_TASK_ORDER_EMAIL_ADDRESS"),
         subject=translate(
             "email.task_order_sent.subject", {"to_number": task_order.number}
@@ -339,3 +339,35 @@ def test_dispatch_send_task_order_files(
     )
 
     assert task_order.pdf_last_sent_at
+
+
+def test_dispatch_send_task_order_files_send_failure(monkeypatch):
+    def _raise_smtp_exception(**kwargs):
+        raise SMTPException
+
+    monkeypatch.setattr("atst.jobs.send_mail", _raise_smtp_exception)
+
+    task_order = TaskOrderFactory.create(create_clins=[{"number": "0001"}])
+    dispatch_send_task_order_files.run()
+
+    # Check that pdf_last_sent_at has not been updated
+    assert not task_order.pdf_last_sent_at
+
+
+def test_dispatch_send_task_order_files_download_failure(monkeypatch):
+    mock = Mock()
+    monkeypatch.setattr("atst.jobs.send_mail", mock)
+
+    def _download_task_order(MockFileService, object_name):
+        raise AzureError("something went wrong")
+
+    monkeypatch.setattr(
+        "atst.domain.csp.files.MockFileService.download_task_order",
+        _download_task_order,
+    )
+
+    task_order = TaskOrderFactory.create(create_clins=[{"number": "0002"}])
+    dispatch_send_task_order_files.run()
+
+    # Check that pdf_last_sent_at has not been updated
+    assert not task_order.pdf_last_sent_at