diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 67a8f10d1..65878f1f6 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,10 +1,10 @@ from flask import current_app -from notifications_utils.recipients import InvalidEmailError from notifications_utils.statsd_decorators import statsd from sqlalchemy.orm.exc import NoResultFound 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.dao import notifications_dao from app.dao.notifications_dao import update_notification_status_by_id @@ -47,8 +47,10 @@ def deliver_email(self, notification_id): if not notification: raise NoResultFound() send_to_providers.send_email_to_provider(notification) - except InvalidEmailError as e: - current_app.logger.exception(e) + except EmailClientNonRetryableException as e: + current_app.logger.exception( + f"Email notification {notification_id} failed: {e}" + ) update_notification_status_by_id(notification_id, 'technical-failure') except Exception as e: try: diff --git a/app/clients/email/__init__.py b/app/clients/email/__init__.py index b5ea93941..6db70a8af 100644 --- a/app/clients/email/__init__.py +++ b/app/clients/email/__init__.py @@ -8,6 +8,18 @@ class EmailClientException(ClientException): pass +class EmailClientNonRetryableException(ClientException): + ''' + Represents an error returned from the email client API with a 4xx response code + that should not be retried and should instead be marked as technical failure. + An example of this would be an email address that makes it through our + validation rules but is rejected by SES. There is no point in retrying this type as + it will always fail however many calls to SES. Whereas a throttling error would not + use this exception as it may succeed if we retry + ''' + pass + + class EmailClient(Client): ''' Base Email client for sending emails. diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 7c765288f..f9d6e8723 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -5,7 +5,7 @@ from time import monotonic from notifications_utils.recipients import InvalidEmailError from app.clients import STATISTICS_DELIVERED, STATISTICS_FAILURE -from app.clients.email import (EmailClientException, EmailClient) +from app.clients.email import EmailClient, EmailClientException, EmailClientNonRetryableException ses_response_map = { 'Permanent': { @@ -122,10 +122,7 @@ class AwsSesClient(EmailClient): # http://docs.aws.amazon.com/ses/latest/DeveloperGuide/api-error-codes.html if e.response['Error']['Code'] == 'InvalidParameterValue': - raise InvalidEmailError('email: "{}" message: "{}"'.format( - to_addresses[0], - e.response['Error']['Message'] - )) + raise EmailClientNonRetryableException(e.response['Error']['Message']) elif ( e.response['Error']['Code'] == 'Throttling' and e.response['Error']['Message'] == 'Maximum sending rate exceeded.' diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 10764e74f..ea30f950a 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,11 +1,11 @@ import pytest from botocore.exceptions import ClientError 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.clients.email import EmailClientNonRetryableException from app.clients.email.aws_ses import AwsSesClientException, AwsSesClientThrottlingSendRateException from app.exceptions import NotificationTechnicalFailureException @@ -93,8 +93,11 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task assert sample_notification.status == 'technical-failure' -def test_should_technical_error_and_not_retry_if_invalid_email(sample_notification, mocker): - mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=InvalidEmailError('bad email')) +def test_should_technical_error_and_not_retry_if_EmailClientNonRetryableException(sample_notification, mocker): + mocker.patch( + 'app.delivery.send_to_providers.send_email_to_provider', + side_effect=EmailClientNonRetryableException('bad email') + ) mocker.patch('app.celery.provider_tasks.deliver_email.retry') deliver_email(sample_notification.id) diff --git a/tests/app/clients/test_aws_ses.py b/tests/app/clients/test_aws_ses.py index 17e76aed0..178b7c63c 100644 --- a/tests/app/clients/test_aws_ses.py +++ b/tests/app/clients/test_aws_ses.py @@ -1,9 +1,9 @@ import botocore import pytest from unittest.mock import Mock, ANY -from notifications_utils.recipients import InvalidEmailError from app import aws_ses_client +from app.clients.email import EmailClientNonRetryableException from app.clients.email.aws_ses import get_aws_responses, AwsSesClientException, AwsSesClientThrottlingSendRateException @@ -91,7 +91,7 @@ def test_send_email_handles_punycode_to_address(notify_api, mocker): ) -def test_send_email_raises_bad_email_as_InvalidEmailError(mocker): +def test_send_email_raises_invalid_parameter_value_error_as_EmailClientNonRetryableException(mocker): boto_mock = mocker.patch.object(aws_ses_client, '_client', create=True) mocker.patch.object(aws_ses_client, 'statsd_client', create=True) error_response = { @@ -104,7 +104,7 @@ def test_send_email_raises_bad_email_as_InvalidEmailError(mocker): boto_mock.send_email.side_effect = botocore.exceptions.ClientError(error_response, 'opname') mocker.patch.object(aws_ses_client, 'statsd_client', create=True) - with pytest.raises(InvalidEmailError) as excinfo: + with pytest.raises(EmailClientNonRetryableException) as excinfo: aws_ses_client.send_email( source=Mock(), to_addresses='definitely@invalid_email.com', @@ -113,7 +113,6 @@ def test_send_email_raises_bad_email_as_InvalidEmailError(mocker): ) assert 'some error message from amazon' in str(excinfo.value) - assert 'definitely@invalid_email.com' in str(excinfo.value) def test_send_email_raises_send_rate_throttling_as_AwsSesClientThrottlingSendRateException(mocker):