diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index ec2866e1d..c232d5f1c 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -54,52 +54,53 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati service = dao_fetch_service_by_id(service_id) provider = provider_to_use('sms', notification_id) notification = get_notification_by_id(notification_id) + if notification.status == 'created': + template = Template( + dao_get_template_by_id(notification.template_id, notification.template_version).__dict__, + values={} if not notification.personalisation else notification.personalisation, + prefix=service.name + ) - template = Template( - dao_get_template_by_id(notification.template_id, notification.template_version).__dict__, - values={} if not notification.personalisation else notification.personalisation, - prefix=service.name - ) - try: - if service.research_mode: - send_sms_response.apply_async( - (provider.get_name(), str(notification_id), notification.to), queue='research-mode' - ) - else: - provider.send_sms( - to=validate_and_format_phone_number(notification.to), - content=template.replaced, - reference=str(notification_id) - ) - - update_provider_stats( - notification_id, - 'sms', - provider.get_name(), - content_char_count=template.replaced_content_count - ) - - notification.sent_at = datetime.utcnow() - notification.sent_by = provider.get_name(), - notification.content_char_count = template.replaced_content_count - notification.status = 'sending' - dao_update_notification(notification) - except SmsClientException as e: try: - current_app.logger.error( - "SMS notification {} failed".format(notification_id) - ) - current_app.logger.exception(e) - self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) - except self.MaxRetriesExceededError: - update_notification_status_by_id(notification.id, 'technical-failure', 'failure') + if service.research_mode: + send_sms_response.apply_async( + (provider.get_name(), str(notification_id), notification.to), queue='research-mode' + ) + else: + provider.send_sms( + to=validate_and_format_phone_number(notification.to), + content=template.replaced, + reference=str(notification_id) + ) - current_app.logger.info( - "SMS {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at) - ) - statsd_client.incr("notifications.tasks.send-sms-to-provider") - statsd_client.timing("notifications.tasks.send-sms-to-provider.task-time", monotonic() - task_start) - statsd_client.timing("notifications.sms.total-time", datetime.utcnow() - notification.created_at) + update_provider_stats( + notification_id, + 'sms', + provider.get_name(), + content_char_count=template.replaced_content_count + ) + + notification.sent_at = datetime.utcnow() + notification.sent_by = provider.get_name(), + notification.content_char_count = template.replaced_content_count + notification.status = 'sending' + dao_update_notification(notification) + except SmsClientException as e: + try: + current_app.logger.error( + "SMS notification {} failed".format(notification_id) + ) + current_app.logger.exception(e) + self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) + except self.MaxRetriesExceededError: + update_notification_status_by_id(notification.id, 'technical-failure', 'failure') + + current_app.logger.info( + "SMS {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at) + ) + statsd_client.incr("notifications.tasks.send-sms-to-provider") + statsd_client.timing("notifications.tasks.send-sms-to-provider.task-time", monotonic() - task_start) + statsd_client.timing("notifications.sms.total-time", datetime.utcnow() - notification.created_at) def provider_to_use(notification_type, notification_id): diff --git a/app/celery/tasks.py b/app/celery/tasks.py index ff74c98bb..cebcb8862 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -161,7 +161,7 @@ def send_sms(self, service_id, notification_id, encrypted_notification, created_ ) dao_create_notification(notification_db_object, TEMPLATE_TYPE_SMS) - send_sms_to_provider.apply_async((service_id, notification_id, encrypted_notification), queue='sms') + send_sms_to_provider.apply_async((service_id, notification_id), queue='sms') current_app.logger.info( "SMS {} created at {} sent at {}".format(notification_id, created_at, sent_at) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 45524a130..cce6458cf 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,20 +1,19 @@ +from datetime import datetime + from celery.exceptions import MaxRetriesExceededError - from mock import ANY, call +from notifications_utils.recipients import validate_phone_number, format_phone_number -from app import statsd_client, mmg_client, encryption +import app +from app import statsd_client, mmg_client from app.celery import provider_tasks from app.celery.provider_tasks import send_sms_to_provider +from app.celery.research_mode_tasks import send_sms_response from app.celery.tasks import provider_to_use from app.clients.sms import SmsClientException -from app.dao import provider_statistics_dao -from datetime import datetime -from freezegun import freeze_time from app.dao import notifications_dao, provider_details_dao -from notifications_utils.recipients import validate_phone_number, format_phone_number -from app.celery.research_mode_tasks import send_sms_response +from app.dao import provider_statistics_dao from app.models import Notification, NotificationStatistics, Job - from tests.app.conftest import ( sample_notification ) @@ -118,7 +117,8 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( sample_template, mocker): db_notification = sample_notification(notify_db, notify_db_session, - template=sample_template, to_field='+447234123123') + template=sample_template, to_field='+447234123123', + status='created') mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") @@ -148,6 +148,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( assert persisted_notification.template_version == version_on_notification assert persisted_notification.template_version != sample_template.version assert persisted_notification.content_char_count == len("Sample service: This is a template") + assert persisted_notification.status == 'sending' assert not persisted_notification.personalisation @@ -219,6 +220,25 @@ def test_not_should_update_provider_stats_on_success_in_research_mode(notify_db, assert len(updated_provider_stats) == 0 +def test_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session, + sample_service, + mocker): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + service=sample_service, + status='sending') + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') + + send_sms_to_provider( + notification.service_id, + notification.id + ) + + app.mmg_client.send_sms.assert_not_called() + app.celery.research_mode_tasks.send_sms_response.apply_async.assert_not_called() + + def test_statsd_updates(notify_db, notify_db_session, sample_service, sample_notification, mocker): mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing') diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 39452a2de..34e07ba31 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -332,8 +332,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( (sample_template_with_placeholders.service_id, - notification_id, - encryption.encrypt(notification)), + notification_id), queue="sms" ) @@ -371,8 +370,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( (service.id, - notification_id, - encrypt_notification), + notification_id), queue="sms" ) @@ -480,8 +478,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, mocker) ) provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( (sample_job.service.id, - notification_id, - encryption.encrypt(notification)), + notification_id), queue="sms" ) persisted_notification = Notification.query.filter_by(id=notification_id).one() diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 8d1b1fed2..28f4a9df1 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -318,7 +318,7 @@ def sample_notification(notify_db, job=None, job_row_number=None, to_field=None, - status='sending', + status='created', reference=None, created_at=datetime.utcnow(), content_char_count=160, diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index d134118df..90c7dea37 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -678,7 +678,7 @@ def test_get_all_notifications_for_job_by_status(notify_db, notify_db_session, s def test_update_notification(sample_notification, sample_template): - assert sample_notification.status == 'sending' + assert sample_notification.status == 'created' sample_notification.status = 'failed' dao_update_notification(sample_notification) notification_from_db = Notification.query.get(sample_notification.id) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 3890da134..5759254b1 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -23,7 +23,7 @@ def test_get_sms_notification_by_id(notify_api, sample_notification): headers=[auth_header]) notification = json.loads(response.get_data(as_text=True))['data']['notification'] - assert notification['status'] == 'sending' + assert notification['status'] == 'created' assert notification['template'] == { 'id': str(sample_notification.template.id), 'name': sample_notification.template.name, @@ -44,7 +44,8 @@ def test_get_email_notification_by_id(notify_api, notify_db, notify_db_session, email_notification = create_sample_notification(notify_db, notify_db_session, service=sample_email_template.service, - template=sample_email_template) + template=sample_email_template, + status='sending') with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -99,7 +100,7 @@ def test_get_all_notifications(notify_api, sample_notification): headers=[auth_header]) notifications = json.loads(response.get_data(as_text=True)) - assert notifications['notifications'][0]['status'] == 'sending' + assert notifications['notifications'][0]['status'] == 'created' assert notifications['notifications'][0]['template'] == { 'id': str(sample_notification.template.id), 'name': sample_notification.template.name, @@ -496,7 +497,8 @@ def test_filter_by_multiple_statuss(notify_api, notify_db, notify_db_session, service=sample_email_template.service, - template=sample_email_template) + template=sample_email_template, + status='sending') auth_header = create_authorization_header(service_id=sample_email_template.service_id) @@ -682,7 +684,7 @@ def test_firetext_callback_should_update_notification_status(notify_api, sample_ with notify_api.test_client() as client: mocker.patch('app.statsd_client.incr') original = get_notification_by_id(sample_notification.id) - assert original.status == 'sending' + assert original.status == 'created' response = client.post( path='/notifications/sms/firetext', @@ -711,7 +713,7 @@ def test_firetext_callback_should_update_notification_status_failed(notify_api, with notify_api.test_client() as client: mocker.patch('app.statsd_client.incr') original = get_notification_by_id(sample_notification.id) - assert original.status == 'sending' + assert original.status == 'created' response = client.post( path='/notifications/sms/firetext', @@ -1003,7 +1005,8 @@ def test_ses_callback_should_update_notification_status( notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' @@ -1093,7 +1096,8 @@ def test_ses_callback_should_update_record_statsd( notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' @@ -1116,7 +1120,8 @@ def test_ses_callback_should_set_status_to_temporary_failure(notify_api, notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' @@ -1175,7 +1180,8 @@ def test_ses_callback_should_set_status_to_permanent_failure(notify_api, notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' diff --git a/tests/app/status/test_delivery_status.py b/tests/app/status/test_delivery_status.py index d8b3f6bb3..21f714860 100644 --- a/tests/app/status/test_delivery_status.py +++ b/tests/app/status/test_delivery_status.py @@ -5,6 +5,8 @@ from datetime import ( timedelta ) +from tests.app.conftest import sample_notification + def test_get_delivery_status_all_ok(notify_api, notify_db): with notify_api.test_request_context(): @@ -17,11 +19,12 @@ def test_get_delivery_status_all_ok(notify_api, notify_db): assert resp_json['message'] == '0 notifications in sending state over 5 minutes' -def test_get_delivery_status_with_undelivered_notification(notify_api, notify_db, sample_notification): +def test_get_delivery_status_with_undelivered_notification(notify_api, notify_db, notify_db_session): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, status='sending') more_than_five_mins_ago = datetime.utcnow() - timedelta(minutes=10) - sample_notification.created_at = more_than_five_mins_ago - notify_db.session.add(sample_notification) + notification.created_at = more_than_five_mins_ago + notify_db.session.add(notification) notify_db.session.commit() with notify_api.test_request_context():