From f52755742c8c0507463b350fa599701589e658af Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 1 Jul 2016 14:14:28 +0100 Subject: [PATCH 1/3] Split send_email task into one task to create the notification and one to send it to the provider. Is there is an exception the task will go to the retry queue. --- app/__init__.py | 1 - app/celery/provider_tasks.py | 75 +++++- app/celery/tasks.py | 115 +++------- tests/app/celery/test_provider_tasks.py | 130 ++++++++++- tests/app/celery/test_tasks.py | 288 +++++------------------- 5 files changed, 274 insertions(+), 335 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 08b5cc640..fad8bd3e5 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -1,7 +1,6 @@ import uuid import os -import statsd from flask import request, url_for, g from flask import Flask, _request_ctx_stack from flask.ext.sqlalchemy import SQLAlchemy diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index e7aeea7ab..75bd1ef06 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,9 +1,8 @@ -import json - from datetime import datetime from monotonic import monotonic from flask import current_app -from app import notify_celery, statsd_client, encryption, clients +from app import notify_celery, statsd_client, clients, create_uuid +from app.clients.email import EmailClientException from app.clients.sms import SmsClientException from app.dao.notifications_dao import ( update_provider_stats, @@ -11,19 +10,17 @@ from app.dao.notifications_dao import ( dao_update_notification, update_notification_status_by_id) from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.dao.services_dao import dao_fetch_service_by_id -from app.celery.research_mode_tasks import send_sms_response - +from app.celery.research_mode_tasks import send_sms_response, send_email_response from notifications_utils.recipients import ( validate_and_format_phone_number ) from app.dao.templates_dao import dao_get_template_by_id from notifications_utils.template import ( - Template, - unlink_govuk_escaped + Template ) -from app.models import SMS_TYPE +from app.models import SMS_TYPE, EMAIL_TYPE def retry_iteration_to_delay(retry=0): @@ -50,7 +47,7 @@ def retry_iteration_to_delay(retry=0): @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) -def send_sms_to_provider(self, service_id, notification_id, encrypted_notification=None): +def send_sms_to_provider(self, service_id, notification_id): task_start = monotonic() service = dao_fetch_service_by_id(service_id) @@ -98,7 +95,7 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati update_notification_status_by_id(notification.id, 'technical-failure', 'failure') current_app.logger.info( - "SMS {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at) + "SMS {} sent at {}".format(notification_id, notification.sent_at) ) statsd_client.incr("notifications.tasks.send-sms-to-provider") statsd_client.timing("notifications.tasks.send-sms-to-provider.task-time", monotonic() - task_start) @@ -117,3 +114,61 @@ def provider_to_use(notification_type, notification_id): raise Exception("No active {} providers".format(notification_type)) return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) + + +@notify_celery.task(bind=True, name="send-email-to-provider", max_retries=5, default_retry_delay=5) +def send_email_to_provider(self, service_id, notification_id, reply_to_addresses=None): + task_start = monotonic() + service = dao_fetch_service_by_id(service_id) + provider = provider_to_use(EMAIL_TYPE, notification_id) + notification = get_notification_by_id(notification_id) + if notification.status == 'created': + try: + template = Template( + dao_get_template_by_id(notification.template_id, notification.template_version).__dict__, + values=notification.personalisation + ) + + if service.research_mode: + reference = str(create_uuid()) + send_email_response.apply_async( + (provider.get_name(), reference, notification.to), queue='research-mode' + ) + else: + from_address = '"{}" <{}@{}>'.format(service.name, service.email_from, + current_app.config['NOTIFY_EMAIL_DOMAIN']) + reference = provider.send_email( + from_address, + notification.to, + template.replaced_subject, + body=template.replaced_govuk_escaped, + html_body=template.as_HTML_email, + reply_to_addresses=reply_to_addresses, + ) + + update_provider_stats( + notification_id, + EMAIL_TYPE, + provider.get_name() + ) + notification.reference = reference + notification.sent_at = datetime.utcnow() + notification.sent_by = provider.get_name(), + notification.status = 'sending' + dao_update_notification(notification) + except EmailClientException as e: + try: + current_app.logger.error( + "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: + update_notification_status_by_id(notification.id, 'technical-failure', 'failure') + + current_app.logger.info( + "Email {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at) + ) + statsd_client.incr("notifications.tasks.send-email-to-provider") + statsd_client.timing("notifications.tasks.send-email-to-provider.task-time", monotonic() - task_start) + statsd_client.timing("notifications.email.total-time", datetime.utcnow() - notification.created_at) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index cc521771b..a5662a6ba 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -19,18 +19,14 @@ from app import ( encryption ) from app.aws import s3 -from app.celery.provider_tasks import send_sms_to_provider -from app.celery.research_mode_tasks import send_email_response -from app.clients.email import EmailClientException +from app.celery.provider_tasks import send_sms_to_provider, send_email_to_provider from app.dao.jobs_dao import ( dao_update_job, dao_get_job_by_id ) from app.dao.notifications_dao import ( dao_create_notification, - dao_update_notification, - dao_get_notification_statistics_for_service_and_day, - update_provider_stats + dao_get_notification_statistics_for_service_and_day ) from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.dao.services_dao import dao_fetch_service_by_id @@ -145,27 +141,12 @@ def send_sms(self, service_id, notification_id, encrypted_notification, created_ return try: - - sent_at = datetime.utcnow() - notification_db_object = Notification( - id=notification_id, - template_id=notification['template'], - template_version=notification['template_version'], - to=notification['to'], - service_id=service_id, - job_id=notification.get('job', None), - job_row_number=notification.get('row_number', None), - status='created', - created_at=datetime.strptime(created_at, DATETIME_FORMAT), - personalisation=notification.get('personalisation'), - notification_type=SMS_TYPE - ) - dao_create_notification(notification_db_object, SMS_TYPE) + _save_notification(created_at, notification, notification_id, service_id, SMS_TYPE) send_sms_to_provider.apply_async((service_id, notification_id), queue='sms') current_app.logger.info( - "SMS {} created at {} sent at {}".format(notification_id, created_at, sent_at) + "SMS {} created at {}".format(notification_id, created_at) ) statsd_client.incr("notifications.tasks.send-sms") @@ -175,8 +156,8 @@ def send_sms(self, service_id, notification_id, encrypted_notification, created_ raise self.retry(queue="retry", exc=e) -@notify_celery.task(name="send-email") -def send_email(service_id, notification_id, encrypted_notification, created_at, reply_to_addresses=None): +@notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=5) +def send_email(self, service_id, notification_id, encrypted_notification, created_at, reply_to_addresses=None): task_start = monotonic() notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) @@ -190,74 +171,34 @@ def send_email(service_id, notification_id, encrypted_notification, created_at, return try: - sent_at = datetime.utcnow() - notification_db_object = Notification( - id=notification_id, - template_id=notification['template'], - template_version=notification['template_version'], - to=notification['to'], - service_id=service_id, - job_id=notification.get('job', None), - job_row_number=notification.get('row_number', None), - status='sending', - created_at=datetime.strptime(created_at, DATETIME_FORMAT), - sent_at=sent_at, - sent_by=provider.get_name(), - personalisation=notification.get('personalisation'), - notification_type=EMAIL_TYPE - ) + _save_notification(created_at, notification, notification_id, service_id, EMAIL_TYPE) - dao_create_notification(notification_db_object, EMAIL_TYPE) - statsd_client.timing_with_dates( - "notifications.tasks.send-email.queued-for", - sent_at, - datetime.strptime(created_at, DATETIME_FORMAT) - ) + send_email_to_provider.apply_async((service_id, notification_id, reply_to_addresses), + queue='email') - try: - template = Template( - dao_get_template_by_id(notification['template'], notification['template_version']).__dict__, - values=notification.get('personalisation', {}) - ) - - if service.research_mode: - reference = create_uuid() - send_email_response.apply_async( - (provider.get_name(), str(reference), notification['to']), queue='research-mode' - ) - else: - from_address = '"{}" <{}@{}>'.format(service.name, service.email_from, - current_app.config['NOTIFY_EMAIL_DOMAIN']) - reference = provider.send_email( - from_address, - notification['to'], - template.replaced_subject, - body=template.replaced_govuk_escaped, - html_body=template.as_HTML_email, - reply_to_addresses=reply_to_addresses, - ) - - update_provider_stats( - notification_id, - 'email', - provider.get_name() - ) - - notification_db_object.reference = reference - dao_update_notification(notification_db_object) - - except EmailClientException as e: - current_app.logger.exception(e) - notification_db_object.status = 'technical-failure' - dao_update_notification(notification_db_object) - - current_app.logger.info( - "Email {} created at {} sent at {}".format(notification_id, created_at, sent_at) - ) + current_app.logger.info("Email {} created at {}".format(notification_id, created_at)) statsd_client.incr("notifications.tasks.send-email") statsd_client.timing("notifications.tasks.send-email.task-time", monotonic() - task_start) except SQLAlchemyError as e: current_app.logger.exception(e) + raise self.retry(queue="retry", exc=e) + + +def _save_notification(created_at, notification, notification_id, service_id, notification_type): + notification_db_object = Notification( + id=notification_id, + template_id=notification['template'], + template_version=notification['template_version'], + to=notification['to'], + service_id=service_id, + job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), + status='created', + created_at=datetime.strptime(created_at, DATETIME_FORMAT), + personalisation=notification.get('personalisation'), + notification_type=EMAIL_TYPE + ) + dao_create_notification(notification_db_object, notification_type) def service_allowed_to_send_to(recipient, service): diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index cce6458cf..bd5b01f0f 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,3 +1,4 @@ +import uuid from datetime import datetime from celery.exceptions import MaxRetriesExceededError @@ -7,12 +8,14 @@ from notifications_utils.recipients import validate_phone_number, format_phone_n import app from app import statsd_client, mmg_client from app.celery import provider_tasks -from app.celery.provider_tasks import send_sms_to_provider -from app.celery.research_mode_tasks import send_sms_response +from app.celery.provider_tasks import send_sms_to_provider, send_email_to_provider +from app.celery.research_mode_tasks import send_sms_response, send_email_response from app.celery.tasks import provider_to_use +from app.clients.email import EmailClientException from app.clients.sms import SmsClientException from app.dao import notifications_dao, provider_details_dao from app.dao import provider_statistics_dao +from app.dao.provider_statistics_dao import get_provider_statistics from app.models import Notification, NotificationStatistics, Job from tests.app.conftest import ( sample_notification @@ -289,3 +292,126 @@ def test_should_go_into_technical_error_if_exceeds_retries( job = Job.query.get(notification.job.id) assert job.notification_count == 1 assert job.notifications_failed == 1 + + +def test_send_email_to_provider_should_call_research_mode_task_response_task_if_research_mode( + notify_db, + notify_db_session, + sample_service, + sample_email_template, + ses_provider, + mocker): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_email_template, + to_field="john@smith.com" + ) + + reference = uuid.uuid4() + print(reference) + mocker.patch('app.uuid.uuid4', return_value=reference) + mocker.patch('app.aws_ses_client.send_email') + mocker.patch('app.aws_ses_client.get_name', return_value="ses") + mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') + + sample_service.research_mode = True + notify_db.session.add(sample_service) + notify_db.session.commit() + assert not get_provider_statistics( + sample_email_template.service, + providers=[ses_provider.identifier]).first() + send_email_to_provider( + sample_service.id, + notification.id + ) + assert not app.aws_ses_client.send_email.called + send_email_response.apply_async.assert_called_once_with( + ('ses', str(reference), 'john@smith.com'), queue="research-mode" + ) + assert not get_provider_statistics( + sample_email_template.service, + providers=[ses_provider.identifier]).first() + persisted_notification = Notification.query.filter_by(id=notification.id).one() + + assert persisted_notification.to == 'john@smith.com' + assert persisted_notification.template_id == sample_email_template.id + assert persisted_notification.status == 'sending' + assert persisted_notification.sent_at <= datetime.utcnow() + assert persisted_notification.created_at <= datetime.utcnow() + assert persisted_notification.sent_by == 'ses' + assert persisted_notification.reference == str(reference) + + +def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retries( + 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.statsd_client.incr') + mocker.patch('app.statsd_client.timing') + 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) + assert statsd_client.incr.assert_not_called + assert statsd_client.timing.assert_not_called + + db_notification = Notification.query.filter_by(id=notification.id).one() + assert db_notification.status == 'technical-failure' + notification_stats = NotificationStatistics.query.filter_by(service_id=notification.service.id).first() + assert notification_stats.emails_requested == 1 + assert notification_stats.emails_failed == 1 + job = Job.query.get(notification.job.id) + assert job.notification_count == 1 + assert job.notifications_failed == 1 + + +def test_send_email_to_provider_statsd_updates(notify_db, notify_db_session, sample_service, + sample_email_template, mocker): + mocker.patch('app.statsd_client.incr') + mocker.patch('app.statsd_client.timing') + mocker.patch('app.aws_ses_client.send_email', return_value='reference') + mocker.patch('app.aws_ses_client.get_name', return_value="ses") + mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_email_template) + send_email_to_provider( + notification.service_id, + notification.id + ) + + statsd_client.incr.assert_called_once_with("notifications.tasks.send-email-to-provider") + statsd_client.timing.assert_has_calls([ + call("notifications.tasks.send-email-to-provider.task-time", ANY), + call("notifications.email.total-time", ANY) + ]) + + +def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session, + sample_service, + sample_email_template, + mocker): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_email_template, + service=sample_service, + status='sending') + mocker.patch('app.aws_ses_client.send_email') + mocker.patch('app.aws_ses_client.get_name', return_value="ses") + mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') + + send_sms_to_provider( + notification.service_id, + notification.id + ) + + app.aws_ses_client.send_email.assert_not_called() + app.celery.research_mode_tasks.send_email_response.apply_async.assert_not_called() diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 6677949e8..36e3708c0 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -7,10 +7,9 @@ from mock import ANY from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound -from app import (aws_ses_client, encryption, DATETIME_FORMAT, statsd_client) +from app import (encryption, DATETIME_FORMAT, statsd_client) from app.celery import provider_tasks from app.celery import tasks -from app.celery.research_mode_tasks import send_email_response from app.celery.tasks import s3 from app.celery.tasks import ( send_sms, @@ -18,9 +17,7 @@ from app.celery.tasks import ( provider_to_use, send_email ) -from app.clients.email.aws_ses import AwsSesClientException from app.dao import jobs_dao, provider_details_dao -from app.dao.provider_statistics_dao import get_provider_statistics from app.models import Notification from tests.app import load_example_csv from tests.app.conftest import ( @@ -408,46 +405,13 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, Notification.query.filter_by(id=notification_id).one() -def test_should_send_email_if_restricted_service_and_valid_email(notify_db, notify_db_session, mocker): - user = sample_user(notify_db, notify_db_session, email="test@restricted.com") - service = sample_service(notify_db, notify_db_session, user=user, restricted=True) - template = sample_template( - notify_db, - notify_db_session, - service=service, - template_type='email') - - notification = _notification_json(template, "test@restricted.com") - mocker.patch('app.aws_ses_client.send_email', return_value="1234") - - notification_id = uuid.uuid4() - now = datetime.utcnow() - send_email( - service.id, - notification_id, - encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT) - ) - - aws_ses_client.send_email.assert_called_once_with( - '"Sample service" ', - "test@restricted.com", - template.subject, - body=template.content, - html_body=AnyStringWith(template.content), - reply_to_addresses=None - ) - - def test_should_not_send_email_if_restricted_service_and_invalid_email_address(notify_db, notify_db_session, mocker): user = sample_user(notify_db, notify_db_session) service = sample_service(notify_db, notify_db_session, user=user, restricted=True) template = sample_template( notify_db, notify_db_session, service=service, template_type='email', subject_line='Hello' ) - notification = _notification_json(template, to="test@example.com") - mocker.patch('app.aws_ses_client.send_email') notification_id = uuid.uuid4() send_email( @@ -457,7 +421,6 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n datetime.utcnow().strftime(DATETIME_FORMAT) ) - aws_ses_client.send_email.assert_not_called() with pytest.raises(NoResultFound): Notification.query.filter_by(id=notification_id).one() @@ -503,8 +466,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing_with_dates') mocker.patch('app.statsd_client.timing') - mocker.patch('app.aws_ses_client.get_name', return_value='ses') - mocker.patch('app.aws_ses_client.send_email', return_value='ses') + mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') notification_id = uuid.uuid4() @@ -522,33 +484,21 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh now.strftime(DATETIME_FORMAT) ) freezer.stop() - - aws_ses_client.send_email.assert_called_once_with( - '"Sample service" ', - "my_email@my_email.com", - "Jo", - body="Hello Jo", - html_body=AnyStringWith("Hello Jo"), - reply_to_addresses=None - ) - statsd_client.incr.assert_called_once_with("notifications.tasks.send-email") - statsd_client.timing_with_dates.assert_called_once_with( - "notifications.tasks.send-email.queued-for", - datetime(2016, 1, 1, 11, 10, 0, 00000), - datetime(2016, 1, 1, 11, 9, 0, 00000) - ) statsd_client.timing.assert_called_once_with("notifications.tasks.send-email.task-time", ANY) persisted_notification = Notification.query.filter_by(id=notification_id).one() + provider_tasks.send_email_to_provider.apply_async.assert_called_once_with( + (sample_email_template_with_placeholders.service_id, + notification_id, None), queue='email') assert persisted_notification.id == notification_id assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id assert persisted_notification.template_version == sample_email_template_with_placeholders.version assert persisted_notification.created_at == now - assert persisted_notification.sent_at > now - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_by == 'ses' + assert not persisted_notification.sent_at + assert persisted_notification.status == 'created' + assert not persisted_notification.sent_by assert persisted_notification.job_row_number == 1 assert persisted_notification.personalisation == {'name': 'Jo'} assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) @@ -556,14 +506,11 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): notification = _notification_json(sample_email_template, 'my_email@my_email.com') - mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.aws_ses_client.send_email', return_value="1234") - mocker.patch('app.aws_ses_client.get_name', return_value='ses') version_on_notification = sample_email_template.version # Change the template from app.dao.templates_dao import dao_update_template, dao_get_template_by_id sample_email_template.content = sample_email_template.content + " another version of the template" - + mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') dao_update_template(sample_email_template) t = dao_get_template_by_id(sample_email_template.id) assert t.version > version_on_notification @@ -572,17 +519,13 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email send_email( sample_email_template.service_id, notification_id, - "encrypted-in-reality", + encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - aws_ses_client.send_email.assert_called_once_with( - '"Sample service" ', - "my_email@my_email.com", - sample_email_template.subject, - body="This is a template", - html_body=AnyStringWith("This is a template"), - reply_to_addresses=None - ) + + provider_tasks.send_email_to_provider.apply_async.assert_called_with((sample_email_template.service_id, + notification_id, + None), queue='email') persisted_notification = Notification.query.filter_by(id=notification_id).one() assert persisted_notification.id == notification_id @@ -590,16 +533,15 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email assert persisted_notification.template_id == sample_email_template.id assert persisted_notification.template_version == version_on_notification assert persisted_notification.created_at == now - assert persisted_notification.sent_at > now - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_by == 'ses' + assert not persisted_notification.sent_at + assert persisted_notification.status == 'created' + assert not persisted_notification.sent_by def test_should_use_email_template_subject_placeholders(sample_email_template_with_placeholders, mocker): notification = _notification_json(sample_email_template_with_placeholders, "my_email@my_email.com", {"name": "Jo"}) - mocker.patch('app.aws_ses_client.send_email', return_value="1234") - mocker.patch('app.aws_ses_client.get_name', return_value='ses') + mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') notification_id = uuid.uuid4() now = datetime.utcnow() @@ -609,30 +551,25 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - aws_ses_client.send_email.assert_called_once_with( - '"Sample service" ', - "my_email@my_email.com", - notification['personalisation']['name'], - body="Hello Jo", - html_body=AnyStringWith("Hello Jo"), - reply_to_addresses=None + provider_tasks.send_email_to_provider.apply_async.assert_called_once_with( + (sample_email_template_with_placeholders.service_id, + notification_id, None), queue='email' ) persisted_notification = Notification.query.filter_by(id=notification_id).one() assert persisted_notification.id == notification_id assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id assert persisted_notification.created_at == now - assert persisted_notification.sent_at > now - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_by == 'ses' + assert not persisted_notification.sent_at + assert persisted_notification.status == 'created' + assert not persisted_notification.sent_by assert persisted_notification.personalisation == {"name": "Jo"} - assert persisted_notification.reference == '1234' + assert not persisted_notification.reference def test_should_use_email_template_and_persist_without_personalisation(sample_email_template, mocker): notification = _notification_json(sample_email_template, "my_email@my_email.com") - mocker.patch('app.aws_ses_client.send_email', return_value="ref") - mocker.patch('app.aws_ses_client.get_name', return_value='ses') + mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') notification_id = uuid.uuid4() now = datetime.utcnow() @@ -642,27 +579,22 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - aws_ses_client.send_email.assert_called_once_with( - '"Sample service" ', - "my_email@my_email.com", - sample_email_template.subject, - body="This is a template", - html_body=AnyStringWith("This is a template"), - reply_to_addresses=None - ) + provider_tasks.send_email_to_provider.apply_async.assert_called_once_with((sample_email_template.service_id, + notification_id, None), queue='email') persisted_notification = Notification.query.filter_by(id=notification_id).one() assert persisted_notification.id == notification_id assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id assert persisted_notification.created_at == now - assert persisted_notification.sent_at > now - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_by == 'ses' + assert not persisted_notification.sent_at + assert persisted_notification.status == 'created' + assert not persisted_notification.sent_by assert not persisted_notification.personalisation - assert persisted_notification.reference == 'ref' + assert not persisted_notification.reference + assert persisted_notification.notification_type == 'email' -def test_should_go_to_retry_queue_if_database_errors(sample_template, mocker): +def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker): notification = _notification_json(sample_template, "+447234123123") expected_exception = SQLAlchemyError() @@ -689,55 +621,31 @@ def test_should_go_to_retry_queue_if_database_errors(sample_template, mocker): assert 'No row was found for one' in str(e.value) -def test_should_persist_notification_as_failed_if_email_client_fails(sample_email_template, mocker): - notification = _notification_json(sample_email_template, "my_email@my_email.com") - mocker.patch('app.aws_ses_client.send_email', side_effect=AwsSesClientException()) - mocker.patch('app.aws_ses_client.get_name', return_value="ses") +def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_template, mocker): + notification = _notification_json(sample_email_template, "test@example.gov.uk") + expected_exception = SQLAlchemyError() + + mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') + mocker.patch('app.celery.tasks.send_email.retry', side_effect=Exception()) + mocker.patch('app.celery.tasks.dao_create_notification', side_effect=expected_exception) now = datetime.utcnow() notification_id = uuid.uuid4() - send_email( - sample_email_template.service_id, - notification_id, - encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT) - ) - aws_ses_client.send_email.assert_called_once_with( - '"Sample service" ', - "my_email@my_email.com", - sample_email_template.subject, - body=sample_email_template.content, - html_body=AnyStringWith(sample_email_template.content), - reply_to_addresses=None - ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id - assert persisted_notification.to == 'my_email@my_email.com' - assert persisted_notification.template_id == sample_email_template.id - assert persisted_notification.template_version == sample_email_template.version - assert persisted_notification.status == 'technical-failure' - assert persisted_notification.created_at == now - assert persisted_notification.sent_by == 'ses' - assert persisted_notification.sent_at >= now + with pytest.raises(Exception): + send_email( + sample_email_template.service_id, + notification_id, + encryption.encrypt(notification), + now.strftime(DATETIME_FORMAT) + ) + provider_tasks.send_email_to_provider.apply_async.assert_not_called() + tasks.send_email.retry.assert_called_with(exc=expected_exception, queue='retry') - -def test_should_not_send_email_if_db_persistance_failed(sample_email_template, mocker): - notification = _notification_json(sample_email_template, "my_email@my_email.com") - mocker.patch('app.aws_ses_client.send_email') - mocker.patch('app.db.session.add', side_effect=SQLAlchemyError()) - now = datetime.utcnow() - - notification_id = uuid.uuid4() - - send_email( - sample_email_template.service_id, - notification_id, - encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT) - ) - aws_ses_client.send_email.assert_not_called() + with pytest.raises(NoResultFound) as e: + Notification.query.filter_by(id=notification_id).one() + assert 'No row was found for one' in str(e.value) def test_process_email_job_should_use_reply_to_email_if_present(sample_email_job, mocker, mock_celery_remove_job): @@ -760,93 +668,3 @@ def test_process_email_job_should_use_reply_to_email_if_present(sample_email_job {'reply_to_addresses': 'somereply@testservice.gov.uk'}, queue="bulk-email" ) - - -def test_should_call_send_email_response_task_if_research_mode( - notify_db, - sample_service, - sample_email_template, - mocker): - notification = _notification_json( - sample_email_template, - to="john@smith.com" - ) - reference = uuid.uuid4() - - mocker.patch('app.uuid.uuid4', return_value=reference) - mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.aws_ses_client.send_email') - mocker.patch('app.aws_ses_client.get_name', return_value="ses") - mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') - - sample_service.research_mode = True - notify_db.session.add(sample_service) - notify_db.session.commit() - - notification_id = uuid.uuid4() - - now = datetime.utcnow() - send_email( - sample_service.id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - assert not aws_ses_client.send_email.called - send_email_response.apply_async.assert_called_once_with( - ('ses', str(reference), 'john@smith.com'), queue="research-mode" - ) - - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id - assert persisted_notification.to == 'john@smith.com' - assert persisted_notification.template_id == sample_email_template.id - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_at > now - assert persisted_notification.created_at == now - assert persisted_notification.sent_by == 'ses' - assert persisted_notification.reference == str(reference) - - -def test_should_use_research_mode_task_and_not_update_provider_stats( - notify_db, - sample_service, - sample_email_template, - ses_provider, - mocker): - notification = _notification_json( - sample_email_template, - to="john@smith.com" - ) - - reference = uuid.uuid4() - - mocker.patch('app.uuid.uuid4', return_value=reference) - mocker.patch('app.aws_ses_client.send_email') - mocker.patch('app.aws_ses_client.get_name', return_value="ses") - mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') - - sample_service.research_mode = True - notify_db.session.add(sample_service) - notify_db.session.commit() - - assert not get_provider_statistics( - sample_email_template.service, - providers=[ses_provider.identifier]).first() - - notification_id = uuid.uuid4() - now = datetime.utcnow() - send_email( - sample_service.id, - notification_id, - encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT) - ) - assert not aws_ses_client.send_email.called - send_email_response.apply_async.assert_called_once_with( - ('ses', str(reference), 'john@smith.com'), queue="research-mode" - ) - - assert not get_provider_statistics( - sample_email_template.service, - providers=[ses_provider.identifier]).first() From 3aff22cf87063f3bab125a5a77918824f61e0280 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 1 Jul 2016 14:42:40 +0100 Subject: [PATCH 2/3] Improve the logging message --- app/celery/provider_tasks.py | 4 ++-- app/celery/tasks.py | 6 +----- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 75bd1ef06..3fb5e3459 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -95,7 +95,7 @@ def send_sms_to_provider(self, service_id, notification_id): update_notification_status_by_id(notification.id, 'technical-failure', 'failure') current_app.logger.info( - "SMS {} sent at {}".format(notification_id, notification.sent_at) + "SMS {} sent to provider at {}".format(notification_id, notification.sent_at) ) statsd_client.incr("notifications.tasks.send-sms-to-provider") statsd_client.timing("notifications.tasks.send-sms-to-provider.task-time", monotonic() - task_start) @@ -167,7 +167,7 @@ def send_email_to_provider(self, service_id, notification_id, reply_to_addresses update_notification_status_by_id(notification.id, 'technical-failure', 'failure') current_app.logger.info( - "Email {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at) + "Email {} sent to provider at {}".format(notification_id, notification.sent_at) ) statsd_client.incr("notifications.tasks.send-email-to-provider") statsd_client.timing("notifications.tasks.send-email-to-provider.task-time", monotonic() - task_start) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a5662a6ba..3176b883e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -162,12 +162,8 @@ def send_email(self, service_id, notification_id, encrypted_notification, create notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) - provider = provider_to_use(EMAIL_TYPE, notification_id) - if not service_allowed_to_send_to(notification['to'], service): - current_app.logger.info( - "Email {} failed as restricted service".format(notification_id) - ) + current_app.logger.info("Email {} failed as restricted service".format(notification_id)) return try: From 4a321497fc51bb9bc4879715d95b2b3be0ee24c4 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 1 Jul 2016 15:03:28 +0100 Subject: [PATCH 3/3] Remove print --- tests/app/celery/test_provider_tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index bd5b01f0f..d0f4abad9 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -307,7 +307,6 @@ def test_send_email_to_provider_should_call_research_mode_task_response_task_if_ ) reference = uuid.uuid4() - print(reference) mocker.patch('app.uuid.uuid4', return_value=reference) mocker.patch('app.aws_ses_client.send_email') mocker.patch('app.aws_ses_client.get_name', return_value="ses")