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"})