Refactor send_to_providers.

- Remove some redundant code for research mode.
- The international parameter in update_notification_to_sending is not needed.
- Update unit tests and removed duplicates

app/delivery/send_to_providers.py

@@ -55,18 +55,9 @@ def send_sms_to_provider(notification):
         )
 
         if service.research_mode or notification.key_type == KEY_TYPE_TEST:
-            notification.billable_units = 0
-            update_notification_to_sending(notification, provider)
-            try:
             send_sms_response(provider.get_name(), str(notification.id), notification.to)
-            except HTTPError:
+            update_notification_to_sending(notification, provider)
-                # when we retry, we only do anything if the notification is in created - it's currently in sending,
+
-                # so set it back so that we actually attempt the callback again
-                notification.sent_at = None
-                notification.sent_by = None
-                notification.status = NOTIFICATION_CREATED
-                dao_update_notification(notification)
-                raise
         else:
             try:
                 provider.send_sms(
@@ -83,7 +74,7 @@ def send_sms_to_provider(notification):
                 raise e
             else:
                 notification.billable_units = template.fragment_count
-                update_notification_to_sending(notification, provider, notification.international)
+                update_notification_to_sending(notification, provider)
 
         current_app.logger.debug(
             "SMS {} sent to provider {} at {}".format(notification.id, provider.get_name(), notification.sent_at)
@@ -145,13 +136,10 @@ def send_email_to_provider(notification):
         statsd_client.timing("email.total-time", delta_milliseconds)
 
 
-def update_notification_to_sending(notification, provider, international=False):
+def update_notification_to_sending(notification, provider):
     notification.sent_at = datetime.utcnow()
     notification.sent_by = provider.get_name()
-    if international:
+    notification.status = NOTIFICATION_SENT if notification.international else NOTIFICATION_SENDING
-        notification.status = NOTIFICATION_SENT
-    else:
-        notification.status = NOTIFICATION_SENDING
     dao_update_notification(notification)
 
 

tests/app/delivery/test_send_to_providers.py

@@ -243,28 +243,6 @@ def test_should_leave_as_created_if_fake_callback_function_fails(sample_notifica
     assert sample_notification.sent_by is None
 
 
-@pytest.mark.parametrize('research_mode,key_type', [
-    (True, KEY_TYPE_NORMAL),
-    (False, KEY_TYPE_TEST)
-])
-def test_should_set_billable_units_to_zero_in_research_mode_or_test_key(
-        notify_db, sample_service, sample_notification, mocker, research_mode, key_type):
-
-    mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.delivery.send_to_providers.send_sms_response')
-
-    if research_mode:
-        sample_service.research_mode = True
-        notify_db.session.add(sample_service)
-        notify_db.session.commit()
-    sample_notification.key_type = key_type
-
-    send_to_providers.send_sms_to_provider(
-        sample_notification
-    )
-    assert notifications_dao.get_notification_by_id(sample_notification.id).billable_units == 0
-
-
 def test_should_not_send_to_provider_when_status_is_not_created(
     sample_template,
     mocker
@@ -514,19 +492,22 @@ def test_get_logo_url_works_for_different_environments(base_url, expected_url):
     assert logo_url == expected_url
 
 
-def test_should_not_set_billable_units_if_research_mode(notify_db, sample_service, sample_notification, mocker):
+def test_should_not_update_notification_if_research_mode_on_exception(
-    mocker.patch('app.mmg_client.send_sms')
+        sample_service, sample_notification, mocker
-    mocker.patch('app.delivery.send_to_providers.send_sms_response')
+):
-
+    mocker.patch('app.delivery.send_to_providers.send_sms_response', side_effect=Exception())
+    update_mock = mocker.patch('app.delivery.send_to_providers.update_notification_to_sending')
     sample_service.research_mode = True
-    notify_db.session.add(sample_service)
+    sample_notification.billable_units = 0
-    notify_db.session.commit()
 
+    with pytest.raises(Exception):
         send_to_providers.send_sms_to_provider(
             sample_notification
         )
+
     persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id)
     assert persisted_notification.billable_units == 0
+    assert not update_mock.called
 
 
 @pytest.mark.parametrize('research_mode,key_type, billable_units', [
@@ -538,33 +519,26 @@ def test_should_not_set_billable_units_if_research_mode(notify_db, sample_servic
     (False, KEY_TYPE_TEAM, 1)
 ])
 def test_should_update_billable_units_according_to_research_mode_and_key_type(
-    notify_db,
+    sample_template,
-    sample_service,
-    sample_notification,
     mocker,
     research_mode,
     key_type,
     billable_units
 ):
+    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')
 
     if research_mode:
-        sample_service.research_mode = True
+        sample_template.service.research_mode = True
-        notify_db.session.add(sample_service)
-        notify_db.session.commit()
-
-    sample_notification.key_type = key_type
 
     send_to_providers.send_sms_to_provider(
-        sample_notification
+        notification
     )
-    assert sample_notification.billable_units == billable_units
+    assert notification.billable_units == billable_units
 
 
 def test_should_set_notification_billable_units_and_provider_if_sending_to_provider_fails(
-    notify_db,
-    sample_service,
     sample_notification,
     mocker,
 ):
@@ -603,7 +577,7 @@ def test_should_send_sms_to_international_providers(
 
     db_notification_international = create_notification(
         template=sample_sms_template_with_html,
-        to_field="+447234123111",
+        to_field="+6011-17224412",
         personalisation={"name": "Jo"},
         status='created',
         international=True,
@@ -629,7 +603,7 @@ def test_should_send_sms_to_international_providers(
     )
 
     mmg_client.send_sms.assert_called_once_with(
-        to="447234123111",
+        to="601117224412",
         content=ANY,
         reference=str(db_notification_international.id),
         sender=current_app.config['FROM_NUMBER']
@@ -644,48 +618,6 @@ def test_should_send_sms_to_international_providers(
     assert notification_int.sent_by == 'mmg'
 
 
-def test_should_send_international_sms_with_formatted_phone_number(
-    notify_db,
-    sample_template,
-    mocker
-):
-    notification = create_notification(
-        template=sample_template,
-        to_field="+6011-17224412",
-        international=True
-    )
-
-    send_notification_mock = mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.delivery.send_to_providers.send_sms_response')
-
-    send_to_providers.send_sms_to_provider(
-        notification
-    )
-
-    assert send_notification_mock.called is True
-
-
-def test_should_set_international_phone_number_to_sent_status(
-    notify_db,
-    sample_template,
-    mocker
-):
-    notification = create_notification(
-        template=sample_template,
-        to_field="+6011-17224412",
-        international=True
-    )
-
-    mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.delivery.send_to_providers.send_sms_response')
-
-    send_to_providers.send_sms_to_provider(
-        notification
-    )
-
-    assert notification.status == 'sent'
-
-
 @pytest.mark.parametrize('sms_sender, expected_sender, prefix_sms, expected_content', [
     ('foo', 'foo', False, 'bar'),
     ('foo', 'foo', True, 'Sample service: bar'),