Updates:

* Add notify user id in config
* Add dao method to get provider history versions along with tests
* BUG: Provider switching did not handle case where priorities were equal. This
* adds a fix to properly cover this case along with tests

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'

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

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):

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'

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