diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index c94120e79..ef98ec5e3 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,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: - try: - current_app.logger.error( - "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 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( diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 63536ae43..b7d24b245 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) @@ -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)) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 7ab7a9c20..7271e9a4c 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.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( - notify_db, - notify_db_session, - mocker): +def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_task(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( - 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') - +def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task(sample_notification, mocker): 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( - notification.id - ) + deliver_sms(sample_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 db_notification.status == 'technical-failure' + assert sample_notification.status == 'technical-failure' -def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retries_on_send_email_to_provider_task( - 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') - +def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task(sample_notification, mocker): 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( - notification.id - ) + deliver_email(sample_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' diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 0f11ecf42..4191857c5 100644 --- a/tests/app/celery/test_tasks.py +++ b/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() diff --git a/tests/app/clients/test_aws_ses.py b/tests/app/clients/test_aws_ses.py index e00d65edf..47b5ce9d0 100644 --- a/tests/app/clients/test_aws_ses.py +++ b/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)