From 0324ba02ff601056d4c8af3285cc46e788c4c96a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 17 Jan 2017 12:00:34 +0000 Subject: [PATCH 1/5] split process_job up into process_job and process_row makes tests a bit cleaner, and makes it much easier to test letter functionality. haven't moved many tests around, just changed a couple of mock calls --- app/celery/tasks.py | 65 +++++++++------- tests/app/celery/test_tasks.py | 131 ++++++++++----------------------- 2 files changed, 77 insertions(+), 119 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d84424333..10852929d 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -24,6 +24,7 @@ from app.dao.templates_dao import dao_get_template_by_id from app.models import ( EMAIL_TYPE, SMS_TYPE, + LETTER_TYPE, KEY_TYPE_NORMAL ) from app.notifications.process_notifications import persist_notification @@ -58,33 +59,7 @@ def process_job(job_id): template_type=template.template_type, placeholders=template.placeholders ).enumerated_recipients_and_personalisation: - - encrypted = encryption.encrypt({ - 'template': str(template.id), - 'template_version': job.template_version, - 'job': str(job.id), - 'to': recipient, - 'row_number': row_number, - 'personalisation': dict(personalisation) - }) - - if template.template_type == SMS_TYPE: - send_sms.apply_async(( - str(job.service_id), - create_uuid(), - encrypted, - datetime.utcnow().strftime(DATETIME_FORMAT)), - queue='db-sms' if not service.research_mode else 'research-mode' - ) - - if template.template_type == EMAIL_TYPE: - send_email.apply_async(( - str(job.service_id), - create_uuid(), - encrypted, - datetime.utcnow().strftime(DATETIME_FORMAT)), - queue='db-email' if not service.research_mode else 'research-mode' - ) + process_row(row_number, recipient, personalisation, template, job, service) finished = datetime.utcnow() job.job_status = 'finished' @@ -96,6 +71,39 @@ def process_job(job_id): ) +def process_row(row_number, recipient, personalisation, template, job, service): + template_type = template.template_type + + encrypted = encryption.encrypt({ + 'template': str(template.id), + 'template_version': job.template_version, + 'job': str(job.id), + 'to': recipient, + 'row_number': row_number, + 'personalisation': dict(personalisation) + }) + + send_fns = { + SMS_TYPE: send_sms, + EMAIL_TYPE: send_email, + } + + queues = { + SMS_TYPE: 'db-sms', + EMAIL_TYPE: 'db-email', + } + + send_fn = send_fns[template_type] + + send_fn.apply_async(( + str(job.service_id), + create_uuid(), + encrypted, + datetime.utcnow().strftime(DATETIME_FORMAT)), + queue=queues[template_type] if not service.research_mode else 'research-mode' + ) + + def __sending_limits_for_job_exceeded(service, job, job_id): total_sent = fetch_todays_total_message_count(service.id) @@ -177,7 +185,8 @@ def send_sms(self, @notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") -def send_email(self, service_id, +def send_email(self, + service_id, notification_id, encrypted_notification, created_at, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 4315d24f6..2dd70329d 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -55,6 +55,14 @@ def test_should_have_decorated_tasks_functions(): assert send_email.__wrapped__.__name__ == 'send_email' +@pytest.fixture +def email_job_with_placeholders(notify_db, notify_db_session, sample_email_template_with_placeholders): + return sample_job(notify_db, notify_db_session, template=sample_email_template_with_placeholders) + + +# -------------- process_job tests -------------- # + + @freeze_time("2016-01-01 11:09:00.061258") def test_should_process_sms_job(sample_job, mocker): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) @@ -83,61 +91,6 @@ def test_should_process_sms_job(sample_job, mocker): assert job.job_status == 'finished' -@freeze_time("2016-01-01 11:09:00.061258") -def test_should_process_sms_job_into_research_mode_queue_if_research_mode_service(notify_db, notify_db_session, mocker): - mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) - mocker.patch('app.celery.tasks.send_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") - - service = sample_service(notify_db, notify_db_session) - service.research_mode = True - services_dao.dao_update_service(service) - job = sample_job(notify_db, notify_db_session, service=service) - - process_job(job.id) - s3.get_job_from_s3.assert_called_once_with( - str(job.service.id), - str(job.id) - ) - tasks.send_sms.apply_async.assert_called_once_with( - (str(job.service_id), - "uuid", - "something_encrypted", - "2016-01-01T11:09:00.061258Z"), - queue="research-mode" - ) - - -@freeze_time("2016-01-01 11:09:00.061258") -def test_should_process_email_job_into_research_mode_queue_if_research_mode_service( - notify_db, notify_db_session, mocker -): - mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) - mocker.patch('app.celery.tasks.send_email.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") - - service = sample_service(notify_db, notify_db_session) - service.research_mode = True - services_dao.dao_update_service(service) - template = sample_email_template(notify_db, notify_db_session, service=service) - job = sample_job(notify_db, notify_db_session, template=template, service=service) - - process_job(job.id) - s3.get_job_from_s3.assert_called_once_with( - str(job.service.id), - str(job.id) - ) - tasks.send_email.apply_async.assert_called_once_with( - (str(job.service_id), - "uuid", - "something_encrypted", - "2016-01-01T11:09:00.061258Z"), - queue="research-mode" - ) - - @freeze_time("2016-01-01 11:09:00.061258") def test_should_not_process_sms_job_if_would_exceed_send_limits(notify_db, notify_db_session, @@ -146,16 +99,14 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits(notify_db, job = sample_job(notify_db, notify_db_session, service=service, notification_count=10) mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) - mocker.patch('app.celery.tasks.send_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") + mocker.patch('app.celery.tasks.process_row') process_job(job.id) job = jobs_dao.dao_get_job_by_id(job.id) assert job.job_status == 'sending limits exceeded' assert s3.get_job_from_s3.called is False - assert tasks.send_sms.apply_async.called is False + assert tasks.process_row.called is False def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify_db, @@ -167,16 +118,14 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify sample_notification(notify_db, notify_db_session, service=service, job=job) mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) - mocker.patch('app.celery.tasks.send_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") + mocker.patch('app.celery.tasks.process_row') process_job(job.id) job = jobs_dao.dao_get_job_by_id(job.id) assert job.job_status == 'sending limits exceeded' assert s3.get_job_from_s3.called is False - assert tasks.send_sms.apply_async.called is False + assert tasks.process_row.called is False def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(notify_db, notify_db_session, mocker): @@ -186,15 +135,15 @@ def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(noti sample_notification(notify_db, notify_db_session, service=service, job=job) - mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('email')) - mocker.patch('app.celery.tasks.send_email.apply_async') + mocker.patch('app.celery.tasks.s3.get_job_from_s3') + mocker.patch('app.celery.tasks.process_row') process_job(job.id) job = jobs_dao.dao_get_job_by_id(job.id) assert job.job_status == 'sending limits exceeded' assert s3.get_job_from_s3.called is False - assert tasks.send_email.apply_async.called is False + assert tasks.process_row.called is False @freeze_time("2016-01-01 11:09:00.061258") @@ -204,26 +153,26 @@ def test_should_not_process_email_job_if_would_exceed_send_limits(notify_db, not job = sample_job(notify_db, notify_db_session, service=service, template=template) mocker.patch('app.celery.tasks.s3.get_job_from_s3') - mocker.patch('app.celery.tasks.send_email.apply_async') + mocker.patch('app.celery.tasks.process_row') process_job(job.id) job = jobs_dao.dao_get_job_by_id(job.id) assert job.job_status == 'sending limits exceeded' assert s3.get_job_from_s3.called is False - assert tasks.send_email.apply_async.called is False + assert tasks.process_row.called is False def test_should_not_process_job_if_already_pending(notify_db, notify_db_session, mocker): job = sample_job(notify_db, notify_db_session, job_status='scheduled') mocker.patch('app.celery.tasks.s3.get_job_from_s3') - mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.celery.tasks.process_row') process_job(job.id) assert s3.get_job_from_s3.called is False - assert tasks.send_sms.apply_async.called is False + assert tasks.process_row.called is False @freeze_time("2016-01-01 11:09:00.061258") @@ -270,11 +219,7 @@ def test_should_not_create_send_task_for_empty_file(sample_job, mocker): ) job = jobs_dao.dao_get_job_by_id(sample_job.id) assert job.job_status == 'finished' - - -@pytest.fixture -def email_job_with_placeholders(notify_db, notify_db_session, sample_email_template_with_placeholders): - return sample_job(notify_db, notify_db_session, template=sample_email_template_with_placeholders) + assert tasks.send_sms.apply_async.called is False @freeze_time("2016-01-01 11:09:00.061258") @@ -329,11 +274,16 @@ def test_should_process_all_sms_job(sample_job, assert encryption.encrypt.call_args[0][0][ 'template_version'] == sample_job_with_placeholdered_template.template.version # noqa assert encryption.encrypt.call_args[0][0]['personalisation'] == {'phonenumber': '+441234123120', 'name': 'chris'} - tasks.send_sms.apply_async.call_count == 10 + assert tasks.send_sms.apply_async.call_count == 10 job = jobs_dao.dao_get_job_by_id(sample_job_with_placeholdered_template.id) assert job.job_status == 'finished' +# -------------- process_row tests -------------- # + +# -------- send_sms and send_email tests -------- # + + def test_should_send_template_to_correct_sms_task_and_persist(sample_template_with_placeholders, mocker): notification = _notification_json(sample_template_with_placeholders, to="+447234123123", personalisation={"name": "Jo"}) @@ -639,28 +589,27 @@ def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, no def test_should_use_email_template_and_persist(sample_email_template_with_placeholders, sample_api_key, mocker): + mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + + now = datetime(2016, 1, 1, 11, 9, 0) + notification_id = uuid.uuid4() + with freeze_time("2016-01-01 12:00:00.000000"): notification = _notification_json( sample_email_template_with_placeholders, 'my_email@my_email.com', {"name": "Jo"}, row_number=1) - mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - notification_id = uuid.uuid4() - - with freeze_time("2016-01-01 11:09:00.00000"): - now = datetime.utcnow() - - with freeze_time("2016-01-01 11:10:00.00000"): - send_email( - sample_email_template_with_placeholders.service_id, - notification_id, - encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT), - api_key_id=str(sample_api_key.id), - key_type=sample_api_key.key_type - ) + with freeze_time("2016-01-01 11:10:00.00000"): + send_email( + sample_email_template_with_placeholders.service_id, + notification_id, + encryption.encrypt(notification), + now.strftime(DATETIME_FORMAT), + api_key_id=str(sample_api_key.id), + key_type=sample_api_key.key_type + ) persisted_notification = Notification.query.one() assert persisted_notification.to == 'my_email@my_email.com' From 542f08d0b50aad2d6a81f462ad5f76572a3cdb39 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 17 Jan 2017 16:51:27 +0000 Subject: [PATCH 2/5] refactor exception handling code in job tasks code it's almost entirely duplicated so share it across. also clean up retrying - `task.retry(...)` raises a celery.exceptions.Retry object, so you do not need to `raise` its response. additionally, cleaned up tests around that since raising Exception and asserting Exception is raised is dangerous as it could mask actual programming errors --- app/celery/tasks.py | 69 +++++++++++++++------------------- tests/app/celery/test_tasks.py | 38 +++++++++++++++---- 2 files changed, 60 insertions(+), 47 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 10852929d..1f130f42d 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -51,7 +51,7 @@ def process_job(job_id): db_template = dao_get_template_by_id(job.template_id, job.template_version) - TemplateClass = SMSMessageTemplate if db_template.template_type == SMS_TYPE else WithSubjectTemplate + TemplateClass = get_template_class(db_template) template = TemplateClass(db_template.__dict__) for row_number, recipient, personalisation in RecipientCSV( @@ -162,25 +162,7 @@ def send_sms(self, ) except SQLAlchemyError as e: - if not get_notification_by_id(notification_id): - # Sometimes, SQS plays the same message twice. We should be able to catch an IntegrityError, but it seems - # SQLAlchemy is throwing a FlushError. So we check if the notification id already exists then do not - # send to the retry queue. - current_app.logger.error( - "RETRY: send_sms notification for job {} row number {} and notification id {}".format( - notification.get('job', None), - notification.get('row_number', None), - notification_id)) - current_app.logger.exception(e) - try: - raise self.retry(queue="retry", exc=e) - except self.MaxRetriesExceededError: - current_app.logger.error( - "RETRY FAILED: send_sms notification for job {} row number {} and notification id {}".format( - notification.get('job', None), - notification.get('row_number', None), - notification_id)) - current_app.logger.exception(e) + handle_exception(self, notification, notification_id, e) @notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=300) @@ -222,22 +204,31 @@ def send_email(self, current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) except SQLAlchemyError as e: - if not get_notification_by_id(notification_id): - # Sometimes, SQS plays the same message twice. We should be able to catch an IntegrityError, but it seems - # SQLAlchemy is throwing a FlushError. So we check if the notification id already exists then do not - # send to the retry queue. - current_app.logger.error( - "RETRY: send_sms notification for job {} row number {} and notification id {}".format( - notification.get('job', None), - notification.get('row_number', None), - notification_id)) - current_app.logger.exception(e) - try: - raise self.retry(queue="retry", exc=e) - except self.MaxRetriesExceededError: - current_app.logger.error( - "RETRY FAILED: send_sms notification for job {} row number {} and notification id {}".format( - notification.get('job', None), - notification.get('row_number', None), - notification_id)) - current_app.logger.exception(e) + handle_exception(self, notification, notification_id, e) + + +def handle_exception(task, notification, notification_id, exc): + if not get_notification_by_id(notification_id): + retry_msg = '{task} notification for job {job} row number {row} and notification id {noti}'.format( + task=task.__name__, + job=notification.get('job', None), + row=notification.get('row_number', None), + noti=notification_id + ) + # Sometimes, SQS plays the same message twice. We should be able to catch an IntegrityError, but it seems + # SQLAlchemy is throwing a FlushError. So we check if the notification id already exists then do not + # send to the retry queue. + current_app.logger.exception('Retry' + retry_msg) + try: + task.retry(queue="retry", exc=exc) + except task.MaxRetriesExceededError: + current_app.logger.exception('Retry' + retry_msg) + + +def get_template_class(template): + if template.template_type == SMS_TYPE: + return SMSMessageTemplate + elif template.template_type in (EMAIL_TYPE, LETTER_TYPE): + # since we don't need rendering capabilities (we only need to extract placeholders) both email and letter can + # use the same base template + return WithSubjectTemplate diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 2dd70329d..78235001a 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,19 +1,27 @@ import uuid -import pytest from datetime import datetime +from unittest.mock import Mock + +import pytest from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError +from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate +from celery.exceptions import Retry + from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks from app.celery import tasks from app.celery.tasks import s3 from app.celery.tasks import ( - send_sms, process_job, - send_email + process_row, + send_sms, + send_email, + get_template_class ) from app.dao import jobs_dao, services_dao -from app.models import Notification, KEY_TYPE_TEAM, KEY_TYPE_TEST, KEY_TYPE_NORMAL +from app.models import Notification, KEY_TYPE_TEAM, KEY_TYPE_TEST, KEY_TYPE_NORMAL, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE + from tests.app import load_example_csv from tests.app.conftest import ( sample_service, @@ -715,19 +723,23 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em queue='send-email') +class MyException(Exception): + pass + + def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker): notification = _notification_json(sample_template, "+447234123123") expected_exception = SQLAlchemyError() mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.celery.tasks.send_sms.retry', side_effect=Exception()) + mocker.patch('app.celery.tasks.send_sms.retry', side_effect=Retry) mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception) now = datetime.utcnow() notification_id = uuid.uuid4() - with pytest.raises(Exception): + with pytest.raises(Retry): send_sms( sample_template.service_id, notification_id, @@ -746,13 +758,13 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem expected_exception = SQLAlchemyError() mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - mocker.patch('app.celery.tasks.send_email.retry', side_effect=Exception()) + mocker.patch('app.celery.tasks.send_email.retry', side_effect=Retry) mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception) now = datetime.utcnow() notification_id = uuid.uuid4() - with pytest.raises(Exception): + with pytest.raises(Retry): send_email( sample_email_template.service_id, notification_id, @@ -801,3 +813,13 @@ def test_send_sms_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample assert Notification.query.count() == 1 assert not deliver_sms.called assert not retry.called + + +@pytest.mark.parametrize('template_type, expected_class', [ + (SMS_TYPE, SMSMessageTemplate), + (EMAIL_TYPE, WithSubjectTemplate), + (LETTER_TYPE, WithSubjectTemplate), +]) +def test_get_template_class(template_type, expected_class): + template = Mock(template_type=template_type) + assert get_template_class(template) == expected_class From c904025ee954eca4639cfbc35d3d5f6a7c012731 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 18 Jan 2017 11:29:38 +0000 Subject: [PATCH 3/5] add comprehensive tests for process_row in the future we can probably remove some of the slower databasey process_job tests, but that's out of scope for this --- app/celery/tasks.py | 49 +++++++++++++++++++++++++++++----- tests/app/celery/test_tasks.py | 40 ++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 1f130f42d..1adc449ee 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -73,7 +73,6 @@ def process_job(job_id): def process_row(row_number, recipient, personalisation, template, job, service): template_type = template.template_type - encrypted = encryption.encrypt({ 'template': str(template.id), 'template_version': job.template_version, @@ -86,20 +85,24 @@ def process_row(row_number, recipient, personalisation, template, job, service): send_fns = { SMS_TYPE: send_sms, EMAIL_TYPE: send_email, + LETTER_TYPE: persist_letter } queues = { SMS_TYPE: 'db-sms', EMAIL_TYPE: 'db-email', + LETTER_TYPE: 'db-letter', } send_fn = send_fns[template_type] - send_fn.apply_async(( - str(job.service_id), - create_uuid(), - encrypted, - datetime.utcnow().strftime(DATETIME_FORMAT)), + send_fn.apply_async( + ( + str(service.id), + create_uuid(), + encrypted, + datetime.utcnow().strftime(DATETIME_FORMAT) + ), queue=queues[template_type] if not service.research_mode else 'research-mode' ) @@ -207,6 +210,40 @@ def send_email(self, handle_exception(self, notification, notification_id, e) +@notify_celery.task(bind=True, name="persist-letter", max_retries=5, default_retry_delay=300) +@statsd(namespace="tasks") +def persist_letter( + self, + service_id, + notification_id, + encrypted_notification, + created_at +): + notification = encryption.decrypt(encrypted_notification) + service = dao_fetch_service_by_id(service_id) + try: + saved_notification = persist_notification( + template_id=notification['template'], + template_version=notification['template_version'], + recipient=notification['to'], + service=service, + personalisation=notification.get('personalisation'), + notification_type=EMAIL_TYPE, + api_key_id=None, + key_type=KEY_TYPE_NORMAL, + created_at=created_at, + job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), + notification_id=notification_id + ) + + # TODO: deliver letters + + current_app.logger.info("Letter {} created at {}".format(saved_notification.id, created_at)) + except SQLAlchemyError as e: + handle_exception(self, notification, notification_id, e) + + def handle_exception(task, notification, notification_id, exc): if not get_notification_by_id(notification_id): retry_msg = '{task} notification for job {job} row number {row} and notification id {noti}'.format( diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 78235001a..66ad2f256 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,6 +1,6 @@ import uuid from datetime import datetime -from unittest.mock import Mock +from unittest.mock import Mock, ANY import pytest from freezegun import freeze_time @@ -289,6 +289,44 @@ def test_should_process_all_sms_job(sample_job, # -------------- process_row tests -------------- # + +@freeze_time('2001-01-01T12:00:00') +@pytest.mark.parametrize('template_type, research_mode, expected_function, expected_queue', [ + (SMS_TYPE, False, 'send_sms', 'db-sms'), + (SMS_TYPE, True, 'send_sms', 'research-mode'), + (EMAIL_TYPE, False, 'send_email', 'db-email'), + (EMAIL_TYPE, True, 'send_email', 'research-mode'), + (LETTER_TYPE, False, 'persist_letter', 'db-letter'), + (LETTER_TYPE, True, 'persist_letter', 'research-mode'), +]) +def test_process_row_sends_letter_task(template_type, research_mode, expected_function, expected_queue, mocker): + mocker.patch('app.celery.tasks.create_uuid', return_value='noti_uuid') + task_mock = mocker.patch('app.celery.tasks.{}.apply_async'.format(expected_function)) + encrypt_mock = mocker.patch('app.celery.tasks.encryption.encrypt') + template = Mock(id='template_id', template_type=template_type) + job = Mock(id='job_id', template_version='temp_vers') + service = Mock(id='service_id', research_mode=research_mode) + + process_row('row_num', 'recip', {'foo': 'bar'}, template, job, service) + + encrypt_mock.assert_called_once_with({ + 'template': 'template_id', + 'template_version': 'temp_vers', + 'job': 'job_id', + 'to': 'recip', + 'row_number': 'row_num', + 'personalisation': {'foo': 'bar'} + }) + task_mock.assert_called_once_with( + ( + 'service_id', + 'noti_uuid', + # encrypted data + encrypt_mock.return_value, + '2001-01-01T12:00:00.000000Z' + ), + queue=expected_queue + ) # -------- send_sms and send_email tests -------- # From 4f238d241afc7c73cf9d0545096def4bbf3860f0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 19 Jan 2017 12:10:32 +0000 Subject: [PATCH 4/5] persist_letter saves address correctly to database the `to` field stores either the phone number or the email address of the recipient - it's a bit more complicated for letters, since there are address lines 1 through 6, and a postcode. In utils, they're stored alongside the personalisation, and we have to ensure that when we persist to the database we keep as much parity with utils to make our work easier. Aside from sending, the `to` field is also used to show recipients on the front end report pages - we've decided that the best thing to store here is address_line_1 - which is probably going to be either a person's name, company name, or PO box number Also, a lot of tests and test cleanup - I added create_template and create_notification functions in db.py, so if you're creating new fixtures you can use these functions, and you won't need to pass notify_db and notify_db_session around, huzzah! also removed create param from sample_notification since it's not used anywhere --- app/celery/tasks.py | 14 +++-- tests/app/celery/test_tasks.py | 96 ++++++++++++++++++++++++++++++---- tests/app/conftest.py | 51 +++++++++++++----- tests/app/db.py | 68 +++++++++++++++++++++++- 4 files changed, 200 insertions(+), 29 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 1adc449ee..96b07ed34 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -220,20 +220,24 @@ def persist_letter( created_at ): notification = encryption.decrypt(encrypted_notification) + + # we store the recipient as just the first item of the person's address + recipient = notification['personalisation']['addressline1'] + service = dao_fetch_service_by_id(service_id) try: saved_notification = persist_notification( template_id=notification['template'], template_version=notification['template_version'], - recipient=notification['to'], + recipient=recipient, service=service, - personalisation=notification.get('personalisation'), - notification_type=EMAIL_TYPE, + personalisation=notification['personalisation'], + notification_type=LETTER_TYPE, api_key_id=None, key_type=KEY_TYPE_NORMAL, created_at=created_at, - job_id=notification.get('job', None), - job_row_number=notification.get('row_number', None), + job_id=notification['job'], + job_row_number=notification['row_number'], notification_id=notification_id ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 66ad2f256..cc1f8df61 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,6 +1,6 @@ import uuid from datetime import datetime -from unittest.mock import Mock, ANY +from unittest.mock import Mock, ANY, call import pytest from freezegun import freeze_time @@ -17,6 +17,7 @@ from app.celery.tasks import ( process_row, send_sms, send_email, + persist_letter, get_template_class ) from app.dao import jobs_dao, services_dao @@ -41,20 +42,16 @@ class AnyStringWith(str): mmg_error = {'Error': '40', 'Description': 'error'} -def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): - notification = { +def _notification_json(template, to, personalisation=None, job_id=None, row_number=0): + return { "template": str(template.id), "template_version": template.version, "to": to, - "notification_type": template.template_type + "notification_type": template.template_type, + "personalisation": personalisation or {}, + "job": job_id and str(job_id), + "row_number": row_number } - if personalisation: - notification.update({"personalisation": personalisation}) - if job_id: - notification.update({"job": str(job_id)}) - if row_number: - notification['row_number'] = row_number - return notification def test_should_have_decorated_tasks_functions(): @@ -263,6 +260,42 @@ def test_should_process_email_job(email_job_with_placeholders, mocker): assert job.job_status == 'finished' +@freeze_time("2016-01-01 11:09:00.061258") +def test_should_process_letter_job(sample_letter_job, mocker): + csv = """address_line_1,address_line_2,address_line_3,address_line_4,postcode,name + A1,A2,A3,A4,A_POST,Alice + """ + s3_mock = mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv) + mocker.patch('app.celery.tasks.send_email.apply_async') + process_row_mock = mocker.patch('app.celery.tasks.process_row') + mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") + + process_job(sample_letter_job.id) + + s3_mock.assert_called_once_with( + str(sample_letter_job.service.id), + str(sample_letter_job.id) + ) + + row_call = process_row_mock.mock_calls[0][1] + + assert row_call[0] == 0 + assert row_call[1] == ['A1', 'A2', 'A3', 'A4', None, None, 'A_POST'] + assert dict(row_call[2]) == { + 'addressline1': 'A1', + 'addressline2': 'A2', + 'addressline3': 'A3', + 'addressline4': 'A4', + 'postcode': 'A_POST' + } + assert row_call[4] == sample_letter_job + assert row_call[5] == sample_letter_job.service + + assert process_row_mock.call_count == 1 + + assert sample_letter_job.job_status == 'finished' + + def test_should_process_all_sms_job(sample_job, sample_job_with_placeholdered_template, mocker): @@ -853,6 +886,47 @@ def test_send_sms_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample assert not retry.called +def test_persist_letter_saves_letter_to_database(sample_letter_job, mocker): + personalisation = { + 'addressline1': 'Foo', + 'addressline2': 'Bar', + 'addressline3': 'Baz', + 'addressline4': 'Wibble', + 'addressline5': 'Wobble', + 'addressline6': 'Wubble', + 'postcode': 'Flob', + } + notification_json = _notification_json( + template=sample_letter_job.template, + to='Foo', + personalisation=personalisation, + job_id=sample_letter_job.id, + row_number=1 + ) + notification_id = uuid.uuid4() + created_at = datetime.utcnow() + + persist_letter( + sample_letter_job.service_id, + notification_id, + encryption.encrypt(notification_json), + created_at + ) + + notification_db = Notification.query.one() + assert notification_db.id == notification_id + assert notification_db.to == 'Foo' + assert notification_db.job_id == sample_letter_job.id + assert notification_db.template_id == sample_letter_job.template.id + assert notification_db.template_version == sample_letter_job.template.version + assert notification_db.status == 'created' + assert notification_db.created_at == created_at + assert notification_db.notification_type == 'letter' + assert notification_db.sent_at is None + assert notification_db.sent_by is None + assert notification_db.personalisation == personalisation + + @pytest.mark.parametrize('template_type, expected_class', [ (SMS_TYPE, SMSMessageTemplate), (EMAIL_TYPE, WithSubjectTemplate), diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 559537744..dfbe54fc9 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -25,7 +25,7 @@ from app.models import ( NotificationStatistics, ServiceWhitelist, KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM, - MOBILE_TYPE, EMAIL_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED) + MOBILE_TYPE, EMAIL_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED) from app.dao.users_dao import (create_user_code, create_secret_code) from app.dao.services_dao import (dao_create_service, dao_add_user_to_service) from app.dao.templates_dao import dao_create_template @@ -35,12 +35,8 @@ from app.dao.notifications_dao import dao_create_notification from app.dao.invited_user_dao import save_invited_user from app.dao.provider_rates_dao import create_provider_rates from app.clients.sms.firetext import FiretextClient -from app.dao.provider_details_dao import ( - dao_update_provider_details, - get_provider_details_by_identifier, - get_alternative_sms_provider -) -from tests.app.db import create_user + +from tests.app.db import create_user, create_template, create_notification @pytest.yield_fixture @@ -228,6 +224,11 @@ def sample_email_template( return template +@pytest.fixture +def sample_letter_template(sample_service): + return create_template(sample_service, template_type=LETTER_TYPE) + + @pytest.fixture(scope='function') def sample_email_template_with_placeholders(notify_db, notify_db_session): return sample_email_template( @@ -363,6 +364,24 @@ def sample_email_job(notify_db, return job +@pytest.fixture +def sample_letter_job(sample_service, sample_letter_template): + data = { + 'id': uuid.uuid4(), + 'service_id': sample_service.id, + 'service': sample_service, + 'template_id': sample_letter_template.id, + 'template_version': sample_letter_template.version, + 'original_file_name': 'some.csv', + 'notification_count': 1, + 'created_at': datetime.utcnow(), + 'created_by': sample_service.created_by, + } + job = Job(**data) + dao_create_job(job) + return job + + @pytest.fixture(scope='function') def sample_notification_with_job( notify_db, @@ -377,7 +396,6 @@ def sample_notification_with_job( created_at=None, sent_at=None, billable_units=1, - create=True, personalisation=None, api_key_id=None, key_type=KEY_TYPE_NORMAL @@ -398,7 +416,6 @@ def sample_notification_with_job( created_at=created_at, sent_at=sent_at, billable_units=billable_units, - create=create, personalisation=personalisation, api_key_id=api_key_id, key_type=key_type @@ -418,7 +435,6 @@ def sample_notification(notify_db, created_at=None, sent_at=None, billable_units=1, - create=True, personalisation=None, api_key_id=None, key_type=KEY_TYPE_NORMAL, @@ -464,11 +480,22 @@ def sample_notification(notify_db, if job_row_number: data['job_row_number'] = job_row_number notification = Notification(**data) - if create: - dao_create_notification(notification) + dao_create_notification(notification) return notification +@pytest.fixture +def sample_letter_notification(sample_letter_template): + address = { + 'addressline1': 'A1', + 'addressline2': 'A2', + 'addressline3': 'A3', + 'addressline4': 'A4', + 'postcode': 'A_POST' + } + return create_notification(sample_letter_template, personalisation=address) + + @pytest.fixture(scope='function') def sample_notification_with_api_key(notify_db, notify_db_session): notification = sample_notification(notify_db, notify_db_session) diff --git a/tests/app/db.py b/tests/app/db.py index cb64f86db..2b9e97b6d 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,5 +1,10 @@ -from app.models import User +from datetime import datetime +import uuid + +from app.models import User, Template, Notification, SMS_TYPE, KEY_TYPE_NORMAL from app.dao.users_dao import save_model_user +from app.dao.notifications_dao import dao_create_notification +from app.dao.templates_dao import dao_create_template def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk"): @@ -15,3 +20,64 @@ def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-off user = User(**data) save_model_user(user) return user + + +def create_template(service, user=None, template_type=SMS_TYPE): + data = { + 'name': '{} Template Name'.format(template_type), + 'template_type': template_type, + 'content': 'Dear Sir/Madam, Hello. Yours Truly, The Government.', + 'service': service, + 'created_by': service.created_by, + } + if template_type != SMS_TYPE: + data['subject'] = 'Template subject' + template = Template(**data) + dao_create_template(template) + return template + + +def create_notification( + template, + job=None, + job_row_number=None, + to_field='+447700900855', + status='created', + reference=None, + created_at=None, + sent_at=None, + billable_units=1, + personalisation=None, + api_key_id=None, + key_type=KEY_TYPE_NORMAL, + sent_by=None, + client_reference=None +): + if created_at is None: + created_at = datetime.utcnow() + data = { + 'id': uuid.uuid4(), + 'to': to_field, + 'job_id': job.id if job else None, + 'job': job, + 'service_id': template.service_id, + 'template_id': template.id if template else None, + 'template': template, + 'template_version': template.version, + 'status': status, + 'reference': reference, + 'created_at': created_at, + 'sent_at': sent_at, + 'billable_units': billable_units, + 'personalisation': personalisation, + 'notification_type': template.template_type, + 'api_key_id': api_key_id, + 'key_type': key_type, + 'sent_by': sent_by, + 'updated_at': None, + 'client_reference': client_reference, + 'job_row_number': None + } + notification = Notification(**data) + dao_create_notification(notification) + return notification From 70cd3fb335879859c11f8be75620d3e939302988 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 24 Jan 2017 10:53:41 +0000 Subject: [PATCH 5/5] ensure that the celery workers know about the new db-letter queue this fixes running locally and on paas, a separate PR is in notifications-aws to fix work on aws --- app/celery/tasks.py | 8 ++++---- app/config.py | 6 ++++-- manifest-delivery-worker-database.yml | 2 +- tests/app/celery/test_tasks.py | 7 +------ 4 files changed, 10 insertions(+), 13 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 96b07ed34..6d09fa785 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -51,7 +51,7 @@ def process_job(job_id): db_template = dao_get_template_by_id(job.template_id, job.template_version) - TemplateClass = get_template_class(db_template) + TemplateClass = get_template_class(db_template.template_type) template = TemplateClass(db_template.__dict__) for row_number, recipient, personalisation in RecipientCSV( @@ -266,10 +266,10 @@ def handle_exception(task, notification, notification_id, exc): current_app.logger.exception('Retry' + retry_msg) -def get_template_class(template): - if template.template_type == SMS_TYPE: +def get_template_class(template_type): + if template_type == SMS_TYPE: return SMSMessageTemplate - elif template.template_type in (EMAIL_TYPE, LETTER_TYPE): + elif template_type in (EMAIL_TYPE, LETTER_TYPE): # since we don't need rendering capabilities (we only need to extract placeholders) both email and letter can # use the same base template return WithSubjectTemplate diff --git a/app/config.py b/app/config.py index 8d10051ef..759d2af43 100644 --- a/app/config.py +++ b/app/config.py @@ -167,8 +167,9 @@ class Development(Config): SQLALCHEMY_ECHO = False CELERY_QUEUES = Config.CELERY_QUEUES + [ Queue('db-sms', Exchange('default'), routing_key='db-sms'), - Queue('send-sms', Exchange('default'), routing_key='send-sms'), Queue('db-email', Exchange('default'), routing_key='db-email'), + Queue('db-letter', Exchange('default'), routing_key='db-letter'), + Queue('send-sms', Exchange('default'), routing_key='send-sms'), Queue('send-email', Exchange('default'), routing_key='send-email'), Queue('research-mode', Exchange('default'), routing_key='research-mode') ] @@ -186,8 +187,9 @@ class Test(Config): STATSD_PORT = 1000 CELERY_QUEUES = Config.CELERY_QUEUES + [ Queue('db-sms', Exchange('default'), routing_key='db-sms'), - Queue('send-sms', Exchange('default'), routing_key='send-sms'), Queue('db-email', Exchange('default'), routing_key='db-email'), + Queue('db-letter', Exchange('default'), routing_key='db-letter'), + Queue('send-sms', Exchange('default'), routing_key='send-sms'), Queue('send-email', Exchange('default'), routing_key='send-email'), Queue('research-mode', Exchange('default'), routing_key='research-mode') ] diff --git a/manifest-delivery-worker-database.yml b/manifest-delivery-worker-database.yml index eaff9e337..ab7704c3b 100644 --- a/manifest-delivery-worker-database.yml +++ b/manifest-delivery-worker-database.yml @@ -14,6 +14,6 @@ applications: - hosted-graphite instances: 2 memory: 256M - command: celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q db-sms,db-email + command: celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q db-sms,db-email,db-letter env: NOTIFY_APP_NAME: delivery-worker-database diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index cc1f8df61..9af7b3ff7 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -794,10 +794,6 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em queue='send-email') -class MyException(Exception): - pass - - def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker): notification = _notification_json(sample_template, "+447234123123") @@ -933,5 +929,4 @@ def test_persist_letter_saves_letter_to_database(sample_letter_job, mocker): (LETTER_TYPE, WithSubjectTemplate), ]) def test_get_template_class(template_type, expected_class): - template = Mock(template_type=template_type) - assert get_template_class(template) == expected_class + assert get_template_class(template_type) == expected_class