don't retry task if InvalidEmailError

just record it as a technical error - retrying wont fix a bad email
This commit is contained in:
Leo Hemsted
2016-10-13 15:27:47 +01:00
parent c0c62e02b7
commit a095aa41f3
3 changed files with 35 additions and 10 deletions

View File

@@ -1,12 +1,12 @@
from flask import current_app from flask import current_app
from notifications_utils.recipients import InvalidEmailError
from sqlalchemy.orm.exc import NoResultFound
from app import notify_celery from app import notify_celery
from app.dao import notifications_dao from app.dao import notifications_dao
from app.dao.notifications_dao import update_notification_status_by_id from app.dao.notifications_dao import update_notification_status_by_id
from app.statsd_decorators import statsd from app.statsd_decorators import statsd
from app.delivery import send_to_providers from app.delivery import send_to_providers
from sqlalchemy.orm.exc import NoResultFound
def retry_iteration_to_delay(retry=0): def retry_iteration_to_delay(retry=0):
@@ -64,6 +64,9 @@ def deliver_email(self, notification_id):
if not notification: if not notification:
raise NoResultFound() raise NoResultFound()
send_to_providers.send_email_to_provider(notification) 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: except Exception as e:
try: try:
current_app.logger.error( 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) @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5)
@statsd(namespace="tasks") @statsd(namespace="tasks")
def send_sms_to_provider(self, service_id, notification_id): def send_sms_to_provider(self, service_id, notification_id):
# todo: delete this task?
try: try:
notification = notifications_dao.get_notification_by_id(notification_id) notification = notifications_dao.get_notification_by_id(notification_id)
if not notification: 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) @notify_celery.task(bind=True, name="send-email-to-provider", max_retries=5, default_retry_delay=5)
@statsd(namespace="tasks") @statsd(namespace="tasks")
def send_email_to_provider(self, service_id, notification_id): def send_email_to_provider(self, service_id, notification_id):
# todo: delete this task?
try: try:
notification = notifications_dao.get_notification_by_id(notification_id) notification = notifications_dao.get_notification_by_id(notification_id)
if not notification: if not notification:

View File

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

View File

@@ -1,10 +1,13 @@
from celery.exceptions import MaxRetriesExceededError from celery.exceptions import MaxRetriesExceededError
from notifications_utils.recipients import InvalidEmailError
import app
from app.celery import provider_tasks 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 send_sms_to_provider, send_email_to_provider, deliver_sms, deliver_email
from app.clients.email import EmailClientException from app.clients.email import EmailClientException
from app.models import Notification from app.models import Notification
from tests.app.conftest import sample_notification from tests.app.conftest import sample_notification
import app
def test_should_have_decorated_tasks_functions(): 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() db_notification = Notification.query.filter_by(id=notification.id).one()
assert db_notification.status == 'technical-failure' 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'