Merge pull request #474 from alphagov/notification-created-status

Notification created status

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(
+            if service.research_mode:
-                "SMS notification {} failed".format(notification_id)
+                send_sms_response.apply_async(
-            )
+                    (provider.get_name(), str(notification_id), notification.to), queue='research-mode'
-            current_app.logger.exception(e)
+                )
-            self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries))
+            else:
-        except self.MaxRetriesExceededError:
+                provider.send_sms(
-            update_notification_status_by_id(notification.id, 'technical-failure', 'failure')
+                    to=validate_and_format_phone_number(notification.to),
+                    content=template.replaced,
+                    reference=str(notification_id)
+                )
 
-    current_app.logger.info(
+                update_provider_stats(
-        "SMS {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at)
+                    notification_id,
-    )
+                    'sms',
-    statsd_client.incr("notifications.tasks.send-sms-to-provider")
+                    provider.get_name(),
-    statsd_client.timing("notifications.tasks.send-sms-to-provider.task-time", monotonic() - task_start)
+                    content_char_count=template.replaced_content_count
-    statsd_client.timing("notifications.sms.total-time", datetime.utcnow() - notification.created_at)
+                )
+
+            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):

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)

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.dao import provider_statistics_dao
-from app.celery.research_mode_tasks import send_sms_response
 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')

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,
+         notification_id),
-         encryption.encrypt(notification)),
         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,
+         notification_id),
-         encrypt_notification),
         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,
+         notification_id),
-         encryption.encrypt(notification)),
         queue="sms"
     )
     persisted_notification = Notification.query.filter_by(id=notification_id).one()

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,

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)

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'

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
+    notification.created_at = more_than_five_mins_ago
-    notify_db.session.add(sample_notification)
+    notify_db.session.add(notification)
     notify_db.session.commit()
 
     with notify_api.test_request_context():