From 7a7d8f8aa663f6e36c93edb416bbd48ab4728f6d Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 11:14:01 +0000 Subject: [PATCH 1/8] Link to history for SMS providers Although the priority history is shown on the "Change priority" page, this doesn't show active/inactive changes. --- app/templates/views/providers/providers.html | 2 +- tests/app/main/views/test_providers.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/templates/views/providers/providers.html b/app/templates/views/providers/providers.html index ee6e933ac..9cf706e1f 100644 --- a/app/templates/views/providers/providers.html +++ b/app/templates/views/providers/providers.html @@ -23,7 +23,7 @@ Providers field_headings_visible=True ) %} - {{ text_field(item.display_name) }} + {{ link_field(item.display_name, url_for('main.view_provider', provider_id=item.id)) }} {{ text_field(item.priority) }} diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 9f3c886cc..2279b9050 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -172,7 +172,7 @@ def test_should_show_all_providers( domestic_sms_first_row = domestic_sms_table.tbody.find_all('tr')[0] table_data = domestic_sms_first_row.find_all('td') - assert not table_data[0].find_all("a") + assert table_data[0].find_all("a")[0]['href'] == '/provider/6005e192-4738-4962-beec-ebd982d0b03f' assert table_data[0].text.strip() == "Domestic SMS Provider" assert table_data[1].text.strip() == "1" assert table_data[2].text.strip() == "42" @@ -183,7 +183,7 @@ def test_should_show_all_providers( domestic_sms_second_row = domestic_sms_table.tbody.find_all('tr')[1] table_data = domestic_sms_second_row.find_all('td') - assert not table_data[0].find_all("a") + assert table_data[0].find_all("a")[0]['href'] == '/provider/0bd529cd-a0fd-43e5-80ee-b95ef6b0d51f' assert table_data[0].text.strip() == "Second Domestic SMS Provider" assert table_data[1].text.strip() == "2" assert table_data[2].text.strip() == "58" From 396e9e37438224b07bff339773b9b75f895f2497 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 11:22:24 +0000 Subject: [PATCH 2/8] Split up provider test data into separate vars This makes it easier to follow which dict is being referred to as the indices didn't convey this. --- tests/app/main/views/test_providers.py | 197 ++++++++++++------------- 1 file changed, 98 insertions(+), 99 deletions(-) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 2279b9050..c935be52f 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -10,103 +10,103 @@ import app from app.main.views.providers import add_monthly_traffic from tests.conftest import normalize_spaces +sms_provider_1 = { + 'id': '6005e192-4738-4962-beec-ebd982d0b03f', + 'active': True, + 'priority': 1, + 'display_name': 'Domestic SMS Provider', + 'identifier': 'first_sms_domestic', + 'notification_type': 'sms', + 'updated_at': datetime(2017, 1, 16, 15, 20, 40).isoformat(), + 'version': 1, + 'created_by_name': 'Test User', + 'supports_international': False, + 'current_month_billable_sms': 5020, +} + +sms_provider_2 = { + 'id': '0bd529cd-a0fd-43e5-80ee-b95ef6b0d51f', + 'active': True, + 'priority': 2, + 'display_name': 'Second Domestic SMS Provider', + 'identifier': 'second_sms_domestic', + 'notification_type': 'sms', + 'updated_at': None, + 'version': 1, + 'created_by': None, + 'supports_international': False, + 'current_month_billable_sms': 6891, +} + +email_provider_1 = { + 'id': '6005e192-4738-4962-beec-ebd982d0b03a', + 'active': True, + 'priority': 1, + 'display_name': 'first_email_provider', + 'identifier': 'first_email', + 'notification_type': 'email', + 'updated_at': None, + 'version': 1, + 'created_by': None, + 'supports_international': False, + 'current_month_billable_sms': 0, +} + +email_provider_2 = { + 'active': True, + 'priority': 2, + 'display_name': 'second_email_provider', + 'identifier': 'second_email', + 'id': '0bd529cd-a0fd-43e5-80ee-b95ef6b0d51b', + 'notification_type': 'email', + 'updated_at': None, + 'version': 1, + 'created_by': None, + 'supports_international': False, + 'current_month_billable_sms': 0, +} + +sms_provider_int_1 = { + 'id': '67c770f5-918e-4afa-a5ff-880b9beb161d', + 'active': False, + 'priority': 10, + 'display_name': 'First International SMS Provider', + 'identifier': 'first_sms_international', + 'notification_type': 'sms', + 'updated_at': None, + 'version': 1, + 'created_by': None, + 'supports_international': True, + 'current_month_billable_sms': 0, +} + +sms_provider_int_2 = { + 'id': '67c770f5-918e-4afa-a5ff-880b9beb161d', + 'active': False, + 'priority': 10, + 'display_name': 'Second International SMS Provider', + 'identifier': 'second_sms_international', + 'notification_type': 'sms', + 'updated_at': None, + 'version': 1, + 'created_by': None, + 'supports_international': True, + 'current_month_billable_sms': 0, +} + stub_providers = { 'provider_details': [ - { - 'id': '6005e192-4738-4962-beec-ebd982d0b03f', - 'active': True, - 'priority': 1, - 'display_name': 'Domestic SMS Provider', - 'identifier': 'first_sms_domestic', - 'notification_type': 'sms', - 'updated_at': datetime(2017, 1, 16, 15, 20, 40).isoformat(), - 'version': 1, - 'created_by_name': 'Test User', - 'supports_international': False, - 'current_month_billable_sms': 5020, - }, - { - 'id': '0bd529cd-a0fd-43e5-80ee-b95ef6b0d51f', - 'active': True, - 'priority': 2, - 'display_name': 'Second Domestic SMS Provider', - 'identifier': 'second_sms_domestic', - 'notification_type': 'sms', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': False, - 'current_month_billable_sms': 6891, - }, - { - 'id': '6005e192-4738-4962-beec-ebd982d0b03a', - 'active': True, - 'priority': 1, - 'display_name': 'first_email_provider', - 'identifier': 'first_email', - 'notification_type': 'email', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': False, - 'current_month_billable_sms': 0, - }, - { - 'active': True, - 'priority': 2, - 'display_name': 'second_email_provider', - 'identifier': 'second_email', - 'id': '0bd529cd-a0fd-43e5-80ee-b95ef6b0d51b', - 'notification_type': 'email', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': False, - 'current_month_billable_sms': 0, - }, - { - 'id': '67c770f5-918e-4afa-a5ff-880b9beb161d', - 'active': False, - 'priority': 10, - 'display_name': 'First International SMS Provider', - 'identifier': 'first_sms_international', - 'notification_type': 'sms', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': True, - 'current_month_billable_sms': 0, - }, - { - 'id': '67c770f5-918e-4afa-a5ff-880b9beb161d', - 'active': False, - 'priority': 10, - 'display_name': 'Second International SMS Provider', - 'identifier': 'second_sms_international', - 'notification_type': 'sms', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': True, - 'current_month_billable_sms': 0, - } + sms_provider_1, + sms_provider_2, + email_provider_1, + email_provider_2, + sms_provider_int_1, + sms_provider_int_2, ] } stub_provider = { - 'provider_details': - { - 'id': '6005e192-4738-4962-beec-ebd982d0b03f', - 'active': True, - 'priority': 1, - 'display_name': 'Domestic SMS Provider', - 'identifier': 'first_sms_domestic', - 'notification_type': 'sms', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': False - } + 'provider_details': sms_provider_1 } stub_provider_history = { @@ -428,15 +428,14 @@ def test_should_show_version_history_for_first_two_sms_providers( ) # second_sms_international will be the primary provider because it’s # the first in the list when reverse sorting the SMS providers - second_sms_international = stub_providers['provider_details'][5] mocker.patch( 'app.provider_client.get_provider_versions', return_value={'data': [ { 'id': id, 'priority': priority, - 'display_name': second_sms_international['display_name'], - 'identifier': second_sms_international['identifier'], + 'display_name': sms_provider_int_2['display_name'], + 'identifier': sms_provider_int_2['identifier'], 'updated_at': updated_at, 'created_by': { 'email_address': 'test@foo.bar', @@ -473,7 +472,7 @@ def test_should_show_version_history_for_first_two_sms_providers( radio['value'] for radio in page.select('input[checked]') ] == [ - str(second_sms_international['priority']) + str(sms_provider_int_2['priority']) ] assert [ @@ -511,15 +510,15 @@ def test_should_show_version_history_for_first_two_sms_providers( ( '10', [ - call(stub_providers['provider_details'][5]['id'], 10), - call(stub_providers['provider_details'][1]['id'], 90), + call(sms_provider_int_2['id'], 10), + call(sms_provider_2['id'], 90), ], ), ( '80', [ - call(stub_providers['provider_details'][5]['id'], 80), - call(stub_providers['provider_details'][1]['id'], 20), + call(sms_provider_int_2['id'], 80), + call(sms_provider_2['id'], 20), ], ), ]) From 0f9b73f73cc190cfe994b76707a98f84c49c882b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 11:28:06 +0000 Subject: [PATCH 3/8] Use fixtures instead deep-copying constants This is more conventional and removes the ambiguity about whether the deepcopy was actually necessary for new tests. --- tests/app/main/views/test_providers.py | 134 ++++++++++++++----------- 1 file changed, 76 insertions(+), 58 deletions(-) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index c935be52f..779c64c4a 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -1,4 +1,3 @@ -import copy from datetime import datetime from unittest.mock import call @@ -94,61 +93,71 @@ sms_provider_int_2 = { 'current_month_billable_sms': 0, } -stub_providers = { - 'provider_details': [ - sms_provider_1, - sms_provider_2, - email_provider_1, - email_provider_2, - sms_provider_int_1, - sms_provider_int_2, - ] -} -stub_provider = { - 'provider_details': sms_provider_1 -} +@pytest.fixture +def stub_providers(): + return { + 'provider_details': [ + sms_provider_1, + sms_provider_2, + email_provider_1, + email_provider_2, + sms_provider_int_1, + sms_provider_int_2, + ] + } -stub_provider_history = { - 'data': [ - { - 'id': 'f9af1ec7-58ef-4f7d-a6f4-5fe7e48644cb', - 'active': True, - 'priority': 20, - 'display_name': 'Foo', - 'identifier': 'foo', - 'notification_type': 'sms', - 'updated_at': None, - 'version': 2, - 'created_by': { - 'email_address': 'test@foo.bar', - 'name': 'Test User', - 'id': '7cc1dddb-bcbc-4739-8fc1-61bedde3332a' + +@pytest.fixture +def stub_provider(): + return { + 'provider_details': sms_provider_1 + } + + +@pytest.fixture +def stub_provider_history(): + return { + 'data': [ + { + 'id': 'f9af1ec7-58ef-4f7d-a6f4-5fe7e48644cb', + 'active': True, + 'priority': 20, + 'display_name': 'Foo', + 'identifier': 'foo', + 'notification_type': 'sms', + 'updated_at': None, + 'version': 2, + 'created_by': { + 'email_address': 'test@foo.bar', + 'name': 'Test User', + 'id': '7cc1dddb-bcbc-4739-8fc1-61bedde3332a' + }, + 'supports_international': False }, - 'supports_international': False - }, - { - 'id': 'f9af1ec7-58ef-4f7d-a6f4-5fe7e48644cb', - 'active': True, - 'priority': 10, - 'display_name': 'Bar', - 'identifier': 'bar', - 'notification_type': 'sms', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': False - } - ] -} + { + 'id': 'f9af1ec7-58ef-4f7d-a6f4-5fe7e48644cb', + 'active': True, + 'priority': 10, + 'display_name': 'Bar', + 'identifier': 'bar', + 'notification_type': 'sms', + 'updated_at': None, + 'version': 1, + 'created_by': None, + 'supports_international': False + } + ] + } def test_should_show_all_providers( client_request, platform_admin_user, mocker, + stub_providers, ): - mocker.patch('app.provider_client.get_all_providers', return_value=copy.deepcopy(stub_providers)) + mocker.patch('app.provider_client.get_all_providers', return_value=stub_providers) client_request.login(platform_admin_user) page = client_request.get('main.view_providers') @@ -260,8 +269,9 @@ def test_should_show_edit_provider_form( platform_admin_user, mocker, fake_uuid, + stub_provider ): - mocker.patch('app.provider_client.get_provider_by_id', return_value=copy.deepcopy(stub_provider)) + mocker.patch('app.provider_client.get_provider_by_id', return_value=stub_provider) client_request.login(platform_admin_user) page = client_request.get('main.edit_provider', provider_id=fake_uuid) @@ -281,8 +291,9 @@ def test_should_show_error_on_bad_provider_priority( client_request, platform_admin_user, mocker, + stub_provider, ): - mocker.patch('app.provider_client.get_provider_by_id', return_value=copy.deepcopy(stub_provider)) + mocker.patch('app.provider_client.get_provider_by_id', return_value=stub_provider) client_request.login(platform_admin_user) page = client_request.post( @@ -301,8 +312,9 @@ def test_should_show_error_on_negative_provider_priority( client_request, platform_admin_user, mocker, + stub_provider, ): - mocker.patch('app.provider_client.get_provider_by_id', return_value=copy.deepcopy(stub_provider)) + mocker.patch('app.provider_client.get_provider_by_id', return_value=stub_provider) client_request.login(platform_admin_user) page = client_request.post( @@ -321,8 +333,9 @@ def test_should_show_error_on_too_big_provider_priority( client_request, platform_admin_user, mocker, + stub_provider, ): - mocker.patch('app.provider_client.get_provider_by_id', return_value=copy.deepcopy(stub_provider)) + mocker.patch('app.provider_client.get_provider_by_id', return_value=stub_provider) client_request.login(platform_admin_user) page = client_request.post( @@ -341,8 +354,9 @@ def test_should_show_error_on_too_little_provider_priority( client_request, platform_admin_user, mocker, + stub_provider, ): - mocker.patch('app.provider_client.get_provider_by_id', return_value=copy.deepcopy(stub_provider)) + mocker.patch('app.provider_client.get_provider_by_id', return_value=stub_provider) client_request.login(platform_admin_user) page = client_request.post( @@ -361,9 +375,10 @@ def test_should_update_provider_priority( client_request, platform_admin_user, mocker, + stub_provider, ): - mocker.patch('app.provider_client.get_provider_by_id', return_value=copy.deepcopy(stub_provider)) - mocker.patch('app.provider_client.update_provider', return_value=copy.deepcopy(stub_provider)) + mocker.patch('app.provider_client.get_provider_by_id', return_value=stub_provider) + mocker.patch('app.provider_client.update_provider', return_value=stub_provider) client_request.login(platform_admin_user) client_request.post( @@ -379,9 +394,10 @@ def test_should_update_provider_priority( def test_should_show_provider_version_history( client_request, platform_admin_user, - mocker + mocker, + stub_provider_history ): - mocker.patch('app.provider_client.get_provider_versions', return_value=copy.deepcopy(stub_provider_history)) + mocker.patch('app.provider_client.get_provider_versions', return_value=stub_provider_history) client_request.login(platform_admin_user) page = client_request.get( @@ -420,11 +436,12 @@ def test_should_show_provider_version_history( def test_should_show_version_history_for_first_two_sms_providers( client_request, platform_admin_user, - mocker + mocker, + stub_providers, ): mocker.patch( 'app.provider_client.get_all_providers', - return_value=copy.deepcopy(stub_providers) + return_value=stub_providers ) # second_sms_international will be the primary provider because it’s # the first in the list when reverse sorting the SMS providers @@ -528,10 +545,11 @@ def test_should_update_priority_of_first_two_sms_providers( mocker, posted_number, expected_calls, + stub_providers, ): mocker.patch( 'app.provider_client.get_all_providers', - return_value=copy.deepcopy(stub_providers) + return_value=stub_providers ) mocker.patch( 'app.provider_client.get_provider_versions', From f9749c0ae3f55ba661f22dbf35209f82f714caf2 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 16:23:10 +0000 Subject: [PATCH 4/8] Standardise priorities / names for test providers Only multiples of 10 are possible in practice. This will make it easier to use this test data in the next commits. --- tests/app/main/views/test_providers.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 779c64c4a..af53b51f4 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -12,8 +12,8 @@ from tests.conftest import normalize_spaces sms_provider_1 = { 'id': '6005e192-4738-4962-beec-ebd982d0b03f', 'active': True, - 'priority': 1, - 'display_name': 'Domestic SMS Provider', + 'priority': 20, + 'display_name': 'First Domestic SMS Provider', 'identifier': 'first_sms_domestic', 'notification_type': 'sms', 'updated_at': datetime(2017, 1, 16, 15, 20, 40).isoformat(), @@ -26,7 +26,7 @@ sms_provider_1 = { sms_provider_2 = { 'id': '0bd529cd-a0fd-43e5-80ee-b95ef6b0d51f', 'active': True, - 'priority': 2, + 'priority': 10, 'display_name': 'Second Domestic SMS Provider', 'identifier': 'second_sms_domestic', 'notification_type': 'sms', @@ -182,8 +182,8 @@ def test_should_show_all_providers( table_data = domestic_sms_first_row.find_all('td') assert table_data[0].find_all("a")[0]['href'] == '/provider/6005e192-4738-4962-beec-ebd982d0b03f' - assert table_data[0].text.strip() == "Domestic SMS Provider" - assert table_data[1].text.strip() == "1" + assert table_data[0].text.strip() == "First Domestic SMS Provider" + assert table_data[1].text.strip() == "20" assert table_data[2].text.strip() == "42" assert table_data[3].text.strip() == "True" assert table_data[4].text.strip() == "16 January at 3:20pm" @@ -194,7 +194,7 @@ def test_should_show_all_providers( assert table_data[0].find_all("a")[0]['href'] == '/provider/0bd529cd-a0fd-43e5-80ee-b95ef6b0d51f' assert table_data[0].text.strip() == "Second Domestic SMS Provider" - assert table_data[1].text.strip() == "2" + assert table_data[1].text.strip() == "10" assert table_data[2].text.strip() == "58" assert table_data[3].text.strip() == "True" assert table_data[4].text.strip() == "None" @@ -278,12 +278,12 @@ def test_should_show_edit_provider_form( h1 = [header.text.strip() for header in page.find_all('h1')] - assert 'Domestic SMS Provider' in h1 + assert 'First Domestic SMS Provider' in h1 form = [form for form in page.find_all('form')] form_elements = [element for element in form[0].find_all('input')] - assert form_elements[0]['value'] == '1' + assert form_elements[0]['value'] == '20' assert form_elements[0]['name'] == 'priority' From 9355c4f8d1195285248e8289a08586ee10a1f378 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 16:30:54 +0000 Subject: [PATCH 5/8] Only show active providers when changing priority This fixes a bug where a third inactive provider would (potentially) appear in place of one of the two active ones, depending on the order of its identifier compared to the other two. In future we may look at simplifying this to cope with more than two active providers. For now, the existing UI will continue to work when we add a new, inactive SMS provider for Reach. --- app/main/views/providers.py | 1 + tests/app/main/views/test_providers.py | 31 +++++++++++++------------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/app/main/views/providers.py b/app/main/views/providers.py index 0868f1a63..fcaf6aa86 100644 --- a/app/main/views/providers.py +++ b/app/main/views/providers.py @@ -65,6 +65,7 @@ def edit_sms_provider_ratio(): provider for provider in provider_client.get_all_providers()['provider_details'] if provider['notification_type'] == 'sms' + and provider['active'] ], key=itemgetter('identifier'), reverse=True) form = AdminProviderRatioForm(ratio=providers[0]['priority']) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index af53b51f4..36abc4519 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -443,16 +443,17 @@ def test_should_show_version_history_for_first_two_sms_providers( 'app.provider_client.get_all_providers', return_value=stub_providers ) - # second_sms_international will be the primary provider because it’s - # the first in the list when reverse sorting the SMS providers + + # we get only get version info for the "primary" (first) provider + # in reverse alphabetical order i.e. sms_provider_2 mocker.patch( 'app.provider_client.get_provider_versions', return_value={'data': [ { 'id': id, 'priority': priority, - 'display_name': sms_provider_int_2['display_name'], - 'identifier': sms_provider_int_2['identifier'], + 'display_name': sms_provider_2['display_name'], + 'identifier': sms_provider_2['identifier'], 'updated_at': updated_at, 'created_by': { 'email_address': 'test@foo.bar', @@ -489,7 +490,7 @@ def test_should_show_version_history_for_first_two_sms_providers( radio['value'] for radio in page.select('input[checked]') ] == [ - str(sms_provider_int_2['priority']) + str(sms_provider_2['priority']) ] assert [ @@ -507,18 +508,18 @@ def test_should_show_version_history_for_first_two_sms_providers( ] == [ ( 'Test User 2:00pm ' - 'Second International SMS Provider 100% ' - 'Second Domestic SMS Provider 0%' + 'Second Domestic SMS Provider 100% ' + 'First Domestic SMS Provider 0%' ), ( 'Test User 5:00am ' - 'Second International SMS Provider 80% ' - 'Second Domestic SMS Provider 20%' + 'Second Domestic SMS Provider 80% ' + 'First Domestic SMS Provider 20%' ), ( 'Test User 3:00am ' - 'Second International SMS Provider 10% ' - 'Second Domestic SMS Provider 90%' + 'Second Domestic SMS Provider 10% ' + 'First Domestic SMS Provider 90%' ), ] @@ -527,15 +528,15 @@ def test_should_show_version_history_for_first_two_sms_providers( ( '10', [ - call(sms_provider_int_2['id'], 10), - call(sms_provider_2['id'], 90), + call(sms_provider_2['id'], 10), + call(sms_provider_1['id'], 90), ], ), ( '80', [ - call(sms_provider_int_2['id'], 80), - call(sms_provider_2['id'], 20), + call(sms_provider_2['id'], 80), + call(sms_provider_1['id'], 20), ], ), ]) From 55e2a2f96b227f713e42ff24aa6dcbf75172f4f4 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 16:36:17 +0000 Subject: [PATCH 6/8] Stop reversing the order of SMS providers This makes no functional difference but does make it easier to read. --- app/main/views/providers.py | 2 +- tests/app/main/views/test_providers.py | 28 +++++++++++++------------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/main/views/providers.py b/app/main/views/providers.py index fcaf6aa86..606db6f8d 100644 --- a/app/main/views/providers.py +++ b/app/main/views/providers.py @@ -66,7 +66,7 @@ def edit_sms_provider_ratio(): for provider in provider_client.get_all_providers()['provider_details'] if provider['notification_type'] == 'sms' and provider['active'] - ], key=itemgetter('identifier'), reverse=True) + ], key=itemgetter('identifier')) form = AdminProviderRatioForm(ratio=providers[0]['priority']) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 36abc4519..0e500d262 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -445,15 +445,15 @@ def test_should_show_version_history_for_first_two_sms_providers( ) # we get only get version info for the "primary" (first) provider - # in reverse alphabetical order i.e. sms_provider_2 + # in alphabetical order i.e. sms_provider_1 mocker.patch( 'app.provider_client.get_provider_versions', return_value={'data': [ { 'id': id, 'priority': priority, - 'display_name': sms_provider_2['display_name'], - 'identifier': sms_provider_2['identifier'], + 'display_name': sms_provider_1['display_name'], + 'identifier': sms_provider_1['identifier'], 'updated_at': updated_at, 'created_by': { 'email_address': 'test@foo.bar', @@ -490,7 +490,7 @@ def test_should_show_version_history_for_first_two_sms_providers( radio['value'] for radio in page.select('input[checked]') ] == [ - str(sms_provider_2['priority']) + str(sms_provider_1['priority']) ] assert [ @@ -508,18 +508,18 @@ def test_should_show_version_history_for_first_two_sms_providers( ] == [ ( 'Test User 2:00pm ' - 'Second Domestic SMS Provider 100% ' - 'First Domestic SMS Provider 0%' + 'First Domestic SMS Provider 100% ' + 'Second Domestic SMS Provider 0%' ), ( 'Test User 5:00am ' - 'Second Domestic SMS Provider 80% ' - 'First Domestic SMS Provider 20%' + 'First Domestic SMS Provider 80% ' + 'Second Domestic SMS Provider 20%' ), ( 'Test User 3:00am ' - 'Second Domestic SMS Provider 10% ' - 'First Domestic SMS Provider 90%' + 'First Domestic SMS Provider 10% ' + 'Second Domestic SMS Provider 90%' ), ] @@ -528,15 +528,15 @@ def test_should_show_version_history_for_first_two_sms_providers( ( '10', [ - call(sms_provider_2['id'], 10), - call(sms_provider_1['id'], 90), + call(sms_provider_1['id'], 10), + call(sms_provider_2['id'], 90), ], ), ( '80', [ - call(sms_provider_2['id'], 80), - call(sms_provider_1['id'], 20), + call(sms_provider_1['id'], 80), + call(sms_provider_2['id'], 20), ], ), ]) From 59f6b0b5d82f64e28c3db5ddcb8958ffd4267e5b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 16:37:36 +0000 Subject: [PATCH 7/8] Replace "primary", etc. with "first", etc. Using "primary" made sense when the other "secondary" provider was new, but today we see them as equivalent and the terminology is a bit confusing. In future we may add a third provider as well. --- app/main/views/providers.py | 12 ++++++------ .../views/providers/edit-sms-provider-ratio.html | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/main/views/providers.py b/app/main/views/providers.py index 606db6f8d..cbdb581d7 100644 --- a/app/main/views/providers.py +++ b/app/main/views/providers.py @@ -73,19 +73,19 @@ def edit_sms_provider_ratio(): if len(providers) < 2: abort(400) - primary_provider, secondary_provider = providers[0:2] + first_provider, second_provider = providers[0:2] if form.validate_on_submit(): - provider_client.update_provider(primary_provider['id'], form.percentage_left) - provider_client.update_provider(secondary_provider['id'], form.percentage_right) + provider_client.update_provider(first_provider['id'], form.percentage_left) + provider_client.update_provider(second_provider['id'], form.percentage_right) return redirect(url_for('.edit_sms_provider_ratio')) return render_template( 'views/providers/edit-sms-provider-ratio.html', - versions=_chunk_versions_by_day(_get_versions_since_switchover(primary_provider['id'])), + versions=_chunk_versions_by_day(_get_versions_since_switchover(first_provider['id'])), form=form, - primary_provider=providers[0]['display_name'], - secondary_provider=providers[1]['display_name'], + first_provider=providers[0]['display_name'], + second_provider=providers[1]['display_name'], ) diff --git a/app/templates/views/providers/edit-sms-provider-ratio.html b/app/templates/views/providers/edit-sms-provider-ratio.html index 35b4c6208..e522d132b 100644 --- a/app/templates/views/providers/edit-sms-provider-ratio.html +++ b/app/templates/views/providers/edit-sms-provider-ratio.html @@ -22,10 +22,10 @@
- {{ primary_provider }} + {{ first_provider }}
- {{ secondary_provider }} + {{ second_provider }}
{% call form_wrapper() %}
@@ -71,11 +71,11 @@
- {{ primary_provider }}

+ {{ first_provider }}

{{ version.priority|format_thousands }}%
- {{ secondary_provider }}

+ {{ second_provider }}

{{ (100 - version.priority)|format_thousands }}%
From e02dbde378301e1b458e78d7103087a2a8039240 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 5 Apr 2022 13:53:45 +0100 Subject: [PATCH 8/8] Small changes to address PR comments In response to [^1] and [^2]. [^1]: https://github.com/alphagov/notifications-admin/pull/4199#discussion_r842653983 [^2]: https://github.com/alphagov/notifications-admin/pull/4199#discussion_r842667340 --- tests/app/main/views/test_providers.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 0e500d262..63f490cc5 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -65,7 +65,7 @@ email_provider_2 = { 'current_month_billable_sms': 0, } -sms_provider_int_1 = { +sms_provider_intl_1 = { 'id': '67c770f5-918e-4afa-a5ff-880b9beb161d', 'active': False, 'priority': 10, @@ -79,7 +79,7 @@ sms_provider_int_1 = { 'current_month_billable_sms': 0, } -sms_provider_int_2 = { +sms_provider_intl_2 = { 'id': '67c770f5-918e-4afa-a5ff-880b9beb161d', 'active': False, 'priority': 10, @@ -102,8 +102,8 @@ def stub_providers(): sms_provider_2, email_provider_1, email_provider_2, - sms_provider_int_1, - sms_provider_int_2, + sms_provider_intl_1, + sms_provider_intl_2, ] } @@ -444,8 +444,10 @@ def test_should_show_version_history_for_first_two_sms_providers( return_value=stub_providers ) - # we get only get version info for the "primary" (first) provider - # in alphabetical order i.e. sms_provider_1 + # Getting the history for one provider implicitly gives us the + # history of the other one (in a world with only two providers). + # The code picks the first provider in alphabetical order of its + # id i.e. sms_provider_1. mocker.patch( 'app.provider_client.get_provider_versions', return_value={'data': [