diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 65878f1f6..60c01d201 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -6,6 +6,7 @@ from app import notify_celery from app.config import QueueNames from app.clients.email import EmailClientNonRetryableException from app.clients.email.aws_ses import AwsSesClientThrottlingSendRateException +from app.clients.sms import SmsClientResponseException from app.dao import notifications_dao from app.dao.notifications_dao import update_notification_status_by_id from app.delivery import send_to_providers @@ -22,11 +23,17 @@ def deliver_sms(self, notification_id): if not notification: raise NoResultFound() send_to_providers.send_sms_to_provider(notification) - except Exception: - try: + except Exception as e: + if isinstance(e, SmsClientResponseException): + current_app.logger.warning( + "SMS notification delivery for id: {} failed".format(notification_id) + ) + else: current_app.logger.exception( "SMS notification delivery for id: {} failed".format(notification_id) ) + + try: if self.request.retries == 0: self.retry(queue=QueueNames.RETRY, countdown=0) else: diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index a8b803613..d659f44f1 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -88,7 +88,7 @@ class FiretextClient(SmsClient): self.statsd_client.incr("clients.firetext.success") else: self.statsd_client.incr("clients.firetext.error") - self.current_app.logger.error(log_message) + self.current_app.logger.warning(log_message) def send_sms(self, to, content, reference, sender=None): diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 752ff7792..57c43a037 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -90,7 +90,7 @@ class MMGClient(SmsClient): self.statsd_client.incr("clients.mmg.success") else: self.statsd_client.incr("clients.mmg.error") - self.current_app.logger.error(log_message) + self.current_app.logger.warning(log_message) def get_name(self): return self.name diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index ea30f950a..ad024e045 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -5,6 +5,7 @@ from celery.exceptions import MaxRetriesExceededError import app from app.celery import provider_tasks from app.celery.provider_tasks import deliver_sms, deliver_email +from app.clients.sms import SmsClientResponseException from app.clients.email import EmailClientNonRetryableException from app.clients.email.aws_ses import AwsSesClientException, AwsSesClientThrottlingSendRateException from app.exceptions import NotificationTechnicalFailureException @@ -37,6 +38,68 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_sms_task app.celery.provider_tasks.deliver_sms.retry.assert_called_with(queue="retry-tasks", countdown=0) +def test_send_sms_should_not_switch_providers_on_non_provider_failure( + sample_notification, + mocker +): + mocker.patch( + 'app.delivery.send_to_providers.send_sms_to_provider', + side_effect=Exception("Non Provider Exception") + ) + mock_dao_reduce_sms_provider_priority = mocker.patch( + 'app.delivery.send_to_providers.dao_reduce_sms_provider_priority' + ) + mocker.patch('app.celery.provider_tasks.deliver_sms.retry') + + deliver_sms(sample_notification.id) + + assert mock_dao_reduce_sms_provider_priority.called is False + + +def test_should_retry_and_log_warning_if_SmsClientResponseException_for_deliver_sms_task(sample_notification, mocker): + mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=SmsClientResponseException("something went wrong")) + mocker.patch('app.celery.provider_tasks.deliver_sms.retry') + mock_logger_warning = mocker.patch('app.celery.tasks.current_app.logger.warning') + + deliver_sms(sample_notification.id) + + assert provider_tasks.deliver_sms.retry.called is True + assert sample_notification.status == 'created' + assert mock_logger_warning.called + + +def test_should_retry_and_log_exception_for_non_SmsClientResponseException_exceptions_for_deliver_sms_task( + sample_notification, mocker +): + mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception("something went wrong")) + mocker.patch('app.celery.provider_tasks.deliver_sms.retry') + mock_logger_exception = mocker.patch('app.celery.tasks.current_app.logger.exception') + + deliver_sms(sample_notification.id) + + assert provider_tasks.deliver_sms.retry.called is True + assert sample_notification.status == 'created' + assert mock_logger_exception.called + + +def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task(sample_notification, mocker): + 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()) + mock_logger_exception = mocker.patch('app.celery.tasks.current_app.logger.exception') + + with pytest.raises(NotificationTechnicalFailureException) as e: + deliver_sms(sample_notification.id) + assert str(sample_notification.id) in str(e.value) + + provider_tasks.deliver_sms.retry.assert_called_with(queue="retry-tasks", countdown=0) + + assert sample_notification.status == 'technical-failure' + assert mock_logger_exception.called + + +# end of deliver_sms task tests, now deliver_email task tests + + def test_should_call_send_email_to_provider_from_deliver_email_task( sample_notification, mocker): @@ -57,21 +120,6 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_ta app.celery.provider_tasks.deliver_email.retry.assert_called_with(queue="retry-tasks") -# DO THESE FOR THE 4 TYPES OF TASK - -def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task(sample_notification, mocker): - 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()) - - with pytest.raises(NotificationTechnicalFailureException) as e: - deliver_sms(sample_notification.id) - assert str(sample_notification.id) in str(e.value) - - provider_tasks.deliver_sms.retry.assert_called_with(queue="retry-tasks", countdown=0) - - assert sample_notification.status == 'technical-failure' - - @pytest.mark.parametrize( 'exception_class', [ Exception(), @@ -106,7 +154,7 @@ def test_should_technical_error_and_not_retry_if_EmailClientNonRetryableExceptio assert sample_notification.status == 'technical-failure' -def test_should_retry_and_log_exception(sample_notification, mocker): +def test_should_retry_and_log_exception_for_deliver_email_task(sample_notification, mocker): error_response = { 'Error': { 'Code': 'SomeError', @@ -149,21 +197,3 @@ def test_if_ses_send_rate_throttle_then_should_retry_and_log_warning(sample_noti assert sample_notification.status == 'created' assert not mock_logger_exception.called assert mock_logger_warning.called - - -def test_send_sms_should_not_switch_providers_on_non_provider_failure( - sample_notification, - mocker -): - mocker.patch( - 'app.delivery.send_to_providers.send_sms_to_provider', - side_effect=Exception("Non Provider Exception") - ) - mock_dao_reduce_sms_provider_priority = mocker.patch( - 'app.delivery.send_to_providers.dao_reduce_sms_provider_priority' - ) - mocker.patch('app.celery.provider_tasks.deliver_sms.retry') - - deliver_sms(sample_notification.id) - - assert mock_dao_reduce_sms_provider_priority.called is False