From eafbbd9809d356572967ba6d28fcae1e477c3b17 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 10:41:39 +0000 Subject: [PATCH 01/16] Refactor send_sms and send_email to use common code to persist the notificaiton. --- app/celery/tasks.py | 61 ++++++++---- app/models.py | 8 +- app/notifications/process_notifications.py | 9 +- tests/app/celery/test_tasks.py | 102 ++++++++++----------- 4 files changed, 101 insertions(+), 79 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 9bf47c9d7..778ff8b29 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -17,7 +17,7 @@ from app.dao.jobs_dao import ( dao_update_job, dao_get_job_by_id ) -from app.dao.notifications_dao import (dao_create_notification) +from app.dao.notifications_dao import dao_create_notification from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id from app.models import ( @@ -26,6 +26,7 @@ from app.models import ( SMS_TYPE, KEY_TYPE_NORMAL ) +from app.notifications.process_notifications import persist_notification from app.service.utils import service_allowed_to_send_to from app.statsd_decorators import statsd @@ -41,16 +42,7 @@ def process_job(job_id): service = job.service - total_sent = fetch_todays_total_message_count(service.id) - - if total_sent + job.notification_count > service.message_limit: - job.job_status = 'sending limits exceeded' - job.processing_finished = datetime.utcnow() - dao_update_job(job) - current_app.logger.info( - "Job {} size {} error. Sending limits {} exceeded".format( - job_id, job.notification_count, service.message_limit) - ) + if __sending_limits_for_job_exceeded(service, job, job_id): return job.job_status = 'in progress' @@ -106,6 +98,21 @@ def process_job(job_id): ) +def __sending_limits_for_job_exceeded(service, job, job_id): + total_sent = fetch_todays_total_message_count(service.id) + + if total_sent + job.notification_count > service.message_limit: + job.job_status = 'sending limits exceeded' + job.processing_finished = datetime.utcnow() + dao_update_job(job) + current_app.logger.info( + "Job {} size {} error. Sending limits {} exceeded".format( + job_id, job.notification_count, service.message_limit) + ) + return True + return False + + @notify_celery.task(bind=True, name="send-sms", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def send_sms(self, @@ -125,11 +132,18 @@ def send_sms(self, return try: - dao_create_notification( - Notification.from_api_request( - created_at, notification, notification_id, service.id, SMS_TYPE, api_key_id, key_type - ) - ) + persist_notification(template_id=notification['template'], + template_version=notification['template_version'], + recipient=notification['to'], + service_id=service.id, + personalisation=notification.get('personalisation'), + notification_type=SMS_TYPE, + api_key_id=api_key_id, + key_type=key_type, + job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), + ) + provider_tasks.deliver_sms.apply_async( [notification_id], queue='send-sms' if not service.research_mode else 'research-mode' @@ -166,10 +180,17 @@ def send_email(self, service_id, return try: - dao_create_notification( - Notification.from_api_request( - created_at, notification, notification_id, service.id, EMAIL_TYPE, api_key_id, key_type - ) + persist_notification( + template_id=notification['template'], + template_version=notification['template_version'], + recipient=notification['to'], + service_id=service.id, + personalisation=notification.get('personalisation'), + notification_type=EMAIL_TYPE, + api_key_id=api_key_id, + key_type=key_type, + job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), ) provider_tasks.deliver_email.apply_async( diff --git a/app/models.py b/app/models.py index 526288990..fb6673ab2 100644 --- a/app/models.py +++ b/app/models.py @@ -572,7 +572,9 @@ class Notification(db.Model): personalisation, notification_type, api_key_id, - key_type): + key_type, + job_id, + job_row_number): return cls( template_id=template_id, template_version=template_version, @@ -583,7 +585,9 @@ class Notification(db.Model): personalisation=personalisation, notification_type=notification_type, api_key_id=api_key_id, - key_type=key_type + key_type=key_type, + job_id=job_id, + job_row_number=job_row_number, ) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 83a3fcf5f..91943d9fb 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -40,7 +40,9 @@ def persist_notification(template_id, personalisation, notification_type, api_key_id, - key_type): + key_type, + job_id=None, + job_row_number=None): notification = Notification.from_v2_api_request(template_id=template_id, template_version=template_version, recipient=recipient, @@ -48,7 +50,10 @@ def persist_notification(template_id, personalisation=personalisation, notification_type=notification_type, api_key_id=api_key_id, - key_type=key_type) + key_type=key_type, + job_id=job_id, + job_row_number=job_row_number + ) dao_create_notification(notification) return notification diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 4191857c5..307d33132 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,11 +1,9 @@ import uuid import pytest from datetime import datetime - from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound - from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks from app.celery import tasks @@ -337,7 +335,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi notification = _notification_json(sample_template_with_placeholders, to="+447234123123", personalisation={"name": "Jo"}) - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() @@ -352,9 +350,9 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi [notification_id], queue="send-sms" ) - - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert mocked_deliver_sms.called + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447234123123' assert persisted_notification.template_id == sample_template_with_placeholders.id assert persisted_notification.template_version == sample_template_with_placeholders.version @@ -377,7 +375,7 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic notification = _notification_json(template, to="+447234123123") - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() @@ -392,6 +390,7 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic [notification_id], queue="research-mode" ) + assert mocked_deliver_sms.called def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker): @@ -400,7 +399,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif template = sample_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, "+447700900890") # The user’s own number, but in a different format - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() encrypt_notification = encryption.encrypt(notification) @@ -416,8 +415,9 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif queue="send-sms" ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert mocked_deliver_sms.called + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447700900890' assert persisted_notification.template_id == template.id assert persisted_notification.template_version == template.version @@ -430,15 +430,15 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif assert persisted_notification.notification_type == 'sms' -def test_should_not_send_sms_if_restricted_service_and_invalid_number_with_test_key(notify_db, - notify_db_session, - mocker): +def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key(notify_db, + notify_db_session, + mocker): user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) template = sample_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, "07700 900849") - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() send_sms( @@ -454,13 +454,13 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number_with_test_ queue="send-sms" ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert mocked_deliver_sms.called + assert Notification.query.count() == 1 -def test_should_not_send_email_if_restricted_service_and_invalid_email_address_with_test_key(notify_db, - notify_db_session, - mocker): +def test_should_send_email_if_restricted_service_and_non_team_email_address_with_test_key(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( @@ -468,7 +468,7 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address_w ) notification = _notification_json(template, to="test@example.com") - mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + mocked_deliver_email = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') notification_id = uuid.uuid4() send_email( @@ -483,9 +483,8 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address_w [notification_id], queue="send-email" ) - - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + assert mocked_deliver_email.called def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): @@ -504,8 +503,7 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, datetime.utcnow().strftime(DATETIME_FORMAT) ) assert provider_tasks.deliver_sms.apply_async.called is False - with pytest.raises(NoResultFound): - Notification.query.filter_by(id=notification_id).one() + assert Notification.query.count() == 0 def test_should_not_send_email_if_restricted_service_and_invalid_email_address(notify_db, notify_db_session, mocker): @@ -524,8 +522,7 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n datetime.utcnow().strftime(DATETIME_FORMAT) ) - with pytest.raises(NoResultFound): - Notification.query.filter_by(id=notification_id).one() + assert Notification.query.count() == 0 def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_service( @@ -577,8 +574,8 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ [notification_id], queue="send-sms" ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447234123123' assert persisted_notification.job_id == sample_job.id assert persisted_notification.template_id == sample_job.template.id @@ -618,14 +615,13 @@ def test_should_not_send_email_if_team_key_and_recipient_not_in_team(sample_emai key_type=KEY_TYPE_TEAM ) - with pytest.raises(NoResultFound): - persisted_notification = Notification.query.filter_by(id=notification_id).one() - print(persisted_notification) + assert Notification.query.count() == 0 apply_async.not_called() def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, notify_db_session, mocker): + assert Notification.query.count() == 0 user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) template = sample_template(notify_db, notify_db_session, service=service) @@ -644,8 +640,7 @@ def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, no datetime.utcnow().strftime(DATETIME_FORMAT) ) assert provider_tasks.deliver_sms.apply_async.called is False - with pytest.raises(NoResultFound): - Notification.query.filter_by(id=notification_id).one() + assert Notification.query.count() == 0 def test_should_use_email_template_and_persist(sample_email_template_with_placeholders, sample_api_key, mocker): @@ -671,15 +666,14 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh key_type=sample_api_key.key_type ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() provider_tasks.deliver_email.apply_async.assert_called_once_with( [notification_id], queue='send-email') - - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] 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.created_at >= now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by @@ -712,12 +706,12 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email') - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' 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.created_at >= now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by @@ -740,11 +734,12 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi provider_tasks.deliver_email.apply_async.assert_called_once_with( [notification_id], queue='send-email' ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id assert persisted_notification.status == 'created' + assert persisted_notification.created_at >= now assert not persisted_notification.sent_by assert persisted_notification.personalisation == {"name": "Jo"} assert not persisted_notification.reference @@ -755,6 +750,8 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em notification = _notification_json(sample_email_template, "my_email@my_email.com") mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + assert Notification.query.count() == 0 + notification_id = uuid.uuid4() now = datetime.utcnow() @@ -765,12 +762,11 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em now.strftime(DATETIME_FORMAT) ) provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email') - - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] 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.created_at >= now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by @@ -786,7 +782,7 @@ def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, m 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.dao_create_notification', side_effect=expected_exception) + mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception) now = datetime.utcnow() notification_id = uuid.uuid4() @@ -801,9 +797,7 @@ def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, m assert provider_tasks.deliver_sms.apply_async.called is False tasks.send_sms.retry.assert_called_with(exc=expected_exception, queue='retry') - 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) + assert Notification.query.count() == 0 def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_template, mocker): @@ -813,7 +807,7 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem 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.dao_create_notification', side_effect=expected_exception) + mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception) now = datetime.utcnow() notification_id = uuid.uuid4() @@ -828,6 +822,4 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem assert provider_tasks.deliver_email.apply_async.called is False tasks.send_email.retry.assert_called_with(exc=expected_exception, queue='retry') - 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) + assert Notification.query.count() == 0 From c45d1f63a70ba461503e5848d16e0c47bfca4d2b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 14:56:33 +0000 Subject: [PATCH 02/16] Use the same method to persist a notification. Refactored the send_sms task to use this method. --- app/celery/tasks.py | 5 +- app/models.py | 53 +++++------------ app/notifications/process_notifications.py | 28 +++++---- tests/app/celery/test_tasks.py | 18 +++--- .../test_process_notification.py | 31 +++++++++- tests/app/test_model.py | 58 ++++++++----------- 6 files changed, 94 insertions(+), 99 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 778ff8b29..dde08958e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -17,11 +17,9 @@ from app.dao.jobs_dao import ( dao_update_job, dao_get_job_by_id ) -from app.dao.notifications_dao import dao_create_notification from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id from app.models import ( - Notification, EMAIL_TYPE, SMS_TYPE, KEY_TYPE_NORMAL @@ -140,6 +138,7 @@ def send_sms(self, notification_type=SMS_TYPE, api_key_id=api_key_id, key_type=key_type, + created_at=created_at, job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), ) @@ -189,8 +188,10 @@ def send_email(self, service_id, notification_type=EMAIL_TYPE, api_key_id=api_key_id, key_type=key_type, + created_at=created_at, job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), + ) provider_tasks.deliver_email.apply_async( diff --git a/app/models.py b/app/models.py index fb6673ab2..fadd057d3 100644 --- a/app/models.py +++ b/app/models.py @@ -538,56 +538,31 @@ class Notification(db.Model): self._personalisation = encryption.encrypt(personalisation) @classmethod - def from_api_request( - cls, - created_at, - notification, - notification_id, - service_id, - notification_type, - api_key_id, - key_type): - return cls( - 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.datetime.strptime(created_at, DATETIME_FORMAT), - personalisation=notification.get('personalisation'), - notification_type=notification_type, - api_key_id=api_key_id, - key_type=key_type - ) - - @classmethod - def from_v2_api_request(cls, - template_id, - template_version, - recipient, - service_id, - personalisation, - notification_type, - api_key_id, - key_type, - job_id, - job_row_number): + def from_request(cls, + template_id, + template_version, + recipient, + service_id, + personalisation, + notification_type, + api_key_id, + key_type, + job_id, + job_row_number, + created_at): return cls( template_id=template_id, template_version=template_version, to=recipient, service_id=service_id, status='created', - created_at=datetime.datetime.utcnow(), + created_at=created_at, personalisation=personalisation, notification_type=notification_type, api_key_id=api_key_id, key_type=key_type, job_id=job_id, - job_row_number=job_row_number, + job_row_number=job_row_number ) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 91943d9fb..ea08144c7 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -1,7 +1,10 @@ +from datetime import datetime + from flask import current_app from notifications_utils.renderers import PassThrough from notifications_utils.template import Template +from app import DATETIME_FORMAT from app.celery import provider_tasks from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE @@ -41,19 +44,22 @@ def persist_notification(template_id, notification_type, api_key_id, key_type, + created_at=None, job_id=None, job_row_number=None): - notification = Notification.from_v2_api_request(template_id=template_id, - template_version=template_version, - recipient=recipient, - service_id=service_id, - personalisation=personalisation, - notification_type=notification_type, - api_key_id=api_key_id, - key_type=key_type, - job_id=job_id, - job_row_number=job_row_number - ) + notification = Notification.from_request( + template_id=template_id, + template_version=template_version, + recipient=recipient, + service_id=service_id, + personalisation=personalisation, + notification_type=notification_type, + api_key_id=api_key_id, + key_type=key_type, + created_at=created_at if created_at else datetime.utcnow().strftime(DATETIME_FORMAT), + job_id=job_id, + job_row_number=job_row_number + ) dao_create_notification(notification) return notification diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 307d33132..2b9086a64 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -3,7 +3,6 @@ import pytest from datetime import datetime from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError -from sqlalchemy.orm.exc import NoResultFound from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks from app.celery import tasks @@ -189,8 +188,6 @@ def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(noti 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.encryption.encrypt', return_value="something_encrypted") - mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") process_job(job.id) @@ -208,8 +205,6 @@ def test_should_not_process_email_job_if_would_exceed_send_limits(notify_db, not mocker.patch('app.celery.tasks.s3.get_job_from_s3') 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") process_job(job.id) @@ -562,11 +557,12 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() + now = datetime.utcnow() send_sms( sample_job.service.id, notification_id, encryption.encrypt(notification), - datetime.utcnow().strftime(DATETIME_FORMAT), + now.strftime(DATETIME_FORMAT), api_key_id=str(sample_api_key.id), key_type=KEY_TYPE_NORMAL ) @@ -581,7 +577,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ assert persisted_notification.template_id == sample_job.template.id assert persisted_notification.status == 'created' assert not persisted_notification.sent_at - assert persisted_notification.created_at <= datetime.utcnow() + assert persisted_notification.created_at <= now assert not persisted_notification.sent_by assert persisted_notification.job_row_number == 2 assert persisted_notification.api_key_id == sample_api_key.id @@ -673,7 +669,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh 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.created_at == now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by @@ -711,7 +707,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email assert persisted_notification.to == 'my_email@my_email.com' 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.created_at == now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by @@ -739,7 +735,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id assert persisted_notification.status == 'created' - assert persisted_notification.created_at >= now + assert persisted_notification.created_at == now assert not persisted_notification.sent_by assert persisted_notification.personalisation == {"name": "Jo"} assert not persisted_notification.reference @@ -766,7 +762,7 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em persisted_notification = Notification.query.all()[0] 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.created_at == now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index ddc559201..851865a6b 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -1,3 +1,5 @@ +import datetime + import pytest from boto3.exceptions import Boto3Error from sqlalchemy.exc import SQLAlchemyError @@ -46,21 +48,44 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api assert NotificationHistory.query.count() == 1 -def test_persist_notification_throws_exception_when_missing_template(sample_template, sample_api_key): +def test_persist_notification_throws_exception_when_missing_template(sample_api_key): assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 with pytest.raises(SQLAlchemyError): persist_notification(template_id=None, template_version=None, recipient='+447111111111', - service_id=sample_template.service.id, - personalisation=None, notification_type='sms', + service_id=sample_api_key.service_id, + personalisation=None, + notification_type='sms', api_key_id=sample_api_key.id, key_type=sample_api_key.key_type) assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 +def test_persist_notification_with_job_and_created(sample_job, sample_api_key): + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 0 + created_at = datetime.datetime(2016, 11, 11, 16, 8, 18) + persist_notification(template_id=sample_job.template.id, + template_version=sample_job.template.version, + recipient='+447111111111', + service_id=sample_job.service.id, + personalisation=None, notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + created_at=created_at, + job_id=sample_job.id, + job_row_number=10) + assert Notification.query.count() == 1 + assert NotificationHistory.query.count() == 1 + persisted_notification = Notification.query.all()[0] + assert persisted_notification.job_id == sample_job.id + assert persisted_notification.job_row_number == 10 + assert persisted_notification.created_at == created_at + + @pytest.mark.parametrize('research_mode, queue, notification_type, key_type', [(True, 'research-mode', 'sms', 'normal'), (True, 'research-mode', 'email', 'normal'), diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 78827235a..e6e5877dd 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -1,3 +1,4 @@ +import uuid from datetime import datetime import pytest @@ -12,23 +13,20 @@ from app.models import ( def test_should_build_notification_from_minimal_set_of_api_derived_params(notify_api): now = datetime.utcnow() - notification = { - 'template': 'template', - 'template_version': '1', - 'to': 'someone', - 'personalisation': {} - } - notification = Notification.from_api_request( - created_at=now.strftime(DATETIME_FORMAT), - notification=notification, - notification_id="notification_id", - service_id="service_id", + notification = Notification.from_request( + template_id='template', + template_version='1', + recipient='someone', + service_id='service_id', notification_type='SMS', + created_at=now, api_key_id='api_key_id', - key_type='key_type' + key_type='key_type', + personalisation={}, + job_id=None, + job_row_number=None ) assert notification.created_at == now - assert notification.id == "notification_id" assert notification.template_id == 'template' assert notification.template_version == '1' assert not notification.job_row_number @@ -44,28 +42,22 @@ def test_should_build_notification_from_minimal_set_of_api_derived_params(notify def test_should_build_notification_from_full_set_of_api_derived_params(notify_api): now = datetime.utcnow() - - notification = { - 'template': 'template', - 'template_version': '1', - 'to': 'someone', - 'personalisation': {'key': 'value'}, - 'job': 'job_id', - 'row_number': 100 - } - notification = Notification.from_api_request( - created_at=now.strftime(DATETIME_FORMAT), - notification=notification, - notification_id="notification_id", - service_id="service_id", - notification_type='SMS', - api_key_id='api_key_id', - key_type='key_type' - ) + notification = Notification.from_request(template_id='template', + template_version=1, + recipient='someone', + service_id='service_id', + personalisation={'key': 'value'}, + notification_type='SMS', + api_key_id='api_key_id', + key_type='key_type', + job_id='job_id', + job_row_number=100, + created_at=now + ) assert notification.created_at == now - assert notification.id == "notification_id" + assert notification.id is None assert notification.template_id == 'template' - assert notification.template_version == '1' + assert notification.template_version == 1 assert notification.job_row_number == 100 assert notification.job_id == 'job_id' assert notification.to == 'someone' From 4fe8d700ae52f8693f79cb3f67142f9d7e122082 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 16:00:31 +0000 Subject: [PATCH 03/16] Fix the send_sms and send_email code to send the notification id that has been persisted. --- app/celery/tasks.py | 32 ++++----- app/notifications/process_notifications.py | 2 +- tests/app/celery/test_tasks.py | 80 +++++++++++----------- 3 files changed, 56 insertions(+), 58 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index dde08958e..52890b6e9 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -130,26 +130,26 @@ def send_sms(self, return try: - persist_notification(template_id=notification['template'], - template_version=notification['template_version'], - recipient=notification['to'], - service_id=service.id, - personalisation=notification.get('personalisation'), - notification_type=SMS_TYPE, - api_key_id=api_key_id, - key_type=key_type, - created_at=created_at, - job_id=notification.get('job', None), - job_row_number=notification.get('row_number', None), - ) + saved_notification = persist_notification(template_id=notification['template'], + template_version=notification['template_version'], + recipient=notification['to'], + service_id=service.id, + personalisation=notification.get('personalisation'), + notification_type=SMS_TYPE, + api_key_id=api_key_id, + key_type=key_type, + created_at=created_at, + job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), + ) provider_tasks.deliver_sms.apply_async( - [notification_id], + [saved_notification.id], queue='send-sms' if not service.research_mode else 'research-mode' ) current_app.logger.info( - "SMS {} created at {} for job {}".format(notification_id, created_at, notification.get('job', None)) + "SMS {} created at {} for job {}".format(saved_notification.id, created_at, notification.get('job', None)) ) except SQLAlchemyError as e: @@ -179,7 +179,7 @@ def send_email(self, service_id, return try: - persist_notification( + saved_notification = persist_notification( template_id=notification['template'], template_version=notification['template_version'], recipient=notification['to'], @@ -195,7 +195,7 @@ def send_email(self, service_id, ) provider_tasks.deliver_email.apply_async( - [notification_id], + [saved_notification.id], queue='send-email' if not service.research_mode else 'research-mode' ) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index ea08144c7..e797350d2 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -77,7 +77,7 @@ def send_notification_to_queue(notification, research_mode): [str(notification.id)], queue='send-email' if not research_mode else 'research-mode' ) - except Exception as e: + except Exception: current_app.logger.exception("Failed to send to SQS exception") dao_delete_notifications_and_history_by_id(notification.id) raise diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 2b9086a64..7dca93dde 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -332,20 +332,13 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - notification_id = uuid.uuid4() - send_sms( sample_template_with_placeholders.service_id, - notification_id, + uuid.uuid4(), encryption.encrypt(notification), datetime.utcnow().strftime(DATETIME_FORMAT) ) - provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [notification_id], - queue="send-sms" - ) - assert mocked_deliver_sms.called assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447234123123' @@ -359,6 +352,10 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi assert persisted_notification.personalisation == {'name': 'Jo'} assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) assert persisted_notification.notification_type == 'sms' + mocked_deliver_sms.assert_called_once_with( + [persisted_notification.id], + queue="send-sms" + ) def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_service(notify_db, notify_db_session, mocker): @@ -380,9 +377,10 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic encryption.encrypt(notification), datetime.utcnow().strftime(DATETIME_FORMAT) ) - + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [notification_id], + [persisted_notification.id], queue="research-mode" ) assert mocked_deliver_sms.called @@ -405,12 +403,6 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif datetime.utcnow().strftime(DATETIME_FORMAT) ) - provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [notification_id], - queue="send-sms" - ) - - assert mocked_deliver_sms.called assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447700900890' @@ -423,6 +415,10 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif assert not persisted_notification.job_id assert not persisted_notification.personalisation assert persisted_notification.notification_type == 'sms' + provider_tasks.deliver_sms.apply_async.assert_called_once_with( + [persisted_notification.id], + queue="send-sms" + ) def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key(notify_db, @@ -444,14 +440,13 @@ def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key key_type=KEY_TYPE_TEST ) - provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [notification_id], + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] + mocked_deliver_sms.assert_called_once_with( + [persisted_notification.id], queue="send-sms" ) - assert mocked_deliver_sms.called - assert Notification.query.count() == 1 - def test_should_send_email_if_restricted_service_and_non_team_email_address_with_test_key(notify_db, notify_db_session, @@ -474,12 +469,12 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with key_type=KEY_TYPE_TEST ) - provider_tasks.deliver_email.apply_async.assert_called_once_with( - [notification_id], + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] + mocked_deliver_email.assert_called_once_with( + [persisted_notification.id], queue="send-email" ) - assert Notification.query.count() == 1 - assert mocked_deliver_email.called def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): @@ -542,8 +537,10 @@ def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_serv datetime.utcnow().strftime(DATETIME_FORMAT) ) + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] provider_tasks.deliver_email.apply_async.assert_called_once_with( - [notification_id], + [persisted_notification.id], queue="research-mode" ) @@ -566,10 +563,6 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ api_key_id=str(sample_api_key.id), key_type=KEY_TYPE_NORMAL ) - provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [notification_id], - queue="send-sms" - ) assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447234123123' @@ -584,6 +577,11 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ assert persisted_notification.key_type == KEY_TYPE_NORMAL assert persisted_notification.notification_type == 'sms' + provider_tasks.deliver_sms.apply_async.assert_called_once_with( + [persisted_notification.id], + queue="send-sms" + ) + def test_should_not_send_email_if_team_key_and_recipient_not_in_team(sample_email_template_with_placeholders, sample_team_api_key, @@ -662,8 +660,6 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh key_type=sample_api_key.key_type ) - provider_tasks.deliver_email.apply_async.assert_called_once_with( - [notification_id], queue='send-email') assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' @@ -680,6 +676,9 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.key_type == KEY_TYPE_NORMAL assert persisted_notification.notification_type == 'email' + provider_tasks.deliver_email.apply_async.assert_called_once_with( + [persisted_notification.id], queue='send-email') + 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') @@ -691,17 +690,14 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email dao_update_template(sample_email_template) t = dao_get_template_by_id(sample_email_template.id) assert t.version > version_on_notification - notification_id = uuid.uuid4() now = datetime.utcnow() send_email( sample_email_template.service_id, - notification_id, + uuid.uuid4(), encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email') - assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' @@ -712,6 +708,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email assert persisted_notification.status == 'created' assert not persisted_notification.sent_by assert persisted_notification.notification_type == 'email' + provider_tasks.deliver_email.apply_async.assert_called_once_with([persisted_notification.id], queue='send-email') def test_should_use_email_template_subject_placeholders(sample_email_template_with_placeholders, mocker): @@ -727,9 +724,6 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - provider_tasks.deliver_email.apply_async.assert_called_once_with( - [notification_id], queue='send-email' - ) assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' @@ -740,6 +734,9 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi assert persisted_notification.personalisation == {"name": "Jo"} assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' + provider_tasks.deliver_email.apply_async.assert_called_once_with( + [persisted_notification.id], queue='send-email' + ) def test_should_use_email_template_and_persist_without_personalisation(sample_email_template, mocker): @@ -757,7 +754,6 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email') assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' @@ -769,6 +765,8 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em assert not persisted_notification.personalisation assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' + provider_tasks.deliver_email.apply_async.assert_called_once_with([persisted_notification.id], + queue='send-email') def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker): @@ -815,7 +813,7 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - assert provider_tasks.deliver_email.apply_async.called is False + assert not provider_tasks.deliver_email.apply_async.called tasks.send_email.retry.assert_called_with(exc=expected_exception, queue='retry') assert Notification.query.count() == 0 From 4f5254134b56b339864c225a27bac81162a04f4b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 16:09:09 +0000 Subject: [PATCH 04/16] Fix logging in send_sms and send_email --- app/celery/tasks.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 52890b6e9..7e29acc51 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -153,12 +153,12 @@ def send_sms(self, ) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_sms notification {}".format(notification_id), e) + current_app.logger.exception("RETRY: send_sms notification {}".format(saved_notification.id), e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.exception( - "RETRY FAILED: task send_sms failed for notification {}".format(notification.id), + "RETRY FAILED: task send_sms failed for notification {}".format(saved_notification.id), e ) @@ -199,13 +199,13 @@ def send_email(self, service_id, queue='send-email' if not service.research_mode else 'research-mode' ) - current_app.logger.info("Email {} created at {}".format(notification_id, created_at)) + current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_email notification {}".format(notification_id), e) + current_app.logger.exception("RETRY: send_email notification {}".format(saved_notification.id), e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.error( - "RETRY FAILED: task send_email failed for notification {}".format(notification.id), + "RETRY FAILED: task send_email failed for notification {}".format(saved_notification.id), e ) From 3e6d581f737a8f027661b543cb4df6157bf37e2e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:20:53 +0000 Subject: [PATCH 05/16] Use constants for template type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handy if we ever want to rename these I guess… --- app/schemas.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/schemas.py b/app/schemas.py index 7f52b9541..a9811dfc3 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -213,7 +213,7 @@ class TemplateSchema(BaseTemplateSchema): @validates_schema def validate_type(self, data): template_type = data.get('template_type') - if template_type and template_type == 'email': + if template_type and template_type == models.EMAIL_TYPE: subject = data.get('subject') if not subject or subject.strip() == '': raise ValidationError('Invalid template subject', 'subject') From 705a0e7ab887060f0777b57b8b6149c292c7d98a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:21:48 +0000 Subject: [PATCH 06/16] Remove redundant assignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This variable is used exactly once, on the next line 🤔 --- app/schemas.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index a9811dfc3..610683a2d 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -212,8 +212,7 @@ class TemplateSchema(BaseTemplateSchema): @validates_schema def validate_type(self, data): - template_type = data.get('template_type') - if template_type and template_type == models.EMAIL_TYPE: + if data.get('template_type') in [models.EMAIL_TYPE, models.LETTER_TYPE]: subject = data.get('subject') if not subject or subject.strip() == '': raise ValidationError('Invalid template subject', 'subject') From aef62dcffb83184be74a4d7665b037dc9ac0c123 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:34:54 +0000 Subject: [PATCH 07/16] Parametrize template create Cleaner than having two almost identical tests. --- tests/app/dao/test_templates_dao.py | 10 ++++++++-- tests/app/template/test_rest.py | 20 ++++++++++++++++---- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index a01126445..287db454c 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -10,14 +10,20 @@ from app.models import Template, TemplateHistory import pytest -def test_create_template(sample_service, sample_user): +@pytest.mark.parametrize('template_type, subject', [ + ('sms', None), + ('email', 'subject'), +]) +def test_create_template(sample_service, sample_user, template_type, subject): data = { 'name': 'Sample Template', - 'template_type': "sms", + 'template_type': template_type, 'content': "Template content", 'service': sample_service, 'created_by': sample_user } + if subject: + data.update({'subject': subject}) template = Template(**data) dao_create_template(template) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 2d36e4360..8f18e5f0e 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -1,3 +1,4 @@ +import pytest import json import random import string @@ -8,16 +9,24 @@ from tests.app.conftest import sample_template as create_sample_template from app.dao.templates_dao import dao_get_template_by_id -def test_should_create_a_new_sms_template_for_a_service(notify_api, sample_user, sample_service): +@pytest.mark.parametrize('template_type, subject', [ + ('sms', None), + ('email', 'subject'), +]) +def test_should_create_a_new_template_for_a_service( + notify_api, sample_user, sample_service, template_type, subject +): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { 'name': 'my template', - 'template_type': 'sms', + 'template_type': template_type, 'content': 'template content', 'service': str(sample_service.id), 'created_by': str(sample_user.id) } + if subject: + data.update({'subject': subject}) data = json.dumps(data) auth_header = create_authorization_header() @@ -29,12 +38,15 @@ def test_should_create_a_new_sms_template_for_a_service(notify_api, sample_user, assert response.status_code == 201 json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['data']['name'] == 'my template' - assert json_resp['data']['template_type'] == 'sms' + assert json_resp['data']['template_type'] == template_type assert json_resp['data']['content'] == 'template content' assert json_resp['data']['service'] == str(sample_service.id) assert json_resp['data']['id'] assert json_resp['data']['version'] == 1 - assert not json_resp['data']['subject'] + if subject: + assert json_resp['data']['subject'] == 'subject' + else: + assert not json_resp['data']['subject'] def test_should_create_a_new_email_template_for_a_service(notify_api, sample_user, sample_service): From f34b58e7cd3c68f6fab7868cf68371266edf5a8f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:35:15 +0000 Subject: [PATCH 08/16] Delete redundant test Not needed now that the test above this one is parametrized. --- tests/app/dao/test_templates_dao.py | 17 ---------------- tests/app/template/test_rest.py | 30 ----------------------------- 2 files changed, 47 deletions(-) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 287db454c..95eb9b19c 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -32,23 +32,6 @@ def test_create_template(sample_service, sample_user, template_type, subject): assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template' -def test_create_email_template(sample_service, sample_user): - data = { - 'name': 'Sample Template', - 'template_type': "email", - 'subject': "subject", - 'content': "Template content", - 'service': sample_service, - 'created_by': sample_user - } - template = Template(**data) - dao_create_template(template) - - assert Template.query.count() == 1 - assert len(dao_get_all_templates_for_service(sample_service.id)) == 1 - assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template' - - def test_update_template(sample_service, sample_user): data = { 'name': 'Sample Template', diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 8f18e5f0e..890410bd3 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -49,36 +49,6 @@ def test_should_create_a_new_template_for_a_service( assert not json_resp['data']['subject'] -def test_should_create_a_new_email_template_for_a_service(notify_api, sample_user, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template', - 'template_type': 'email', - 'subject': 'subject', - 'content': 'template content', - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - auth_header = create_authorization_header() - - response = client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - assert response.status_code == 201 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['data']['name'] == 'my template' - assert json_resp['data']['template_type'] == 'email' - assert json_resp['data']['content'] == 'template content' - assert json_resp['data']['service'] == str(sample_service.id) - assert json_resp['data']['subject'] == 'subject' - assert json_resp['data']['version'] == 1 - assert json_resp['data']['id'] - - def test_should_be_error_if_service_does_not_exist_on_create(notify_api, sample_user, fake_uuid): with notify_api.test_request_context(): with notify_api.test_client() as client: From 73fe331dc8dce4560d056b4df35e6c1af7f93659 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:38:04 +0000 Subject: [PATCH 09/16] Test can create letters template It works already, just making sure it stays working. --- tests/app/dao/test_templates_dao.py | 1 + tests/app/template/test_rest.py | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 95eb9b19c..94be5c965 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -13,6 +13,7 @@ import pytest @pytest.mark.parametrize('template_type, subject', [ ('sms', None), ('email', 'subject'), + ('letter', 'subject'), ]) def test_create_template(sample_service, sample_user, template_type, subject): data = { diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 890410bd3..d448f3d62 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -12,6 +12,7 @@ from app.dao.templates_dao import dao_get_template_by_id @pytest.mark.parametrize('template_type, subject', [ ('sms', None), ('email', 'subject'), + ('letter', 'subject'), ]) def test_should_create_a_new_template_for_a_service( notify_api, sample_user, sample_service, template_type, subject @@ -116,12 +117,13 @@ def test_should_be_error_if_service_does_not_exist_on_update(notify_api, fake_uu assert json_resp['message'] == 'No result found' -def test_must_have_a_subject_on_an_email_template(notify_api, sample_user, sample_service): +@pytest.mark.parametrize('template_type', ['email', 'letter']) +def test_must_have_a_subject_on_an_email_or_letter_template(notify_api, sample_user, sample_service, template_type): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { 'name': 'my template', - 'template_type': 'email', + 'template_type': template_type, 'content': 'template content', 'service': str(sample_service.id), 'created_by': str(sample_user.id) From 6127ee422aa513692d0510751882cbce7e991867 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:44:13 +0000 Subject: [PATCH 10/16] Test can get letter template It works already, just making sure it stays working. --- tests/app/conftest.py | 2 +- tests/app/template/test_rest.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 435ea3bf5..566c888e1 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -180,7 +180,7 @@ def sample_template(notify_db, 'created_by': created_by, 'archived': archived } - if template_type == 'email': + if template_type in ['email', 'letter']: data.update({ 'subject': subject_line }) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index d448f3d62..7c6525244 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -321,6 +321,11 @@ def test_should_get_only_templates_for_that_service(notify_api, sample_user, ser None, 'hello ((name)) we’ve received your ((thing))', 'sms' + ), + ( + 'about your ((thing))', + 'hello ((name)) we’ve received your ((thing))', + 'letter' ) ] ) From 28d5da638b2cd6711962ad1736617f6b94183a6c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:37:39 +0000 Subject: [PATCH 11/16] Remove old style fixture Use new `client` one instead --- tests/app/template/test_rest.py | 713 +++++++++++++++----------------- 1 file changed, 343 insertions(+), 370 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 7c6525244..fbd8c83ad 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -15,299 +15,281 @@ from app.dao.templates_dao import dao_get_template_by_id ('letter', 'subject'), ]) def test_should_create_a_new_template_for_a_service( - notify_api, sample_user, sample_service, template_type, subject + client, sample_user, sample_service, template_type, subject ): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template', - 'template_type': template_type, - 'content': 'template content', - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - if subject: - data.update({'subject': subject}) - data = json.dumps(data) - auth_header = create_authorization_header() + data = { + 'name': 'my template', + 'template_type': template_type, + 'content': 'template content', + 'service': str(sample_service.id), + 'created_by': str(sample_user.id) + } + if subject: + data.update({'subject': subject}) + data = json.dumps(data) + auth_header = create_authorization_header() - response = client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - assert response.status_code == 201 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['data']['name'] == 'my template' - assert json_resp['data']['template_type'] == template_type - assert json_resp['data']['content'] == 'template content' - assert json_resp['data']['service'] == str(sample_service.id) - assert json_resp['data']['id'] - assert json_resp['data']['version'] == 1 - if subject: - assert json_resp['data']['subject'] == 'subject' - else: - assert not json_resp['data']['subject'] + response = client.post( + '/service/{}/template'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + assert response.status_code == 201 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['data']['name'] == 'my template' + assert json_resp['data']['template_type'] == template_type + assert json_resp['data']['content'] == 'template content' + assert json_resp['data']['service'] == str(sample_service.id) + assert json_resp['data']['id'] + assert json_resp['data']['version'] == 1 + if subject: + assert json_resp['data']['subject'] == 'subject' + else: + assert not json_resp['data']['subject'] -def test_should_be_error_if_service_does_not_exist_on_create(notify_api, sample_user, fake_uuid): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template', - 'template_type': 'sms', - 'content': 'template content', - 'service': fake_uuid, - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_should_be_error_if_service_does_not_exist_on_create(client, sample_user, fake_uuid): + data = { + 'name': 'my template', + 'template_type': 'sms', + 'content': 'template content', + 'service': fake_uuid, + 'created_by': str(sample_user.id) + } + data = json.dumps(data) + auth_header = create_authorization_header() - response = client.post( - '/service/{}/template'.format(fake_uuid), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 404 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'No result found' + response = client.post( + '/service/{}/template'.format(fake_uuid), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 404 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'No result found' -def test_should_error_if_created_by_missing(notify_api, sample_user, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - service_id = str(sample_service.id) - data = { - 'name': 'my template', - 'template_type': 'sms', - 'content': 'template content', - 'service': service_id - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_should_error_if_created_by_missing(client, sample_user, sample_service): + service_id = str(sample_service.id) + data = { + 'name': 'my template', + 'template_type': 'sms', + 'content': 'template content', + 'service': service_id + } + data = json.dumps(data) + auth_header = create_authorization_header() - response = client.post( - '/service/{}/template'.format(service_id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' + response = client.post( + '/service/{}/template'.format(service_id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' -def test_should_be_error_if_service_does_not_exist_on_update(notify_api, fake_uuid): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template' - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_should_be_error_if_service_does_not_exist_on_update(client, fake_uuid): + data = { + 'name': 'my template' + } + data = json.dumps(data) + auth_header = create_authorization_header() - response = client.post( - '/service/{}/template/{}'.format(fake_uuid, fake_uuid), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 404 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'No result found' + response = client.post( + '/service/{}/template/{}'.format(fake_uuid, fake_uuid), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 404 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'No result found' @pytest.mark.parametrize('template_type', ['email', 'letter']) -def test_must_have_a_subject_on_an_email_or_letter_template(notify_api, sample_user, sample_service, template_type): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template', - 'template_type': template_type, - 'content': 'template content', - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_must_have_a_subject_on_an_email_or_letter_template(client, sample_user, sample_service, template_type): + data = { + 'name': 'my template', + 'template_type': template_type, + 'content': 'template content', + 'service': str(sample_service.id), + 'created_by': str(sample_user.id) + } + data = json.dumps(data) + auth_header = create_authorization_header() - response = client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert json_resp['message'] == {'subject': ['Invalid template subject']} + response = client.post( + '/service/{}/template'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert json_resp['message'] == {'subject': ['Invalid template subject']} -def test_update_should_update_a_template(notify_api, sample_user, sample_template): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'content': 'my template has new content ', - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_update_should_update_a_template(client, sample_user, sample_template): + data = { + 'content': 'my template has new content ', + 'created_by': str(sample_user.id) + } + data = json.dumps(data) + auth_header = create_authorization_header() - update_response = client.post( - '/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) + update_response = client.post( + '/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) - assert update_response.status_code == 200 - update_json_resp = json.loads(update_response.get_data(as_text=True)) - assert update_json_resp['data']['content'] == 'my template has new content alert("foo")' - assert update_json_resp['data']['name'] == sample_template.name - assert update_json_resp['data']['template_type'] == sample_template.template_type - assert update_json_resp['data']['version'] == 2 + assert update_response.status_code == 200 + update_json_resp = json.loads(update_response.get_data(as_text=True)) + assert update_json_resp['data']['content'] == 'my template has new content alert("foo")' + assert update_json_resp['data']['name'] == sample_template.name + assert update_json_resp['data']['template_type'] == sample_template.template_type + assert update_json_resp['data']['version'] == 2 -def test_should_be_able_to_archive_template(notify_api, sample_template): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': sample_template.name, - 'template_type': sample_template.template_type, - 'content': sample_template.content, - 'archived': True, - 'service': str(sample_template.service.id), - 'created_by': str(sample_template.created_by.id) - } +def test_should_be_able_to_archive_template(client, sample_template): + data = { + 'name': sample_template.name, + 'template_type': sample_template.template_type, + 'content': sample_template.content, + 'archived': True, + 'service': str(sample_template.service.id), + 'created_by': str(sample_template.created_by.id) + } - json_data = json.dumps(data) + json_data = json.dumps(data) - auth_header = create_authorization_header() + auth_header = create_authorization_header() - resp = client.post( - '/service/{}/template/{}'.format(sample_template.service.id, sample_template.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=json_data - ) + resp = client.post( + '/service/{}/template/{}'.format(sample_template.service.id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=json_data + ) - assert resp.status_code == 200 - assert Template.query.first().archived + assert resp.status_code == 200 + assert Template.query.first().archived -def test_should_be_able_to_get_all_templates_for_a_service(notify_api, sample_user, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template 1', - 'template_type': 'email', - 'subject': 'subject 1', - 'content': 'template content', - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - data_1 = json.dumps(data) - data = { - 'name': 'my template 2', - 'template_type': 'email', - 'subject': 'subject 2', - 'content': 'template content', - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - data_2 = json.dumps(data) - auth_header = create_authorization_header() - client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data_1 - ) - auth_header = create_authorization_header() +def test_should_be_able_to_get_all_templates_for_a_service(client, sample_user, sample_service): + data = { + 'name': 'my template 1', + 'template_type': 'email', + 'subject': 'subject 1', + 'content': 'template content', + 'service': str(sample_service.id), + 'created_by': str(sample_user.id) + } + data_1 = json.dumps(data) + data = { + 'name': 'my template 2', + 'template_type': 'email', + 'subject': 'subject 2', + 'content': 'template content', + 'service': str(sample_service.id), + 'created_by': str(sample_user.id) + } + data_2 = json.dumps(data) + auth_header = create_authorization_header() + client.post( + '/service/{}/template'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data_1 + ) + auth_header = create_authorization_header() - client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data_2 - ) + client.post( + '/service/{}/template'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data_2 + ) - auth_header = create_authorization_header() + auth_header = create_authorization_header() - response = client.get( - '/service/{}/template'.format(sample_service.id), - headers=[auth_header] - ) + response = client.get( + '/service/{}/template'.format(sample_service.id), + headers=[auth_header] + ) - assert response.status_code == 200 - update_json_resp = json.loads(response.get_data(as_text=True)) - assert update_json_resp['data'][0]['name'] == 'my template 2' - assert update_json_resp['data'][0]['version'] == 1 - assert update_json_resp['data'][0]['created_at'] - assert update_json_resp['data'][1]['name'] == 'my template 1' - assert update_json_resp['data'][1]['version'] == 1 - assert update_json_resp['data'][1]['created_at'] + assert response.status_code == 200 + update_json_resp = json.loads(response.get_data(as_text=True)) + assert update_json_resp['data'][0]['name'] == 'my template 2' + assert update_json_resp['data'][0]['version'] == 1 + assert update_json_resp['data'][0]['created_at'] + assert update_json_resp['data'][1]['name'] == 'my template 1' + assert update_json_resp['data'][1]['version'] == 1 + assert update_json_resp['data'][1]['created_at'] -def test_should_get_only_templates_for_that_service(notify_api, sample_user, service_factory): - with notify_api.test_request_context(): - with notify_api.test_client() as client: +def test_should_get_only_templates_for_that_service(client, sample_user, service_factory): - service_1 = service_factory.get('service 1', email_from='service.1') - service_2 = service_factory.get('service 2', email_from='service.2') + service_1 = service_factory.get('service 1', email_from='service.1') + service_2 = service_factory.get('service 2', email_from='service.2') - auth_header_1 = create_authorization_header() + auth_header_1 = create_authorization_header() - response_1 = client.get( - '/service/{}/template'.format(service_1.id), - headers=[auth_header_1] - ) + response_1 = client.get( + '/service/{}/template'.format(service_1.id), + headers=[auth_header_1] + ) - auth_header_2 = create_authorization_header() + auth_header_2 = create_authorization_header() - response_2 = client.get( - '/service/{}/template'.format(service_2.id), - headers=[auth_header_2] - ) + response_2 = client.get( + '/service/{}/template'.format(service_2.id), + headers=[auth_header_2] + ) - assert response_1.status_code == 200 - assert response_2.status_code == 200 + assert response_1.status_code == 200 + assert response_2.status_code == 200 - json_resp_1 = json.loads(response_1.get_data(as_text=True)) - json_resp_2 = json.loads(response_2.get_data(as_text=True)) + json_resp_1 = json.loads(response_1.get_data(as_text=True)) + json_resp_2 = json.loads(response_2.get_data(as_text=True)) - assert len(json_resp_1['data']) == 1 - assert len(json_resp_2['data']) == 1 + assert len(json_resp_1['data']) == 1 + assert len(json_resp_2['data']) == 1 - data = { - 'name': 'my template 2', - 'template_type': 'email', - 'subject': 'subject 2', - 'content': 'template content', - 'service': str(service_1.id), - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - create_auth_header = create_authorization_header() - resp = client.post( - '/service/{}/template'.format(service_1.id), - headers=[('Content-Type', 'application/json'), create_auth_header], - data=data - ) + data = { + 'name': 'my template 2', + 'template_type': 'email', + 'subject': 'subject 2', + 'content': 'template content', + 'service': str(service_1.id), + 'created_by': str(sample_user.id) + } + data = json.dumps(data) + create_auth_header = create_authorization_header() + resp = client.post( + '/service/{}/template'.format(service_1.id), + headers=[('Content-Type', 'application/json'), create_auth_header], + data=data + ) - response_3 = client.get( - '/service/{}/template'.format(service_1.id), - headers=[auth_header_1] - ) + response_3 = client.get( + '/service/{}/template'.format(service_1.id), + headers=[auth_header_1] + ) - response_4 = client.get( - '/service/{}/template'.format(service_2.id), - headers=[auth_header_2] - ) + response_4 = client.get( + '/service/{}/template'.format(service_2.id), + headers=[auth_header_2] + ) - assert response_3.status_code == 200 - assert response_4.status_code == 200 + assert response_3.status_code == 200 + assert response_4.status_code == 200 - json_resp_3 = json.loads(response_3.get_data(as_text=True)) - json_resp_4 = json.loads(response_4.get_data(as_text=True)) + json_resp_3 = json.loads(response_3.get_data(as_text=True)) + json_resp_4 = json.loads(response_4.get_data(as_text=True)) - assert len(json_resp_3['data']) == 2 - assert len(json_resp_4['data']) == 1 + assert len(json_resp_3['data']) == 2 + assert len(json_resp_4['data']) == 1 @pytest.mark.parametrize( @@ -331,29 +313,28 @@ def test_should_get_only_templates_for_that_service(notify_api, sample_user, ser ) def test_should_get_a_single_template( notify_db, - notify_api, + client, sample_user, service_factory, subject, content, template_type ): - with notify_api.test_request_context(), notify_api.test_client() as client: - template = create_sample_template( - notify_db, notify_db.session, subject_line=subject, content=content, template_type=template_type - ) + template = create_sample_template( + notify_db, notify_db.session, subject_line=subject, content=content, template_type=template_type + ) - response = client.get( - '/service/{}/template/{}'.format(template.service.id, template.id), - headers=[create_authorization_header()] - ) + response = client.get( + '/service/{}/template/{}'.format(template.service.id, template.id), + headers=[create_authorization_header()] + ) - data = json.loads(response.get_data(as_text=True))['data'] + data = json.loads(response.get_data(as_text=True))['data'] - assert response.status_code == 200 - assert data['content'] == content - assert data['subject'] == subject + assert response.status_code == 200 + assert data['content'] == content + assert data['subject'] == subject @pytest.mark.parametrize( @@ -392,7 +373,7 @@ def test_should_get_a_single_template( ) def test_should_preview_a_single_template( notify_db, - notify_api, + client, sample_user, service_factory, subject, @@ -402,144 +383,136 @@ def test_should_preview_a_single_template( expected_content, expected_error ): - with notify_api.test_request_context(), notify_api.test_client() as client: - template = create_sample_template( - notify_db, notify_db.session, subject_line=subject, content=content, template_type='email' - ) + template = create_sample_template( + notify_db, notify_db.session, subject_line=subject, content=content, template_type='email' + ) - response = client.get( - path.format(template.service.id, template.id), - headers=[create_authorization_header()] - ) + response = client.get( + path.format(template.service.id, template.id), + headers=[create_authorization_header()] + ) - content = json.loads(response.get_data(as_text=True)) + content = json.loads(response.get_data(as_text=True)) - if expected_error: - assert response.status_code == 400 - assert content['message']['template'] == [expected_error] - else: - assert response.status_code == 200 - assert content['content'] == expected_content - assert content['subject'] == expected_subject + if expected_error: + assert response.status_code == 400 + assert content['message']['template'] == [expected_error] + else: + assert response.status_code == 200 + assert content['content'] == expected_content + assert content['subject'] == expected_subject -def test_should_return_empty_array_if_no_templates_for_service(notify_api, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: +def test_should_return_empty_array_if_no_templates_for_service(client, sample_service): - auth_header = create_authorization_header() + auth_header = create_authorization_header() - response = client.get( - '/service/{}/template'.format(sample_service.id), - headers=[auth_header] - ) + response = client.get( + '/service/{}/template'.format(sample_service.id), + headers=[auth_header] + ) - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 0 + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 0 -def test_should_return_404_if_no_templates_for_service_with_id(notify_api, sample_service, fake_uuid): - with notify_api.test_request_context(): - with notify_api.test_client() as client: +def test_should_return_404_if_no_templates_for_service_with_id(client, sample_service, fake_uuid): - auth_header = create_authorization_header() + auth_header = create_authorization_header() - response = client.get( - '/service/{}/template/{}'.format(sample_service.id, fake_uuid), - headers=[auth_header] - ) + response = client.get( + '/service/{}/template/{}'.format(sample_service.id, fake_uuid), + headers=[auth_header] + ) - assert response.status_code == 404 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'No result found' + assert response.status_code == 404 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'No result found' -def test_create_400_for_over_limit_content(notify_api, sample_user, sample_service, fake_uuid): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - limit = notify_api.config.get('SMS_CHAR_COUNT_LIMIT') - content = ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(limit + 1)) - data = { - 'name': 'too big template', - 'template_type': 'sms', - 'content': content, - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_create_400_for_over_limit_content(client, notify_api, sample_user, sample_service, fake_uuid): - response = client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - assert response.status_code == 400 - json_resp = json.loads(response.get_data(as_text=True)) - assert ( - 'Content has a character count greater than the limit of {}' - ).format(limit) in json_resp['message']['content'] + limit = notify_api.config.get('SMS_CHAR_COUNT_LIMIT') + content = ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(limit + 1)) + data = { + 'name': 'too big template', + 'template_type': 'sms', + 'content': content, + 'service': str(sample_service.id), + 'created_by': str(sample_user.id) + } + data = json.dumps(data) + auth_header = create_authorization_header() + + response = client.post( + '/service/{}/template'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert ( + 'Content has a character count greater than the limit of {}' + ).format(limit) in json_resp['message']['content'] -def test_update_400_for_over_limit_content(notify_api, sample_user, sample_template): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - limit = notify_api.config.get('SMS_CHAR_COUNT_LIMIT') - json_data = json.dumps({ - 'content': ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(limit + 1)), - 'created_by': str(sample_user.id) - }) - auth_header = create_authorization_header() - resp = client.post( - '/service/{}/template/{}'.format(sample_template.service.id, sample_template.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=json_data - ) - assert resp.status_code == 400 - json_resp = json.loads(resp.get_data(as_text=True)) - assert ( - 'Content has a character count greater than the limit of {}' - ).format(limit) in json_resp['message']['content'] +def test_update_400_for_over_limit_content(client, notify_api, sample_user, sample_template): + + limit = notify_api.config.get('SMS_CHAR_COUNT_LIMIT') + json_data = json.dumps({ + 'content': ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(limit + 1)), + 'created_by': str(sample_user.id) + }) + auth_header = create_authorization_header() + resp = client.post( + '/service/{}/template/{}'.format(sample_template.service.id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=json_data + ) + assert resp.status_code == 400 + json_resp = json.loads(resp.get_data(as_text=True)) + assert ( + 'Content has a character count greater than the limit of {}' + ).format(limit) in json_resp['message']['content'] -def test_should_return_all_template_versions_for_service_and_template_id(notify_api, sample_template): +def test_should_return_all_template_versions_for_service_and_template_id(client, sample_template): original_content = sample_template.content from app.dao.templates_dao import dao_update_template sample_template.content = original_content + '1' dao_update_template(sample_template) sample_template.content = original_content + '2' dao_update_template(sample_template) - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - resp = client.get('/service/{}/template/{}/versions'.format(sample_template.service_id, sample_template.id), - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 200 - resp_json = json.loads(resp.get_data(as_text=True))['data'] - assert len(resp_json) == 3 - for x in resp_json: - if x['version'] == 1: - assert x['content'] == original_content - elif x['version'] == 2: - assert x['content'] == original_content + '1' - else: - assert x['content'] == original_content + '2' + + auth_header = create_authorization_header() + resp = client.get('/service/{}/template/{}/versions'.format(sample_template.service_id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 200 + resp_json = json.loads(resp.get_data(as_text=True))['data'] + assert len(resp_json) == 3 + for x in resp_json: + if x['version'] == 1: + assert x['content'] == original_content + elif x['version'] == 2: + assert x['content'] == original_content + '1' + else: + assert x['content'] == original_content + '2' -def test_update_does_not_create_new_version_when_there_is_no_change(notify_api, sample_template): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - data = { - 'template_type': sample_template.template_type, - 'content': sample_template.content, - } - resp = client.post('/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 200 +def test_update_does_not_create_new_version_when_there_is_no_change(client, sample_template): + + auth_header = create_authorization_header() + data = { + 'template_type': sample_template.template_type, + 'content': sample_template.content, + } + resp = client.post('/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 200 + template = dao_get_template_by_id(sample_template.id) assert template.version == 1 From 4379308189668e6fcab2a8b09c3fb346aeeb13dd Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 17:28:15 +0000 Subject: [PATCH 12/16] Fix test failures. --- app/celery/tasks.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 75109fd85..1de9b3be9 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -150,12 +150,12 @@ def send_sms(self, ) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_sms notification {}".format(saved_notification.id), e) + current_app.logger.exception("RETRY: send_sms notification", e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.exception( - "RETRY FAILED: task send_sms failed for notification {}".format(saved_notification.id), + "RETRY FAILED: task send_sms failed for notification", e ) @@ -198,11 +198,11 @@ def send_email(self, service_id, current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_email notification {}".format(saved_notification.id), e) + current_app.logger.exception("RETRY: send_email notification", e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.error( - "RETRY FAILED: task send_email failed for notification {}".format(saved_notification.id), + "RETRY FAILED: task send_email failed for notification", e ) From d706d86c9c7e73a65be57c8171a841c01cf6d36f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 14 Nov 2016 14:10:40 +0000 Subject: [PATCH 13/16] don't re-delete already archived/revoked templates/api-keys --- app/dao/services_dao.py | 6 +++-- tests/app/service/test_deactivate.py | 38 ++++++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index bead23a6e..08b30b381 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -86,10 +86,12 @@ def dao_deactive_service(service_id): service.email_from = '_archived_' + service.email_from for template in service.templates: - template.archived = True + if not template.archived: + template.archived = True for api_key in service.api_keys: - api_key.expiry_date = datetime.utcnow() + if not api_key.expiry_date: + api_key.expiry_date = datetime.utcnow() def dao_fetch_service_by_id_and_user(service_id, user_id): diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py index 0f9a3dcce..2ed2c01a6 100644 --- a/tests/app/service/test_deactivate.py +++ b/tests/app/service/test_deactivate.py @@ -1,11 +1,14 @@ import uuid -from unittest import mock +from datetime import datetime import pytest +from freezegun import freeze_time from app import db -from app.models import Service, TemplateHistory, ApiKey +from app.models import Service from app.dao.services_dao import dao_deactive_service +from app.dao.api_key_dao import expire_api_key +from app.dao.templates_dao import dao_update_template from tests import create_authorization_header, unwrap_function from tests.app.conftest import ( @@ -79,6 +82,37 @@ def test_deactivating_service_creates_history(deactivated_service): assert history.active is False +@pytest.fixture +def deactivated_service_with_deleted_stuff(client, notify_db, notify_db_session, sample_service): + with freeze_time('2001-01-01'): + template = create_template(notify_db, notify_db_session, template_name='a') + api_key = create_api_key(notify_db, notify_db_session) + + expire_api_key(sample_service.id, api_key.id) + + template.archived = True + dao_update_template(template) + + with freeze_time('2002-02-02'): + auth_header = create_authorization_header() + response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header]) + + assert response.status_code == 204 + assert response.data == b'' + return sample_service + + +def test_deactivating_service_doesnt_affect_existing_archived_templates(deactivated_service_with_deleted_stuff): + assert deactivated_service_with_deleted_stuff.templates[0].archived is True + assert deactivated_service_with_deleted_stuff.templates[0].updated_at == datetime(2001, 1, 1, 0, 0, 0) + assert deactivated_service_with_deleted_stuff.templates[0].version == 2 + + +def test_deactivating_service_doesnt_affect_existing_revoked_api_keys(deactivated_service_with_deleted_stuff): + assert deactivated_service_with_deleted_stuff.api_keys[0].expiry_date == datetime(2001, 1, 1, 0, 0, 0) + assert deactivated_service_with_deleted_stuff.api_keys[0].version == 2 + + def test_deactivating_service_rolls_back_everything_on_error(sample_service, sample_api_key, sample_template): unwrapped_deactive_service = unwrap_function(dao_deactive_service) From c00eb0f5a4a33d44a0716cd09a6a4752f184c424 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 14 Nov 2016 14:41:32 +0000 Subject: [PATCH 14/16] Remove from_request method on the Notification db model, was not needed anymore. Use Notifications.query.one(), rather than assert count is 1 and query for all and get first item in list. --- app/models.py | 28 ---------- app/notifications/process_notifications.py | 4 +- tests/app/celery/test_tasks.py | 34 ++++--------- tests/app/test_model.py | 59 ---------------------- 4 files changed, 13 insertions(+), 112 deletions(-) diff --git a/app/models.py b/app/models.py index 5fa8f6a98..5e2d16f6f 100644 --- a/app/models.py +++ b/app/models.py @@ -537,34 +537,6 @@ class Notification(db.Model): if personalisation: self._personalisation = encryption.encrypt(personalisation) - @classmethod - def from_request(cls, - template_id, - template_version, - recipient, - service_id, - personalisation, - notification_type, - api_key_id, - key_type, - job_id, - job_row_number, - created_at): - return cls( - template_id=template_id, - template_version=template_version, - to=recipient, - service_id=service_id, - status='created', - created_at=created_at, - personalisation=personalisation, - notification_type=notification_type, - api_key_id=api_key_id, - key_type=key_type, - job_id=job_id, - job_row_number=job_row_number - ) - class NotificationHistory(db.Model): __tablename__ = 'notification_history' diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index e797350d2..5df9e7b36 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -47,10 +47,10 @@ def persist_notification(template_id, created_at=None, job_id=None, job_row_number=None): - notification = Notification.from_request( + notification = Notification( template_id=template_id, template_version=template_version, - recipient=recipient, + to=recipient, service_id=service_id, personalisation=personalisation, notification_type=notification_type, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 7dca93dde..44dcb7ce5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -339,8 +339,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi datetime.utcnow().strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == '+447234123123' assert persisted_notification.template_id == sample_template_with_placeholders.id assert persisted_notification.template_version == sample_template_with_placeholders.version @@ -377,8 +376,7 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic encryption.encrypt(notification), datetime.utcnow().strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() provider_tasks.deliver_sms.apply_async.assert_called_once_with( [persisted_notification.id], queue="research-mode" @@ -392,7 +390,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif template = sample_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, "+447700900890") # The user’s own number, but in a different format - mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() encrypt_notification = encryption.encrypt(notification) @@ -403,8 +401,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif datetime.utcnow().strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == '+447700900890' assert persisted_notification.template_id == template.id assert persisted_notification.template_version == template.version @@ -440,8 +437,7 @@ def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key key_type=KEY_TYPE_TEST ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() mocked_deliver_sms.assert_called_once_with( [persisted_notification.id], queue="send-sms" @@ -469,8 +465,7 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with key_type=KEY_TYPE_TEST ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() mocked_deliver_email.assert_called_once_with( [persisted_notification.id], queue="send-email" @@ -537,8 +532,7 @@ def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_serv datetime.utcnow().strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() provider_tasks.deliver_email.apply_async.assert_called_once_with( [persisted_notification.id], queue="research-mode" @@ -563,8 +557,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ api_key_id=str(sample_api_key.id), key_type=KEY_TYPE_NORMAL ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == '+447234123123' assert persisted_notification.job_id == sample_job.id assert persisted_notification.template_id == sample_job.template.id @@ -660,8 +653,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh key_type=sample_api_key.key_type ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() 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 @@ -698,8 +690,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email now.strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id assert persisted_notification.template_version == version_on_notification @@ -724,8 +715,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id assert persisted_notification.status == 'created' @@ -743,8 +733,6 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em notification = _notification_json(sample_email_template, "my_email@my_email.com") mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - assert Notification.query.count() == 0 - notification_id = uuid.uuid4() now = datetime.utcnow() diff --git a/tests/app/test_model.py b/tests/app/test_model.py index e6e5877dd..abbd167a5 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -10,65 +10,6 @@ from app.models import ( MOBILE_TYPE, EMAIL_TYPE) -def test_should_build_notification_from_minimal_set_of_api_derived_params(notify_api): - now = datetime.utcnow() - - notification = Notification.from_request( - template_id='template', - template_version='1', - recipient='someone', - service_id='service_id', - notification_type='SMS', - created_at=now, - api_key_id='api_key_id', - key_type='key_type', - personalisation={}, - job_id=None, - job_row_number=None - ) - assert notification.created_at == now - assert notification.template_id == 'template' - assert notification.template_version == '1' - assert not notification.job_row_number - assert not notification.job_id - assert notification.to == 'someone' - assert notification.service_id == 'service_id' - assert notification.status == 'created' - assert not notification.personalisation - assert notification.notification_type == 'SMS' - assert notification.api_key_id == 'api_key_id' - assert notification.key_type == 'key_type' - - -def test_should_build_notification_from_full_set_of_api_derived_params(notify_api): - now = datetime.utcnow() - notification = Notification.from_request(template_id='template', - template_version=1, - recipient='someone', - service_id='service_id', - personalisation={'key': 'value'}, - notification_type='SMS', - api_key_id='api_key_id', - key_type='key_type', - job_id='job_id', - job_row_number=100, - created_at=now - ) - assert notification.created_at == now - assert notification.id is None - assert notification.template_id == 'template' - assert notification.template_version == 1 - assert notification.job_row_number == 100 - assert notification.job_id == 'job_id' - assert notification.to == 'someone' - assert notification.service_id == 'service_id' - assert notification.status == 'created' - assert notification.personalisation == {'key': 'value'} - assert notification.notification_type == 'SMS' - assert notification.api_key_id == 'api_key_id' - assert notification.key_type == 'key_type' - - @pytest.mark.parametrize('mobile_number', [ '07700 900678', '+44 7700 900678' From 2d9d4013e8132e890c65ddc73bd2020cab03023c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 14 Nov 2016 15:29:23 +0000 Subject: [PATCH 15/16] Missed one --- tests/app/celery/test_tasks.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 44dcb7ce5..04b449cff 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -742,8 +742,7 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id assert persisted_notification.created_at == now From d6c537cf440aba48b34c3514dd49386d948e9008 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 15 Nov 2016 11:44:48 +0000 Subject: [PATCH 16/16] Added job_id and row_number to the exception error message if the task needs to be retried. That way we have a way to tie the exception messages together --- app/celery/tasks.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 1de9b3be9..a4b4fd94b 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -117,6 +117,7 @@ def send_sms(self, created_at, api_key_id=None, key_type=KEY_TYPE_NORMAL): + # notification_id is not used, it is created by the db model object notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) @@ -150,14 +151,15 @@ def send_sms(self, ) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_sms notification", e) + current_app.logger.exception( + "RETRY: send_sms notification for job {} row number {}".format(notification.get('job', None), + notification.get('row_number', None)), e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.exception( - "RETRY FAILED: task send_sms failed for notification", - e - ) + "RETRY FAILED: task send_sms failed for notification".format(notification.get('job', None), + notification.get('row_number', None)), e) @notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=300) @@ -168,6 +170,7 @@ def send_email(self, service_id, created_at, api_key_id=None, key_type=KEY_TYPE_NORMAL): + # notification_id is not used, it is created by the db model object notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) @@ -198,11 +201,11 @@ def send_email(self, service_id, current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_email notification", e) + current_app.logger.exception("RETRY: send_email notification".format(notification.get('job', None), + notification.get('row_number', None)), e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.error( - "RETRY FAILED: task send_email failed for notification", - e - ) + "RETRY FAILED: task send_email failed for notification".format(notification.get('job', None), + notification.get('row_number', None)), e)