Merge pull request #713 from alphagov/email-exception

Don't retry invalid emails

app/celery/provider_tasks.py

@@ -1,12 +1,12 @@
 from flask import current_app
+from notifications_utils.recipients import InvalidEmailError
+from sqlalchemy.orm.exc import NoResultFound
 
 from app import notify_celery
 from app.dao import notifications_dao
 from app.dao.notifications_dao import update_notification_status_by_id
 from app.statsd_decorators import statsd
-
 from app.delivery import send_to_providers
-from sqlalchemy.orm.exc import NoResultFound
 
 
 def retry_iteration_to_delay(retry=0):
@@ -64,52 +64,9 @@ def deliver_email(self, notification_id):
         if not notification:
             raise NoResultFound()
         send_to_providers.send_email_to_provider(notification)
-    except Exception as e:
+    except InvalidEmailError as e:
-        try:
+        current_app.logger.exception(e)
-            current_app.logger.error(
+        update_notification_status_by_id(notification_id, 'technical-failure')
-                "RETRY: Email notification {} failed".format(notification_id)
-            )
-            current_app.logger.exception(e)
-            self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries))
-        except self.MaxRetriesExceededError:
-            current_app.logger.error(
-                "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification_id),
-                e
-            )
-            update_notification_status_by_id(notification_id, 'technical-failure')
-
-
-@notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5)
-@statsd(namespace="tasks")
-def send_sms_to_provider(self, service_id, notification_id):
-    try:
-        notification = notifications_dao.get_notification_by_id(notification_id)
-        if not notification:
-            raise NoResultFound()
-        send_to_providers.send_sms_to_provider(notification)
-    except Exception as e:
-        try:
-            current_app.logger.error(
-                "RETRY: SMS notification {} failed".format(notification_id)
-            )
-            current_app.logger.exception(e)
-            self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries))
-        except self.MaxRetriesExceededError:
-            current_app.logger.error(
-                "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification_id),
-                e
-            )
-            update_notification_status_by_id(notification_id, 'technical-failure')
-
-
-@notify_celery.task(bind=True, name="send-email-to-provider", max_retries=5, default_retry_delay=5)
-@statsd(namespace="tasks")
-def send_email_to_provider(self, service_id, notification_id):
-    try:
-        notification = notifications_dao.get_notification_by_id(notification_id)
-        if not notification:
-            raise NoResultFound()
-        send_to_providers.send_email_to_provider(notification)
     except Exception as e:
         try:
             current_app.logger.error(

app/clients/email/aws_ses.py

@@ -1,6 +1,9 @@
 import boto3
+import botocore
 from flask import current_app
 from monotonic import monotonic
+from notifications_utils.recipients import InvalidEmailError
+
 from app.clients import STATISTICS_DELIVERED, STATISTICS_FAILURE
 from app.clients.email import (EmailClientException, EmailClient)
 
@@ -90,13 +93,26 @@ class AwsSesClient(EmailClient):
                     },
                     'Body': body
                 },
-                ReplyToAddresses=reply_to_addresses)
+                ReplyToAddresses=reply_to_addresses
+            )
+        except botocore.exceptions.ClientError as e:
+            self.statsd_client.incr("clients.ses.error")
+
+            # 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']
+                ))
+            else:
+                self.statsd_client.incr("clients.ses.error")
+                raise AwsSesClientException(str(e))
+        except Exception as e:
+            self.statsd_client.incr("clients.ses.error")
+            raise AwsSesClientException(str(e))
+        else:
             elapsed_time = monotonic() - start_time
             current_app.logger.info("AWS SES request finished in {}".format(elapsed_time))
             self.statsd_client.timing("clients.ses.request-time", elapsed_time)
             self.statsd_client.incr("clients.ses.success")
             return response['MessageId']
-        except Exception as e:
-            # TODO logging exceptions
-            self.statsd_client.incr("clients.ses.error")
-            raise AwsSesClientException(str(e))

tests/app/celery/test_provider_tasks.py

@@ -1,10 +1,13 @@
 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 send_sms_to_provider, send_email_to_provider, deliver_sms, deliver_email
+from app.celery.provider_tasks import deliver_sms, deliver_email
 from app.clients.email import EmailClientException
 from app.models import Notification
+
 from tests.app.conftest import sample_notification
-import app
 
 
 def test_should_have_decorated_tasks_functions():
@@ -65,32 +68,6 @@ 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", countdown=10)
 
 
-def test_should_call_send_sms_to_provider_from_send_sms_to_provider_task(
-        notify_db,
-        notify_db_session,
-        sample_notification,
-        mocker):
-    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider')
-
-    send_sms_to_provider(sample_notification.service_id, sample_notification.id)
-    app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification)
-
-
-def test_should_add_to_retry_queue_if_notification_not_found_in_send_sms_to_provider_task(
-        notify_db,
-        notify_db_session,
-        mocker):
-    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider')
-    mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry')
-
-    notification_id = app.create_uuid()
-    service_id = app.create_uuid()
-
-    send_sms_to_provider(service_id, notification_id)
-    app.delivery.send_to_providers.send_sms_to_provider.assert_not_called()
-    app.celery.provider_tasks.send_sms_to_provider.retry.assert_called_with(queue="retry", countdown=10)
-
-
 def test_should_call_send_email_to_provider_from_deliver_email_task(
         notify_db,
         notify_db_session,
@@ -102,10 +79,7 @@ def test_should_call_send_email_to_provider_from_deliver_email_task(
     app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_notification)
 
 
-def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_task(
+def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_task(mocker):
-        notify_db,
-        notify_db_session,
-        mocker):
     mocker.patch('app.delivery.send_to_providers.send_email_to_provider')
     mocker.patch('app.celery.provider_tasks.deliver_email.retry')
 
@@ -116,116 +90,34 @@ 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", countdown=10)
 
 
-def test_should_call_send_email_to_provider_from_email_task(
-        notify_db,
-        notify_db_session,
-        sample_notification,
-        mocker):
-    mocker.patch('app.delivery.send_to_providers.send_email_to_provider')
-
-    send_email_to_provider(sample_notification.service_id, sample_notification.id)
-    app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_notification)
-
-
-def test_should_add_to_retry_queue_if_notification_not_found_in_send_email_to_provider_task(
-        notify_db,
-        notify_db_session,
-        mocker):
-    mocker.patch('app.delivery.send_to_providers.send_email_to_provider')
-    mocker.patch('app.celery.provider_tasks.send_email_to_provider.retry')
-
-    notification_id = app.create_uuid()
-    service_id = app.create_uuid()
-
-    send_email_to_provider(service_id, notification_id)
-    app.delivery.send_to_providers.send_email_to_provider.assert_not_called()
-    app.celery.provider_tasks.send_email_to_provider.retry.assert_called_with(queue="retry", countdown=10)
-
-
 # DO THESE FOR THE 4 TYPES OF TASK
 
-def test_should_go_into_technical_error_if_exceeds_retries_on_send_sms_to_provider_task(
+def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task(sample_notification, mocker):
-        notify_db,
-        notify_db_session,
-        sample_service,
-        mocker):
-    notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
-                                       service=sample_service, status='created')
-
-    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception("EXPECTED"))
-    mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry', side_effect=MaxRetriesExceededError())
-
-    send_sms_to_provider(
-        notification.service_id,
-        notification.id
-    )
-
-    provider_tasks.send_sms_to_provider.retry.assert_called_with(queue='retry', countdown=10)
-
-    db_notification = Notification.query.filter_by(id=notification.id).one()
-    assert db_notification.status == 'technical-failure'
-
-
-def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task(
-        notify_db,
-        notify_db_session,
-        sample_service,
-        mocker):
-    notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
-                                       service=sample_service, status='created')
-
     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(
+    deliver_sms(sample_notification.id)
-        notification.id
-    )
 
     provider_tasks.deliver_sms.retry.assert_called_with(queue='retry', countdown=10)
 
-    db_notification = Notification.query.filter_by(id=notification.id).one()
+    assert sample_notification.status == 'technical-failure'
-    assert db_notification.status == 'technical-failure'
 
 
-def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retries_on_send_email_to_provider_task(
+def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task(sample_notification, mocker):
-        notify_db,
-        notify_db_session,
-        sample_service,
-        sample_email_template,
-        mocker):
-    notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
-                                       service=sample_service, status='created', template=sample_email_template)
-
-    mocker.patch('app.aws_ses_client.send_email', side_effect=EmailClientException("EXPECTED"))
-    mocker.patch('app.celery.provider_tasks.send_email_to_provider.retry', side_effect=MaxRetriesExceededError())
-
-    send_email_to_provider(
-        notification.service_id,
-        notification.id
-    )
-
-    provider_tasks.send_email_to_provider.retry.assert_called_with(queue='retry', countdown=10)
-
-    db_notification = Notification.query.filter_by(id=notification.id).one()
-    assert db_notification.status == 'technical-failure'
-
-
-def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task(
-        notify_db,
-        notify_db_session,
-        sample_service,
-        mocker):
-    notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
-                                       service=sample_service, status='created')
-
     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(
+    deliver_email(sample_notification.id)
-        notification.id
-    )
 
     provider_tasks.deliver_email.retry.assert_called_with(queue='retry', countdown=10)
+    assert sample_notification.status == 'technical-failure'
 
-    db_notification = Notification.query.filter_by(id=notification.id).one()
+
-    assert db_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'))
+    mocker.patch('app.celery.provider_tasks.deliver_email.retry')
+
+    deliver_email(sample_notification.id)
+
+    assert provider_tasks.deliver_email.retry.called is False
+    assert sample_notification.status == 'technical-failure'

tests/app/celery/test_tasks.py

@@ -634,7 +634,7 @@ def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, no
     assert "07890 300000" not in team_members
 
     notification = _notification_json(template, "07700 900849")
-    mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async')
+    mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
 
     notification_id = uuid.uuid4()
     send_sms(
@@ -643,7 +643,7 @@ def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, no
         encryption.encrypt(notification),
         datetime.utcnow().strftime(DATETIME_FORMAT)
     )
-    assert provider_tasks.send_sms_to_provider.apply_async.called is False
+    assert provider_tasks.deliver_sms.apply_async.called is False
     with pytest.raises(NoResultFound):
         Notification.query.filter_by(id=notification_id).one()
 

tests/app/clients/test_aws_ses.py

@@ -1,8 +1,10 @@
+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.aws_ses import get_aws_responses
+from app.clients.email.aws_ses import get_aws_responses, AwsSesClientException
 
 
 def test_should_return_correct_details_for_delivery():
@@ -67,3 +69,52 @@ def test_send_email_handles_reply_to_address(notify_api, mocker, reply_to_addres
         Message=ANY,
         ReplyToAddresses=expected_value
     )
+
+
+def test_send_email_raises_bad_email_as_InvalidEmailError(mocker):
+    boto_mock = mocker.patch.object(aws_ses_client, '_client', create=True)
+    mocker.patch.object(aws_ses_client, 'statsd_client', create=True)
+    error_response = {
+        'Error': {
+            'Code': 'InvalidParameterValue',
+            'Message': 'some error message from amazon',
+            'Type': 'Sender'
+        }
+    }
+    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:
+        aws_ses_client.send_email(
+            source=Mock(),
+            to_addresses='clearly@invalid@email.com',
+            subject=Mock(),
+            body=Mock()
+        )
+
+    assert 'some error message from amazon' in excinfo.value.message
+    assert 'clearly@invalid@email.com' in excinfo.value.message
+
+
+def test_send_email_raises_other_errs_as_AwsSesClientException(mocker):
+    boto_mock = mocker.patch.object(aws_ses_client, '_client', create=True)
+    mocker.patch.object(aws_ses_client, 'statsd_client', create=True)
+    error_response = {
+        'Error': {
+            'Code': 'ServiceUnavailable',
+            'Message': 'some error message from amazon',
+            'Type': 'Sender'
+        }
+    }
+    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(AwsSesClientException) as excinfo:
+        aws_ses_client.send_email(
+            source=Mock(),
+            to_addresses=Mock(),
+            subject=Mock(),
+            body=Mock()
+        )
+
+    assert 'some error message from amazon' in str(excinfo.value)