From 0a277b26b6f0c695c2b570c813b345ca44d74a6d Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Fri, 20 Jan 2017 16:14:29 +0000
Subject: [PATCH] Switch providers ONLY on provider exception

---
 app/celery/provider_tasks.py            |  2 --
 app/delivery/send_to_providers.py       | 25 ++++++++++++++++---------
 tests/app/celery/test_provider_tasks.py | 23 ++++++++++++++++++++---
 3 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py
index 48f5ea2fb..b0df268b3 100644
--- a/app/celery/provider_tasks.py
+++ b/app/celery/provider_tasks.py
@@ -5,7 +5,6 @@ from sqlalchemy.orm.exc import NoResultFound
 from app import notify_celery
 from app.dao import notifications_dao
 from app.dao.notifications_dao import update_notification_status_by_id
-from app.dao.provider_details_dao import dao_toggle_sms_provider
 from app.statsd_decorators import statsd
 from app.delivery import send_to_providers
 
@@ -44,7 +43,6 @@ def deliver_sms(self, notification_id):
         send_to_providers.send_sms_to_provider(notification)
     except Exception as e:
         try:
-            dao_toggle_sms_provider()
             current_app.logger.exception(
                 "RETRY: SMS notification {} failed".format(notification_id)
             )
diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py
index cd401f4f1..526cd5b13 100644
--- a/app/delivery/send_to_providers.py
+++ b/app/delivery/send_to_providers.py
@@ -9,11 +9,13 @@ from notifications_utils.template import HTMLEmailTemplate, PlainTextEmailTempla
 
 from app import clients, statsd_client, create_uuid
 from app.dao.notifications_dao import dao_update_notification
-from app.dao.provider_details_dao import get_provider_details_by_notification_type
+from app.dao.provider_details_dao import (
+    get_provider_details_by_notification_type,
+    dao_toggle_sms_provider
+)
 from app.dao.services_dao import dao_fetch_service_by_id
 from app.celery.research_mode_tasks import send_sms_response, send_email_response
 from app.dao.templates_dao import dao_get_template_by_id
-
 from app.models import SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, EMAIL_TYPE
 
 
@@ -37,13 +39,18 @@ def send_sms_to_provider(notification):
             )
             notification.billable_units = 0
         else:
-            provider.send_sms(
-                to=validate_and_format_phone_number(notification.to),
-                content=str(template),
-                reference=str(notification.id),
-                sender=service.sms_sender
-            )
-            notification.billable_units = template.fragment_count
+            try:
+                provider.send_sms(
+                    to=validate_and_format_phone_number(notification.to),
+                    content=str(template),
+                    reference=str(notification.id),
+                    sender=service.sms_sender
+                )
+            except Exception as e:
+                dao_toggle_sms_provider()
+                raise e
+            else:
+                notification.billable_units = template.fragment_count
 
         notification.sent_at = datetime.utcnow()
         notification.sent_by = provider.get_name()
diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py
index c9f5c29d9..6a60d3964 100644
--- a/tests/app/celery/test_provider_tasks.py
+++ b/tests/app/celery/test_provider_tasks.py
@@ -124,10 +124,27 @@ def test_should_technical_error_and_not_retry_if_invalid_email(sample_notificati
 
 
 def test_send_sms_should_switch_providers_on_provider_failure(sample_notification, mocker):
-    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception("Provider Exception"))
-    switch_provider_mock = mocker.patch('app.celery.provider_tasks.dao_toggle_sms_provider')
+    provider_to_use = mocker.patch('app.delivery.send_to_providers.provider_to_use')
+    provider_to_use.return_value.send_sms.side_effect = Exception('Error')
+    switch_provider_mock = mocker.patch('app.delivery.send_to_providers.dao_toggle_sms_provider')
     mocker.patch('app.celery.provider_tasks.deliver_sms.retry')
 
-    deliver_sms(sample_notification.service_id)
+    deliver_sms(sample_notification.id)
 
     assert switch_provider_mock.called is True
+
+
+def test_send_sms_should_not_switch_providers_on_non_provider_failure(
+    sample_notification,
+    mocker
+):
+    mocker.patch(
+        'app.delivery.send_to_providers.send_sms_to_provider',
+        side_effect=Exception("Non Provider Exception")
+    )
+    switch_provider_mock = mocker.patch('app.delivery.send_to_providers.dao_toggle_sms_provider')
+    mocker.patch('app.celery.provider_tasks.deliver_sms.retry')
+
+    deliver_sms(sample_notification.id)
+
+    assert switch_provider_mock.called is False