From fa7e0a1e84a679579b52050b1ce27aade7c84283 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 11 Nov 2019 14:31:58 +0000 Subject: [PATCH] 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 | 4 +- app/dao/provider_details_dao.py | 28 +++++++---- app/delivery/send_to_providers.py | 2 +- tests/app/dao/test_provider_details_dao.py | 55 +++++++++++++++------- 4 files changed, 61 insertions(+), 28 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 30f9a09bf..0d069ea63 100644 --- a/app/celery/scheduled_tasks.py +++ b/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') diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index a6b8063b6..fdbbb86de 100644 --- a/app/dao/provider_details_dao.py +++ b/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 ProviderDetails.query.filter_by(identifier=alternate_provider).one() + return 'firetext' + raise ValueError('Unrecognised sms provider {}'.format(identifier)) def get_current_provider(notification_type): @@ -49,9 +47,23 @@ def dao_get_provider_versions(provider_id): @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) +def dao_reduce_sms_provider_priority(identifier): + # get current priority of both providers + 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 diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 657939415..394478036 100644 --- a/app/delivery/send_to_providers.py +++ b/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 diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 20b662d36..403a4161c 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/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']) -def test_get_alternative_sms_provider_returns_expected_provider(notify_db, provider_identifier): - provider = get_alternative_sms_provider(provider_identifier) - assert provider.identifier != provider +@pytest.mark.parametrize('identifier, expected', [ + ('firetext', 'mmg'), + ('mmg', 'firetext'), +]) +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( - mocker, +@pytest.mark.parametrize(['starting_priorities', 'expected_priorities'], [ + ({'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, - sample_user - + starting_priorities, + expected_priorities ): - 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') + mmg = get_provider_details_by_identifier('mmg') + firetext = get_provider_details_by_identifier('firetext') - 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 - assert new_provider.priority < old_starting_provider.priority + # switch away from mmg. currently both 50/50 + 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(