From 8f19ad19f8491c2411f64b2a6db9f8778852f2f9 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 23 Jun 2016 09:41:21 +0100 Subject: [PATCH 1/6] Stop sending the encrypted message to the send_sms_to_provider task. Everything the task needs is now stored in the db. --- app/celery/tasks.py | 2 +- tests/app/celery/test_provider_tasks.py | 14 ++++++-------- tests/app/celery/test_tasks.py | 9 +++------ 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index ff74c98bb..cebcb8862 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -161,7 +161,7 @@ def send_sms(self, service_id, notification_id, encrypted_notification, created_ ) dao_create_notification(notification_db_object, TEMPLATE_TYPE_SMS) - send_sms_to_provider.apply_async((service_id, notification_id, encrypted_notification), queue='sms') + send_sms_to_provider.apply_async((service_id, notification_id), queue='sms') current_app.logger.info( "SMS {} created at {} sent at {}".format(notification_id, created_at, sent_at) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 45524a130..e0fff90f6 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,20 +1,18 @@ +from datetime import datetime + from celery.exceptions import MaxRetriesExceededError - from mock import ANY, call +from notifications_utils.recipients import validate_phone_number, format_phone_number -from app import statsd_client, mmg_client, encryption +from app import statsd_client, mmg_client from app.celery import provider_tasks from app.celery.provider_tasks import send_sms_to_provider +from app.celery.research_mode_tasks import send_sms_response from app.celery.tasks import provider_to_use from app.clients.sms import SmsClientException -from app.dao import provider_statistics_dao -from datetime import datetime -from freezegun import freeze_time from app.dao import notifications_dao, provider_details_dao -from notifications_utils.recipients import validate_phone_number, format_phone_number -from app.celery.research_mode_tasks import send_sms_response +from app.dao import provider_statistics_dao from app.models import Notification, NotificationStatistics, Job - from tests.app.conftest import ( sample_notification ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 39452a2de..34e07ba31 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -332,8 +332,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( (sample_template_with_placeholders.service_id, - notification_id, - encryption.encrypt(notification)), + notification_id), queue="sms" ) @@ -371,8 +370,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( (service.id, - notification_id, - encrypt_notification), + notification_id), queue="sms" ) @@ -480,8 +478,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, mocker) ) provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( (sample_job.service.id, - notification_id, - encryption.encrypt(notification)), + notification_id), queue="sms" ) persisted_notification = Notification.query.filter_by(id=notification_id).one() From 8a0211b3eb847ec9c962ac0849627c7aa72a5e10 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 27 Jun 2016 14:47:20 +0100 Subject: [PATCH 2/6] Only send to the provider if the notification has a created status. If the notification ends up in the retry queue and the delivery app is restarted the notification will get sent twice. This is because when the app is restarted another message will be in the retry queue as message available which is a duplicate of the one in the queue that is a message in flight. This should complete https://www.pivotaltracker.com/story/show/121844427 --- app/celery/provider_tasks.py | 87 ++++++++++++------------ tests/app/celery/test_provider_tasks.py | 4 +- tests/app/conftest.py | 2 +- tests/app/dao/test_notification_dao.py | 2 +- tests/app/notifications/test_rest.py | 23 ++++--- tests/app/status/test_delivery_status.py | 9 ++- 6 files changed, 69 insertions(+), 58 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index ec2866e1d..c232d5f1c 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -54,52 +54,53 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati service = dao_fetch_service_by_id(service_id) provider = provider_to_use('sms', notification_id) notification = get_notification_by_id(notification_id) + if notification.status == 'created': + template = Template( + dao_get_template_by_id(notification.template_id, notification.template_version).__dict__, + values={} if not notification.personalisation else notification.personalisation, + prefix=service.name + ) - template = Template( - dao_get_template_by_id(notification.template_id, notification.template_version).__dict__, - values={} if not notification.personalisation else notification.personalisation, - prefix=service.name - ) - try: - if service.research_mode: - send_sms_response.apply_async( - (provider.get_name(), str(notification_id), notification.to), queue='research-mode' - ) - else: - provider.send_sms( - to=validate_and_format_phone_number(notification.to), - content=template.replaced, - reference=str(notification_id) - ) - - update_provider_stats( - notification_id, - 'sms', - provider.get_name(), - content_char_count=template.replaced_content_count - ) - - notification.sent_at = datetime.utcnow() - notification.sent_by = provider.get_name(), - notification.content_char_count = template.replaced_content_count - notification.status = 'sending' - dao_update_notification(notification) - except SmsClientException as e: try: - current_app.logger.error( - "SMS notification {} failed".format(notification_id) - ) - current_app.logger.exception(e) - self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) - except self.MaxRetriesExceededError: - update_notification_status_by_id(notification.id, 'technical-failure', 'failure') + if service.research_mode: + send_sms_response.apply_async( + (provider.get_name(), str(notification_id), notification.to), queue='research-mode' + ) + else: + provider.send_sms( + to=validate_and_format_phone_number(notification.to), + content=template.replaced, + reference=str(notification_id) + ) - current_app.logger.info( - "SMS {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at) - ) - statsd_client.incr("notifications.tasks.send-sms-to-provider") - statsd_client.timing("notifications.tasks.send-sms-to-provider.task-time", monotonic() - task_start) - statsd_client.timing("notifications.sms.total-time", datetime.utcnow() - notification.created_at) + update_provider_stats( + notification_id, + 'sms', + provider.get_name(), + content_char_count=template.replaced_content_count + ) + + notification.sent_at = datetime.utcnow() + notification.sent_by = provider.get_name(), + notification.content_char_count = template.replaced_content_count + notification.status = 'sending' + dao_update_notification(notification) + except SmsClientException as e: + try: + current_app.logger.error( + "SMS notification {} failed".format(notification_id) + ) + current_app.logger.exception(e) + self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) + except self.MaxRetriesExceededError: + update_notification_status_by_id(notification.id, 'technical-failure', 'failure') + + current_app.logger.info( + "SMS {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at) + ) + statsd_client.incr("notifications.tasks.send-sms-to-provider") + statsd_client.timing("notifications.tasks.send-sms-to-provider.task-time", monotonic() - task_start) + statsd_client.timing("notifications.sms.total-time", datetime.utcnow() - notification.created_at) def provider_to_use(notification_type, notification_id): diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index e0fff90f6..4dd244911 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -116,7 +116,8 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( sample_template, mocker): db_notification = sample_notification(notify_db, notify_db_session, - template=sample_template, to_field='+447234123123') + template=sample_template, to_field='+447234123123', + status='created') mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") @@ -146,6 +147,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( assert persisted_notification.template_version == version_on_notification assert persisted_notification.template_version != sample_template.version assert persisted_notification.content_char_count == len("Sample service: This is a template") + assert persisted_notification.status == 'sending' assert not persisted_notification.personalisation diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 8fe1a1e83..8773218fc 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -316,7 +316,7 @@ def sample_notification(notify_db, job=None, job_row_number=None, to_field=None, - status='sending', + status='created', reference=None, created_at=datetime.utcnow(), content_char_count=160, diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index d134118df..90c7dea37 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -678,7 +678,7 @@ def test_get_all_notifications_for_job_by_status(notify_db, notify_db_session, s def test_update_notification(sample_notification, sample_template): - assert sample_notification.status == 'sending' + assert sample_notification.status == 'created' sample_notification.status = 'failed' dao_update_notification(sample_notification) notification_from_db = Notification.query.get(sample_notification.id) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index ee62cc4cd..b681f43a4 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -23,7 +23,7 @@ def test_get_notification_by_id(notify_api, sample_notification): headers=[auth_header]) notification = json.loads(response.get_data(as_text=True))['data']['notification'] - assert notification['status'] == 'sending' + assert notification['status'] == 'created' assert notification['template'] == { 'id': str(sample_notification.template.id), 'name': sample_notification.template.name, @@ -63,7 +63,7 @@ def test_get_all_notifications(notify_api, sample_notification): headers=[auth_header]) notifications = json.loads(response.get_data(as_text=True)) - assert notifications['notifications'][0]['status'] == 'sending' + assert notifications['notifications'][0]['status'] == 'created' assert notifications['notifications'][0]['template'] == { 'id': str(sample_notification.template.id), 'name': sample_notification.template.name, @@ -459,7 +459,8 @@ def test_filter_by_multiple_statuss(notify_api, notify_db, notify_db_session, service=sample_email_template.service, - template=sample_email_template) + template=sample_email_template, + status='sending') auth_header = create_authorization_header(service_id=sample_email_template.service_id) @@ -645,7 +646,7 @@ def test_firetext_callback_should_update_notification_status(notify_api, sample_ with notify_api.test_client() as client: mocker.patch('app.statsd_client.incr') original = get_notification_by_id(sample_notification.id) - assert original.status == 'sending' + assert original.status == 'created' response = client.post( path='/notifications/sms/firetext', @@ -674,7 +675,7 @@ def test_firetext_callback_should_update_notification_status_failed(notify_api, with notify_api.test_client() as client: mocker.patch('app.statsd_client.incr') original = get_notification_by_id(sample_notification.id) - assert original.status == 'sending' + assert original.status == 'created' response = client.post( path='/notifications/sms/firetext', @@ -966,7 +967,8 @@ def test_ses_callback_should_update_notification_status( notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' @@ -1056,7 +1058,8 @@ def test_ses_callback_should_update_record_statsd( notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' @@ -1079,7 +1082,8 @@ def test_ses_callback_should_set_status_to_temporary_failure(notify_api, notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' @@ -1138,7 +1142,8 @@ def test_ses_callback_should_set_status_to_permanent_failure(notify_api, notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' diff --git a/tests/app/status/test_delivery_status.py b/tests/app/status/test_delivery_status.py index d8b3f6bb3..21f714860 100644 --- a/tests/app/status/test_delivery_status.py +++ b/tests/app/status/test_delivery_status.py @@ -5,6 +5,8 @@ from datetime import ( timedelta ) +from tests.app.conftest import sample_notification + def test_get_delivery_status_all_ok(notify_api, notify_db): with notify_api.test_request_context(): @@ -17,11 +19,12 @@ def test_get_delivery_status_all_ok(notify_api, notify_db): assert resp_json['message'] == '0 notifications in sending state over 5 minutes' -def test_get_delivery_status_with_undelivered_notification(notify_api, notify_db, sample_notification): +def test_get_delivery_status_with_undelivered_notification(notify_api, notify_db, notify_db_session): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, status='sending') more_than_five_mins_ago = datetime.utcnow() - timedelta(minutes=10) - sample_notification.created_at = more_than_five_mins_ago - notify_db.session.add(sample_notification) + notification.created_at = more_than_five_mins_ago + notify_db.session.add(notification) notify_db.session.commit() with notify_api.test_request_context(): From 84352178088b8546893e941c6722c4177bbc1ca6 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 27 Jun 2016 15:02:53 +0100 Subject: [PATCH 3/6] Fix new test for the default status of a notification --- tests/app/notifications/test_rest.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 6c23908bd..5759254b1 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -44,7 +44,8 @@ def test_get_email_notification_by_id(notify_api, notify_db, notify_db_session, email_notification = create_sample_notification(notify_db, notify_db_session, service=sample_email_template.service, - template=sample_email_template) + template=sample_email_template, + status='sending') with notify_api.test_request_context(): with notify_api.test_client() as client: From 90e81547006ea789cebbe146913a69e8ad8b270c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 27 Jun 2016 15:23:23 +0100 Subject: [PATCH 4/6] Added a test that the message is not sent if the status of the notification is not created. --- tests/app/celery/test_provider_tasks.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 4dd244911..3176d9901 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -4,6 +4,7 @@ from celery.exceptions import MaxRetriesExceededError from mock import ANY, call from notifications_utils.recipients import validate_phone_number, format_phone_number +import app from app import statsd_client, mmg_client from app.celery import provider_tasks from app.celery.provider_tasks import send_sms_to_provider @@ -219,6 +220,26 @@ def test_not_should_update_provider_stats_on_success_in_research_mode(notify_db, assert len(updated_provider_stats) == 0 +def test_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session, + sample_service, + mocker): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + service=sample_service, + status='sending') + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') + + send_sms_to_provider( + notification.service_id, + notification.id + ) + + app.mmg_client.send_sms.assert_not_called() + app.celery.research_mode_tasks.send_sms_response.apply_async.assert_not_called() + + + def test_statsd_updates(notify_db, notify_db_session, sample_service, sample_notification, mocker): mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing') From 3346582634c32b62035947f8417680a3ebc147db Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 27 Jun 2016 15:32:26 +0100 Subject: [PATCH 5/6] Fix codestyle --- tests/app/celery/test_provider_tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 3176d9901..76f2d239b 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -239,7 +239,6 @@ def test_should_not_send_to_provider_when_status_is_not_created(notify_db, notif app.celery.research_mode_tasks.send_sms_response.apply_async.assert_not_called() - def test_statsd_updates(notify_db, notify_db_session, sample_service, sample_notification, mocker): mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing') From b3fabae958f793171fe92c8b0536cdcb70a90509 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 27 Jun 2016 15:38:25 +0100 Subject: [PATCH 6/6] Merge and codestyle --- tests/app/celery/test_provider_tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 76f2d239b..cce6458cf 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -224,8 +224,8 @@ def test_should_not_send_to_provider_when_status_is_not_created(notify_db, notif sample_service, mocker): notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - service=sample_service, - status='sending') + service=sample_service, + status='sending') mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async')