From 12503d6291b4f1f974dfd6351b44961a6a7224ec Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 3 Jun 2016 14:54:46 +0100 Subject: [PATCH 1/7] First spike to split up send-sms task - 2 separate tasks - DB and Provider - DB to persist notification - Provider to contact provider - Each piece has separate retries - Provider retries have configured back-off --- app/celery/provider_tasks.py | 104 +++++++++++++++++++++++++++++++++++ app/celery/tasks.py | 56 ++++--------------- app/dao/notifications_dao.py | 10 ++-- 3 files changed, 119 insertions(+), 51 deletions(-) create mode 100644 app/celery/provider_tasks.py diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py new file mode 100644 index 000000000..1ab706670 --- /dev/null +++ b/app/celery/provider_tasks.py @@ -0,0 +1,104 @@ +import json + +from celery.exceptions import MaxRetriesExceededError + +from datetime import datetime +from monotonic import monotonic +from flask import current_app +from app import notify_celery, statsd_client, encryption, clients +from app.clients.sms import SmsClientException +from app.dao.notifications_dao import ( + update_provider_stats, + get_notification_by_id, + dao_update_notification) +from app.dao.provider_details_dao import get_provider_details_by_notification_type +from app.dao.services_dao import dao_fetch_service_by_id +from app.celery.research_mode_tasks import send_email_response, send_sms_response + +from notifications_utils.recipients import ( + validate_and_format_phone_number +) + +from app.dao.templates_dao import dao_get_template_by_id +from notifications_utils.template import ( + Template, + unlink_govuk_escaped +) + + +retries = { + 0: 5, # 5 seconds + 1: 30, # 30 seconds + 2: 60 * 5, # 5 minutes + 3: 60 * 15, # 15 minutes + 4: 60 * 30 # 30 minutes +} + +@notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) +def send_sms_to_provider(self, service_id, notification_id, encrypted_notification): + task_start = monotonic() + + service = dao_fetch_service_by_id(service_id) + provider = provider_to_use('sms', notification_id) + notification = get_notification_by_id(notification_id) + + notification_json = encryption.decrypt(encrypted_notification) + + template = Template( + dao_get_template_by_id(notification.template_id, notification.template_version).__dict__, + values=notification_json.get('personalisation', {}), + prefix=service.name + ) + try: + if service.research_mode: + send_sms_response.apply_async( + (provider.get_name(), str(notification_id), notification_json['to']), queue='research-mode' + ) + else: + provider.send_sms( + to=validate_and_format_phone_number(notification_json['to']), + content=template.replaced, + reference=str(notification_id) + ) + + update_provider_stats( + notification_id, + 'sms', + provider.get_name(), + content_char_count=template.replaced_content_count + ) + + except SmsClientException as e: + try: + current_app.logger.error( + "SMS notification {} failed".format(notification_id) + ) + current_app.logger.exception(e) + raise self.retry(queue="sms", countdown=retries[self.request.retries]) + except self.MaxRetriesExceededError: + notification.status = 'technical-failure' + + notification.sent_at = datetime.utcnow() + notification.sent_by = provider.get_name(), + notification.content_char_count = template.replaced_content_count + dao_update_notification(notification) + + 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) + + +def provider_to_use(notification_type, notification_id): + active_providers_in_order = [ + provider for provider in get_provider_details_by_notification_type(notification_type) if provider.active + ] + + if not active_providers_in_order: + current_app.logger.error( + "{} {} failed as no active providers".format(notification_type, notification_id) + ) + raise Exception("No active {} providers".format(notification_type)) + + return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 80925b5af..a736de8b5 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -11,7 +11,9 @@ from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.celery.research_mode_tasks import send_email_response, send_sms_response - +from app.celery.provider_tasks import ( + send_sms_to_provider +) from notifications_utils.template import Template, unlink_govuk_escaped from notifications_utils.recipients import ( @@ -212,14 +214,12 @@ def remove_job(job_id): current_app.logger.info("Job {} has been removed from s3.".format(job_id)) -@notify_celery.task(name="send-sms") -def send_sms(service_id, notification_id, encrypted_notification, created_at): +@notify_celery.task(bind=True, name="send-sms", max_retries=5, default_retry_delay=5) +def send_sms(self, service_id, notification_id, encrypted_notification, created_at): task_start = monotonic() notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) - provider = provider_to_use('sms', notification_id) - if not service_allowed_to_send_to(notification['to'], service): current_app.logger.info( "SMS {} failed as restricted service".format(notification_id) @@ -228,12 +228,6 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): try: - template = Template( - dao_get_template_by_id(notification['template'], notification['template_version']).__dict__, - values=notification.get('personalisation', {}), - prefix=service.name - ) - sent_at = datetime.utcnow() notification_db_object = Notification( id=notification_id, @@ -244,51 +238,21 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), status='sending', - created_at=datetime.strptime(created_at, DATETIME_FORMAT), - sent_at=sent_at, - sent_by=provider.get_name(), - content_char_count=template.replaced_content_count + created_at=datetime.strptime(created_at, DATETIME_FORMAT) ) - statsd_client.timing_with_dates( - "notifications.tasks.send-sms.queued-for", - sent_at, - datetime.strptime(created_at, DATETIME_FORMAT) - ) - dao_create_notification(notification_db_object, TEMPLATE_TYPE_SMS, provider.get_name()) + dao_create_notification(notification_db_object, TEMPLATE_TYPE_SMS) - 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() - ) - - except SmsClientException as e: - current_app.logger.error( - "SMS notification {} failed".format(notification_id) - ) - current_app.logger.exception(e) - notification_db_object.status = 'technical-failure' - dao_update_notification(notification_db_object) + send_sms_to_provider.apply_async((service_id, notification_id, encrypted_notification), queue='sms') current_app.logger.info( "SMS {} created at {} sent at {}".format(notification_id, created_at, sent_at) ) + statsd_client.incr("notifications.tasks.send-sms") statsd_client.timing("notifications.tasks.send-sms.task-time", monotonic() - task_start) except SQLAlchemyError as e: current_app.logger.exception(e) + raise self.retry(queue="sms", exc=e) @notify_celery.task(name="send-email") diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index ac9d597d9..4260e4817 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -140,9 +140,7 @@ def dao_get_template_statistics_for_service(service_id, limit_days=None): @transactional -def dao_create_notification(notification, notification_type, provider_identifier): - provider = ProviderDetails.query.filter_by(identifier=provider_identifier).one() - +def dao_create_notification(notification, notification_type): if notification.job_id: db.session.query(Job).filter_by( id=notification.job_id @@ -281,8 +279,8 @@ def dao_update_notification(notification): def update_provider_stats( id_, notification_type, - provider_name): - + provider_name, + content_char_count=None): notification = Notification.query.filter(Notification.id == id_).one() provider = ProviderDetails.query.filter_by(identifier=provider_name).one() @@ -290,6 +288,8 @@ def update_provider_stats( if notification_type == TEMPLATE_TYPE_EMAIL: return 1 else: + if (content_char_count): + return get_sms_fragment_count(content_char_count) return get_sms_fragment_count(notification.content_char_count) update_count = db.session.query(ProviderStatistics).filter_by( From 642aa2890ec6f984e1b3ed34006afc9071dc8805 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 7 Jun 2016 11:09:54 +0100 Subject: [PATCH 2/7] Starting to implement tests across the new tasks file --- app/celery/provider_tasks.py | 9 +- app/celery/tasks.py | 4 +- config.py | 3 +- tests/app/celery/test_provider_tasks.py | 116 ++++++++++++++++++++++++ tests/app/celery/test_tasks.py | 2 + 5 files changed, 129 insertions(+), 5 deletions(-) create mode 100644 tests/app/celery/test_provider_tasks.py diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 1ab706670..82c05f815 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -2,6 +2,7 @@ import json from celery.exceptions import MaxRetriesExceededError +from sqlalchemy.exc import SQLAlchemyError from datetime import datetime from monotonic import monotonic from flask import current_app @@ -26,7 +27,7 @@ from notifications_utils.template import ( ) -retries = { +retry_iteration_to_delay = { 0: 5, # 5 seconds 1: 30, # 30 seconds 2: 60 * 5, # 5 minutes @@ -74,9 +75,12 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati "SMS notification {} failed".format(notification_id) ) current_app.logger.exception(e) - raise self.retry(queue="sms", countdown=retries[self.request.retries]) + raise self.retry(queue="retry", countdown=retry_iteration_to_delay[self.request.retries]) except self.MaxRetriesExceededError: notification.status = 'technical-failure' + except SQLAlchemyError as e: + current_app.logger.exception(e) + raise self.retry(queue="retry", exc=e) notification.sent_at = datetime.utcnow() notification.sent_by = provider.get_name(), @@ -88,6 +92,7 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati ) 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", monotonic() - notification.created_at) def provider_to_use(notification_type, notification_id): diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a736de8b5..cbb505b66 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -3,7 +3,7 @@ from datetime import datetime from flask import current_app from monotonic import monotonic -from sqlalchemy.exc import SQLAlchemyError + from app import clients, statsd_client from app.clients.email import EmailClientException from app.clients.sms import SmsClientException @@ -252,7 +252,7 @@ def send_sms(self, service_id, notification_id, encrypted_notification, created_ statsd_client.timing("notifications.tasks.send-sms.task-time", monotonic() - task_start) except SQLAlchemyError as e: current_app.logger.exception(e) - raise self.retry(queue="sms", exc=e) + raise self.retry(queue="retry", exc=e) @notify_celery.task(name="send-email") diff --git a/config.py b/config.py index 69b0e6694..6282babbd 100644 --- a/config.py +++ b/config.py @@ -82,7 +82,8 @@ class Config(object): Queue('bulk-email', Exchange('default'), routing_key='bulk-email'), Queue('email-invited-user', Exchange('default'), routing_key='email-invited-user'), Queue('email-registration-verification', Exchange('default'), routing_key='email-registration-verification'), - Queue('research-mode', Exchange('default'), routing_key='research-mode') + Queue('research-mode', Exchange('default'), routing_key='research-mode'), + Queue('retry', Exchange('default'), routing_key='retry') ] TWILIO_ACCOUNT_SID = os.getenv('TWILIO_ACCOUNT_SID') TWILIO_AUTH_TOKEN = os.getenv('TWILIO_AUTH_TOKEN') diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py new file mode 100644 index 000000000..c0cf5a146 --- /dev/null +++ b/tests/app/celery/test_provider_tasks.py @@ -0,0 +1,116 @@ +import uuid + +from mock import ANY + +from app import statsd_client, mmg_client, DATETIME_FORMAT +from app.celery.provider_tasks import send_sms_to_provider +from app.celery.tasks import provider_to_use +from app.dao import provider_details_dao +from datetime import datetime, timedelta +from freezegun import freeze_time +from app.dao import notifications_dao, jobs_dao, provider_details_dao +from notifications_utils.recipients import validate_phone_number, format_phone_number + +from tests.app.conftest import ( + sample_service, + sample_user, + sample_template, + sample_job, + sample_email_template, + sample_notification +) + +def test_should_return_highest_priority_active_provider(notify_db, notify_db_session): + providers = provider_details_dao.get_provider_details_by_notification_type('sms') + first = providers[0] + second = providers[1] + + assert provider_to_use('sms', '1234').name == first.identifier + + first.priority = 20 + second.priority = 10 + + provider_details_dao.dao_update_provider_details(first) + provider_details_dao.dao_update_provider_details(second) + + assert provider_to_use('sms', '1234').name == second.identifier + + first.priority = 10 + first.active = False + second.priority = 20 + + provider_details_dao.dao_update_provider_details(first) + provider_details_dao.dao_update_provider_details(second) + + assert provider_to_use('sms', '1234').name == second.identifier + + first.active = True + provider_details_dao.dao_update_provider_details(first) + + assert provider_to_use('sms', '1234').name == first.identifier + + +def test_should_send_template_to_correct_sms_provider_and_persist(sample_notification, sample_template_with_placeholders, mocker): + notification = _notification_json( + sample_template_with_placeholders, + to="+447234123123", + personalisation={"name": "Jo"} + ) + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.dao.notifications_dao.get_notification_by_id', return_value=sample_notification) + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.statsd_client.incr') + mocker.patch('app.statsd_client.timing_with_dates') + mocker.patch('app.statsd_client.timing') + + notification_id = uuid.uuid4() + + freezer = freeze_time("2016-01-01 11:09:00.00000") + freezer.start() + now = datetime.utcnow() + freezer.stop() + + freezer = freeze_time("2016-01-01 11:10:00.00000") + freezer.start() + + send_sms_to_provider( + sample_template_with_placeholders.service_id, + notification_id, + "encrypted-in-reality" + ) + freezer.stop() + + + mmg_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123123")), + content="Sample service: Hello Jo", + reference=str(notification_id) + ) + + statsd_client.incr.assert_called_once_with("notifications.tasks.send-sms") + statsd_client.timing.assert_called_once_with("notifications.tasks.send-sms-to-provider.task-time", ANY) + statsd_client.timing.assert_called_once_with("notifications.sms.total-time", ANY) + + + notification = notifications_dao.get_notification( + sample_template_with_placeholders.service_id, notification_id + ) + assert notification.status == 'sending' + assert notification.sent_at > now + assert notification.sent_by == 'mmg' + + +def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): + notification = { + "template": template.id, + "template_version": template.version, + "to": to, + } + if personalisation: + notification.update({"personalisation": personalisation}) + if job_id: + notification.update({"job": job_id}) + if row_number: + notification['row_number'] = row_number + return notification \ No newline at end of file diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index e0b8c7166..0c6b911ae 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -51,6 +51,7 @@ class AnyStringWith(str): mmg_error = {'Error': '40', 'Description': 'error'} +# TODO moved to test_provider_tasks once send-email migrated def test_should_return_highest_priority_active_provider(notify_db, notify_db_session): providers = provider_details_dao.get_provider_details_by_notification_type('sms') first = providers[0] @@ -338,6 +339,7 @@ def test_should_process_all_sms_job(sample_job, assert job.status == 'finished' +### START OF SEND-SMS def test_should_send_template_to_correct_sms_provider_and_persist(sample_template_with_placeholders, mocker): notification = _notification_json(sample_template_with_placeholders, to="+447234123123", personalisation={"name": "Jo"}) From 92e4c0872b8a72f0350f6480c050aa8fdee82c4e Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 7 Jun 2016 12:53:31 +0100 Subject: [PATCH 3/7] Pulled out the tests that send SMSs into provider_tasks file/tests - updated tests to validate creation of next task, not sending direct to provider --- app/celery/provider_tasks.py | 3 +- app/celery/tasks.py | 5 +- tests/app/celery/test_provider_tasks.py | 157 +++++++++++++-- tests/app/celery/test_tasks.py | 255 +++++------------------- tests/app/conftest.py | 2 +- 5 files changed, 199 insertions(+), 223 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 82c05f815..1a62d7d71 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -35,6 +35,7 @@ retry_iteration_to_delay = { 4: 60 * 30 # 30 minutes } + @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) def send_sms_to_provider(self, service_id, notification_id, encrypted_notification): task_start = monotonic() @@ -92,7 +93,7 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati ) 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", monotonic() - notification.created_at) + statsd_client.timing("notifications.sms.total-time", notification.sent_at - notification.created_at) def provider_to_use(notification_type, notification_id): diff --git a/app/celery/tasks.py b/app/celery/tasks.py index cbb505b66..54026a465 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -3,6 +3,7 @@ from datetime import datetime from flask import current_app from monotonic import monotonic +from sqlalchemy.exc import SQLAlchemyError from app import clients, statsd_client from app.clients.email import EmailClientException @@ -10,7 +11,7 @@ from app.clients.sms import SmsClientException from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id from app.dao.provider_details_dao import get_provider_details_by_notification_type -from app.celery.research_mode_tasks import send_email_response, send_sms_response +from app.celery.research_mode_tasks import send_email_response from app.celery.provider_tasks import ( send_sms_to_provider ) @@ -285,7 +286,7 @@ def send_email(service_id, notification_id, from_address, encrypted_notification sent_by=provider.get_name() ) - dao_create_notification(notification_db_object, TEMPLATE_TYPE_EMAIL, provider.get_name()) + dao_create_notification(notification_db_object, TEMPLATE_TYPE_EMAIL) statsd_client.timing_with_dates( "notifications.tasks.send-email.queued-for", sent_at, diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index c0cf5a146..560aae564 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,6 +1,6 @@ import uuid -from mock import ANY +from mock import ANY, call from app import statsd_client, mmg_client, DATETIME_FORMAT from app.celery.provider_tasks import send_sms_to_provider @@ -12,11 +12,6 @@ from app.dao import notifications_dao, jobs_dao, provider_details_dao from notifications_utils.recipients import validate_phone_number, format_phone_number from tests.app.conftest import ( - sample_service, - sample_user, - sample_template, - sample_job, - sample_email_template, sample_notification ) @@ -50,22 +45,25 @@ def test_should_return_highest_priority_active_provider(notify_db, notify_db_ses assert provider_to_use('sms', '1234').name == first.identifier -def test_should_send_template_to_correct_sms_provider_and_persist(sample_notification, sample_template_with_placeholders, mocker): +def test_should_send_personalised_template_to_correct_sms_provider_and_persist( + notify_db, + notify_db_session, + sample_template_with_placeholders, + mocker): + db_notification = sample_notification(notify_db, notify_db_session, template=sample_template_with_placeholders) + notification = _notification_json( sample_template_with_placeholders, to="+447234123123", personalisation={"name": "Jo"} ) mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.dao.notifications_dao.get_notification_by_id', return_value=sample_notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing_with_dates') mocker.patch('app.statsd_client.timing') - notification_id = uuid.uuid4() - freezer = freeze_time("2016-01-01 11:09:00.00000") freezer.start() now = datetime.utcnow() @@ -75,8 +73,8 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_notific freezer.start() send_sms_to_provider( - sample_template_with_placeholders.service_id, - notification_id, + db_notification.service_id, + db_notification.id, "encrypted-in-reality" ) freezer.stop() @@ -85,22 +83,145 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_notific mmg_client.send_sms.assert_called_once_with( to=format_phone_number(validate_phone_number("+447234123123")), content="Sample service: Hello Jo", - reference=str(notification_id) + reference=str(db_notification.id) ) - statsd_client.incr.assert_called_once_with("notifications.tasks.send-sms") - statsd_client.timing.assert_called_once_with("notifications.tasks.send-sms-to-provider.task-time", ANY) - statsd_client.timing.assert_called_once_with("notifications.sms.total-time", ANY) - + statsd_client.incr.assert_called_once_with("notifications.tasks.send-sms-to-provider") + statsd_client.timing.assert_has_calls([ + call("notifications.tasks.send-sms-to-provider.task-time", ANY), + call("notifications.sms.total-time", ANY) + ]) notification = notifications_dao.get_notification( - sample_template_with_placeholders.service_id, notification_id + db_notification.service_id, db_notification.id ) + assert notification.status == 'sending' assert notification.sent_at > now assert notification.sent_by == 'mmg' +def test_should_send_template_to_correct_sms_provider_and_persist( + notify_db, + notify_db_session, + sample_template, + mocker): + db_notification = sample_notification(notify_db, notify_db_session, template=sample_template) + + + notification = _notification_json( + sample_template, + to="+447234123123" + ) + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.statsd_client.incr') + mocker.patch('app.statsd_client.timing_with_dates') + mocker.patch('app.statsd_client.timing') + + freezer = freeze_time("2016-01-01 11:09:00.00000") + freezer.start() + now = datetime.utcnow() + freezer.stop() + + freezer = freeze_time("2016-01-01 11:10:00.00000") + freezer.start() + + send_sms_to_provider( + db_notification.service_id, + db_notification.id, + "encrypted-in-reality" + ) + freezer.stop() + + mmg_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123123")), + content="Sample service: This is a template", + reference=str(db_notification.id) + ) + +### FIXME +def test_send_sms_should_use_template_version_from_job_not_latest(sample_template, mocker): + notification = _notification_json(sample_template, '+447234123123') + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + version_on_notification = sample_template.version + + # Change the template + from app.dao.templates_dao import dao_update_template, dao_get_template_by_id + sample_template.content = sample_template.content + " another version of the template" + dao_update_template(sample_template) + t = dao_get_template_by_id(sample_template.id) + assert t.version > version_on_notification + + notification_id = uuid.uuid4() + now = datetime.utcnow() + send_sms( + sample_template.service_id, + notification_id, + "encrypted-in-reality", + now.strftime(DATETIME_FORMAT) + ) + + mmg_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123123")), + content="Sample service: This is a template", + reference=str(notification_id) + ) + + persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) + assert persisted_notification.id == notification_id + assert persisted_notification.to == '+447234123123' + assert persisted_notification.template_id == sample_template.id + assert persisted_notification.template_version == version_on_notification + assert persisted_notification.template_version != sample_template.version + assert persisted_notification.created_at == now + assert persisted_notification.sent_at > now + assert persisted_notification.status == 'sending' + assert persisted_notification.sent_by == 'mmg' + assert persisted_notification.content_char_count == len("Sample service: This is a template") + + +### FIXME +def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_service, sample_template, mocker): + notification = _notification_json( + sample_template, + to="+447234123123" + ) + mocker.patch('app.encryption.decrypt', return_value=notification) + 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') + + sample_service.research_mode = True + notify_db.session.add(sample_service) + notify_db.session.commit() + + notification_id = uuid.uuid4() + now = datetime.utcnow() + send_sms( + sample_service.id, + notification_id, + "encrypted-in-reality", + now.strftime(DATETIME_FORMAT) + ) + assert not mmg_client.send_sms.called + send_sms_response.apply_async.assert_called_once_with( + ('mmg', str(notification_id), "+447234123123"), queue='research-mode' + ) + + persisted_notification = notifications_dao.get_notification(sample_service.id, notification_id) + assert persisted_notification.id == notification_id + assert persisted_notification.to == '+447234123123' + assert persisted_notification.template_id == sample_template.id + assert persisted_notification.status == 'sending' + assert persisted_notification.sent_at > now + assert persisted_notification.created_at == now + assert persisted_notification.sent_by == 'mmg' + + def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): notification = { "template": template.id, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 0c6b911ae..1b2724021 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -4,6 +4,7 @@ from flask import current_app from mock import ANY from notifications_utils.recipients import validate_phone_number, format_phone_number +from app.celery import provider_tasks from app.celery.tasks import ( send_sms, send_sms_code, @@ -340,15 +341,15 @@ def test_should_process_all_sms_job(sample_job, ### START OF SEND-SMS + def test_should_send_template_to_correct_sms_provider_and_persist(sample_template_with_placeholders, mocker): notification = _notification_json(sample_template_with_placeholders, to="+447234123123", personalisation={"name": "Jo"}) mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing_with_dates') mocker.patch('app.statsd_client.timing') + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') notification_id = uuid.uuid4() @@ -368,18 +369,15 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat ) freezer.stop() - statsd_client.timing_with_dates.assert_called_once_with( - "notifications.tasks.send-sms.queued-for", - datetime(2016, 1, 1, 11, 10, 0, 00000), - datetime(2016, 1, 1, 11, 9, 0, 00000) - ) statsd_client.timing.assert_called_once_with("notifications.tasks.send-sms.task-time", ANY) - mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: Hello Jo", - reference=str(notification_id) + provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( + (sample_template_with_placeholders.service_id, + notification_id, + "encrypted-in-reality"), + queue="sms" ) + statsd_client.incr.assert_called_once_with("notifications.tasks.send-sms") persisted_notification = notifications_dao.get_notification( sample_template_with_placeholders.service_id, notification_id @@ -390,16 +388,15 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat assert persisted_notification.template_version == sample_template_with_placeholders.version assert persisted_notification.status == 'sending' assert persisted_notification.created_at == now - assert persisted_notification.sent_at > now - assert persisted_notification.sent_by == 'mmg' + assert not persisted_notification.sent_at + assert not persisted_notification.sent_by assert not persisted_notification.job_id def test_should_send_sms_without_personalisation(sample_template, mocker): notification = _notification_json(sample_template, "+447234123123") mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') notification_id = uuid.uuid4() now = datetime.utcnow() @@ -410,10 +407,12 @@ def test_should_send_sms_without_personalisation(sample_template, mocker): now.strftime(DATETIME_FORMAT) ) - mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: This is a template", - reference=str(notification_id) + + provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( + (sample_template.service_id, + notification_id, + "encrypted-in-reality"), + queue="sms" ) @@ -425,8 +424,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif notification = _notification_json(template, "+447700900890") # The user’s own number, but in a different format mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') notification_id = uuid.uuid4() now = datetime.utcnow() @@ -437,10 +435,11 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif now.strftime(DATETIME_FORMAT) ) - mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447700900890")), - content="Sample service: This is a template", - reference=str(notification_id) + provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( + (service.id, + notification_id, + "encrypted-in-reality"), + queue="sms" ) @@ -451,8 +450,7 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notification = _notification_json(template, "07700 900849") mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') notification_id = uuid.uuid4() now = datetime.utcnow() @@ -462,53 +460,11 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, "encrypted-in-reality", now.strftime(DATETIME_FORMAT) ) - mmg_client.send_sms.assert_not_called() + provider_tasks.send_sms_to_provider.apply_async.assert_not_called() with pytest.raises(NoResultFound): notifications_dao.get_notification(service.id, notification_id) -def test_send_sms_should_use_template_version_from_job_not_latest(sample_template, mocker): - notification = _notification_json(sample_template, '+447234123123') - mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") - version_on_notification = sample_template.version - - # Change the template - from app.dao.templates_dao import dao_update_template, dao_get_template_by_id - sample_template.content = sample_template.content + " another version of the template" - dao_update_template(sample_template) - t = dao_get_template_by_id(sample_template.id) - assert t.version > version_on_notification - - notification_id = uuid.uuid4() - now = datetime.utcnow() - send_sms( - sample_template.service_id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - - mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: This is a template", - reference=str(notification_id) - ) - - persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) - assert persisted_notification.id == notification_id - assert persisted_notification.to == '+447234123123' - assert persisted_notification.template_id == sample_template.id - assert persisted_notification.template_version == version_on_notification - assert persisted_notification.template_version != sample_template.version - assert persisted_notification.created_at == now - assert persisted_notification.sent_at > now - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_by == 'mmg' - assert persisted_notification.content_char_count == len("Sample service: This is a template") - - def test_should_send_email_if_restricted_service_and_valid_email(notify_db, notify_db_session, mocker): user = sample_user(notify_db, notify_db_session, email="test@restricted.com") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) @@ -568,15 +524,14 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n notifications_dao.get_notification(service.id, notification_id) -def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sample_job, mocker): +def test_should_send_template_to_and_persist_with_job_id(sample_job, mocker): notification = _notification_json( sample_job.template, to="+447234123123", job_id=sample_job.id, row_number=2) mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') notification_id = uuid.uuid4() now = datetime.utcnow() @@ -586,10 +541,11 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa "encrypted-in-reality", now.strftime(DATETIME_FORMAT) ) - mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: This is a template", - reference=str(notification_id) + provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( + (sample_job.service.id, + notification_id, + "encrypted-in-reality"), + queue="sms" ) persisted_notification = notifications_dao.get_notification(sample_job.template.service_id, notification_id) assert persisted_notification.id == notification_id @@ -597,9 +553,9 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa assert persisted_notification.job_id == sample_job.id assert persisted_notification.template_id == sample_job.template.id assert persisted_notification.status == 'sending' - assert persisted_notification.sent_at > now + assert not persisted_notification.sent_at assert persisted_notification.created_at == now - assert persisted_notification.sent_by == 'mmg' + assert not persisted_notification.sent_by assert persisted_notification.job_row_number == 2 @@ -789,36 +745,32 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em ) -def test_should_persist_notification_as_failed_if_sms_client_fails(sample_template, mocker): +def test_should_persist_notification_as_failed_if_database_fails(sample_template, mocker): notification = _notification_json(sample_template, "+447234123123") + + expected_exception = SQLAlchemyError() + mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.mmg_client.send_sms', side_effect=MMGClientException(mmg_error)) - mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.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) now = datetime.utcnow() notification_id = uuid.uuid4() - send_sms( - sample_template.service_id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: This is a template", - reference=str(notification_id) - ) - persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) - assert persisted_notification.id == notification_id - assert persisted_notification.to == '+447234123123' - assert persisted_notification.template_id == sample_template.id - assert persisted_notification.template_version == sample_template.version - assert persisted_notification.status == 'technical-failure' - assert persisted_notification.created_at == now - assert persisted_notification.sent_at > now - assert persisted_notification.sent_by == 'mmg' + with pytest.raises(Exception): + send_sms( + sample_template.service_id, + notification_id, + "encrypted-in-reality", + now.strftime(DATETIME_FORMAT) + ) + provider_tasks.send_sms_to_provider.apply_async.assert_not_called() + tasks.send_sms.retry.assert_called_with(exc=expected_exception, queue='retry') + with pytest.raises(NoResultFound) as e: + notifications_dao.get_notification(sample_template.service_id, notification_id) + assert 'No row was found for one' in str(e.value) def test_should_persist_notification_as_failed_if_email_client_fails(sample_email_template, mocker): notification = _notification_json(sample_email_template, "my_email@my_email.com") @@ -856,27 +808,6 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai assert persisted_notification.sent_at > now -def test_should_not_send_sms_if_db_peristance_failed(sample_template, mocker): - notification = _notification_json(sample_template, "+447234123123") - mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.db.session.add', side_effect=SQLAlchemyError()) - now = datetime.utcnow() - - notification_id = uuid.uuid4() - - send_sms( - sample_template.service_id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - mmg_client.send_sms.assert_not_called() - with pytest.raises(NoResultFound) as e: - notifications_dao.get_notification(sample_template.service_id, notification_id) - assert 'No row was found for one' in str(e.value) - - def test_should_not_send_email_if_db_peristance_failed(sample_email_template, mocker): notification = _notification_json(sample_email_template, "my_email@my_email.com") mocker.patch('app.encryption.decrypt', return_value=notification) @@ -1002,43 +933,6 @@ def test_process_email_job_should_use_reply_to_email_if_present(sample_email_job ) -def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_service, sample_template, mocker): - notification = _notification_json( - sample_template, - to="+447234123123" - ) - mocker.patch('app.encryption.decrypt', return_value=notification) - 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') - - sample_service.research_mode = True - notify_db.session.add(sample_service) - notify_db.session.commit() - - notification_id = uuid.uuid4() - now = datetime.utcnow() - send_sms( - sample_service.id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - assert not mmg_client.send_sms.called - send_sms_response.apply_async.assert_called_once_with( - ('mmg', str(notification_id), "+447234123123"), queue='research-mode' - ) - - persisted_notification = notifications_dao.get_notification(sample_service.id, notification_id) - assert persisted_notification.id == notification_id - assert persisted_notification.to == '+447234123123' - assert persisted_notification.template_id == sample_template.id - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_at > now - assert persisted_notification.created_at == now - assert persisted_notification.sent_by == 'mmg' - - def test_should_call_send_email_response_task_if_research_mode( notify_db, sample_service, @@ -1132,47 +1026,6 @@ def test_should_call_send_not_update_provider_email_stats_if_research_mode( providers=[ses_provider.identifier]).first() -def test_should_call_send_sms_response_task_if_research_mode( - notify_db, - sample_service, - sample_template, - mmg_provider, - mocker): - notification = _notification_json( - sample_template, - to="+447234123123" - ) - mocker.patch('app.encryption.decrypt', return_value=notification) - 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') - - sample_service.research_mode = True - notify_db.session.add(sample_service) - notify_db.session.commit() - - assert not get_provider_statistics( - sample_template.service, - providers=[mmg_provider.identifier]).first() - - notification_id = uuid.uuid4() - now = datetime.utcnow() - send_sms( - sample_service.id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - assert not mmg_client.send_sms.called - send_sms_response.apply_async.assert_called_once_with( - ('mmg', str(notification_id), "+447234123123"), queue='research-mode' - ) - - assert not get_provider_statistics( - sample_template.service, - providers=[mmg_provider.identifier]).first() - - def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): notification = { "template": template.id, diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 27ef72278..ac3804a01 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -356,7 +356,7 @@ def sample_notification(notify_db, data['job_row_number'] = job_row_number notification = Notification(**data) if create: - dao_create_notification(notification, template.template_type, provider_name) + dao_create_notification(notification, template.template_type) return notification From b6fa6815ee84469772a4110c593033a2f3017082 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 7 Jun 2016 12:54:25 +0100 Subject: [PATCH 4/7] Commented out tests in progress --- tests/app/celery/test_provider_tasks.py | 160 ++++++++++++------------ 1 file changed, 80 insertions(+), 80 deletions(-) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 560aae564..fabd0f973 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -140,86 +140,86 @@ def test_should_send_template_to_correct_sms_provider_and_persist( content="Sample service: This is a template", reference=str(db_notification.id) ) - -### FIXME -def test_send_sms_should_use_template_version_from_job_not_latest(sample_template, mocker): - notification = _notification_json(sample_template, '+447234123123') - mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") - version_on_notification = sample_template.version - - # Change the template - from app.dao.templates_dao import dao_update_template, dao_get_template_by_id - sample_template.content = sample_template.content + " another version of the template" - dao_update_template(sample_template) - t = dao_get_template_by_id(sample_template.id) - assert t.version > version_on_notification - - notification_id = uuid.uuid4() - now = datetime.utcnow() - send_sms( - sample_template.service_id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - - mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: This is a template", - reference=str(notification_id) - ) - - persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) - assert persisted_notification.id == notification_id - assert persisted_notification.to == '+447234123123' - assert persisted_notification.template_id == sample_template.id - assert persisted_notification.template_version == version_on_notification - assert persisted_notification.template_version != sample_template.version - assert persisted_notification.created_at == now - assert persisted_notification.sent_at > now - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_by == 'mmg' - assert persisted_notification.content_char_count == len("Sample service: This is a template") - - -### FIXME -def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_service, sample_template, mocker): - notification = _notification_json( - sample_template, - to="+447234123123" - ) - mocker.patch('app.encryption.decrypt', return_value=notification) - 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') - - sample_service.research_mode = True - notify_db.session.add(sample_service) - notify_db.session.commit() - - notification_id = uuid.uuid4() - now = datetime.utcnow() - send_sms( - sample_service.id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - assert not mmg_client.send_sms.called - send_sms_response.apply_async.assert_called_once_with( - ('mmg', str(notification_id), "+447234123123"), queue='research-mode' - ) - - persisted_notification = notifications_dao.get_notification(sample_service.id, notification_id) - assert persisted_notification.id == notification_id - assert persisted_notification.to == '+447234123123' - assert persisted_notification.template_id == sample_template.id - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_at > now - assert persisted_notification.created_at == now - assert persisted_notification.sent_by == 'mmg' +# +# ### FIXME +# def test_send_sms_should_use_template_version_from_job_not_latest(sample_template, mocker): +# notification = _notification_json(sample_template, '+447234123123') +# mocker.patch('app.encryption.decrypt', return_value=notification) +# mocker.patch('app.mmg_client.send_sms') +# mocker.patch('app.mmg_client.get_name', return_value="mmg") +# version_on_notification = sample_template.version +# +# # Change the template +# from app.dao.templates_dao import dao_update_template, dao_get_template_by_id +# sample_template.content = sample_template.content + " another version of the template" +# dao_update_template(sample_template) +# t = dao_get_template_by_id(sample_template.id) +# assert t.version > version_on_notification +# +# notification_id = uuid.uuid4() +# now = datetime.utcnow() +# send_sms( +# sample_template.service_id, +# notification_id, +# "encrypted-in-reality", +# now.strftime(DATETIME_FORMAT) +# ) +# +# mmg_client.send_sms.assert_called_once_with( +# to=format_phone_number(validate_phone_number("+447234123123")), +# content="Sample service: This is a template", +# reference=str(notification_id) +# ) +# +# persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) +# assert persisted_notification.id == notification_id +# assert persisted_notification.to == '+447234123123' +# assert persisted_notification.template_id == sample_template.id +# assert persisted_notification.template_version == version_on_notification +# assert persisted_notification.template_version != sample_template.version +# assert persisted_notification.created_at == now +# assert persisted_notification.sent_at > now +# assert persisted_notification.status == 'sending' +# assert persisted_notification.sent_by == 'mmg' +# assert persisted_notification.content_char_count == len("Sample service: This is a template") +# +# +# ### FIXME +# def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_service, sample_template, mocker): +# notification = _notification_json( +# sample_template, +# to="+447234123123" +# ) +# mocker.patch('app.encryption.decrypt', return_value=notification) +# 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') +# +# sample_service.research_mode = True +# notify_db.session.add(sample_service) +# notify_db.session.commit() +# +# notification_id = uuid.uuid4() +# now = datetime.utcnow() +# send_sms( +# sample_service.id, +# notification_id, +# "encrypted-in-reality", +# now.strftime(DATETIME_FORMAT) +# ) +# assert not mmg_client.send_sms.called +# send_sms_response.apply_async.assert_called_once_with( +# ('mmg', str(notification_id), "+447234123123"), queue='research-mode' +# ) +# +# persisted_notification = notifications_dao.get_notification(sample_service.id, notification_id) +# assert persisted_notification.id == notification_id +# assert persisted_notification.to == '+447234123123' +# assert persisted_notification.template_id == sample_template.id +# assert persisted_notification.status == 'sending' +# assert persisted_notification.sent_at > now +# assert persisted_notification.created_at == now +# assert persisted_notification.sent_by == 'mmg' def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): From 6b5b40b9533347a2cb1eaeedbc9ef2e84ab30917 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 13 Jun 2016 11:38:25 +0100 Subject: [PATCH 5/7] Added tests for statsd and provider stats outcomes with the new provider stats tasks - note large change to DAO to remove provider from create notification. Added on send now not creation. --- app/celery/provider_tasks.py | 24 +- app/dao/notifications_dao.py | 16 +- tests/app/celery/test_provider_tasks.py | 250 ++++++++++++------ tests/app/celery/test_tasks.py | 35 +-- tests/app/conftest.py | 5 +- tests/app/dao/test_notification_dao.py | 100 ++++--- ...t_notifications_dao_provider_statistics.py | 12 +- 7 files changed, 239 insertions(+), 203 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 1a62d7d71..f613e6619 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -26,13 +26,12 @@ from notifications_utils.template import ( unlink_govuk_escaped ) - retry_iteration_to_delay = { - 0: 5, # 5 seconds - 1: 30, # 30 seconds - 2: 60 * 5, # 5 minutes - 3: 60 * 15, # 15 minutes - 4: 60 * 30 # 30 minutes + 0: 5, # 5 seconds + 1: 30, # 30 seconds + 2: 60 * 5, # 5 minutes + 3: 60 * 15, # 15 minutes + 4: 60 * 30 # 30 minutes } @@ -70,6 +69,11 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati 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 + dao_update_notification(notification) + except SmsClientException as e: try: current_app.logger.error( @@ -79,14 +83,6 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati raise self.retry(queue="retry", countdown=retry_iteration_to_delay[self.request.retries]) except self.MaxRetriesExceededError: notification.status = 'technical-failure' - except SQLAlchemyError as e: - current_app.logger.exception(e) - raise self.retry(queue="retry", exc=e) - - notification.sent_at = datetime.utcnow() - notification.sent_by = provider.get_name(), - notification.content_char_count = template.replaced_content_count - dao_update_notification(notification) current_app.logger.info( "SMS {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 4260e4817..6c01d9f5d 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -197,8 +197,9 @@ def _update_notification_stats_query(notification_type, status): def _update_statistics(notification, notification_statistics_status): if notification.job_id: - db.session.query(Job).filter_by(id=notification.job_id - ).update(_update_job_stats_query(notification_statistics_status)) + db.session.query(Job).filter_by( + id=notification.job_id + ).update(_update_job_stats_query(notification_statistics_status)) db.session.query(NotificationStatistics).filter_by( day=notification.created_at.date(), @@ -231,17 +232,16 @@ def _update_notification_status(notification, status, notification_statistics_st if notification_statistics_status: _update_statistics(notification, notification_statistics_status) - db.session.query(Notification).filter(Notification.id == notification.id - ).update({Notification.status: status}) + db.session.query(Notification).filter(Notification.id == notification.id).update({Notification.status: status}) return True @transactional def update_notification_status_by_id(notification_id, status, notification_statistics_status=None): - notification = Notification.query.filter(Notification.id == notification_id, - or_(Notification.status == 'sending', - Notification.status == 'pending') - ).first() + notification = Notification.query.filter( + Notification.id == notification_id, + or_(Notification.status == 'sending', + Notification.status == 'pending')).first() if not notification: return False diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index fabd0f973..a0ae9cec1 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -2,19 +2,21 @@ import uuid from mock import ANY, call -from app import statsd_client, mmg_client, DATETIME_FORMAT +from app import statsd_client, mmg_client from app.celery.provider_tasks import send_sms_to_provider from app.celery.tasks import provider_to_use -from app.dao import provider_details_dao -from datetime import datetime, timedelta +from app.dao import provider_statistics_dao +from datetime import datetime from freezegun import freeze_time -from app.dao import notifications_dao, jobs_dao, provider_details_dao +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 tests.app.conftest import ( sample_notification ) + def test_should_return_highest_priority_active_provider(notify_db, notify_db_session): providers = provider_details_dao.get_provider_details_by_notification_type('sms') first = providers[0] @@ -79,7 +81,6 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( ) freezer.stop() - mmg_client.send_sms.assert_called_once_with( to=format_phone_number(validate_phone_number("+447234123123")), content="Sample service: Hello Jo", @@ -108,7 +109,6 @@ def test_should_send_template_to_correct_sms_provider_and_persist( mocker): db_notification = sample_notification(notify_db, notify_db_session, template=sample_template) - notification = _notification_json( sample_template, to="+447234123123" @@ -140,86 +140,162 @@ def test_should_send_template_to_correct_sms_provider_and_persist( content="Sample service: This is a template", reference=str(db_notification.id) ) -# -# ### FIXME -# def test_send_sms_should_use_template_version_from_job_not_latest(sample_template, mocker): -# notification = _notification_json(sample_template, '+447234123123') -# mocker.patch('app.encryption.decrypt', return_value=notification) -# mocker.patch('app.mmg_client.send_sms') -# mocker.patch('app.mmg_client.get_name', return_value="mmg") -# version_on_notification = sample_template.version -# -# # Change the template -# from app.dao.templates_dao import dao_update_template, dao_get_template_by_id -# sample_template.content = sample_template.content + " another version of the template" -# dao_update_template(sample_template) -# t = dao_get_template_by_id(sample_template.id) -# assert t.version > version_on_notification -# -# notification_id = uuid.uuid4() -# now = datetime.utcnow() -# send_sms( -# sample_template.service_id, -# notification_id, -# "encrypted-in-reality", -# now.strftime(DATETIME_FORMAT) -# ) -# -# mmg_client.send_sms.assert_called_once_with( -# to=format_phone_number(validate_phone_number("+447234123123")), -# content="Sample service: This is a template", -# reference=str(notification_id) -# ) -# -# persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) -# assert persisted_notification.id == notification_id -# assert persisted_notification.to == '+447234123123' -# assert persisted_notification.template_id == sample_template.id -# assert persisted_notification.template_version == version_on_notification -# assert persisted_notification.template_version != sample_template.version -# assert persisted_notification.created_at == now -# assert persisted_notification.sent_at > now -# assert persisted_notification.status == 'sending' -# assert persisted_notification.sent_by == 'mmg' -# assert persisted_notification.content_char_count == len("Sample service: This is a template") -# -# -# ### FIXME -# def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_service, sample_template, mocker): -# notification = _notification_json( -# sample_template, -# to="+447234123123" -# ) -# mocker.patch('app.encryption.decrypt', return_value=notification) -# 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') -# -# sample_service.research_mode = True -# notify_db.session.add(sample_service) -# notify_db.session.commit() -# -# notification_id = uuid.uuid4() -# now = datetime.utcnow() -# send_sms( -# sample_service.id, -# notification_id, -# "encrypted-in-reality", -# now.strftime(DATETIME_FORMAT) -# ) -# assert not mmg_client.send_sms.called -# send_sms_response.apply_async.assert_called_once_with( -# ('mmg', str(notification_id), "+447234123123"), queue='research-mode' -# ) -# -# persisted_notification = notifications_dao.get_notification(sample_service.id, notification_id) -# assert persisted_notification.id == notification_id -# assert persisted_notification.to == '+447234123123' -# assert persisted_notification.template_id == sample_template.id -# assert persisted_notification.status == 'sending' -# assert persisted_notification.sent_at > now -# assert persisted_notification.created_at == now -# assert persisted_notification.sent_by == 'mmg' + + +def test_send_sms_should_use_template_version_from_notification_not_latest( + notify_db, + notify_db_session, + sample_template, + mocker): + db_notification = sample_notification(notify_db, notify_db_session, template=sample_template) + + notification = _notification_json(sample_template, '+447234123123') + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + version_on_notification = sample_template.version + + # Change the template + from app.dao.templates_dao import dao_update_template, dao_get_template_by_id + sample_template.content = sample_template.content + " another version of the template" + dao_update_template(sample_template) + t = dao_get_template_by_id(sample_template.id) + assert t.version > version_on_notification + + now = datetime.utcnow() + + send_sms_to_provider( + db_notification.service_id, + db_notification.id, + "encrypted-in-reality" + ) + + mmg_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123123")), + content="Sample service: This is a template", + reference=str(db_notification.id) + ) + + persisted_notification = notifications_dao.get_notification(sample_template.service_id, db_notification.id) + assert persisted_notification.to == db_notification.to + assert persisted_notification.template_id == sample_template.id + assert persisted_notification.template_version == version_on_notification + assert persisted_notification.template_version != sample_template.version + assert persisted_notification.sent_at > now + assert persisted_notification.status == 'sending' + assert persisted_notification.sent_by == 'mmg' + assert persisted_notification.content_char_count == len("Sample service: This is a template") + + +def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_service, sample_notification, mocker): + notification = _notification_json( + sample_notification.template, + to=sample_notification.to + ) + mocker.patch('app.encryption.decrypt', return_value=notification) + 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') + + sample_service.research_mode = True + notify_db.session.add(sample_service) + notify_db.session.commit() + + now = datetime.utcnow() + send_sms_to_provider( + sample_notification.service_id, + sample_notification.id, + "encrypted-in-reality" + ) + assert not mmg_client.send_sms.called + send_sms_response.apply_async.assert_called_once_with( + ('mmg', str(sample_notification.id), sample_notification.to), queue='research-mode' + ) + + persisted_notification = notifications_dao.get_notification(sample_service.id, sample_notification.id) + assert persisted_notification.to == sample_notification.to + assert persisted_notification.template_id == sample_notification.template_id + assert persisted_notification.status == 'sending' + assert persisted_notification.sent_at > now + assert persisted_notification.sent_by == 'mmg' + + +def test_should_update_provider_stats_on_success(notify_db, sample_service, sample_notification, mocker): + provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() + assert len(provider_stats) == 0 + + notification = _notification_json( + sample_notification.template, + to=sample_notification.to + ) + mocker.patch('app.encryption.decrypt', return_value=notification) + 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( + sample_notification.service_id, + sample_notification.id, + "encrypted-in-reality" + ) + + updated_provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() + assert updated_provider_stats[0].provider.identifier == 'mmg' + assert updated_provider_stats[0].unit_count == 1 + + +def test_not_should_update_provider_stats_on_success(notify_db, sample_service, sample_notification, mocker): + provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() + assert len(provider_stats) == 0 + + notification = _notification_json( + sample_notification.template, + to=sample_notification.to + ) + mocker.patch('app.encryption.decrypt', return_value=notification) + 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') + + sample_service.research_mode = True + notify_db.session.add(sample_service) + notify_db.session.commit() + + send_sms_to_provider( + sample_notification.service_id, + sample_notification.id, + "encrypted-in-reality" + ) + + updated_provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() + assert len(updated_provider_stats) == 0 + + +def test_statsd_updates(notify_db, notify_db_session, sample_service, sample_notification, mocker): + notification = _notification_json( + sample_notification.template, + to=sample_notification.to + ) + + mocker.patch('app.statsd_client.incr') + mocker.patch('app.statsd_client.timing') + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.encryption.decrypt', return_value=notification) + 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( + sample_notification.service_id, + sample_notification.id, + "encrypted-in-reality" + ) + + statsd_client.incr.assert_called_once_with("notifications.tasks.send-sms-to-provider") + statsd_client.timing.assert_has_calls([ + call("notifications.tasks.send-sms-to-provider.task-time", ANY), + call("notifications.sms.total-time", ANY) + ]) def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): @@ -234,4 +310,4 @@ def _notification_json(template, to, personalisation=None, job_id=None, row_numb notification.update({"job": job_id}) if row_number: notification['row_number'] = row_number - return notification \ No newline at end of file + return notification diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 1b2724021..00adde3e5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -333,16 +333,15 @@ def test_should_process_all_sms_job(sample_job, ) assert encryption.encrypt.call_args[0][0]['to'] == '+441234123120' assert encryption.encrypt.call_args[0][0]['template'] == str(sample_job_with_placeholdered_template.template.id) - assert encryption.encrypt.call_args[0][0]['template_version'] == sample_job_with_placeholdered_template.template.version # noqa + assert encryption.encrypt.call_args[0][0][ + 'template_version'] == sample_job_with_placeholdered_template.template.version # noqa assert encryption.encrypt.call_args[0][0]['personalisation'] == {'name': 'chris'} tasks.send_sms.apply_async.call_count == 10 job = jobs_dao.dao_get_job_by_id(sample_job_with_placeholdered_template.id) assert job.status == 'finished' -### START OF SEND-SMS - -def test_should_send_template_to_correct_sms_provider_and_persist(sample_template_with_placeholders, mocker): +def test_should_send_template_to_correct_sms_task_and_persist(sample_template_with_placeholders, mocker): notification = _notification_json(sample_template_with_placeholders, to="+447234123123", personalisation={"name": "Jo"}) mocker.patch('app.encryption.decrypt', return_value=notification) @@ -393,29 +392,6 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat assert not persisted_notification.job_id -def test_should_send_sms_without_personalisation(sample_template, mocker): - notification = _notification_json(sample_template, "+447234123123") - mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') - - notification_id = uuid.uuid4() - now = datetime.utcnow() - send_sms( - sample_template.service_id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - - - provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( - (sample_template.service_id, - notification_id, - "encrypted-in-reality"), - queue="sms" - ) - - def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker): user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900890") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) @@ -437,8 +413,8 @@ 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, - "encrypted-in-reality"), + notification_id, + "encrypted-in-reality"), queue="sms" ) @@ -772,6 +748,7 @@ def test_should_persist_notification_as_failed_if_database_fails(sample_template notifications_dao.get_notification(sample_template.service_id, notification_id) assert 'No row was found for one' in str(e.value) + def test_should_persist_notification_as_failed_if_email_client_fails(sample_email_template, mocker): notification = _notification_json(sample_email_template, "my_email@my_email.com") mocker.patch('app.encryption.decrypt', return_value=notification) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index ac3804a01..c69c8cc59 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -319,7 +319,6 @@ def sample_notification(notify_db, status='sending', reference=None, created_at=datetime.utcnow(), - provider_name=None, content_char_count=160, create=True): if service is None: @@ -331,9 +330,6 @@ def sample_notification(notify_db, notification_id = uuid.uuid4() - if provider_name is None: - provider_name = mmg_provider().identifier if template.template_type == 'sms' else ses_provider().identifier - if to_field: to = to_field else: @@ -342,6 +338,7 @@ def sample_notification(notify_db, data = { 'id': notification_id, 'to': to, + 'job_id': job.id, 'job': job, 'service_id': service.id, 'service': service, diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index e4a7d9694..06492c18f 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -42,8 +42,7 @@ def test_should_by_able_to_update_status_by_reference(sample_email_template, ses notification = Notification(**data) dao_create_notification( notification, - sample_email_template.template_type, - ses_provider.identifier) + sample_email_template.template_type) assert Notification.query.get(notification.id).status == "sending" notification.reference = 'reference' @@ -57,7 +56,7 @@ def test_should_by_able_to_update_status_by_reference(sample_email_template, ses def test_should_by_able_to_update_status_by_id(sample_template, sample_job, mmg_provider): data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, 'mmg') + dao_create_notification(notification, sample_template.template_type) assert Notification.query.get(notification.id).status == 'sending' assert update_notification_status_by_id(notification.id, 'delivered', 'delivered') assert Notification.query.get(notification.id).status == 'delivered' @@ -77,7 +76,7 @@ def test_should_not_update_status_by_id_if_not_sending_and_does_not_update_job(n def test_should_by_able_to_update_status_by_id_from_pending_to_delivered(sample_template, sample_job): data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, 'mmg') + dao_create_notification(notification, sample_template.template_type) assert Notification.query.get(notification.id).status == 'sending' assert update_notification_status_by_id(notification_id=notification.id, status='pending') assert Notification.query.get(notification.id).status == 'pending' @@ -93,7 +92,7 @@ def test_should_by_able_to_update_status_by_id_from_pending_to_delivered(sample_ def test_should_by_able_to_update_status_by_id_from_pending_to_temporary_failure(sample_template, sample_job): data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, 'mmg') + dao_create_notification(notification, sample_template.template_type) assert Notification.query.get(notification.id).status == 'sending' assert update_notification_status_by_id(notification_id=notification.id, status='pending') assert Notification.query.get(notification.id).status == 'pending' @@ -112,7 +111,7 @@ def test_should_by_able_to_update_status_by_id_from_pending_to_temporary_failure def test_should_by_able_to_update_status_by_id_from_sending_to_permanent_failure(sample_template, sample_job): data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, 'firetext') + dao_create_notification(notification, sample_template.template_type) assert Notification.query.get(notification.id).status == 'sending' assert update_notification_status_by_id( @@ -131,8 +130,7 @@ def test_should_not_update_status_one_notification_status_is_delivered(sample_em notification = Notification(**data) dao_create_notification( notification, - sample_email_template.template_type, - ses_provider.identifier) + sample_email_template.template_type) assert Notification.query.get(notification.id).status == "sending" update_provider_stats( @@ -161,7 +159,7 @@ def test_should_be_able_to_record_statistics_failure_for_sms(sample_notification def test_should_be_able_to_record_statistics_failure_for_email(sample_email_template, sample_job, ses_provider): data = _notification_json(sample_email_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_email_template.template_type, ses_provider.identifier) + dao_create_notification(notification, sample_email_template.template_type) update_provider_stats( notification.id, @@ -188,7 +186,7 @@ def test_should_be_able_to_get_statistics_for_a_service(sample_template, mmg_pro data = _notification_json(sample_template) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) _assert_notification_stats(notification.service_id, sms_requested=1, notification_created_at=notification.created_at.date()) @@ -198,7 +196,7 @@ def test_should_be_able_to_get_statistics_for_a_service_for_a_day(sample_templat now = datetime.utcnow() data = _notification_json(sample_template) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) stat = dao_get_notification_statistics_for_service_and_day( sample_template.service.id, now.date() ) @@ -216,7 +214,7 @@ def test_should_return_none_if_no_statistics_for_a_service_for_a_day(sample_temp data = _notification_json(sample_template) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) assert not dao_get_notification_statistics_for_service_and_day( sample_template.service.id, (datetime.utcnow() - timedelta(days=1)).date()) @@ -227,9 +225,9 @@ def test_should_be_able_to_get_all_statistics_for_a_service(sample_template, mmg notification_1 = Notification(**data) notification_2 = Notification(**data) notification_3 = Notification(**data) - dao_create_notification(notification_1, sample_template.template_type, mmg_provider.identifier) - dao_create_notification(notification_2, sample_template.template_type, mmg_provider.identifier) - dao_create_notification(notification_3, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification_1, sample_template.template_type) + dao_create_notification(notification_2, sample_template.template_type) + dao_create_notification(notification_3, sample_template.template_type) _assert_notification_stats(sample_template.service.id, sms_requested=3) @@ -247,9 +245,9 @@ def test_should_be_able_to_get_all_statistics_for_a_service_for_several_days(sam data.update({'created_at': two_days_ago}) notification_3 = Notification(**data) - dao_create_notification(notification_1, sample_template.template_type, mmg_provider.identifier) - dao_create_notification(notification_2, sample_template.template_type, mmg_provider.identifier) - dao_create_notification(notification_3, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification_1, sample_template.template_type) + dao_create_notification(notification_2, sample_template.template_type) + dao_create_notification(notification_3, sample_template.template_type) stats = dao_get_notification_statistics_for_service(sample_template.service.id) assert len(stats) == 3 @@ -281,9 +279,9 @@ def test_should_be_able_to_get_all_statistics_for_a_service_for_several_days_pre notification_2 = Notification(**data) data.update({'created_at': eight_days_ago}) notification_3 = Notification(**data) - dao_create_notification(notification_1, sample_template.template_type, mmg_provider.identifier) - dao_create_notification(notification_2, sample_template.template_type, mmg_provider.identifier) - dao_create_notification(notification_3, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification_1, sample_template.template_type) + dao_create_notification(notification_2, sample_template.template_type) + dao_create_notification(notification_3, sample_template.template_type) stats = dao_get_notification_statistics_for_service(sample_template.service.id, 7) assert len(stats) == 2 @@ -303,7 +301,7 @@ def test_save_notification_creates_sms_and_template_stats(sample_template, sampl data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) assert Notification.query.count() == 1 notification_from_db = Notification.query.all()[0] @@ -336,7 +334,7 @@ def test_save_notification_and_create_email_and_template_stats(sample_email_temp data = _notification_json(sample_email_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_email_template.template_type, ses_provider.identifier) + dao_create_notification(notification, sample_email_template.template_type) assert Notification.query.count() == 1 notification_from_db = Notification.query.all()[0] @@ -370,7 +368,7 @@ def test_save_notification_handles_midnight_properly(sample_template, sample_job data = _notification_json(sample_template, sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) assert Notification.query.count() == 1 @@ -386,7 +384,7 @@ def test_save_notification_handles_just_before_midnight_properly(sample_template data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) assert Notification.query.count() == 1 @@ -402,7 +400,7 @@ def test_save_notification_and_increment_email_stats(sample_email_template, samp notification_1 = Notification(**data) notification_2 = Notification(**data) - dao_create_notification(notification_1, sample_email_template.template_type, ses_provider.identifier) + dao_create_notification(notification_1, sample_email_template.template_type) assert Notification.query.count() == 1 @@ -412,7 +410,7 @@ def test_save_notification_and_increment_email_stats(sample_email_template, samp assert stats1.emails_requested == 1 assert stats1.sms_requested == 0 - dao_create_notification(notification_2, sample_email_template.template_type, ses_provider.identifier) + dao_create_notification(notification_2, sample_email_template.template_type) assert Notification.query.count() == 2 @@ -429,7 +427,7 @@ def test_save_notification_and_increment_sms_stats(sample_template, sample_job, notification_1 = Notification(**data) notification_2 = Notification(**data) - dao_create_notification(notification_1, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification_1, sample_template.template_type) assert Notification.query.count() == 1 @@ -440,7 +438,7 @@ def test_save_notification_and_increment_sms_stats(sample_template, sample_job, assert stats1.emails_requested == 0 assert stats1.sms_requested == 1 - dao_create_notification(notification_2, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification_2, sample_template.template_type) assert Notification.query.count() == 2 @@ -460,7 +458,7 @@ def test_not_save_notification_and_not_create_stats_on_commit_error(sample_templ notification = Notification(**data) with pytest.raises(SQLAlchemyError): - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) assert Notification.query.count() == 0 assert Job.query.get(sample_job.id).notifications_sent == 0 @@ -473,7 +471,7 @@ def test_save_notification_and_increment_job(sample_template, sample_job, mmg_pr data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) assert Notification.query.count() == 1 notification_from_db = Notification.query.all()[0] @@ -488,7 +486,7 @@ def test_save_notification_and_increment_job(sample_template, sample_job, mmg_pr assert Job.query.get(sample_job.id).notifications_sent == 1 notification_2 = Notification(**data) - dao_create_notification(notification_2, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification_2, sample_template.template_type) assert Notification.query.count() == 2 # count is off because the count is defaulted to 1 in the sample_job _assert_job_stats(sample_job.id, sent=2, count=1) @@ -500,7 +498,7 @@ def test_should_not_increment_job_if_notification_fails_to_persist(sample_templa data = _notification_json(sample_template, job_id=sample_job.id, id=random_id) notification_1 = Notification(**data) - dao_create_notification(notification_1, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification_1, sample_template.template_type) assert Notification.query.count() == 1 assert Job.query.get(sample_job.id).notifications_sent == 1 @@ -508,7 +506,7 @@ def test_should_not_increment_job_if_notification_fails_to_persist(sample_templa notification_2 = Notification(**data) with pytest.raises(SQLAlchemyError): - dao_create_notification(notification_2, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification_2, sample_template.template_type) assert Notification.query.count() == 1 assert Job.query.get(sample_job.id).notifications_sent == 1 @@ -524,7 +522,7 @@ def test_save_notification_and_increment_correct_job(notify_db, notify_db_sessio data = _notification_json(sample_template, job_id=job_1.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) assert Notification.query.count() == 1 notification_from_db = Notification.query.all()[0] @@ -546,7 +544,7 @@ def test_save_notification_with_no_job(sample_template, mmg_provider): data = _notification_json(sample_template) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) assert Notification.query.count() == 1 notification_from_db = Notification.query.all()[0] @@ -571,7 +569,7 @@ def test_save_notification_no_job_id(sample_template, mmg_provider): data = _notification_json(sample_template) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) assert Notification.query.count() == 1 notification_from_db = Notification.query.all()[0] @@ -655,7 +653,7 @@ def test_save_new_notification_creates_template_stats(sample_template, sample_jo data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) assert TemplateStatistics.query.count() == 1 template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, @@ -673,7 +671,7 @@ def test_save_new_notification_creates_template_stats_per_day(sample_template, s data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) assert TemplateStatistics.query.count() == 1 template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, @@ -687,7 +685,7 @@ def test_save_new_notification_creates_template_stats_per_day(sample_template, s with freeze_time('2016-03-31'): assert TemplateStatistics.query.count() == 1 new_notification = Notification(**data) - dao_create_notification(new_notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(new_notification, sample_template.template_type) assert TemplateStatistics.query.count() == 2 first_stats = TemplateStatistics.query.filter(TemplateStatistics.day == datetime(2016, 3, 30)).first() @@ -710,7 +708,7 @@ def test_save_another_notification_increments_template_stats(sample_template, sa notification_1 = Notification(**data) notification_2 = Notification(**data) - dao_create_notification(notification_1, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification_1, sample_template.template_type) assert TemplateStatistics.query.count() == 1 template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, @@ -719,7 +717,7 @@ def test_save_another_notification_increments_template_stats(sample_template, sa assert template_stats.template_id == sample_template.id assert template_stats.usage_count == 1 - dao_create_notification(notification_2, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification_2, sample_template.template_type) assert TemplateStatistics.query.count() == 1 template_stats = TemplateStatistics.query.filter(TemplateStatistics.service_id == sample_template.service.id, @@ -739,9 +737,9 @@ def test_successful_notification_inserts_followed_by_failure_does_not_increment_ notification_1 = Notification(**data) notification_2 = Notification(**data) notification_3 = Notification(**data) - dao_create_notification(notification_1, sample_template.template_type, mmg_provider.identifier) - dao_create_notification(notification_2, sample_template.template_type, mmg_provider.identifier) - dao_create_notification(notification_3, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification_1, sample_template.template_type) + dao_create_notification(notification_2, sample_template.template_type) + dao_create_notification(notification_3, sample_template.template_type) _assert_notification_stats(sample_template.service.id, sms_requested=3) @@ -756,32 +754,30 @@ def test_successful_notification_inserts_followed_by_failure_does_not_increment_ try: # Mess up db in really bad way db.session.execute('DROP TABLE TEMPLATE_STATISTICS') - dao_create_notification(failing_notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(failing_notification, sample_template.template_type) except Exception as e: # There should be no additional notification stats or counts _assert_notification_stats(sample_template.service.id, sms_requested=3) @freeze_time("2016-03-30") -def test_get_template_stats_for_service_returns_stats_in_reverse_date_order(sample_template, - sample_job, - mmg_provider): +def test_get_template_stats_for_service_returns_stats_in_reverse_date_order(sample_template, sample_job): template_stats = dao_get_template_statistics_for_service(sample_template.service.id) assert len(template_stats) == 0 data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) - dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(notification, sample_template.template_type) # move on one day with freeze_time('2016-03-31'): new_notification = Notification(**data) - dao_create_notification(new_notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(new_notification, sample_template.template_type) # move on one more day with freeze_time('2016-04-01'): new_notification = Notification(**data) - dao_create_notification(new_notification, sample_template.template_type, mmg_provider.identifier) + dao_create_notification(new_notification, sample_template.template_type) template_stats = dao_get_template_statistics_for_service(sample_template.service_id) assert len(template_stats) == 3 diff --git a/tests/app/dao/test_notifications_dao_provider_statistics.py b/tests/app/dao/test_notifications_dao_provider_statistics.py index 7c7abcfcb..262a008f4 100644 --- a/tests/app/dao/test_notifications_dao_provider_statistics.py +++ b/tests/app/dao/test_notifications_dao_provider_statistics.py @@ -44,21 +44,18 @@ def test_should_update_provider_statistics_sms_multi(notify_db, notify_db, notify_db_session, template=sample_template, - provider_name=mmg_provider.identifier, content_char_count=160) update_provider_stats(n1.id, 'sms', mmg_provider.identifier) n2 = create_sample_notification( notify_db, notify_db_session, template=sample_template, - provider_name=mmg_provider.identifier, content_char_count=161) update_provider_stats(n2.id, 'sms', mmg_provider.identifier) n3 = create_sample_notification( notify_db, notify_db_session, template=sample_template, - provider_name=mmg_provider.identifier, content_char_count=307) update_provider_stats(n3.id, 'sms', mmg_provider.identifier) provider_stats = get_provider_statistics( @@ -74,20 +71,17 @@ def test_should_update_provider_statistics_email_multi(notify_db, n1 = create_sample_notification( notify_db, notify_db_session, - template=sample_email_template, - provider_name=ses_provider.identifier) + template=sample_email_template) update_provider_stats(n1.id, 'email', ses_provider.identifier) n2 = create_sample_notification( notify_db, notify_db_session, - template=sample_email_template, - provider_name=ses_provider.identifier) + template=sample_email_template) update_provider_stats(n2.id, 'email', ses_provider.identifier) n3 = create_sample_notification( notify_db, notify_db_session, - template=sample_email_template, - provider_name=ses_provider.identifier) + template=sample_email_template) update_provider_stats(n3.id, 'email', ses_provider.identifier) provider_stats = get_provider_statistics( sample_email_template.service, From f143aade71089a9b34933469986c8351cbf5924e Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 13 Jun 2016 16:40:46 +0100 Subject: [PATCH 6/7] Added new tests/code to ensure failure stats recorded --- app/celery/provider_tasks.py | 35 +++++++++++--------- app/celery/tasks.py | 1 - tests/app/celery/test_provider_tasks.py | 44 +++++++++++++++++++++++-- tests/app/celery/test_tasks.py | 9 ++--- 4 files changed, 63 insertions(+), 26 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index f613e6619..57d01889c 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,8 +1,5 @@ import json -from celery.exceptions import MaxRetriesExceededError - -from sqlalchemy.exc import SQLAlchemyError from datetime import datetime from monotonic import monotonic from flask import current_app @@ -11,10 +8,10 @@ from app.clients.sms import SmsClientException from app.dao.notifications_dao import ( update_provider_stats, get_notification_by_id, - dao_update_notification) + dao_update_notification, update_notification_status_by_id) from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.dao.services_dao import dao_fetch_service_by_id -from app.celery.research_mode_tasks import send_email_response, send_sms_response +from app.celery.research_mode_tasks import send_sms_response from notifications_utils.recipients import ( validate_and_format_phone_number @@ -26,13 +23,20 @@ from notifications_utils.template import ( unlink_govuk_escaped ) -retry_iteration_to_delay = { - 0: 5, # 5 seconds - 1: 30, # 30 seconds - 2: 60 * 5, # 5 minutes - 3: 60 * 15, # 15 minutes - 4: 60 * 30 # 30 minutes -} + +def retry_iteration_to_delay(retry): + """ + Given current retry calculate some delay before retrying + Delay calculated as (1 + retry number) squared * 60 + 0: 60 seconds (1 minute) + 1: 240 seconds (4 minutes) + 2: 540 seconds (9 minutes) + 3: 960 seconds (16 minutes) + 4: 1500 seconds (25 minutes) + :param retry (zero indexed): + :return length to retry in seconds: + """ + return ((retry + 1) ** 2) * 60 @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) @@ -73,23 +77,22 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati notification.sent_by = provider.get_name(), notification.content_char_count = template.replaced_content_count dao_update_notification(notification) - except SmsClientException as e: try: current_app.logger.error( "SMS notification {} failed".format(notification_id) ) current_app.logger.exception(e) - raise self.retry(queue="retry", countdown=retry_iteration_to_delay[self.request.retries]) + self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) except self.MaxRetriesExceededError: - notification.status = 'technical-failure' + 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", notification.sent_at - notification.created_at) + statsd_client.timing("notifications.sms.total-time", datetime.utcnow() - notification.created_at) def provider_to_use(notification_type, notification_id): diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 5fbb7fb0f..fd0446444 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -8,7 +8,6 @@ from sqlalchemy.exc import SQLAlchemyError from app import clients, statsd_client from app.clients import STATISTICS_FAILURE from app.clients.email import EmailClientException -from app.clients.sms import SmsClientException from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id from app.dao.provider_details_dao import get_provider_details_by_notification_type diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index a0ae9cec1..44d2440fa 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,16 +1,19 @@ -import uuid +from celery.exceptions import MaxRetriesExceededError from mock import ANY, call 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.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.models import Notification, NotificationStatistics, Job from tests.app.conftest import ( sample_notification @@ -280,7 +283,6 @@ def test_statsd_updates(notify_db, notify_db_session, sample_service, sample_not mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing') mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.encryption.decrypt', return_value=notification) 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') @@ -298,6 +300,44 @@ def test_statsd_updates(notify_db, notify_db_session, sample_service, sample_not ]) +def test_should_go_into_technical_error_if_exceeds_retries( + notify_db, + notify_db_session, + sample_service, + sample_notification, + mocker): + + notification = _notification_json( + sample_notification.template, + to=sample_notification.to + ) + + mocker.patch('app.statsd_client.incr') + mocker.patch('app.statsd_client.timing') + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.mmg_client.send_sms', side_effect=SmsClientException("EXPECTED")) + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry', side_effect=MaxRetriesExceededError()) + + send_sms_to_provider( + sample_notification.service_id, + sample_notification.id, + "encrypted-in-reality" + ) + + provider_tasks.send_sms_to_provider.retry.assert_called_with(queue='retry', countdown=60) + assert statsd_client.incr.assert_not_called + assert statsd_client.timing.assert_not_called + + db_notification = Notification.query.get(sample_notification.id) + assert db_notification.status == 'technical-failure' + notification_stats = NotificationStatistics.query.filter_by(service_id=sample_notification.service.id).first() + assert notification_stats.sms_requested == 1 + assert notification_stats.sms_failed == 1 + job = Job.query.get(sample_notification.job.id) + assert job.notification_count == 1 + assert job.notifications_failed == 1 + + def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): notification = { "template": template.id, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 954b53db4..fd0833c29 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -2,7 +2,6 @@ import uuid import pytest from flask import current_app from mock import ANY -from notifications_utils.recipients import validate_phone_number, format_phone_number from app.celery import provider_tasks from app.celery.tasks import ( @@ -18,13 +17,9 @@ from app.celery.tasks import ( provider_to_use, timeout_notifications ) -from app.celery.research_mode_tasks import ( - send_email_response, - send_sms_response -) -from app import (aws_ses_client, encryption, DATETIME_FORMAT, mmg_client, statsd_client) +from app.celery.research_mode_tasks import send_email_response +from app import (aws_ses_client, encryption, DATETIME_FORMAT, statsd_client) from app.clients.email.aws_ses import AwsSesClientException -from app.clients.sms.mmg import MMGClientException from app.dao import notifications_dao, jobs_dao, provider_details_dao from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound From 51c6d57a86dd1b13fe74a8127099313a59b8c4af Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 17 Jun 2016 16:32:56 +0100 Subject: [PATCH 7/7] Changed the delay period. Now waits 10 seconds, 1 minute, 5 minutes, 1 hour and 4 hours. Total elapsed wait is max 5 hours 6 minutes and 10 seconds. Changed visibility window of SQS to be 4 hours 10 seconds, longer the max retry period. --- app/celery/provider_tasks.py | 26 +++++++++++++-------- config.py | 2 +- tests/app/celery/test_provider_tasks.py | 30 ++++++++++++++++++++++++- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 57d01889c..c599313ab 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -24,19 +24,27 @@ from notifications_utils.template import ( ) -def retry_iteration_to_delay(retry): +def retry_iteration_to_delay(retry=0): """ Given current retry calculate some delay before retrying - Delay calculated as (1 + retry number) squared * 60 - 0: 60 seconds (1 minute) - 1: 240 seconds (4 minutes) - 2: 540 seconds (9 minutes) - 3: 960 seconds (16 minutes) - 4: 1500 seconds (25 minutes) + 0: 10 seconds + 1: 60 seconds (1 minutes) + 2: 300 seconds (5 minutes) + 3: 3600 seconds (60 minutes) + 4: 14400 seconds (4 hours) :param retry (zero indexed): - :return length to retry in seconds: + :return length to retry in seconds, default 10 seconds """ - return ((retry + 1) ** 2) * 60 + + delays = { + 0: 10, + 1: 60, + 2: 300, + 3: 3600, + 4: 14400 + } + + return delays.get(retry, 10) @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) diff --git a/config.py b/config.py index 6b7f56eae..6973fe36f 100644 --- a/config.py +++ b/config.py @@ -38,7 +38,7 @@ class Config(object): BROKER_TRANSPORT_OPTIONS = { 'region': 'eu-west-1', 'polling_interval': 1, # 1 second - 'visibility_timeout': 60, # 60 seconds + 'visibility_timeout': 14410, # 4 hours 10 seconds. 10 seconds longer than max retry 'queue_name_prefix': os.environ['NOTIFICATION_QUEUE_PREFIX'] + '-' } CELERY_ENABLE_UTC = True, diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 44d2440fa..f4a899ffd 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -20,6 +20,34 @@ from tests.app.conftest import ( ) +def test_should_by_10_second_delay_as_default(): + assert provider_tasks.retry_iteration_to_delay() == 10 + + +def test_should_by_10_second_delay_on_unmapped_retry_iteration(): + assert provider_tasks.retry_iteration_to_delay(99) == 10 + + +def test_should_by_10_second_delay_on_retry_one(): + assert provider_tasks.retry_iteration_to_delay(0) == 10 + + +def test_should_by_1_minute_delay_on_retry_two(): + assert provider_tasks.retry_iteration_to_delay(1) == 60 + + +def test_should_by_5_minute_delay_on_retry_two(): + assert provider_tasks.retry_iteration_to_delay(2) == 300 + + +def test_should_by_60_minute_delay_on_retry_two(): + assert provider_tasks.retry_iteration_to_delay(3) == 3600 + + +def test_should_by_240_minute_delay_on_retry_two(): + assert provider_tasks.retry_iteration_to_delay(4) == 14400 + + def test_should_return_highest_priority_active_provider(notify_db, notify_db_session): providers = provider_details_dao.get_provider_details_by_notification_type('sms') first = providers[0] @@ -324,7 +352,7 @@ def test_should_go_into_technical_error_if_exceeds_retries( "encrypted-in-reality" ) - provider_tasks.send_sms_to_provider.retry.assert_called_with(queue='retry', countdown=60) + provider_tasks.send_sms_to_provider.retry.assert_called_with(queue='retry', countdown=10) assert statsd_client.incr.assert_not_called assert statsd_client.timing.assert_not_called