From 2844fa530b97d557b5e4ef1d964ca33c71562ab4 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Thu, 23 May 2019 16:40:17 +0100
Subject: [PATCH] Fix a bug introduced when refactoring some code. The
 notification status happened in the wrong order - this resolved that. This
 meant notifications sent with a test key never got a 'delivered' status.

---
 app/delivery/send_to_providers.py            |  2 +-
 tests/app/delivery/test_send_to_providers.py | 37 ++++++++++++--------
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py
index b4475623d..2da0feaa0 100644
--- a/app/delivery/send_to_providers.py
+++ b/app/delivery/send_to_providers.py
@@ -51,8 +51,8 @@ def send_sms_to_provider(notification):
         )
 
         if service.research_mode or notification.key_type == KEY_TYPE_TEST:
-            send_sms_response(provider.get_name(), str(notification.id), notification.to)
             update_notification_to_sending(notification, provider)
+            send_sms_response(provider.get_name(), str(notification.id), notification.to)
 
         else:
             try:
diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py
index 2f331896e..d6cc6346a 100644
--- a/tests/app/delivery/test_send_to_providers.py
+++ b/tests/app/delivery/test_send_to_providers.py
@@ -229,7 +229,7 @@ def test_should_call_send_sms_response_task_if_research_mode(
     assert not persisted_notification.personalisation
 
 
-def test_should_leave_as_created_if_fake_callback_function_fails(sample_notification, mocker):
+def test_should_have_sending_status_if_fake_callback_function_fails(sample_notification, mocker):
     mocker.patch('app.delivery.send_to_providers.send_sms_response', side_effect=HTTPError)
 
     sample_notification.key_type = KEY_TYPE_TEST
@@ -238,9 +238,8 @@ def test_should_leave_as_created_if_fake_callback_function_fails(sample_notifica
         send_to_providers.send_sms_to_provider(
             sample_notification
         )
-    assert sample_notification.status == 'created'
-    assert sample_notification.sent_at is None
-    assert sample_notification.sent_by is None
+    assert sample_notification.status == 'sending'
+    assert sample_notification.sent_by == 'mmg'
 
 
 def test_should_not_send_to_provider_when_status_is_not_created(
@@ -507,27 +506,34 @@ def test_should_not_update_notification_if_research_mode_on_exception(
 
     persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id)
     assert persisted_notification.billable_units == 0
-    assert not update_mock.called
+    assert update_mock.called
 
 
-@pytest.mark.parametrize('research_mode,key_type, billable_units', [
-    (True, KEY_TYPE_NORMAL, 0),
-    (False, KEY_TYPE_NORMAL, 1),
-    (False, KEY_TYPE_TEST, 0),
-    (True, KEY_TYPE_TEST, 0),
-    (True, KEY_TYPE_TEAM, 0),
-    (False, KEY_TYPE_TEAM, 1)
+def __update_notification(notification_to_update, research_mode, expected_status):
+    if research_mode or notification_to_update.key_type == KEY_TYPE_TEST:
+        notification_to_update.status = expected_status
+
+
+@pytest.mark.parametrize('research_mode,key_type, billable_units, expected_status', [
+    (True, KEY_TYPE_NORMAL, 0, 'delivered'),
+    (False, KEY_TYPE_NORMAL, 1, 'sending'),
+    (False, KEY_TYPE_TEST, 0, 'sending'),
+    (True, KEY_TYPE_TEST, 0, 'sending'),
+    (True, KEY_TYPE_TEAM, 0, 'delivered'),
+    (False, KEY_TYPE_TEAM, 1, 'sending')
 ])
-def test_should_update_billable_units_according_to_research_mode_and_key_type(
+def test_should_update_billable_units_and_status_according_to_research_mode_and_key_type(
     sample_template,
     mocker,
     research_mode,
     key_type,
-    billable_units
+    billable_units,
+    expected_status
 ):
     notification = create_notification(template=sample_template, billable_units=0, status='created', key_type=key_type)
     mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.delivery.send_to_providers.send_sms_response')
+    mocker.patch('app.delivery.send_to_providers.send_sms_response',
+                 side_effect=__update_notification(notification, research_mode, expected_status))
 
     if research_mode:
         sample_template.service.research_mode = True
@@ -536,6 +542,7 @@ def test_should_update_billable_units_according_to_research_mode_and_key_type(
         notification
     )
     assert notification.billable_units == billable_units
+    assert notification.status == expected_status
 
 
 def test_should_set_notification_billable_units_if_sending_to_provider_fails(