From eafbbd9809d356572967ba6d28fcae1e477c3b17 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 10:41:39 +0000 Subject: [PATCH 1/8] 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 2/8] 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 3/8] 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 4/8] 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 4379308189668e6fcab2a8b09c3fb346aeeb13dd Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 17:28:15 +0000 Subject: [PATCH 5/8] 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 c00eb0f5a4a33d44a0716cd09a6a4752f184c424 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 14 Nov 2016 14:41:32 +0000 Subject: [PATCH 6/8] 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 7/8] 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 8/8] 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)