add dao_reduce_sms_provider_priority function

retrive the sms providers from the DB, and decrease the chosen
provider's priority by 10, while increasing the other by 10.

add a check in to ensure we never decrease below 0 or increase above 100
- this is per provider, we don't check that the two add up to 100 or
  anything. If the values are outside of this range (eg: set via the UI)
then they'll probably* fix themselves at some point - we've added tests
to document these cases.

Use with_for_update to ensure that the method can only run once at a
time - other invocations of the function will be held on that line until
the currently running one ends and commits the transaction. This doesn't
affect anyone doing things from the UI.

app/celery/scheduled_tasks.py

@@ -35,7 +35,7 @@ from app.dao.notifications_dao import (
 )
 from app.dao.provider_details_dao import (
     get_current_provider,
-    dao_toggle_sms_provider
+    dao_reduce_sms_provider_priority
 )
 from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago
 from app.models import (
@@ -129,7 +129,7 @@ def switch_current_sms_provider_on_slow_delivery():
             )
         )
 
-        dao_toggle_sms_provider(current_provider.identifier)
+        dao_reduce_sms_provider_priority(current_provider.identifier)
 
 
 @notify_celery.task(name='check-job-status')

app/dao/provider_details_dao.py

@@ -22,13 +22,11 @@ def get_provider_details_by_identifier(identifier):
 
 
 def get_alternative_sms_provider(identifier):
-    alternate_provider = None
     if identifier == 'firetext':
-        alternate_provider = 'mmg'
+        return 'mmg'
     elif identifier == 'mmg':
-        alternate_provider = 'firetext'
+        return 'firetext'
-
+    raise ValueError('Unrecognised sms provider {}'.format(identifier))
-    return ProviderDetails.query.filter_by(identifier=alternate_provider).one()
 
 
 def get_current_provider(notification_type):
@@ -49,9 +47,23 @@ def dao_get_provider_versions(provider_id):
 
 
 @transactional
-def dao_toggle_sms_provider(identifier):
+def dao_reduce_sms_provider_priority(identifier):
-    alternate_provider = get_alternative_sms_provider(identifier)
+    # get current priority of both providers
-    dao_switch_sms_provider_to_provider_with_identifier(alternate_provider.identifier)
+    q = ProviderDetails.query.filter(
+        ProviderDetails.notification_type == 'sms',
+        ProviderDetails.active
+    ).with_for_update()
+
+    providers = {provider.identifier: provider for provider in q}
+    other = get_alternative_sms_provider(identifier)
+
+    # always keep values between 0 and 100
+    providers[identifier].priority = max(0, providers[identifier].priority - 10)
+    providers[other].priority = min(100, providers[other].priority + 10)
+
+
+def dao_toggle_sms_provider(*args, **kwargs):
+    raise NotImplementedError
 
 
 @transactional

app/delivery/send_to_providers.py

@@ -66,7 +66,7 @@ def send_sms_to_provider(notification):
             except Exception as e:
                 notification.billable_units = template.fragment_count
                 dao_update_notification(notification)
-                dao_toggle_sms_provider(provider.name)
+                dao_reduce_sms_provider_priority(provider)
                 raise e
             else:
                 notification.billable_units = template.fragment_count

tests/app/dao/test_provider_details_dao.py

@@ -12,11 +12,11 @@ from app.dao.provider_details_dao import (
     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,
     dao_get_provider_stats,
     dao_get_provider_versions,
-    dao_get_sms_provider_with_equal_priority
+    dao_get_sms_provider_with_equal_priority,
+    dao_reduce_sms_provider_priority,
 )
 from tests.app.db import (
     create_ft_billing,
@@ -122,10 +122,17 @@ def test_get_current_sms_provider_returns_correct_provider(restore_provider_deta
     assert provider.identifier == 'mmg'
 
 
-@pytest.mark.parametrize('provider_identifier', ['firetext', 'mmg'])
+@pytest.mark.parametrize('identifier, expected', [
-def test_get_alternative_sms_provider_returns_expected_provider(notify_db, provider_identifier):
+    ('firetext', 'mmg'),
-    provider = get_alternative_sms_provider(provider_identifier)
+    ('mmg', 'firetext'),
-    assert provider.identifier != provider
+])
+def test_get_alternative_sms_provider_returns_expected_provider(identifier, expected):
+    assert get_alternative_sms_provider(identifier) == expected
+
+
+def test_get_alternative_sms_provider_fails_if_unrecognised():
+    with pytest.raises(ValueError):
+        get_alternative_sms_provider('ses')
 
 
 def test_switch_sms_provider_to_current_provider_does_not_switch(
@@ -154,21 +161,35 @@ def test_switch_sms_provider_to_inactive_provider_does_not_switch(
     assert new_provider.identifier == current_sms_provider.identifier
 
 
-def test_toggle_sms_provider_switches_provider(
+@pytest.mark.parametrize(['starting_priorities', 'expected_priorities'], [
-    mocker,
+    ({'mmg': 50, 'firetext': 50}, {'mmg': 40, 'firetext': 60}),
+    ({'mmg': 0, 'firetext': 20}, {'mmg': 0, 'firetext': 30}),  # lower bound respected
+    ({'mmg': 50, 'firetext': 100}, {'mmg': 40, 'firetext': 100}),  # upper bound respected
+
+    # document what happens if they have unexpected values outside of the 0 - 100 range (due to manual setting from
+    # the admin app). the code never causes further issues, but sometimes doesn't actively reset the vaues to 0-100.
+    ({'mmg': 150, 'firetext': 50}, {'mmg': 140, 'firetext': 60}),
+    ({'mmg': 50, 'firetext': 150}, {'mmg': 40, 'firetext': 100}),
+
+    ({'mmg': -100, 'firetext': 50}, {'mmg': 0, 'firetext': 60}),
+    ({'mmg': 50, 'firetext': -100}, {'mmg': 40, 'firetext': -90}),
+])
+def test_change_sms_provider_priority_switches_provider(
     restore_provider_details,
-    current_sms_provider,
+    starting_priorities,
-    sample_user
+    expected_priorities
-
 ):
-    mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
+    mmg = get_provider_details_by_identifier('mmg')
-    dao_toggle_sms_provider(current_sms_provider.identifier)
+    firetext = get_provider_details_by_identifier('firetext')
-    new_provider = get_current_provider('sms')
 
-    old_starting_provider = get_provider_details_by_identifier(current_sms_provider.identifier)
+    mmg.priority = starting_priorities['mmg']
+    firetext.priority = starting_priorities['firetext']
 
-    assert new_provider.identifier != old_starting_provider.identifier
+    # switch away from mmg. currently both 50/50
-    assert new_provider.priority < old_starting_provider.priority
+    dao_reduce_sms_provider_priority('mmg')
+
+    assert firetext.priority == expected_priorities['firetext']
+    assert mmg.priority == expected_priorities['mmg']
 
 
 def test_toggle_sms_provider_switches_when_provider_priorities_are_equal(