From a095aa41f3b3eb2cab607000b838061ed4ffd1f1 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Thu, 13 Oct 2016 15:27:47 +0100
Subject: [PATCH] don't retry task if InvalidEmailError

just record it as a technical error - retrying wont fix a bad email
---
 app/celery/provider_tasks.py            |  9 +++++++--
 app/clients/email/aws_ses.py            | 22 +++++++++++++++-------
 tests/app/celery/test_provider_tasks.py | 14 +++++++++++++-
 3 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py
index c94120e79..b2e3a346d 100644
--- a/app/celery/provider_tasks.py
+++ b/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,6 +64,9 @@ 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)
+        update_notification_status_by_id(notification_id, 'technical-failure')
     except Exception as e:
         try:
             current_app.logger.error(
@@ -82,6 +85,7 @@ def deliver_email(self, notification_id):
 @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):
+    # todo: delete this task?
     try:
         notification = notifications_dao.get_notification_by_id(notification_id)
         if not notification:
@@ -105,6 +109,7 @@ def send_sms_to_provider(self, service_id, notification_id):
 @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):
+    # todo: delete this task?
     try:
         notification = notifications_dao.get_notification_by_id(notification_id)
         if not notification:
diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py
index 4607d5fda..aeab927cd 100644
--- a/app/clients/email/aws_ses.py
+++ b/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)
 
@@ -92,13 +95,18 @@ class AwsSesClient(EmailClient):
                 },
                 ReplyToAddresses=reply_to_addresses
             )
+        except botocore.exceptions.ClientError as e:
+            if e.response['Error']['Code'] == 'InvalidParameterValue':
+                raise InvalidEmailError('email: "{}" message: "{}"'.format(
+                    to_addresses[0],
+                    e.response['Error']['Message']
+                ))
         except Exception as e:
-            # TODO logging exceptions
             self.statsd_client.incr("clients.ses.error")
             raise AwsSesClientException(str(e))
-
-        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']
+        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']
diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py
index 7ab7a9c20..290b30722 100644
--- a/tests/app/celery/test_provider_tasks.py
+++ b/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.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():
@@ -229,3 +232,12 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task
 
     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'