Merge pull request #3081 from alphagov/ses-error-logs

SES error logs

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:
+    except EmailClientNonRetryableException as e:
-        current_app.logger.exception(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:

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.

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(
+                raise EmailClientNonRetryableException(e.response['Error']['Message'])
-                    to_addresses[0],
-                    e.response['Error']['Message']
-                ))
             elif (
                 e.response['Error']['Code'] == 'Throttling'
                 and e.response['Error']['Message'] == 'Maximum sending rate exceeded.'

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):
+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=InvalidEmailError('bad email'))
+    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)

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):