fix dao and provider_task tests

app/celery/provider_tasks.py

@@ -1,3 +1,5 @@
+from uuid import UUID
+
 from app import notify_celery
 from app.celery.common import (
     can_retry,
@@ -10,6 +12,8 @@
 from app.clients.email.aws_ses import AwsSesClientThrottlingSendRateException
 from app.config import QueueNames
 from app.constants import (
+    EMAIL_TYPE,
+    SMS_TYPE,
     STATUS_REASON_BLOCKED,
     STATUS_REASON_INVALID_NUMBER,
     STATUS_REASON_UNDELIVERABLE,
@@ -64,7 +68,7 @@ def deliver_sms(
         send_to_providers.send_sms_to_provider(notification, sms_sender_id)
         current_app.logger.info('Successfully sent sms for notification id: %s', notification_id)
     except Exception as e:
-        _handle_delivery_failure(task, notification, 'deliver_sms', e)
+        _handle_delivery_failure(task, notification, 'deliver_sms', e, notification_id, SMS_TYPE)
 
 
 # Including sms_sender_id is necessary in case it's passed in when being called
@@ -113,7 +117,7 @@ def deliver_sms_with_rate_limiting(
 
         task.retry(queue=QueueNames.RETRY, max_retries=None, countdown=retry_time)
     except Exception as e:
-        _handle_delivery_failure(task, notification, 'deliver_sms_with_rate_limiting', e)
+        _handle_delivery_failure(task, notification, 'deliver_sms_with_rate_limiting', e, notification_id, SMS_TYPE)
 
 
 # Including sms_sender_id is necessary in case it's passed in when being called.
@@ -129,8 +133,8 @@ def deliver_sms_with_rate_limiting(
 @statsd(namespace='tasks')
 def deliver_email(
     task: Task,
-    notification_id: str,
-    sms_sender_id=None,
+    notification_id: UUID,
+    sms_sender_id: UUID = None,
 ):
     current_app.logger.info('Start sending email for notification id: %s', notification_id)
 
@@ -154,30 +158,34 @@ def deliver_email(
         raise AutoRetryException(f'Found {type(e).__name__}, autoretrying...', e, e.args)
 
     except Exception as e:
-        _handle_delivery_failure(task, notification, 'deliver_email', e)
+        _handle_delivery_failure(task, notification, 'deliver_email', e, notification_id, EMAIL_TYPE)
 
 
 def _handle_delivery_failure(
     celery_task: Task,
-    notification: Notification,
+    notification: Notification | None,
     method_name: str,
     e: Exception,
+    notification_id: UUID,
+    notification_type: str,
 ) -> None:
     """Handle the various exceptions that can be raised during the delivery of an email or SMS notification
 
     Args:
         celery_task (Task): The task that raised an exception
-        notification (Notification): The notification that failed to send
+        notification (Notification | None): The notification that failed to send, this can be None is rare cases
         method_name (str): The name of the method that raised an exception
         e (Exception): The exception that was raised
+        notification_id (UUID): The UUID of the notification that was attempted to send when the exception was raised
+        notification_type (str): This will be sms or email in this case
 
     Raises:
         NotificationTechnicalFailureException: If the exception is a technical failure
         AutoRetryException: If the exception can be retried
     """
     if isinstance(e, (InactiveServiceException, InvalidProviderException)):
         log_and_update_critical_failure(
-            notification.id,
+            notification_id,
             method_name,
             e,
             STATUS_REASON_UNDELIVERABLE,
@@ -186,7 +194,7 @@ def _handle_delivery_failure(
 
     elif isinstance(e, InvalidPhoneError):
         log_and_update_permanent_failure(
-            notification.id,
+            notification_id,
             method_name,
             e,
             STATUS_REASON_INVALID_NUMBER,
@@ -195,7 +203,7 @@ def _handle_delivery_failure(
 
     elif isinstance(e, InvalidEmailError):
         log_and_update_permanent_failure(
-            notification.id,
+            notification_id,
             method_name,
             e,
             STATUS_REASON_UNREACHABLE,
@@ -210,7 +218,7 @@ def _handle_delivery_failure(
             status_reason = STATUS_REASON_UNDELIVERABLE
 
         log_and_update_permanent_failure(
-            notification.id,
+            notification_id,
             method_name,
             e,
             status_reason,
@@ -220,27 +228,26 @@ def _handle_delivery_failure(
 
     elif isinstance(e, (NullValueForNonConditionalPlaceholderException, AttributeError, RuntimeError)):
         log_and_update_critical_failure(
-            notification.id,
+            notification_id,
             method_name,
             e,
             STATUS_REASON_UNDELIVERABLE,
         )
         raise NotificationTechnicalFailureException(f'Found {type(e).__name__}, NOT retrying...', e, e.args)
 
     else:
-        current_app.logger.exception(
-            '%s delivery failed for notification %s', notification.notification_type, notification.id
-        )
+        current_app.logger.exception('%s delivery failed for notification %s', notification_type, notification_id)
 
-        if can_retry(celery_task.request.retries, celery_task.max_retries, notification.id):
+        if can_retry(celery_task.request.retries, celery_task.max_retries, notification_id):
             current_app.logger.warning(
                 '%s unable to send for notification %s, retrying',
-                notification.notification_type,
-                notification.id,
+                notification_type,
+                notification_id,
             )
             raise AutoRetryException(f'Found {type(e).__name__}, autoretrying...', e, e.args)
 
         else:
-            msg = handle_max_retries_exceeded(notification.id, method_name)
-            check_and_queue_callback_task(notification)
+            msg = handle_max_retries_exceeded(notification_id, method_name)
+            if notification:
+                check_and_queue_callback_task(notification)
             raise NotificationTechnicalFailureException(msg)

tests/app/celery/test_provider_tasks.py

@@ -116,9 +116,7 @@ def test_should_permanent_error_and_not_retry_if_invalid_email(
     sample_notification,
 ):
     mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=InvalidEmailError('bad email'))
-    mocked_check_and_queue_callback_task = mocker.patch(
-        'app.celery.provider_tasks.check_and_queue_callback_task',
-    )
+    mocked_check_and_queue_callback_task = mocker.patch('app.celery.common.check_and_queue_callback_task')
 
     template = sample_template(template_type=EMAIL_TYPE)
     assert template.template_type == EMAIL_TYPE
@@ -300,7 +298,7 @@ def test_should_queue_callback_task_if_technical_failure_exception_is_thrown(
 
     notify_db_session.session.refresh(notification)
     assert notification.status == NOTIFICATION_PERMANENT_FAILURE
-    assert notification.status_reason == STATUS_REASON_UNREACHABLE
+    assert notification.status_reason == STATUS_REASON_UNDELIVERABLE
     assert callback_mocker.called_once
 
 

tests/app/dao/test_fact_notification_status_dao.py

@@ -19,7 +19,6 @@
     NOTIFICATION_PERMANENT_FAILURE,
     NOTIFICATION_SENDING,
     NOTIFICATION_SENT,
-    NOTIFICATION_TECHNICAL_FAILURE,
     NOTIFICATION_TEMPORARY_FAILURE,
     SMS_TYPE,
 )
@@ -941,7 +940,7 @@ def test_fetch_notification_statuses_per_service_and_template_for_date(
     )
 
     sample_ft_notification_status(
-        date(2019, 4, 30), job=job_sms, notification_status=NOTIFICATION_TECHNICAL_FAILURE, count=5
+        date(2019, 4, 30), job=job_sms, notification_status=NOTIFICATION_PERMANENT_FAILURE, count=5
     )
 
     sample_ft_notification_status(
@@ -954,6 +953,7 @@ def test_fetch_notification_statuses_per_service_and_template_for_date(
     # "service id", "service name", "template id", "template name", "status", "reason", "count", "channel_type"
     assert [
         (service.id, service.name, sms_template.id, sms_template.name, NOTIFICATION_DELIVERED, 'foo', 2, SMS_TYPE),
+        (service.id, service.name, sms_template.id, sms_template.name, NOTIFICATION_PERMANENT_FAILURE, '', 5, SMS_TYPE),
         (
             service.id,
             service.name,
@@ -964,7 +964,6 @@ def test_fetch_notification_statuses_per_service_and_template_for_date(
             5,
             SMS_TYPE,
         ),
-        (service.id, service.name, sms_template.id, sms_template.name, NOTIFICATION_TECHNICAL_FAILURE, '', 5, SMS_TYPE),
         (
             service.id,
             service.name,
@@ -1039,9 +1038,11 @@ def test_fetch_monthly_notification_statuses_per_service(
     sample_ft_notification_status(date(2019, 4, 30), job=job_letter, notification_status=NOTIFICATION_DELIVERED)
     sample_ft_notification_status(date(2019, 3, 1), job=job_email, notification_status=NOTIFICATION_SENDING, count=4)
     sample_ft_notification_status(
-        date(2019, 3, 2), job=job_email, notification_status=NOTIFICATION_TECHNICAL_FAILURE, count=2
+        date(2019, 3, 2), job=job_email, notification_status=NOTIFICATION_PERMANENT_FAILURE, count=2
+    )
+    sample_ft_notification_status(
+        date(2019, 3, 7), job=job_email, notification_status=NOTIFICATION_PERMANENT_FAILURE, count=1
     )
-    sample_ft_notification_status(date(2019, 3, 7), job=job_email, notification_status=NOTIFICATION_FAILED, count=1)
     sample_ft_notification_status(
         date(2019, 3, 7),
         job=job2_letter,
@@ -1066,7 +1067,7 @@ def test_fetch_monthly_notification_statuses_per_service(
 
     # column order: date, service_id, service_name, notifaction_type, count_sending, count_delivered,
     # count_technical_failure, count_temporary_failure, count_permanent_failure, count_sent
-    assert [x for x in results[0]] == [date(2019, 3, 1), service_one.id, service_one.name, EMAIL_TYPE, 4, 0, 3, 0, 0, 0]
+    assert [x for x in results[0]] == [date(2019, 3, 1), service_one.id, service_one.name, EMAIL_TYPE, 4, 0, 0, 0, 3, 0]
     assert [x for x in results[1]] == [
         date(2019, 3, 1),
         service_one.id,