diff --git a/app/config.py b/app/config.py index 07c1c419e..5cb6cf2a4 100644 --- a/app/config.py +++ b/app/config.py @@ -81,6 +81,7 @@ class Config(object): MAX_VERIFY_CODE_COUNT = 10 NOTIFY_SERVICE_ID = 'd6aa2c68-a2d9-4437-ab19-3ae8eb202553' + NOTIFY_USER_ID = '6af522d0-2915-4e52-83a3-3690455a5fe6' INVITATION_EMAIL_TEMPLATE_ID = '4f46df42-f795-4cc4-83bb-65ca312f49cc' SMS_CODE_TEMPLATE_ID = '36fb0730-6259-4da1-8a80-c8de22ad4246' EMAIL_VERIFY_CODE_TEMPLATE_ID = 'ece42649-22a8-4d06-b87f-d52d5d3f0a27' diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index edbcef54a..2992a0107 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -1,11 +1,12 @@ from datetime import datetime -from sqlalchemy import asc +from sqlalchemy import asc, desc from app.dao.dao_utils import transactional from app.provider_details.switch_providers import ( - provider_is_already_primary_or_inactive, - update_provider_priorities + provider_is_inactive, + provider_is_primary, + switch_providers ) from app.models import ProviderDetails, ProviderDetailsHistory from app import db @@ -41,6 +42,14 @@ def get_current_provider(notification_type): ).first() +def dao_get_provider_versions(provider_id): + return ProviderDetailsHistory.query.filter_by( + id=provider_id + ).order_by( + desc(ProviderDetailsHistory.version) + ).all() + + @transactional def dao_toggle_sms_provider(identifier): alternate_provider = get_alternative_sms_provider(identifier) @@ -49,18 +58,24 @@ def dao_toggle_sms_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 provider_is_inactive(new_provider): + return - 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) + # Check first to see if there is another provider with the same priority + # as this needs to be updated differently + conflicting_provider = dao_get_sms_provider_with_equal_priority(new_provider.identifier, new_provider.priority) + providers_to_update = [] - if not provider_is_already_primary_or_inactive(current_provider, new_provider, identifier): - update_provider_priorities(current_provider, new_provider) - dao_update_provider_details(current_provider) - dao_update_provider_details(new_provider) + if conflicting_provider: + providers_to_update = switch_providers(conflicting_provider, new_provider) + else: + current_provider = get_current_provider('sms') + if not provider_is_primary(current_provider, new_provider, identifier): + providers_to_update = switch_providers(current_provider, new_provider) + + for provider in providers_to_update: + dao_update_provider_details(provider) def get_provider_details_by_notification_type(notification_type): @@ -76,3 +91,16 @@ def dao_update_provider_details(provider_details): history = ProviderDetailsHistory.from_original(provider_details) db.session.add(provider_details) db.session.add(history) + + +def dao_get_sms_provider_with_equal_priority(identifier, priority): + provider = db.session.query(ProviderDetails).filter( + ProviderDetails.identifier != identifier, + ProviderDetails.notification_type == 'sms', + ProviderDetails.priority == priority, + ProviderDetails.active + ).order_by( + asc(ProviderDetails.priority) + ).first() + + return provider diff --git a/app/provider_details/switch_providers.py b/app/provider_details/switch_providers.py index 6d2a992cb..4ed279de3 100644 --- a/app/provider_details/switch_providers.py +++ b/app/provider_details/switch_providers.py @@ -1,32 +1,39 @@ from flask import current_app +from app.dao.users_dao import get_user_by_id -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)) + +def provider_is_inactive(new_provider): + if not new_provider.active: + current_app.logger.warning('Cancelling switch to {} as they are inactive'.format( + new_provider.identifier, + )) 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 - )) + +def provider_is_primary(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 return False -def update_provider_priorities(current_provider, new_provider): +def switch_providers(current_provider, new_provider): + # Automatic update so set as notify user + notify_user = get_user_by_id(current_app.config['NOTIFY_USER_ID']) + current_provider.created_by_id = new_provider.created_by_id = notify_user.id + # 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 + # Increase 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, new_provider def _print_provider_switch_logs(current_provider, new_provider): diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 06ab343fa..bc7113c7f 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -316,8 +316,11 @@ def test_switch_providers_on_slow_delivery_runs_if_config_set( def test_switch_providers_triggers_on_slow_notification_delivery( notify_api, - prepare_current_provider + mocker, + prepare_current_provider, + sample_user ): + mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) starting_provider = get_current_provider('sms') with set_config_values(notify_api, { @@ -335,8 +338,11 @@ def test_switch_providers_triggers_on_slow_notification_delivery( def test_switch_providers_on_slow_delivery_does_not_switch_if_already_switched( notify_api, - prepare_current_provider + mocker, + prepare_current_provider, + sample_user ): + mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) starting_provider = get_current_provider('sms') with set_config_values(notify_api, { @@ -356,7 +362,10 @@ def test_switch_providers_on_slow_delivery_does_not_switch_if_already_switched( def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifications( notify_api, - prepare_current_provider + mocker, + prepare_current_provider, + sample_user, + ): """ Assume we have three slow delivery notifications for the current provider x. This triggers @@ -367,7 +376,9 @@ def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifi based on these as they are old. We only want to look for slow notifications after the point at which we switched back to provider x. """ + mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) starting_provider = get_current_provider('sms') + with set_config_values(notify_api, { 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3', 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7' diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 02344832b..51339370c 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -1,9 +1,8 @@ import pytest from datetime import datetime - from freezegun import freeze_time -from sqlalchemy import desc +from sqlalchemy import asc, desc from app.models import ProviderDetails, ProviderDetailsHistory from app import clients @@ -15,7 +14,9 @@ from app.dao.provider_details_dao import ( get_provider_details_by_notification_type, dao_switch_sms_provider_to_provider_with_identifier, dao_toggle_sms_provider, - dao_update_provider_details + dao_update_provider_details, + dao_get_provider_versions, + dao_get_sms_provider_with_equal_priority ) @@ -148,9 +149,13 @@ def test_switch_sms_provider_to_inactive_provider_does_not_switch( def test_toggle_sms_provider_switches_provider( + mocker, restore_provider_details, - current_sms_provider + current_sms_provider, + sample_user + ): + mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) dao_toggle_sms_provider(current_sms_provider.identifier) new_provider = get_current_provider('sms') @@ -161,8 +166,11 @@ def test_toggle_sms_provider_switches_provider( def test_toggle_sms_provider_switches_when_provider_priorities_are_equal( - restore_provider_details + mocker, + restore_provider_details, + sample_user ): + mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) current_provider = get_current_provider('sms') new_provider = get_alternative_sms_provider(current_provider.identifier) @@ -178,9 +186,12 @@ def test_toggle_sms_provider_switches_when_provider_priorities_are_equal( def test_toggle_sms_provider_updates_provider_history( + mocker, restore_provider_details, - current_sms_provider + current_sms_provider, + sample_user ): + mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) provider_history_rows = ProviderDetailsHistory.query.filter( ProviderDetailsHistory.id == current_sms_provider.id ).order_by( @@ -197,3 +208,68 @@ def test_toggle_sms_provider_updates_provider_history( assert len(updated_provider_history_rows) - len(provider_history_rows) == 1 assert updated_provider_history_rows[0].version - provider_history_rows[0].version == 1 + + +def test_toggle_sms_provider_switches_provider_stores_notify_user_id( + restore_provider_details, + sample_user, + mocker +): + mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) + + current_provider = get_current_provider('sms') + dao_toggle_sms_provider(current_provider.identifier) + new_provider = get_current_provider('sms') + + assert current_provider.identifier != new_provider.identifier + assert new_provider.created_by.id == sample_user.id + assert new_provider.created_by_id == sample_user.id + + +def test_toggle_sms_provider_switches_provider_stores_notify_user_id_in_history( + restore_provider_details, + sample_user, + mocker +): + mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) + + old_provider = get_current_provider('sms') + dao_toggle_sms_provider(old_provider.identifier) + new_provider = get_current_provider('sms') + + old_provider_from_history = ProviderDetailsHistory.query.filter_by( + identifier=old_provider.identifier, + version=old_provider.version + ).order_by( + asc(ProviderDetailsHistory.priority) + ).first() + new_provider_from_history = ProviderDetailsHistory.query.filter_by( + identifier=new_provider.identifier, + version=new_provider.version + ).order_by( + asc(ProviderDetailsHistory.priority) + ).first() + + assert old_provider.version == old_provider_from_history.version + assert new_provider.version == new_provider_from_history.version + assert new_provider_from_history.created_by_id == sample_user.id + assert old_provider_from_history.created_by_id == sample_user.id + + +def test_can_get_all_provider_history(current_sms_provider): + assert len(dao_get_provider_versions(current_sms_provider.id)) == 1 + + +def test_get_sms_provider_with_equal_priority_returns_provider( + restore_provider_details +): + current_provider = get_current_provider('sms') + new_provider = get_alternative_sms_provider(current_provider.identifier) + + current_provider.priority = new_provider.priority + dao_update_provider_details(current_provider) + + conflicting_provider = \ + dao_get_sms_provider_with_equal_priority(current_provider.identifier, current_provider.priority) + + assert conflicting_provider