diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index ec2866e1d..c232d5f1c 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -54,52 +54,53 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati service = dao_fetch_service_by_id(service_id) provider = provider_to_use('sms', notification_id) notification = get_notification_by_id(notification_id) + if notification.status == 'created': + template = Template( + dao_get_template_by_id(notification.template_id, notification.template_version).__dict__, + values={} if not notification.personalisation else notification.personalisation, + prefix=service.name + ) - template = Template( - dao_get_template_by_id(notification.template_id, notification.template_version).__dict__, - values={} if not notification.personalisation else notification.personalisation, - prefix=service.name - ) - try: - if service.research_mode: - send_sms_response.apply_async( - (provider.get_name(), str(notification_id), notification.to), queue='research-mode' - ) - else: - provider.send_sms( - to=validate_and_format_phone_number(notification.to), - content=template.replaced, - reference=str(notification_id) - ) - - update_provider_stats( - notification_id, - 'sms', - provider.get_name(), - content_char_count=template.replaced_content_count - ) - - notification.sent_at = datetime.utcnow() - notification.sent_by = provider.get_name(), - notification.content_char_count = template.replaced_content_count - notification.status = 'sending' - dao_update_notification(notification) - except SmsClientException as e: try: - current_app.logger.error( - "SMS notification {} failed".format(notification_id) - ) - current_app.logger.exception(e) - self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) - except self.MaxRetriesExceededError: - update_notification_status_by_id(notification.id, 'technical-failure', 'failure') + if service.research_mode: + send_sms_response.apply_async( + (provider.get_name(), str(notification_id), notification.to), queue='research-mode' + ) + else: + provider.send_sms( + to=validate_and_format_phone_number(notification.to), + content=template.replaced, + reference=str(notification_id) + ) - current_app.logger.info( - "SMS {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at) - ) - statsd_client.incr("notifications.tasks.send-sms-to-provider") - statsd_client.timing("notifications.tasks.send-sms-to-provider.task-time", monotonic() - task_start) - statsd_client.timing("notifications.sms.total-time", datetime.utcnow() - notification.created_at) + update_provider_stats( + notification_id, + 'sms', + provider.get_name(), + content_char_count=template.replaced_content_count + ) + + notification.sent_at = datetime.utcnow() + notification.sent_by = provider.get_name(), + notification.content_char_count = template.replaced_content_count + notification.status = 'sending' + dao_update_notification(notification) + except SmsClientException as e: + try: + current_app.logger.error( + "SMS notification {} failed".format(notification_id) + ) + current_app.logger.exception(e) + self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) + except self.MaxRetriesExceededError: + update_notification_status_by_id(notification.id, 'technical-failure', 'failure') + + current_app.logger.info( + "SMS {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at) + ) + statsd_client.incr("notifications.tasks.send-sms-to-provider") + statsd_client.timing("notifications.tasks.send-sms-to-provider.task-time", monotonic() - task_start) + statsd_client.timing("notifications.sms.total-time", datetime.utcnow() - notification.created_at) def provider_to_use(notification_type, notification_id): diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index e0fff90f6..4dd244911 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -116,7 +116,8 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( sample_template, mocker): db_notification = sample_notification(notify_db, notify_db_session, - template=sample_template, to_field='+447234123123') + template=sample_template, to_field='+447234123123', + status='created') mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") @@ -146,6 +147,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( assert persisted_notification.template_version == version_on_notification assert persisted_notification.template_version != sample_template.version assert persisted_notification.content_char_count == len("Sample service: This is a template") + assert persisted_notification.status == 'sending' assert not persisted_notification.personalisation diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 8fe1a1e83..8773218fc 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -316,7 +316,7 @@ def sample_notification(notify_db, job=None, job_row_number=None, to_field=None, - status='sending', + status='created', reference=None, created_at=datetime.utcnow(), content_char_count=160, diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index d134118df..90c7dea37 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -678,7 +678,7 @@ def test_get_all_notifications_for_job_by_status(notify_db, notify_db_session, s def test_update_notification(sample_notification, sample_template): - assert sample_notification.status == 'sending' + assert sample_notification.status == 'created' sample_notification.status = 'failed' dao_update_notification(sample_notification) notification_from_db = Notification.query.get(sample_notification.id) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index ee62cc4cd..b681f43a4 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -23,7 +23,7 @@ def test_get_notification_by_id(notify_api, sample_notification): headers=[auth_header]) notification = json.loads(response.get_data(as_text=True))['data']['notification'] - assert notification['status'] == 'sending' + assert notification['status'] == 'created' assert notification['template'] == { 'id': str(sample_notification.template.id), 'name': sample_notification.template.name, @@ -63,7 +63,7 @@ def test_get_all_notifications(notify_api, sample_notification): headers=[auth_header]) notifications = json.loads(response.get_data(as_text=True)) - assert notifications['notifications'][0]['status'] == 'sending' + assert notifications['notifications'][0]['status'] == 'created' assert notifications['notifications'][0]['template'] == { 'id': str(sample_notification.template.id), 'name': sample_notification.template.name, @@ -459,7 +459,8 @@ def test_filter_by_multiple_statuss(notify_api, notify_db, notify_db_session, service=sample_email_template.service, - template=sample_email_template) + template=sample_email_template, + status='sending') auth_header = create_authorization_header(service_id=sample_email_template.service_id) @@ -645,7 +646,7 @@ def test_firetext_callback_should_update_notification_status(notify_api, sample_ with notify_api.test_client() as client: mocker.patch('app.statsd_client.incr') original = get_notification_by_id(sample_notification.id) - assert original.status == 'sending' + assert original.status == 'created' response = client.post( path='/notifications/sms/firetext', @@ -674,7 +675,7 @@ def test_firetext_callback_should_update_notification_status_failed(notify_api, with notify_api.test_client() as client: mocker.patch('app.statsd_client.incr') original = get_notification_by_id(sample_notification.id) - assert original.status == 'sending' + assert original.status == 'created' response = client.post( path='/notifications/sms/firetext', @@ -966,7 +967,8 @@ def test_ses_callback_should_update_notification_status( notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' @@ -1056,7 +1058,8 @@ def test_ses_callback_should_update_record_statsd( notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' @@ -1079,7 +1082,8 @@ def test_ses_callback_should_set_status_to_temporary_failure(notify_api, notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' @@ -1138,7 +1142,8 @@ def test_ses_callback_should_set_status_to_permanent_failure(notify_api, notify_db, notify_db_session, template=sample_email_template, - reference='ref' + reference='ref', + status='sending' ) assert get_notification_by_id(notification.id).status == 'sending' diff --git a/tests/app/status/test_delivery_status.py b/tests/app/status/test_delivery_status.py index d8b3f6bb3..21f714860 100644 --- a/tests/app/status/test_delivery_status.py +++ b/tests/app/status/test_delivery_status.py @@ -5,6 +5,8 @@ from datetime import ( timedelta ) +from tests.app.conftest import sample_notification + def test_get_delivery_status_all_ok(notify_api, notify_db): with notify_api.test_request_context(): @@ -17,11 +19,12 @@ def test_get_delivery_status_all_ok(notify_api, notify_db): assert resp_json['message'] == '0 notifications in sending state over 5 minutes' -def test_get_delivery_status_with_undelivered_notification(notify_api, notify_db, sample_notification): +def test_get_delivery_status_with_undelivered_notification(notify_api, notify_db, notify_db_session): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, status='sending') more_than_five_mins_ago = datetime.utcnow() - timedelta(minutes=10) - sample_notification.created_at = more_than_five_mins_ago - notify_db.session.add(sample_notification) + notification.created_at = more_than_five_mins_ago + notify_db.session.add(notification) notify_db.session.commit() with notify_api.test_request_context():