diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 7510f3f75e..b80cec2f24 100644 --- a/app/celery/provider_tasks.py +++ b/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,22 +158,26 @@ 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 @@ -177,7 +185,7 @@ def _handle_delivery_failure( """ 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,7 +228,7 @@ 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, @@ -228,19 +236,18 @@ def _handle_delivery_failure( 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) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 1626d171b3..931518f23d 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/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 diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index f86d3aae17..26be4acc4c 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/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,