diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index 9d559ee0b..1ee2a99ae 100644 --- a/app/dao/provider_details_dao.py +++ b/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 diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index cd401f4f1..e74cc174d 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(provider.name) + raise e + else: + notification.billable_units = template.fragment_count notification.sent_at = datetime.utcnow() notification.sent_by = provider.get_name() diff --git a/app/provider_details/switch_providers.py b/app/provider_details/switch_providers.py new file mode 100644 index 000000000..6d2a992cb --- /dev/null +++ b/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 + )) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 7271e9a4c..6a60d3964 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/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 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 9780ec38d..559537744 100644 --- a/tests/app/conftest.py +++ b/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') diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 1bb2d8c09..9050248c7 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/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