Merge pull request #795 from alphagov/switch-providers-on-error

Auto-switch providers on error

app/dao/provider_details_dao.py

@@ -1,7 +1,12 @@
 from datetime import datetime
 
 from sqlalchemy import asc
+
 from app.dao.dao_utils import transactional
+from app.provider_details.switch_providers import (
+    provider_is_already_primary_or_inactive,
+    update_provider_priorities
+)
 from app.models import ProviderDetails, ProviderDetailsHistory
 from app import db
 
@@ -14,6 +19,49 @@ def get_provider_details_by_id(provider_details_id):
     return ProviderDetails.query.get(provider_details_id)
 
 
+def get_provider_details_by_identifier(identifier):
+    return ProviderDetails.query.filter_by(identifier=identifier).one()
+
+
+def get_alternative_sms_provider(identifier):
+    alternate_provider = None
+    if identifier == 'firetext':
+        alternate_provider = 'mmg'
+    elif identifier == 'mmg':
+        alternate_provider = 'firetext'
+
+    return ProviderDetails.query.filter_by(identifier=alternate_provider).one()
+
+
+def get_current_provider(notification_type):
+    return ProviderDetails.query.filter_by(
+        notification_type=notification_type
+    ).order_by(
+        asc(ProviderDetails.priority)
+    ).first()
+
+
+@transactional
+def dao_toggle_sms_provider(identifier):
+    alternate_provider = get_alternative_sms_provider(identifier)
+    dao_switch_sms_provider_to_provider_with_identifier(alternate_provider.identifier)
+
+
+@transactional
+def dao_switch_sms_provider_to_provider_with_identifier(identifier):
+    current_provider = get_current_provider('sms')
+    new_provider = get_provider_details_by_identifier(identifier)
+
+    if current_provider.priority == new_provider.priority:
+        # Since both priorities are equal, set the current provider
+        # to the one that we want to switch from
+        current_provider = get_alternative_sms_provider(identifier)
+
+    if not provider_is_already_primary_or_inactive(current_provider, new_provider, identifier):
+        update_provider_priorities(current_provider, new_provider)
+        db.session.add_all([current_provider, new_provider])
+
+
 def get_provider_details_by_notification_type(notification_type):
     return ProviderDetails.query.filter_by(
         notification_type=notification_type

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,12 +39,17 @@ def send_sms_to_provider(notification):
             )
             notification.billable_units = 0
         else:
+            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(provider.name)
+                raise e
+            else:
                 notification.billable_units = template.fragment_count
 
         notification.sent_at = datetime.utcnow()

app/provider_details/switch_providers.py

@@ -0,0 +1,46 @@
+from flask import current_app
+
+
+def provider_is_already_primary_or_inactive(current_provider, new_provider, identifier):
+    if current_provider.identifier == identifier:
+        current_app.logger.warning('Provider {} is already activated'.format(current_provider.display_name))
+        return True
+
+    elif not new_provider.active:
+        current_app.logger.warning('Cancelling switch from {} to {} as {} is inactive'.format(
+            current_provider.identifier,
+            new_provider.identifier,
+            new_provider.identifier
+        ))
+        return True
+
+    return False
+
+
+def update_provider_priorities(current_provider, new_provider):
+    # Swap priority to change primary provider
+    if new_provider.priority > current_provider.priority:
+        new_provider.priority, current_provider.priority = current_provider.priority, new_provider.priority
+
+    # Incease other provider priority if equal
+    elif new_provider.priority == current_provider.priority:
+        current_provider.priority += 10
+
+    _print_provider_switch_logs(current_provider, new_provider)
+
+
+def _print_provider_switch_logs(current_provider, new_provider):
+    current_app.logger.warning('Switching provider from {} to {}'.format(
+        current_provider.identifier,
+        new_provider.identifier
+    ))
+
+    current_app.logger.warning('Provider {} now updated with priority of {}'.format(
+        current_provider.identifier,
+        current_provider.priority
+    ))
+
+    current_app.logger.warning('Provider {} now updated with priority of {}'.format(
+        new_provider.identifier,
+        new_provider.priority
+    ))

tests/app/celery/test_provider_tasks.py

@@ -121,3 +121,30 @@ def test_should_technical_error_and_not_retry_if_invalid_email(sample_notificati
 
     assert provider_tasks.deliver_email.retry.called is False
     assert sample_notification.status == 'technical-failure'
+
+
+def test_send_sms_should_switch_providers_on_provider_failure(sample_notification, mocker):
+    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.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

tests/app/conftest.py

@@ -1,9 +1,10 @@
+import requests_mock
+import pytest
 import uuid
 from datetime import (datetime, date, timedelta)
 
+from sqlalchemy import asc
 from sqlalchemy.orm.session import make_transient
-import requests_mock
-import pytest
 from flask import current_app
 
 from app import db
@@ -34,7 +35,11 @@ from app.dao.notifications_dao import dao_create_notification
 from app.dao.invited_user_dao import save_invited_user
 from app.dao.provider_rates_dao import create_provider_rates
 from app.clients.sms.firetext import FiretextClient
-
+from app.dao.provider_details_dao import (
+    dao_update_provider_details,
+    get_provider_details_by_identifier,
+    get_alternative_sms_provider
+)
 from tests.app.db import create_user
 
 
@@ -647,6 +652,15 @@ def fake_uuid():
     return "6ce466d0-fd6a-11e5-82f5-e0accb9d11a6"
 
 
+@pytest.fixture(scope='function')
+def current_sms_provider():
+    return ProviderDetails.query.filter_by(
+        notification_type='sms'
+    ).order_by(
+        asc(ProviderDetails.priority)
+    ).first()
+
+
 @pytest.fixture(scope='function')
 def ses_provider():
     return ProviderDetails.query.filter_by(identifier='ses').one()
@@ -654,7 +668,7 @@ def ses_provider():
 
 @pytest.fixture(scope='function')
 def firetext_provider():
-    return ProviderDetails.query.filter_by(identifier='mmg').one()
+    return ProviderDetails.query.filter_by(identifier='firetext').one()
 
 
 @pytest.fixture(scope='function')

tests/app/dao/test_provider_details_dao.py

@@ -1,3 +1,5 @@
+import pytest
+
 from datetime import datetime
 
 from freezegun import freeze_time
@@ -5,12 +7,28 @@ from freezegun import freeze_time
 from app.models import ProviderDetails, ProviderDetailsHistory
 from app import clients
 from app.dao.provider_details_dao import (
+    get_alternative_sms_provider,
+    get_current_provider,
     get_provider_details,
+    get_provider_details_by_identifier,
     get_provider_details_by_notification_type,
+    dao_switch_sms_provider_to_provider_with_identifier,
+    dao_toggle_sms_provider,
     dao_update_provider_details
 )
 
 
+def set_primary_sms_provider(identifier='mmg'):
+    primary_provider = get_provider_details_by_identifier(identifier)
+    secondary_provider = get_alternative_sms_provider(identifier)
+
+    primary_provider.priority = 10
+    secondary_provider.priority = 20
+
+    dao_update_provider_details(primary_provider)
+    dao_update_provider_details(secondary_provider)
+
+
 def test_can_get_all_providers(restore_provider_details):
     assert len(get_provider_details()) == 4
 
@@ -21,15 +39,13 @@ def test_can_get_sms_providers(restore_provider_details):
     assert all('sms' == prov.notification_type for prov in sms_providers)
 
 
-def test_can_get_sms_providers_in_order(restore_provider_details):
+def test_can_get_sms_providers_in_order_of_priority(restore_provider_details):
     providers = get_provider_details_by_notification_type('sms')
 
-    assert providers[0].identifier == "mmg"
-    assert providers[1].identifier == "firetext"
-    assert providers[2].identifier == "loadtesting"
+    assert providers[0].priority < providers[1].priority < providers[2].priority
 
 
-def test_can_get_email_providers_in_order(restore_provider_details):
+def test_can_get_email_providers_in_order_of_priority(restore_provider_details):
     providers = get_provider_details_by_notification_type('email')
 
     assert providers[0].identifier == "ses"
@@ -78,3 +94,81 @@ def test_update_adds_history(restore_provider_details):
     assert not ses_history[1].active
     assert ses_history[1].version == 2
     assert ses_history[1].updated_at == datetime(2000, 1, 1, 0, 0, 0)
+
+
+def test_update_sms_provider_to_inactive_sets_inactive(restore_provider_details):
+    set_primary_sms_provider('mmg')
+    primary_provider = get_current_provider('sms')
+    primary_provider.active = False
+
+    dao_update_provider_details(primary_provider)
+
+    assert not primary_provider.active
+
+
+def test_get_current_sms_provider_returns_correct_provider(restore_provider_details):
+    set_primary_sms_provider('mmg')
+
+    provider = get_current_provider('sms')
+
+    assert provider.identifier == 'mmg'
+
+
+@pytest.mark.parametrize('provider_identifier', ['firetext', 'mmg'])
+def test_get_alternative_sms_provider_returns_expected_provider(notify_db, provider_identifier):
+    provider = get_alternative_sms_provider(provider_identifier)
+    assert provider.identifier != provider
+
+
+def test_switch_sms_provider_to_current_provider_does_not_switch(
+    restore_provider_details,
+    current_sms_provider,
+    mocker
+):
+    dao_switch_sms_provider_to_provider_with_identifier(current_sms_provider.identifier)
+    new_provider = get_current_provider('sms')
+
+    assert current_sms_provider.id == new_provider.id
+    assert current_sms_provider.identifier == new_provider.identifier
+
+
+def test_switch_sms_provider_to_inactive_provider_does_not_switch(
+    restore_provider_details,
+    current_sms_provider,
+    mocker
+):
+    alternative_sms_provider = get_alternative_sms_provider(current_sms_provider.identifier)
+    alternative_sms_provider.active = False
+    dao_update_provider_details(alternative_sms_provider)
+
+    dao_switch_sms_provider_to_provider_with_identifier(alternative_sms_provider.identifier)
+    new_provider = get_current_provider('sms')
+
+    assert new_provider.id == current_sms_provider.id
+    assert new_provider.identifier == current_sms_provider.identifier
+
+
+def test_toggle_sms_provider_switches_provider(
+    restore_provider_details,
+    current_sms_provider,
+    mocker
+):
+    dao_toggle_sms_provider(current_sms_provider.identifier)
+    new_provider = get_current_provider('sms')
+    assert new_provider.identifier != current_sms_provider.identifier
+    assert new_provider.priority < current_sms_provider.priority
+
+
+def test_toggle_sms_provider_switches_when_provider_priorities_are_equal(
+    restore_provider_details,
+    current_sms_provider,
+    mocker
+):
+    new_provider = get_alternative_sms_provider(current_sms_provider.identifier)
+    current_sms_provider.priority = new_provider.priority
+    dao_update_provider_details(current_sms_provider)
+
+    dao_toggle_sms_provider(current_sms_provider.identifier)
+
+    assert new_provider.identifier != current_sms_provider.identifier
+    assert new_provider.priority < current_sms_provider.priority