From 2896ed27f26b648b1c7e8d8f17a5cf0037ad619a Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 17 Jan 2017 15:38:54 +0000 Subject: [PATCH 1/6] Add dao methods to retrieve providers, switch, toggle along with tests --- app/dao/provider_details_dao.py | 82 ++++++++++++++++++++- tests/app/conftest.py | 34 +++++++-- tests/app/dao/test_provider_details_dao.py | 84 ++++++++++++++++++++-- 3 files changed, 190 insertions(+), 10 deletions(-) diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index 9d559ee0b..ed04f6754 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -1,19 +1,99 @@ from datetime import datetime +from flask import current_app from sqlalchemy import asc + from app.dao.dao_utils import transactional from app.models import ProviderDetails, ProviderDetailsHistory from app import db def get_provider_details(): - return ProviderDetails.query.order_by(asc(ProviderDetails.priority), asc(ProviderDetails.notification_type)).all() + providers = ProviderDetails.query.order_by(asc(ProviderDetails.priority), asc(ProviderDetails.notification_type)).all() + return providers 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(): + current_provider = get_current_provider('sms') + alternate_provider = get_alternative_sms_provider(current_provider.identifier) + + dao_switch_sms_provider_to_provider_with_identifier(alternate_provider.identifier) + + +@transactional +def dao_switch_sms_provider_to_provider_with_identifier(identifier): + # Do nothing if the provider is already primary or set as inactive + current_provider = get_current_provider('sms') + if current_provider.identifier == identifier: + current_app.logger.warning('Provider {} is already activated'.format(current_provider.display_name)) + return current_provider + + new_provider = get_provider_details_by_identifier(identifier) + if 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 current_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 + _print_provider_switch_logs(current_provider, new_provider) + db.session.add_all([current_provider, new_provider]) + + # 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) + db.session.add(current_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 + )) + + def get_provider_details_by_notification_type(notification_type): return ProviderDetails.query.filter_by( notification_type=notification_type diff --git a/tests/app/conftest.py b/tests/app/conftest.py index ea702ecd4..876d4519b 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 @@ -628,6 +633,27 @@ 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 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) + + @pytest.fixture(scope='function') def ses_provider(): return ProviderDetails.query.filter_by(identifier='ses').one() @@ -635,7 +661,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..09f34a7c2 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,10 +7,15 @@ 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_notification_type, + dao_switch_sms_provider_to_provider_with_identifier, + dao_toggle_sms_provider, dao_update_provider_details ) +from tests.app.conftest import set_primary_sms_provider def test_can_get_all_providers(restore_provider_details): @@ -21,15 +28,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 +83,72 @@ 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( + notify_db, + notify_db_session, + 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( + notify_db, + notify_db_session, + 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( + notify_db, + notify_db_session, + restore_provider_details, + current_sms_provider, + mocker +): + dao_toggle_sms_provider() + new_provider = get_current_provider('sms') + + assert new_provider.identifier != current_sms_provider.identifier From c2211186698db213034dedb4415877e634eaf401 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 17 Jan 2017 15:46:02 +0000 Subject: [PATCH 2/6] Auto switch providers if exception is returned on sms delivery --- app/celery/provider_tasks.py | 2 ++ tests/app/celery/test_provider_tasks.py | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index b0df268b3..48f5ea2fb 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -5,6 +5,7 @@ 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 @@ -43,6 +44,7 @@ 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/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 7271e9a4c..c9f5c29d9 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -121,3 +121,13 @@ 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): + 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') + mocker.patch('app.celery.provider_tasks.deliver_sms.retry') + + deliver_sms(sample_notification.service_id) + + assert switch_provider_mock.called is True From 6a99b89d2ea55269743d869da7232a6d5f1bc918 Mon Sep 17 00:00:00 2001 From: imdadahad Date: Tue, 17 Jan 2017 17:01:47 +0000 Subject: [PATCH 3/6] Fix long line --- app/dao/provider_details_dao.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index ed04f6754..52938057e 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -9,8 +9,7 @@ from app import db def get_provider_details(): - providers = ProviderDetails.query.order_by(asc(ProviderDetails.priority), asc(ProviderDetails.notification_type)).all() - return providers + return ProviderDetails.query.order_by(asc(ProviderDetails.priority), asc(ProviderDetails.notification_type)).all() def get_provider_details_by_id(provider_details_id): From d466265a9dcbfcd7c5e403485ab517f6cf7a4868 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 20 Jan 2017 16:13:13 +0000 Subject: [PATCH 4/6] Refactor provider switch methods in dao and tests --- app/dao/provider_details_dao.py | 52 +++++----------------- app/provider_details/switch_providers.py | 48 ++++++++++++++++++++ tests/app/conftest.py | 12 ----- tests/app/dao/test_provider_details_dao.py | 15 +++++-- 4 files changed, 70 insertions(+), 57 deletions(-) create mode 100644 app/provider_details/switch_providers.py diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index 52938057e..b4a017483 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -1,9 +1,12 @@ from datetime import datetime -from flask import current_app 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 @@ -42,55 +45,20 @@ def get_current_provider(notification_type): def dao_toggle_sms_provider(): current_provider = get_current_provider('sms') alternate_provider = get_alternative_sms_provider(current_provider.identifier) - dao_switch_sms_provider_to_provider_with_identifier(alternate_provider.identifier) @transactional def dao_switch_sms_provider_to_provider_with_identifier(identifier): - # Do nothing if the provider is already primary or set as inactive current_provider = get_current_provider('sms') - if current_provider.identifier == identifier: - current_app.logger.warning('Provider {} is already activated'.format(current_provider.display_name)) - return current_provider - new_provider = get_provider_details_by_identifier(identifier) - if 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 - )) + + if provider_is_already_primary_or_inactive(current_provider, new_provider, identifier): return current_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 - _print_provider_switch_logs(current_provider, new_provider) - db.session.add_all([current_provider, new_provider]) - - # 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) - db.session.add(current_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 - )) + else: + updated_providers = update_provider_priorities(current_provider, new_provider) + for provider in updated_providers: + db.session.add(provider) def get_provider_details_by_notification_type(notification_type): diff --git a/app/provider_details/switch_providers.py b/app/provider_details/switch_providers.py new file mode 100644 index 000000000..ebb2e7d23 --- /dev/null +++ b/app/provider_details/switch_providers.py @@ -0,0 +1,48 @@ +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 + _print_provider_switch_logs(current_provider, new_provider) + return [current_provider, new_provider] + + # 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) + return [current_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/conftest.py b/tests/app/conftest.py index 876d4519b..1d62aa996 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -642,18 +642,6 @@ def current_sms_provider(): ).first() -@pytest.fixture(scope='function') -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) - - @pytest.fixture(scope='function') def ses_provider(): return ProviderDetails.query.filter_by(identifier='ses').one() diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 09f34a7c2..db932f7f9 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -10,12 +10,23 @@ 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 ) -from tests.app.conftest import set_primary_sms_provider + + +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): @@ -110,8 +121,6 @@ def test_get_alternative_sms_provider_returns_expected_provider(notify_db, provi def test_switch_sms_provider_to_current_provider_does_not_switch( - notify_db, - notify_db_session, restore_provider_details, current_sms_provider, mocker From 0a277b26b6f0c695c2b570c813b345ca44d74a6d Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 20 Jan 2017 16:14:29 +0000 Subject: [PATCH 5/6] 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 From e1d176934509f61f66110f5187cb1731b6ba1612 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 23 Jan 2017 13:36:04 +0000 Subject: [PATCH 6/6] Refactor to switch properly when providers have equal priority with test --- app/dao/provider_details_dao.py | 19 ++++++++++--------- app/delivery/send_to_providers.py | 2 +- app/provider_details/switch_providers.py | 6 ++---- tests/app/dao/test_provider_details_dao.py | 21 ++++++++++++++++----- 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index b4a017483..1ee2a99ae 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -42,9 +42,8 @@ def get_current_provider(notification_type): @transactional -def dao_toggle_sms_provider(): - current_provider = get_current_provider('sms') - alternate_provider = get_alternative_sms_provider(current_provider.identifier) +def dao_toggle_sms_provider(identifier): + alternate_provider = get_alternative_sms_provider(identifier) dao_switch_sms_provider_to_provider_with_identifier(alternate_provider.identifier) @@ -53,12 +52,14 @@ 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 provider_is_already_primary_or_inactive(current_provider, new_provider, identifier): - return current_provider - else: - updated_providers = update_provider_priorities(current_provider, new_provider) - for provider in updated_providers: - db.session.add(provider) + 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): diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 526cd5b13..e74cc174d 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -47,7 +47,7 @@ def send_sms_to_provider(notification): sender=service.sms_sender ) except Exception as e: - dao_toggle_sms_provider() + dao_toggle_sms_provider(provider.name) raise e else: notification.billable_units = template.fragment_count diff --git a/app/provider_details/switch_providers.py b/app/provider_details/switch_providers.py index ebb2e7d23..6d2a992cb 100644 --- a/app/provider_details/switch_providers.py +++ b/app/provider_details/switch_providers.py @@ -21,14 +21,12 @@ 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 - _print_provider_switch_logs(current_provider, new_provider) - return [current_provider, new_provider] # 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) - return [current_provider] + + _print_provider_switch_logs(current_provider, new_provider) def _print_provider_switch_logs(current_provider, new_provider): diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index db932f7f9..9050248c7 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -133,8 +133,6 @@ def test_switch_sms_provider_to_current_provider_does_not_switch( def test_switch_sms_provider_to_inactive_provider_does_not_switch( - notify_db, - notify_db_session, restore_provider_details, current_sms_provider, mocker @@ -151,13 +149,26 @@ def test_switch_sms_provider_to_inactive_provider_does_not_switch( def test_toggle_sms_provider_switches_provider( - notify_db, - notify_db_session, restore_provider_details, current_sms_provider, mocker ): - dao_toggle_sms_provider() + 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