diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 50cfd8506..2852124ac 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -9,6 +9,8 @@ from app.config import QueueNames from app.dao import notifications_dao from app.dao.notifications_dao import update_notification_status_by_id from app.delivery import send_to_providers +from app.exceptions import NotificationTechnicalFailureException +from app.models import NOTIFICATION_TECHNICAL_FAILURE @worker_process_shutdown.connect @@ -31,10 +33,10 @@ def deliver_sms(self, notification_id): ) self.retry(queue=QueueNames.RETRY) except self.MaxRetriesExceededError: - current_app.logger.exception( - "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification_id), - ) - update_notification_status_by_id(notification_id, 'technical-failure') + message = "RETRY FAILED: task send_sms_to_provider failed for notification {}. " \ + "Notification has been updated to technical-failure".format(notification_id) + update_notification_status_by_id(notification_id, NOTIFICATION_TECHNICAL_FAILURE) + raise NotificationTechnicalFailureException(message) @notify_celery.task(bind=True, name="deliver_email", max_retries=48, default_retry_delay=300) @@ -55,7 +57,7 @@ def deliver_email(self, notification_id): ) self.retry(queue=QueueNames.RETRY) except self.MaxRetriesExceededError: - current_app.logger.error( - "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification_id) - ) - update_notification_status_by_id(notification_id, 'technical-failure') + message = "RETRY FAILED: task send_email_to_provider failed for notification {}. " \ + "Notification has been updated to technical-failure".format(notification_id) + update_notification_status_by_id(notification_id, NOTIFICATION_TECHNICAL_FAILURE) + raise NotificationTechnicalFailureException(message) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index c4f511d34..d863dda19 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -16,6 +16,7 @@ from app.dao.services_dao import ( dao_fetch_monthly_historical_stats_by_template ) from app.dao.stats_template_usage_by_month_dao import insert_or_update_stats_for_template +from app.exceptions import NotificationTechnicalFailureException from app.performance_platform import total_sent_notifications, processing_time from app import performance_platform_client, deskpro_client from app.dao.date_util import get_month_start_and_end_date_in_utc @@ -201,8 +202,10 @@ def delete_invitations(): @notify_celery.task(name='timeout-sending-notifications') @statsd(namespace="tasks") def timeout_notifications(): - notifications = dao_timeout_notifications(current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD')) + technical_failure_notifications, temporary_failure_notifications = \ + dao_timeout_notifications(current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD')) + notifications = technical_failure_notifications + temporary_failure_notifications for notification in notifications: # queue callback task only if the service_callback_api exists service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) @@ -213,6 +216,11 @@ def timeout_notifications(): current_app.logger.info( "Timeout period reached for {} notifications, status has been updated.".format(len(notifications))) + if technical_failure_notifications: + message = "{} notifications have been updated to technical-failure because they " \ + "have timed out and are still in created.Notification ids: {}".format( + len(technical_failure_notifications), [str(x.id) for x in technical_failure_notifications]) + raise NotificationTechnicalFailureException(message) @notify_celery.task(name='send-daily-performance-platform-stats') diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d69e95ade..de626a733 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -50,7 +50,7 @@ from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id -from app.exceptions import DVLAException +from app.exceptions import DVLAException, NotificationTechnicalFailureException from app.models import ( DVLA_RESPONSE_STATUS_SENT, EMAIL_TYPE, @@ -371,8 +371,10 @@ def update_letter_notifications_to_error(self, notification_references): 'updated_at': datetime.utcnow() } ) - - current_app.logger.debug("Updated {} letter notifications to technical-failure".format(updated_count)) + message = "Updated {} letter notifications to technical-failure with references {}".format( + updated_count, notification_references + ) + raise NotificationTechnicalFailureException(message=message) def handle_exception(task, notification, notification_id, exc): diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 37c7dddfb..2e0a77011 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -375,14 +375,15 @@ def dao_timeout_notifications(timeout_period_in_seconds): timeout = functools.partial(_timeout_notifications, timeout_start=timeout_start, updated_at=updated_at) # Notifications still in created status are marked with a technical-failure: - updated_ids = timeout([NOTIFICATION_CREATED], NOTIFICATION_TECHNICAL_FAILURE) + technical_failure_notifications = timeout([NOTIFICATION_CREATED], NOTIFICATION_TECHNICAL_FAILURE) # Notifications still in sending or pending status are marked with a temporary-failure: - updated_ids += timeout([NOTIFICATION_SENDING, NOTIFICATION_PENDING], NOTIFICATION_TEMPORARY_FAILURE) + temporary_failure_notifications = timeout([NOTIFICATION_SENDING, NOTIFICATION_PENDING], + NOTIFICATION_TEMPORARY_FAILURE) db.session.commit() - return updated_ids + return technical_failure_notifications, temporary_failure_notifications def get_total_sent_notifications_in_date_range(start_date, end_date, notification_type): diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 4c1a3600b..7167202db 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -19,6 +19,7 @@ from app.dao.provider_details_dao import ( ) from app.celery.research_mode_tasks import send_sms_response, send_email_response from app.dao.templates_dao import dao_get_template_by_id +from app.exceptions import NotificationTechnicalFailureException from app.models import ( SMS_TYPE, KEY_TYPE_TEST, @@ -211,7 +212,7 @@ def get_html_email_options(service): def technical_failure(notification): notification.status = NOTIFICATION_TECHNICAL_FAILURE dao_update_notification(notification) - current_app.logger.warn( + raise NotificationTechnicalFailureException( "Send {} for notification id {} to provider is not allowed: service {} is inactive".format( notification.notification_type, notification.id, diff --git a/app/exceptions.py b/app/exceptions.py index 0fe1b7fe2..6f43f10ea 100644 --- a/app/exceptions.py +++ b/app/exceptions.py @@ -1,3 +1,8 @@ class DVLAException(Exception): def __init__(self, message): self.message = message + + +class NotificationTechnicalFailureException(Exception): + def __init__(self, message): + self.message = message diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 67c7d994e..de118f5dd 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -5,7 +5,7 @@ import pytest from freezegun import freeze_time from flask import current_app -from app.exceptions import DVLAException +from app.exceptions import DVLAException, NotificationTechnicalFailureException from app.models import ( Job, Notification, @@ -276,7 +276,9 @@ def test_update_letter_notifications_to_error_updates_based_on_notification_refe create_service_callback_api(service=sample_letter_template.service, url="https://original_url.com") dt = datetime.utcnow() with freeze_time(dt): - update_letter_notifications_to_error([first.reference]) + with pytest.raises(NotificationTechnicalFailureException) as e: + update_letter_notifications_to_error([first.reference]) + assert first.reference in e.value.message assert first.status == NOTIFICATION_TECHNICAL_FAILURE assert first.sent_by is None diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 5b640a7ec..79cf03021 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,9 +1,11 @@ +import pytest from celery.exceptions import MaxRetriesExceededError from notifications_utils.recipients import InvalidEmailError import app from app.celery import provider_tasks from app.celery.provider_tasks import deliver_sms, deliver_email +from app.exceptions import NotificationTechnicalFailureException def test_should_have_decorated_tasks_functions(): @@ -59,21 +61,25 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task(s mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception("EXPECTED")) mocker.patch('app.celery.provider_tasks.deliver_sms.retry', side_effect=MaxRetriesExceededError()) - deliver_sms(sample_notification.id) + with pytest.raises(NotificationTechnicalFailureException) as e: + deliver_sms(sample_notification.id) provider_tasks.deliver_sms.retry.assert_called_with(queue="retry-tasks") assert sample_notification.status == 'technical-failure' + assert str(sample_notification.id) in e.value.message def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task(sample_notification, mocker): mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=Exception("EXPECTED")) mocker.patch('app.celery.provider_tasks.deliver_email.retry', side_effect=MaxRetriesExceededError()) - deliver_email(sample_notification.id) + with pytest.raises(NotificationTechnicalFailureException) as e: + deliver_email(sample_notification.id) provider_tasks.deliver_email.retry.assert_called_with(queue="retry-tasks") assert sample_notification.status == 'technical-failure' + assert str(sample_notification.id) in e.value.message def test_should_technical_error_and_not_retry_if_invalid_email(sample_notification, mocker): diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 4947190bd..b8bdf5a13 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -44,6 +44,7 @@ from app.dao.provider_details_dao import ( dao_update_provider_details, get_current_provider ) +from app.exceptions import NotificationTechnicalFailureException from app.models import ( MonthlyBilling, NotificationHistory, @@ -181,8 +182,10 @@ def test_update_status_of_notifications_after_timeout(notify_api, sample_templat status='pending', created_at=datetime.utcnow() - timedelta( seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) - timeout_notifications() - + with pytest.raises(NotificationTechnicalFailureException) as e: + timeout_notifications() + print(e.value.message) + assert str(not2.id) in e.value.message assert not1.status == 'temporary-failure' assert not2.status == 'technical-failure' assert not3.status == 'temporary-failure' diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 24dffbdfa..1d6d63755 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -1222,7 +1222,7 @@ def test_dao_timeout_notifications(sample_template): assert Notification.query.get(sending.id).status == 'sending' assert Notification.query.get(pending.id).status == 'pending' assert Notification.query.get(delivered.id).status == 'delivered' - updated_ids = dao_timeout_notifications(1) + technical_failure_notifications, temporary_failure_notifications = dao_timeout_notifications(1) assert Notification.query.get(created.id).status == 'technical-failure' assert Notification.query.get(sending.id).status == 'temporary-failure' assert Notification.query.get(pending.id).status == 'temporary-failure' @@ -1231,7 +1231,7 @@ def test_dao_timeout_notifications(sample_template): assert NotificationHistory.query.get(sending.id).status == 'temporary-failure' assert NotificationHistory.query.get(pending.id).status == 'temporary-failure' assert NotificationHistory.query.get(delivered.id).status == 'delivered' - assert len(updated_ids) == 3 + assert len(technical_failure_notifications + temporary_failure_notifications) == 3 def test_dao_timeout_notifications_only_updates_for_older_notifications(sample_template): @@ -1245,12 +1245,12 @@ def test_dao_timeout_notifications_only_updates_for_older_notifications(sample_t assert Notification.query.get(sending.id).status == 'sending' assert Notification.query.get(pending.id).status == 'pending' assert Notification.query.get(delivered.id).status == 'delivered' - updated_ids = dao_timeout_notifications(1) + technical_failure_notifications, temporary_failure_notifications = dao_timeout_notifications(1) assert NotificationHistory.query.get(created.id).status == 'created' assert NotificationHistory.query.get(sending.id).status == 'sending' assert NotificationHistory.query.get(pending.id).status == 'pending' assert NotificationHistory.query.get(delivered.id).status == 'delivered' - assert len(updated_ids) == 0 + assert len(technical_failure_notifications + temporary_failure_notifications) == 0 def test_dao_timeout_notifications_doesnt_affect_letters(sample_letter_template): @@ -1265,13 +1265,13 @@ def test_dao_timeout_notifications_doesnt_affect_letters(sample_letter_template) assert Notification.query.get(pending.id).status == 'pending' assert Notification.query.get(delivered.id).status == 'delivered' - updated_ids = dao_timeout_notifications(1) + technical_failure_notifications, temporary_failure_notifications = dao_timeout_notifications(1) assert NotificationHistory.query.get(created.id).status == 'created' assert NotificationHistory.query.get(sending.id).status == 'sending' assert NotificationHistory.query.get(pending.id).status == 'pending' assert NotificationHistory.query.get(delivered.id).status == 'delivered' - assert len(updated_ids) == 0 + assert len(technical_failure_notifications + temporary_failure_notifications) == 0 def test_should_return_notifications_excluding_jobs_by_default(sample_template, sample_job, sample_api_key): diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 627768220..4befcf7c1 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -13,6 +13,7 @@ from app import mmg_client, firetext_client from app.dao import (provider_details_dao, notifications_dao) from app.dao.provider_details_dao import dao_switch_sms_provider_to_provider_with_identifier from app.delivery import send_to_providers +from app.exceptions import NotificationTechnicalFailureException from app.models import ( Notification, EmailBranding, @@ -137,9 +138,11 @@ def test_should_not_send_email_message_when_service_is_inactive_notifcation_is_i sample_service.active = False send_mock = mocker.patch("app.aws_ses_client.send_email", return_value='reference') - send_to_providers.send_email_to_provider(sample_notification) + with pytest.raises(NotificationTechnicalFailureException) as e: + send_to_providers.send_email_to_provider(sample_notification) send_mock.assert_not_called() assert Notification.query.get(sample_notification.id).status == 'technical-failure' + assert str(sample_notification.id) in e.value.message @pytest.mark.parametrize("client_send", ["app.mmg_client.send_sms", "app.firetext_client.send_sms"]) @@ -148,9 +151,11 @@ def test_should_not_send_sms_message_when_service_is_inactive_notifcation_is_in_ sample_service.active = False send_mock = mocker.patch(client_send, return_value='reference') - send_to_providers.send_sms_to_provider(sample_notification) + with pytest.raises(NotificationTechnicalFailureException) as e: + send_to_providers.send_sms_to_provider(sample_notification) send_mock.assert_not_called() assert Notification.query.get(sample_notification.id).status == 'technical-failure' + assert str(sample_notification.id) in e.value.message def test_send_sms_should_use_template_version_from_notification_not_latest(