From 6900505b05adb08da95b302bd8d67396a6b9bad1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 31 Aug 2021 11:01:22 +0100 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20call=20random.choices=20with=20?= =?UTF-8?q?zero=20weighting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As of https://github.com/python/cpython/commit/041d8b48a2e59fa642b2c5124d78086baf74e339 it’s not valid to call `random.choices` without giving at least one of the options a positive weighting. This makes sense, because giving a zero weighting is effectively saying ‘theres’s only one choice, but don’t choose it’. In our codebase this is applicable where there’s only one international provider, which we want to use even when it’s been de-prioritised for domestic SMS. This doesn’t cause a problem now, but will if we upgrade to Python versions greater than 3.9.0. --- app/delivery/send_to_providers.py | 7 ++++++- tests/app/delivery/test_send_to_providers.py | 14 ++++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 23d7f4f0e..e761956f3 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -171,7 +171,12 @@ def provider_to_use(notification_type, international=False): ) raise Exception("No active {} providers".format(notification_type)) - chosen_provider = random.choices(active_providers, weights=[p.priority for p in active_providers])[0] + if len(active_providers) == 1: + weights = [100] + else: + weights = [p.priority for p in active_providers] + + chosen_provider = random.choices(active_providers, weights=weights)[0] return notification_provider_clients.get_client_by_name_and_type(chosen_provider.identifier, notification_type) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 16e8a817c..cf1ef1552 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -72,8 +72,18 @@ def test_provider_to_use_should_cache_repeated_calls(mocker, notify_db_session): assert len(mock_choices.call_args_list) == 1 -def test_provider_to_use_should_only_return_mmg_for_international(mocker, notify_db_session): +@pytest.mark.parametrize('international_provider_priority', ( + # Since there’s only one international provider it should always + # be used, no matter what its priority is set to + 0, 50, 100, +)) +def test_provider_to_use_should_only_return_mmg_for_international( + mocker, + notify_db_session, + international_provider_priority, +): mmg = get_provider_details_by_identifier('mmg') + mmg.priority = international_provider_priority mock_choices = mocker.patch('app.delivery.send_to_providers.random.choices', return_value=[mmg]) ret = send_to_providers.provider_to_use('sms', international=True) @@ -90,7 +100,7 @@ def test_provider_to_use_should_only_return_active_providers(mocker, restore_pro ret = send_to_providers.provider_to_use('sms') - mock_choices.assert_called_once_with([firetext], weights=[0]) + mock_choices.assert_called_once_with([firetext], weights=[100]) assert ret.get_name() == 'firetext'