From a3231effb191cfb30e3f4d1325304cf520a5b47a Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 11:56:05 +0100 Subject: [PATCH 01/17] Remove version history from provider ratio page This doesn't work with 3 providers. You can still see the version history for each provider on their dedicated page. --- app/main/views/providers.py | 32 +----- .../providers/edit-sms-provider-ratio.html | 43 -------- tests/app/main/views/test_providers.py | 99 ------------------- 3 files changed, 1 insertion(+), 173 deletions(-) diff --git a/app/main/views/providers.py b/app/main/views/providers.py index d08128f7b..a73735a94 100644 --- a/app/main/views/providers.py +++ b/app/main/views/providers.py @@ -1,11 +1,10 @@ -from collections import defaultdict from datetime import datetime from operator import itemgetter from flask import abort, render_template, url_for from werkzeug.utils import redirect -from app import format_date_numeric, provider_client +from app import provider_client from app.main import main from app.main.forms import AdminProviderForm, AdminProviderRatioForm from app.utils.user import user_is_platform_admin @@ -82,41 +81,12 @@ def edit_sms_provider_ratio(): return render_template( 'views/providers/edit-sms-provider-ratio.html', - versions=_chunk_versions_by_day(_get_versions_since_switchover(first_provider['id'])), form=form, first_provider=providers[0]['display_name'], second_provider=providers[1]['display_name'], ) -def _get_versions_since_switchover(provider_id): - - for version in sorted( # noqa: B020 - provider_client.get_provider_versions(provider_id)['data'], - key=lambda version: version['updated_at'] or '' - ): - - if not version['updated_at']: - continue - - if version['updated_at'] < PROVIDER_PRIORITY_MEANING_SWITCHOVER: - continue - - yield version - - -def _chunk_versions_by_day(versions): - - days = defaultdict(list) - - for version in sorted(versions, key=lambda version: version['updated_at'] or '', reverse=True): # noqa: B020 - days[ - format_date_numeric(version['updated_at']) - ].append(version) - - return sorted(days.items(), reverse=True) - - @main.route("/provider/") @user_is_platform_admin def view_provider(provider_id): diff --git a/app/templates/views/providers/edit-sms-provider-ratio.html b/app/templates/views/providers/edit-sms-provider-ratio.html index e522d132b..bbe3b0d04 100644 --- a/app/templates/views/providers/edit-sms-provider-ratio.html +++ b/app/templates/views/providers/edit-sms-provider-ratio.html @@ -44,47 +44,4 @@ - - - {% for day, versions in versions %} -

- {% if day %} - {{ versions[0]['updated_at']|format_date_human|title }} - {% else %} - Start - {% endif %} -

- - {% endfor %} - {% endblock %} diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 63f490cc5..8dbac9b5d 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -3,7 +3,6 @@ from unittest.mock import call import pytest from flask import url_for -from freezegun import freeze_time import app from app.main.views.providers import add_monthly_traffic @@ -432,100 +431,6 @@ def test_should_show_provider_version_history( assert second_row[4].text.strip() == "True" -@freeze_time('2022-2-22 15:00') -def test_should_show_version_history_for_first_two_sms_providers( - client_request, - platform_admin_user, - mocker, - stub_providers, -): - mocker.patch( - 'app.provider_client.get_all_providers', - return_value=stub_providers - ) - - # 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': [ - { - 'id': id, - 'priority': priority, - 'display_name': sms_provider_1['display_name'], - 'identifier': sms_provider_1['identifier'], - 'updated_at': updated_at, - 'created_by': { - 'email_address': 'test@foo.bar', - 'name': 'Test User', - 'id': '7cc1dddb-bcbc-4739-8fc1-61bedde3332a' - }, - 'supports_international': False, - } - for updated_at, priority in [ - (datetime(2022, 2, 22, 14).isoformat(), 100), - (datetime(2020, 1, 1, 5).isoformat(), 80), - (datetime(2020, 1, 1, 3).isoformat(), 10), - # Anything older than 11am on 29 November 2019 - # should be ignored because the priority numbers - # didn’t mean the same thing before then - (datetime(2019, 11, 29, 10, 59).isoformat(), 123), - (datetime(2000, 1, 1, 0).isoformat(), 1999), - (None, 30), - ] - ]} - ) - - client_request.login(platform_admin_user) - page = client_request.get('main.edit_sms_provider_ratio') - - assert [ - radio['value'] - for radio in page.select('input[name=ratio]') - ] == [ - '100', '90', '80', '70', '60', '50', '40', '30', '20', '10', '0', - ] - - assert [ - radio['value'] - for radio in page.select('input[checked]') - ] == [ - str(sms_provider_1['priority']) - ] - - assert [ - normalize_spaces(heading.text) - for heading in page.select('main h2') - ] == [ - 'Now', - 'Today', - '1 January 2020', - ] - - assert [ - normalize_spaces(version.text) - for version in page.select('li.history-list-item') - ] == [ - ( - 'Test User 2:00pm ' - 'First Domestic SMS Provider 100% ' - 'Second Domestic SMS Provider 0%' - ), - ( - 'Test User 5:00am ' - 'First Domestic SMS Provider 80% ' - 'Second Domestic SMS Provider 20%' - ), - ( - 'Test User 3:00am ' - 'First Domestic SMS Provider 10% ' - 'Second Domestic SMS Provider 90%' - ), - ] - - @pytest.mark.parametrize('posted_number, expected_calls', [ ( '10', @@ -554,10 +459,6 @@ def test_should_update_priority_of_first_two_sms_providers( 'app.provider_client.get_all_providers', return_value=stub_providers ) - mocker.patch( - 'app.provider_client.get_provider_versions', - return_value={'data': []} - ) mock_update_provider = mocker.patch( 'app.provider_client.update_provider' ) From 66335d20c26e496e772c6b1194e400423fa9439d Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 12:00:30 +0100 Subject: [PATCH 02/17] Return to main provider page after editing ratio Staying on the edit page is atypical behaviour for an edit form, especially now it no longer shows the version history. --- app/main/views/providers.py | 2 +- tests/app/main/views/test_providers.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main/views/providers.py b/app/main/views/providers.py index a73735a94..15a9a4078 100644 --- a/app/main/views/providers.py +++ b/app/main/views/providers.py @@ -77,7 +77,7 @@ def edit_sms_provider_ratio(): if form.validate_on_submit(): 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 redirect(url_for('.view_providers')) return render_template( 'views/providers/edit-sms-provider-ratio.html', diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 8dbac9b5d..25684a3ab 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -470,7 +470,7 @@ def test_should_update_priority_of_first_two_sms_providers( 'ratio': posted_number, }, _expected_redirect=url_for( - '.edit_sms_provider_ratio', + '.view_providers', _external=True, ), ) From 25a26c0b4ddd7ed1f78738b0dffb4eecf43f017b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 12:09:50 +0100 Subject: [PATCH 03/17] Add caveat about provider version history To go with https://github.com/alphagov/notifications-api/pull/3500 --- app/templates/views/providers/provider.html | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/templates/views/providers/provider.html b/app/templates/views/providers/provider.html index 8bb831da1..bdaa5e1f2 100644 --- a/app/templates/views/providers/provider.html +++ b/app/templates/views/providers/provider.html @@ -18,6 +18,11 @@
{{ page_header(provider_versions[0].display_name) }} + +

+ Only showing the latest 100 versions. +

+ {% call(item, row_number) list_table( provider_versions, caption='', From 8655ab7deab70a12b9d40bc77888f6e3bfb1b900 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 12:53:12 +0100 Subject: [PATCH 04/17] Stop showing priorities for other provider types This isn't used and showing priorities when we only have a single provider or where they have no effect is unnecessarily confusing. Removing the form makes it clearer that there's only one way to adjust priorities for domestic SMS providers. If we add another email or international SMS provider in future, we would need to rewrite the form here anyway as the priorities need to be adjusted in tandem, not individually. --- app/main/forms.py | 6 - app/main/views/providers.py | 15 +- .../views/providers/edit-provider.html | 37 ----- app/templates/views/providers/providers.html | 11 +- tests/app/main/views/test_providers.py | 155 +----------------- tests/app/test_navigation.py | 1 - 6 files changed, 9 insertions(+), 216 deletions(-) delete mode 100644 app/templates/views/providers/edit-provider.html diff --git a/app/main/forms.py b/app/main/forms.py index 127cf8358..6b1af4675 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1740,12 +1740,6 @@ class EstimateUsageForm(StripWhitespaceForm): return super().validate(*args, **kwargs) -class AdminProviderForm(StripWhitespaceForm): - priority = GovukIntegerField( - 'Priority', [validators.NumberRange(min=1, max=100, message="Must be between 1 and 100")] - ) - - class AdminProviderRatioForm(StripWhitespaceForm): ratio = GovukRadiosField(choices=[ diff --git a/app/main/views/providers.py b/app/main/views/providers.py index 15a9a4078..78bdcba89 100644 --- a/app/main/views/providers.py +++ b/app/main/views/providers.py @@ -6,7 +6,7 @@ from werkzeug.utils import redirect from app import provider_client from app.main import main -from app.main.forms import AdminProviderForm, AdminProviderRatioForm +from app.main.forms import AdminProviderRatioForm from app.utils.user import user_is_platform_admin PROVIDER_PRIORITY_MEANING_SWITCHOVER = datetime(2019, 11, 29, 11, 0).isoformat() @@ -43,19 +43,6 @@ def add_monthly_traffic(domestic_sms_providers): provider['monthly_traffic'] = round(percentage) -@main.route("/provider//edit", methods=['GET', 'POST']) -@user_is_platform_admin -def edit_provider(provider_id): - provider = provider_client.get_provider_by_id(provider_id)['provider_details'] - form = AdminProviderForm(active=provider['active'], priority=provider['priority']) - - if form.validate_on_submit(): - provider_client.update_provider(provider_id, form.priority.data) - return redirect(url_for('.view_providers')) - - return render_template('views/providers/edit-provider.html', form=form, provider=provider) - - @main.route("/provider/edit-sms-provider-ratio", methods=['GET', 'POST']) @user_is_platform_admin def edit_sms_provider_ratio(): diff --git a/app/templates/views/providers/edit-provider.html b/app/templates/views/providers/edit-provider.html deleted file mode 100644 index 98c2a7245..000000000 --- a/app/templates/views/providers/edit-provider.html +++ /dev/null @@ -1,37 +0,0 @@ -{% extends "withoutnav_template.html" %} -{% from "components/page-header.html" import page_header %} -{% from "components/page-footer.html" import page_footer %} -{% from "components/form.html" import form_wrapper %} -{% from "components/back-link/macro.njk" import govukBackLink %} - -{% block per_page_title %} - {{provider.display_name}} -{% endblock %} - -{% block backLink %} - {{ govukBackLink({ "href": url_for('.view_providers') }) }} -{% endblock %} - -{% block maincolumn_content %} - -
-
- - {{ page_header(provider.display_name) }} - -

Update provider:

- -
    -
  • We only send from the highest priority provider
    • -
- - {% call form_wrapper() %} - {{ form.priority }} - {{ page_footer('Save') }} - {% endcall %} - -
- -
- -{% endblock %} diff --git a/app/templates/views/providers/providers.html b/app/templates/views/providers/providers.html index 9cf706e1f..0b9bd190e 100644 --- a/app/templates/views/providers/providers.html +++ b/app/templates/views/providers/providers.html @@ -48,14 +48,12 @@ Providers caption="Email providers", caption_visible=False, empty_message='No email providers', - field_headings=['Provider', 'Priority', 'Active', 'Last Updated', 'Updated By'], + field_headings=['Provider', 'Active', 'Last Updated', 'Updated By'], field_headings_visible=True ) %} {{ link_field(item.display_name, url_for('main.view_provider', provider_id=item.id)) }} - {{ text_field(item.priority) }} - {{ text_field(item.active) }} {% if item.updated_at %} @@ -65,9 +63,6 @@ Providers {% endif %} {{ optional_text_field(item.created_by_name, default='None') }} - - {{ link_field('change', url_for('main.edit_provider', provider_id=item.id)) }} - {% endcall %}

International SMS Providers

@@ -77,14 +72,12 @@ Providers caption="International SMS providers", caption_visible=False, empty_message='No international sms providers', - field_headings=['Provider', 'Priority', 'Active', 'Last Updated', 'Updated By'], + field_headings=['Provider', 'Active', 'Last Updated', 'Updated By'], field_headings_visible=True ) %} {{ link_field(item.display_name, url_for('main.view_provider', provider_id=item.id)) }} - {{ text_field(item.priority) }} - {{ text_field(item.active) }} {% if item.updated_at %} diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 25684a3ab..57fc17961 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -4,9 +4,7 @@ from unittest.mock import call import pytest from flask import url_for -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', @@ -107,13 +105,6 @@ def stub_providers(): } -@pytest.fixture -def stub_provider(): - return { - 'provider_details': sms_provider_1 - } - - @pytest.fixture def stub_provider_history(): return { @@ -204,34 +195,27 @@ def test_should_show_all_providers( assert domestic_email_table_data[0].find_all("a")[0]['href'] == '/provider/6005e192-4738-4962-beec-ebd982d0b03a' assert domestic_email_table_data[0].text.strip() == "first_email_provider" - assert domestic_email_table_data[1].text.strip() == "1" - assert domestic_email_table_data[2].text.strip() == "True" + assert domestic_email_table_data[1].text.strip() == "True" + assert domestic_email_table_data[2].text.strip() == "None" assert domestic_email_table_data[3].text.strip() == "None" - assert domestic_email_table_data[4].text.strip() == "None" - assert domestic_email_table_data[5].find_all("a")[0]['href'] \ - == '/provider/6005e192-4738-4962-beec-ebd982d0b03a/edit' domestic_email_second_row = domestic_email_table.tbody.find_all('tr')[1] domestic_email_table_data = domestic_email_second_row.find_all('td') assert domestic_email_table_data[0].find_all("a")[0]['href'] == '/provider/0bd529cd-a0fd-43e5-80ee-b95ef6b0d51b' assert domestic_email_table_data[0].text.strip() == "second_email_provider" - assert domestic_email_table_data[1].text.strip() == "2" - assert domestic_email_table_data[2].text.strip() == "True" + assert domestic_email_table_data[1].text.strip() == "True" + assert domestic_email_table_data[2].text.strip() == "None" assert domestic_email_table_data[3].text.strip() == "None" - assert domestic_email_table_data[4].text.strip() == "None" - assert domestic_email_table_data[5].find_all("a")[0]['href'] \ - == '/provider/0bd529cd-a0fd-43e5-80ee-b95ef6b0d51b/edit' international_sms_first_row = international_sms_table.tbody.find_all('tr')[0] table_data = international_sms_first_row.find_all('td') assert table_data[0].find_all("a")[0]['href'] == '/provider/67c770f5-918e-4afa-a5ff-880b9beb161d' assert table_data[0].text.strip() == "First International SMS Provider" - assert table_data[1].text.strip() == "10" - assert table_data[2].text.strip() == "False" + assert table_data[1].text.strip() == "False" + assert table_data[2].text.strip() == "None" assert table_data[3].text.strip() == "None" - assert table_data[4].text.strip() == "None" def test_add_monthly_traffic(): @@ -263,133 +247,6 @@ def test_add_monthly_traffic(): }] -def test_should_show_edit_provider_form( - client_request, - platform_admin_user, - mocker, - fake_uuid, - 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) - - h1 = [header.text.strip() for header in page.find_all('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'] == '20' - assert form_elements[0]['name'] == 'priority' - - -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=stub_provider) - - client_request.login(platform_admin_user) - page = client_request.post( - 'main.edit_provider', - provider_id=stub_provider['provider_details']['id'], - _data={'priority': "not valid"}, - _expected_status=200, - ) - - assert normalize_spaces( - page.select_one('.govuk-error-message').text - ) == "Error: Not a valid integer value." - - -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=stub_provider) - - client_request.login(platform_admin_user) - page = client_request.post( - 'main.edit_provider', - provider_id=stub_provider['provider_details']['id'], - _data={'priority': -1}, - _expected_status=200, - ) - - assert normalize_spaces( - page.select_one('.govuk-error-message').text - ) == "Error: Must be between 1 and 100" - - -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=stub_provider) - - client_request.login(platform_admin_user) - page = client_request.post( - 'main.edit_provider', - provider_id=stub_provider['provider_details']['id'], - _data={'priority': 101}, - _expected_status=200, - ) - - assert normalize_spaces( - page.select_one('.govuk-error-message').text - ) == "Error: Must be between 1 and 100" - - -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=stub_provider) - - client_request.login(platform_admin_user) - page = client_request.post( - 'main.edit_provider', - provider_id=stub_provider['provider_details']['id'], - _data={'priority': 0}, - _expected_status=200, - ) - - assert normalize_spaces( - page.select_one('.govuk-error-message').text - ) == "Error: Must be between 1 and 100" - - -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=stub_provider) - mocker.patch('app.provider_client.update_provider', return_value=stub_provider) - - client_request.login(platform_admin_user) - client_request.post( - 'main.edit_provider', - provider_id=stub_provider['provider_details']['id'], - _data={'priority': 2}, - _expected_redirect='http://localhost/providers', - ) - - app.provider_client.update_provider.assert_called_with(stub_provider['provider_details']['id'], 2) - - def test_should_show_provider_version_history( client_request, platform_admin_user, diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index 06eccdd28..25735da28 100644 --- a/tests/app/test_navigation.py +++ b/tests/app/test_navigation.py @@ -102,7 +102,6 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, { 'edit_organisation_notes', 'edit_organisation_type', 'edit_organisation_user', - 'edit_provider', 'edit_service_billing_details', 'edit_service_notes', 'edit_service_template', From 01389b5edfc77657e778d45ed465cb34f9e88dcb Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 15:01:46 +0100 Subject: [PATCH 05/17] Rename provider tests to match endpoint under test This makes it clearer which tests we have for what. --- tests/app/main/views/test_providers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 57fc17961..e1f10b5cd 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -141,7 +141,7 @@ def stub_provider_history(): } -def test_should_show_all_providers( +def test_view_providers_shows_all_providers( client_request, platform_admin_user, mocker, @@ -247,7 +247,7 @@ def test_add_monthly_traffic(): }] -def test_should_show_provider_version_history( +def test_view_provider_shows_version_history( client_request, platform_admin_user, mocker, @@ -304,7 +304,7 @@ def test_should_show_provider_version_history( ], ), ]) -def test_should_update_priority_of_first_two_sms_providers( +def test_edit_sms_provider_ratio_should_update_priority_of_first_two_sms_providers( client_request, platform_admin_user, mocker, From 7f333ba5fe0e570284d47c55aa0b7b0c082558d1 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 15:08:28 +0100 Subject: [PATCH 06/17] Rewrite SMS ratio form to cope with 3 providers This replaces the slider with an integer input for each provider. Unfortunately showing a variable number of inputs isn't easy to achieve in WTForms [^1], but we think this is the least worst way to do it vs e.g. not using WTForms at all. [^1]: https://github.com/wtforms/wtforms/issues/736 --- app/assets/stylesheets/views/history.scss | 63 ------------------- app/main/forms.py | 37 +++++------ app/main/views/providers.py | 12 ++-- .../providers/edit-sms-provider-ratio.html | 46 ++++---------- tests/app/main/views/test_providers.py | 20 +++--- 5 files changed, 46 insertions(+), 132 deletions(-) diff --git a/app/assets/stylesheets/views/history.scss b/app/assets/stylesheets/views/history.scss index a8d16514e..0a0ed11d1 100644 --- a/app/assets/stylesheets/views/history.scss +++ b/app/assets/stylesheets/views/history.scss @@ -29,67 +29,4 @@ $item-top-padding: govuk-spacing(3); display: block; color: $secondary-text-colour; } - - &-percentage { - - $axis-thickness: 2px; - border-bottom: $axis-thickness solid $black; - position: relative; - margin-bottom: 35px; - height: 35px; - - &:before, - &:after { - content: ""; - position: absolute; - bottom: -6px; - left: 0; - height: 10px; - width: $axis-thickness; - background: $black; - } - - &:after { - left: auto; - right: 0; - } - - &-without-border { - position: relative; - padding-top: 35px; - } - - &-marker { - $size: 20px; - $border-thickness: 7px; - $text-width: 100px; - display: block; - width: $size; - height: $size; - position: absolute; - top: 35px - ($size / 2) - ($border-thickness - ($axis-thickness / 2)); - margin-left: 0 - ($size / 2) - $border-thickness; - background: $black; - border-radius: $size; - border: $border-thickness solid $white; - } - - &-left-label, - &-right-label { - width: 100%; - position: absolute; - top: 0; - } - - &-left-label { - left: 0; - } - - &-right-label { - right: 0; - text-align: right; - } - - } - } diff --git a/app/main/forms.py b/app/main/forms.py index 6b1af4675..403e3317c 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1740,29 +1740,26 @@ class EstimateUsageForm(StripWhitespaceForm): return super().validate(*args, **kwargs) -class AdminProviderRatioForm(StripWhitespaceForm): +class AdminProviderRatioForm(Form): + def __init__(self, providers): + # hack: https://github.com/wtforms/wtforms/issues/736 + self._unbound_fields = [ + ( + provider['identifier'], + GovukIntegerField( + provider['display_name'], + validators=[validators.NumberRange( + min=0, max=100, message="Must be between 0 and 100" + )] + ) + ) for provider in providers + ] - ratio = GovukRadiosField(choices=[ - (str(value), '{}% / {}%'.format(value, 100 - value)) - for value in range(100, -10, -10) - ], - param_extensions={ - "classes": "govuk-radios--inline", - "fieldset": { - "legend": { - "classes": "govuk-visually-hidden" - } - } + super().__init__(data={ + provider['identifier']: provider['priority'] + for provider in providers }) - @property - def percentage_left(self): - return int(self.ratio.data) - - @property - def percentage_right(self): - return 100 - self.percentage_left - class ServiceContactDetailsForm(StripWhitespaceForm): contact_details_type = RadioField( diff --git a/app/main/views/providers.py b/app/main/views/providers.py index 78bdcba89..80e8fbe0e 100644 --- a/app/main/views/providers.py +++ b/app/main/views/providers.py @@ -54,23 +54,21 @@ def edit_sms_provider_ratio(): and provider['active'] ], key=itemgetter('identifier')) - form = AdminProviderRatioForm(ratio=providers[0]['priority']) + form = AdminProviderRatioForm(providers) if len(providers) < 2: abort(400) - first_provider, second_provider = providers[0:2] - if form.validate_on_submit(): - provider_client.update_provider(first_provider['id'], form.percentage_left) - provider_client.update_provider(second_provider['id'], form.percentage_right) + for provider in providers: + field = getattr(form, provider['identifier']) + provider_client.update_provider(provider['id'], field.data) return redirect(url_for('.view_providers')) return render_template( 'views/providers/edit-sms-provider-ratio.html', form=form, - first_provider=providers[0]['display_name'], - second_provider=providers[1]['display_name'], + providers=providers ) diff --git a/app/templates/views/providers/edit-sms-provider-ratio.html b/app/templates/views/providers/edit-sms-provider-ratio.html index bbe3b0d04..53485cc26 100644 --- a/app/templates/views/providers/edit-sms-provider-ratio.html +++ b/app/templates/views/providers/edit-sms-provider-ratio.html @@ -4,44 +4,22 @@ {% from "components/form.html" import form_wrapper %} {% block per_page_title %} - Text message providers + Edit SMS provider priorities {% endblock %} {% block platform_admin_content %} - {{ page_header('Text message providers') }} + {{ page_header('Edit SMS provider priorities') }} +

+ If you change one priority you will need to change the others so they add up to 100%. +

-

- Now -

-
-
-
-
 
-
-
-
- {{ first_provider }} -
-
- {{ second_provider }} -
- {% call form_wrapper() %} -
- {{ form.ratio }} -
- — -
-
- — -
-
- {{ page_footer('Update', centered_button=True) }} - {% endcall %} -
-
-
-
-
+ {% call form_wrapper() %} + {% for field in form %} + {{ field }} + {% endfor %} + + {{ page_footer('Save') }} + {% endcall %} {% endblock %} diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index e1f10b5cd..1770493ff 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -288,27 +288,33 @@ def test_view_provider_shows_version_history( assert second_row[4].text.strip() == "True" -@pytest.mark.parametrize('posted_number, expected_calls', [ +@pytest.mark.parametrize('post_data, expected_calls', [ ( - '10', + { + sms_provider_1['identifier']: 10, + sms_provider_2['identifier']: 90 + }, [ call(sms_provider_1['id'], 10), call(sms_provider_2['id'], 90), ], ), ( - '80', + { + sms_provider_1['identifier']: 80, + sms_provider_2['identifier']: 20 + }, [ call(sms_provider_1['id'], 80), call(sms_provider_2['id'], 20), ], ), ]) -def test_edit_sms_provider_ratio_should_update_priority_of_first_two_sms_providers( +def test_edit_sms_provider_ratio_submit( client_request, platform_admin_user, mocker, - posted_number, + post_data, expected_calls, stub_providers, ): @@ -323,9 +329,7 @@ def test_edit_sms_provider_ratio_should_update_priority_of_first_two_sms_provide client_request.login(platform_admin_user) client_request.post( '.edit_sms_provider_ratio', - _data={ - 'ratio': posted_number, - }, + _data=post_data, _expected_redirect=url_for( '.view_providers', _external=True, From 55e89b3f12d7964f118b800138e783c2f6cb6e59 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 16:54:09 +0100 Subject: [PATCH 07/17] Add validation for provider SMS percentages We could alternatively put the "add up to 100%" error on the page using form-level errors [^1] and a custom flash message. Putting the error on each field is slightly simpler and does make it clear the issue is with all of the fields together. [^1]: https://github.com/wtforms/wtforms/commit/22636b55eda9300b549c8bbaae6f9ae31595d445 --- app/main/forms.py | 21 +++++++++++++ tests/app/main/views/test_providers.py | 43 ++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/app/main/forms.py b/app/main/forms.py index 403e3317c..243534d9b 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1742,6 +1742,8 @@ class EstimateUsageForm(StripWhitespaceForm): class AdminProviderRatioForm(Form): def __init__(self, providers): + self._providers = providers + # hack: https://github.com/wtforms/wtforms/issues/736 self._unbound_fields = [ ( @@ -1760,6 +1762,25 @@ class AdminProviderRatioForm(Form): for provider in providers }) + def validate(self): + if not super().validate(): + return False + + total = sum( + getattr(self, provider['identifier']).data + for provider in self._providers + ) + + if total == 100: + return True + + for provider in self._providers: + getattr(self, provider['identifier']).errors += [ + 'Must add up to 100%' + ] + + return False + class ServiceContactDetailsForm(StripWhitespaceForm): contact_details_type = RadioField( diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 1770493ff..aefacdffc 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -337,3 +337,46 @@ def test_edit_sms_provider_ratio_submit( ) assert mock_update_provider.call_args_list == expected_calls + + +@pytest.mark.parametrize('post_data, expected_error', [ + ( + { + sms_provider_1['identifier']: 90, + sms_provider_2['identifier']: 20 + }, + "Must add up to 100%" + ), + ( + { + sms_provider_1['identifier']: 101, + sms_provider_2['identifier']: 20 + }, + "Must be between 0 and 100" + ), +]) +def test_edit_sms_provider_submit_invalid_percentages( + client_request, + platform_admin_user, + mocker, + post_data, + expected_error, + stub_providers, +): + mocker.patch( + 'app.provider_client.get_all_providers', + return_value=stub_providers + ) + mock_update_provider = mocker.patch( + 'app.provider_client.update_provider' + ) + + client_request.login(platform_admin_user) + page = client_request.post( + '.edit_sms_provider_ratio', + _data=post_data, + _follow_redirects=True + ) + + assert expected_error in page.select_one('.govuk-error-message').text + mock_update_provider.assert_not_called() From c4e1f40b431fd837593a373274516318cd609327 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 17:04:08 +0100 Subject: [PATCH 08/17] Add missing test for viewing SMS ratio edit page --- tests/app/main/views/test_providers.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index aefacdffc..e0af6efa1 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -288,6 +288,29 @@ def test_view_provider_shows_version_history( assert second_row[4].text.strip() == "True" +def test_edit_sms_provider_provider_ratio( + client_request, + platform_admin_user, + mocker, + stub_providers, +): + mocker.patch( + 'app.provider_client.get_all_providers', + return_value=stub_providers + ) + + client_request.login(platform_admin_user) + page = client_request.get( + '.edit_sms_provider_ratio', + ) + + inputs = page.select('.govuk-input[type="text"]') + assert len(inputs) == 2 + + first_input = page.select_one('.govuk-input[name="first_sms_domestic"]') + assert first_input.attrs['value'] == str(sms_provider_1['priority']) + + @pytest.mark.parametrize('post_data, expected_calls', [ ( { From dd0022968c845d48fc309b35e65b80c58b77c409 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 17:05:05 +0100 Subject: [PATCH 09/17] Remove redundant constraint on multiple providers We can now handle a single provider (with a priority of 100%). I don't think we need to handle the case of no providers. --- app/main/views/providers.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/main/views/providers.py b/app/main/views/providers.py index 80e8fbe0e..2f2f4a40a 100644 --- a/app/main/views/providers.py +++ b/app/main/views/providers.py @@ -1,7 +1,7 @@ from datetime import datetime from operator import itemgetter -from flask import abort, render_template, url_for +from flask import render_template, url_for from werkzeug.utils import redirect from app import provider_client @@ -56,9 +56,6 @@ def edit_sms_provider_ratio(): form = AdminProviderRatioForm(providers) - if len(providers) < 2: - abort(400) - if form.validate_on_submit(): for provider in providers: field = getattr(form, provider['identifier']) From 264bf8302f31d8182dcdeadf2ea87284f423f0f9 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 17:07:02 +0100 Subject: [PATCH 10/17] Remove redundant sorting when editing SMS ratios This is irrelevant since we no longer pick a "first" provider. --- app/main/views/providers.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/app/main/views/providers.py b/app/main/views/providers.py index 2f2f4a40a..08dfa0060 100644 --- a/app/main/views/providers.py +++ b/app/main/views/providers.py @@ -1,5 +1,4 @@ from datetime import datetime -from operator import itemgetter from flask import render_template, url_for from werkzeug.utils import redirect @@ -46,13 +45,11 @@ def add_monthly_traffic(domestic_sms_providers): @main.route("/provider/edit-sms-provider-ratio", methods=['GET', 'POST']) @user_is_platform_admin def edit_sms_provider_ratio(): - - providers = sorted([ + providers = [ provider for provider in provider_client.get_all_providers()['provider_details'] - if provider['notification_type'] == 'sms' - and provider['active'] - ], key=itemgetter('identifier')) + if provider['notification_type'] == 'sms' and provider['active'] + ] form = AdminProviderRatioForm(providers) From 7f52e8dedf33543b2a60528b5112a893ec77094c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 18:02:07 +0100 Subject: [PATCH 11/17] Add missing test for editing inactive providers --- tests/app/main/views/test_providers.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index e0af6efa1..f4af14eae 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -1,3 +1,4 @@ +import copy from datetime import datetime from unittest.mock import call @@ -311,6 +312,31 @@ def test_edit_sms_provider_provider_ratio( assert first_input.attrs['value'] == str(sms_provider_1['priority']) +def test_edit_sms_provider_provider_ratio_only_shows_active_providers( + client_request, + platform_admin_user, + mocker, + stub_providers, +): + sms_provider_1_inactive = copy.deepcopy(sms_provider_1) + sms_provider_1_inactive['active'] = False + + mocker.patch( + 'app.provider_client.get_all_providers', + return_value={ + 'provider_details': [sms_provider_1_inactive, sms_provider_2] + } + ) + + client_request.login(platform_admin_user) + page = client_request.get( + '.edit_sms_provider_ratio', + ) + + inputs = page.select('.govuk-input[type="text"]') + assert len(inputs) == 1 + + @pytest.mark.parametrize('post_data, expected_calls', [ ( { From 0437e4a2483356f2d9d573802e782c1a3ec66c6c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 18:14:43 +0100 Subject: [PATCH 12/17] Standardise appearance of missing data This makes the rendering the same as for "created_by_name" when the data isn't present. It's a bit more complicated for "updated_at" so I checked that it's implicitly covered by the tests, which fail if I remove the "if" conditional for any of these fields. --- app/templates/views/providers/providers.html | 27 +++++++++----------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/app/templates/views/providers/providers.html b/app/templates/views/providers/providers.html index 0b9bd190e..37efad72b 100644 --- a/app/templates/views/providers/providers.html +++ b/app/templates/views/providers/providers.html @@ -31,11 +31,10 @@ Providers {{ text_field(item.active) }} - {% if item.updated_at %} - {{ text_field(item.updated_at|format_datetime_short) }} - {% else %} - {{ text_field('None') }} - {% endif %} + {{ optional_text_field( + item.updated_at|format_datetime_short if item.updated_at, + default='None' + ) }} {{ optional_text_field(item.created_by_name, default='None') }} @@ -56,11 +55,10 @@ Providers {{ text_field(item.active) }} - {% if item.updated_at %} - {{ text_field(item.updated_at|format_datetime_short) }} - {% else %} - {{ text_field('None') }} - {% endif %} + {{ optional_text_field( + item.updated_at|format_datetime_short if item.updated_at, + default='None' + ) }} {{ optional_text_field(item.created_by_name, default='None') }} {% endcall %} @@ -80,11 +78,10 @@ Providers {{ text_field(item.active) }} - {% if item.updated_at %} - {{ text_field(item.updated_at|format_datetime_short) }} - {% else %} - {{ text_field('None') }} - {% endif %} + {{ optional_text_field( + item.updated_at|format_datetime_short if item.updated_at, + default='None' + ) }} {{ optional_text_field(item.created_by_name, default='None') }} From efea27d4339bf1a46c99e5e40148187d465fa446 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 18:15:19 +0100 Subject: [PATCH 13/17] Don't show priority for inactive providers This is consistent with not being able to set it either. I think it's OK to not have a test for this as it's purely cosmetic. --- app/templates/views/providers/providers.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/providers/providers.html b/app/templates/views/providers/providers.html index 37efad72b..3874cd0bd 100644 --- a/app/templates/views/providers/providers.html +++ b/app/templates/views/providers/providers.html @@ -25,7 +25,7 @@ Providers {{ link_field(item.display_name, url_for('main.view_provider', provider_id=item.id)) }} - {{ text_field(item.priority) }} + {{ optional_text_field(item.priority if item.active, default='None') }} {{ text_field(item.monthly_traffic) }} From 6d77ff54b18c69e429c6f3c22429cb9a7e8cc087 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 7 Apr 2022 13:07:32 +0100 Subject: [PATCH 14/17] Standardise provider psuedo fixture data Previously it was hard to distinguish ids from identifiers from display names (in the case of the email fixtures). Using predictable attributes will also make it easier to convert this dictionaries to proper fixtures. --- tests/app/main/views/test_providers.py | 82 +++++++++++++------------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index f4af14eae..93f6ea58d 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -8,11 +8,11 @@ from flask import url_for from app.main.views.providers import add_monthly_traffic sms_provider_1 = { - 'id': '6005e192-4738-4962-beec-ebd982d0b03f', + 'id': 'sms_provider_1-id', 'active': True, 'priority': 20, - 'display_name': 'First Domestic SMS Provider', - 'identifier': 'first_sms_domestic', + 'display_name': 'SMS Provider 1', + 'identifier': 'sms_provider_1', 'notification_type': 'sms', 'updated_at': datetime(2017, 1, 16, 15, 20, 40).isoformat(), 'version': 1, @@ -22,11 +22,11 @@ sms_provider_1 = { } sms_provider_2 = { - 'id': '0bd529cd-a0fd-43e5-80ee-b95ef6b0d51f', + 'id': 'sms_provider_2-id', 'active': True, 'priority': 10, - 'display_name': 'Second Domestic SMS Provider', - 'identifier': 'second_sms_domestic', + 'display_name': 'SMS Provider 2', + 'identifier': 'sms_provider_2', 'notification_type': 'sms', 'updated_at': None, 'version': 1, @@ -36,11 +36,11 @@ sms_provider_2 = { } email_provider_1 = { - 'id': '6005e192-4738-4962-beec-ebd982d0b03a', + 'id': 'email_provider_1-id', 'active': True, 'priority': 1, - 'display_name': 'first_email_provider', - 'identifier': 'first_email', + 'display_name': 'Email Provider 1', + 'identifier': 'email_provider_1', 'notification_type': 'email', 'updated_at': None, 'version': 1, @@ -50,11 +50,11 @@ email_provider_1 = { } email_provider_2 = { + 'id': 'email_provider_2-id', 'active': True, 'priority': 2, - 'display_name': 'second_email_provider', - 'identifier': 'second_email', - 'id': '0bd529cd-a0fd-43e5-80ee-b95ef6b0d51b', + 'display_name': 'Email Provider 2', + 'identifier': 'email_provider_2', 'notification_type': 'email', 'updated_at': None, 'version': 1, @@ -64,11 +64,11 @@ email_provider_2 = { } sms_provider_intl_1 = { - 'id': '67c770f5-918e-4afa-a5ff-880b9beb161d', + 'id': 'sms_provider_intl_1-id', 'active': False, 'priority': 10, - 'display_name': 'First International SMS Provider', - 'identifier': 'first_sms_international', + 'display_name': 'SMS Provider Intl 1', + 'identifier': 'sms_provider_intl_1', 'notification_type': 'sms', 'updated_at': None, 'version': 1, @@ -78,11 +78,11 @@ sms_provider_intl_1 = { } sms_provider_intl_2 = { - 'id': '67c770f5-918e-4afa-a5ff-880b9beb161d', + 'id': 'sms_provider_intl_2-id', 'active': False, 'priority': 10, - 'display_name': 'Second International SMS Provider', - 'identifier': 'second_sms_international', + 'display_name': 'SMS Provider Intl 2', + 'identifier': 'sms_provider_intl_2', 'notification_type': 'sms', 'updated_at': None, 'version': 1, @@ -172,8 +172,8 @@ def test_view_providers_shows_all_providers( domestic_sms_first_row = domestic_sms_table.tbody.find_all('tr')[0] 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() == "First Domestic SMS Provider" + assert table_data[0].find_all("a")[0]['href'] == '/provider/sms_provider_1-id' + assert table_data[0].text.strip() == "SMS Provider 1" assert table_data[1].text.strip() == "20" assert table_data[2].text.strip() == "42" assert table_data[3].text.strip() == "True" @@ -183,8 +183,8 @@ def test_view_providers_shows_all_providers( domestic_sms_second_row = domestic_sms_table.tbody.find_all('tr')[1] table_data = domestic_sms_second_row.find_all('td') - 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[0].find_all("a")[0]['href'] == '/provider/sms_provider_2-id' + assert table_data[0].text.strip() == "SMS Provider 2" assert table_data[1].text.strip() == "10" assert table_data[2].text.strip() == "58" assert table_data[3].text.strip() == "True" @@ -194,8 +194,8 @@ def test_view_providers_shows_all_providers( domestic_email_first_row = domestic_email_table.tbody.find_all('tr')[0] domestic_email_table_data = domestic_email_first_row.find_all('td') - assert domestic_email_table_data[0].find_all("a")[0]['href'] == '/provider/6005e192-4738-4962-beec-ebd982d0b03a' - assert domestic_email_table_data[0].text.strip() == "first_email_provider" + assert domestic_email_table_data[0].find_all("a")[0]['href'] == '/provider/email_provider_1-id' + assert domestic_email_table_data[0].text.strip() == "Email Provider 1" assert domestic_email_table_data[1].text.strip() == "True" assert domestic_email_table_data[2].text.strip() == "None" assert domestic_email_table_data[3].text.strip() == "None" @@ -203,8 +203,8 @@ def test_view_providers_shows_all_providers( domestic_email_second_row = domestic_email_table.tbody.find_all('tr')[1] domestic_email_table_data = domestic_email_second_row.find_all('td') - assert domestic_email_table_data[0].find_all("a")[0]['href'] == '/provider/0bd529cd-a0fd-43e5-80ee-b95ef6b0d51b' - assert domestic_email_table_data[0].text.strip() == "second_email_provider" + assert domestic_email_table_data[0].find_all("a")[0]['href'] == '/provider/email_provider_2-id' + assert domestic_email_table_data[0].text.strip() == "Email Provider 2" assert domestic_email_table_data[1].text.strip() == "True" assert domestic_email_table_data[2].text.strip() == "None" assert domestic_email_table_data[3].text.strip() == "None" @@ -212,8 +212,8 @@ def test_view_providers_shows_all_providers( international_sms_first_row = international_sms_table.tbody.find_all('tr')[0] table_data = international_sms_first_row.find_all('td') - assert table_data[0].find_all("a")[0]['href'] == '/provider/67c770f5-918e-4afa-a5ff-880b9beb161d' - assert table_data[0].text.strip() == "First International SMS Provider" + assert table_data[0].find_all("a")[0]['href'] == '/provider/sms_provider_intl_1-id' + assert table_data[0].text.strip() == "SMS Provider Intl 1" assert table_data[1].text.strip() == "False" assert table_data[2].text.strip() == "None" assert table_data[3].text.strip() == "None" @@ -308,7 +308,7 @@ def test_edit_sms_provider_provider_ratio( inputs = page.select('.govuk-input[type="text"]') assert len(inputs) == 2 - first_input = page.select_one('.govuk-input[name="first_sms_domestic"]') + first_input = page.select_one('.govuk-input[name="sms_provider_1"]') assert first_input.attrs['value'] == str(sms_provider_1['priority']) @@ -340,22 +340,22 @@ def test_edit_sms_provider_provider_ratio_only_shows_active_providers( @pytest.mark.parametrize('post_data, expected_calls', [ ( { - sms_provider_1['identifier']: 10, - sms_provider_2['identifier']: 90 + 'sms_provider_1': 10, + 'sms_provider_2': 90 }, [ - call(sms_provider_1['id'], 10), - call(sms_provider_2['id'], 90), + call('sms_provider_1-id', 10), + call('sms_provider_2-id', 90), ], ), ( { - sms_provider_1['identifier']: 80, - sms_provider_2['identifier']: 20 + 'sms_provider_1': 80, + 'sms_provider_2': 20 }, [ - call(sms_provider_1['id'], 80), - call(sms_provider_2['id'], 20), + call('sms_provider_1-id', 80), + call('sms_provider_2-id', 20), ], ), ]) @@ -391,15 +391,15 @@ def test_edit_sms_provider_ratio_submit( @pytest.mark.parametrize('post_data, expected_error', [ ( { - sms_provider_1['identifier']: 90, - sms_provider_2['identifier']: 20 + 'sms_provider_1': 90, + 'sms_provider_2': 20 }, "Must add up to 100%" ), ( { - sms_provider_1['identifier']: 101, - sms_provider_2['identifier']: 20 + 'sms_provider_1': 101, + 'sms_provider_2': 20 }, "Must be between 0 and 100" ), From 05c396434d4782fbb52e2ff050fbaba2a27ce97a Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 17:30:35 +0100 Subject: [PATCH 15/17] Use fixtures for SMS provider test data This avoids the need to manually copy psuedo fixture data to avoid breaking other tests. --- tests/app/main/views/test_providers.py | 201 ++++++++++++++----------- 1 file changed, 112 insertions(+), 89 deletions(-) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 93f6ea58d..274a966d3 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 @@ -7,93 +6,118 @@ from flask import url_for from app.main.views.providers import add_monthly_traffic -sms_provider_1 = { - 'id': 'sms_provider_1-id', - 'active': True, - 'priority': 20, - 'display_name': 'SMS Provider 1', - 'identifier': 'sms_provider_1', - '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': 'sms_provider_2-id', - 'active': True, - 'priority': 10, - 'display_name': 'SMS Provider 2', - 'identifier': 'sms_provider_2', - 'notification_type': 'sms', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': False, - 'current_month_billable_sms': 6891, -} - -email_provider_1 = { - 'id': 'email_provider_1-id', - 'active': True, - 'priority': 1, - 'display_name': 'Email Provider 1', - 'identifier': 'email_provider_1', - 'notification_type': 'email', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': False, - 'current_month_billable_sms': 0, -} - -email_provider_2 = { - 'id': 'email_provider_2-id', - 'active': True, - 'priority': 2, - 'display_name': 'Email Provider 2', - 'identifier': 'email_provider_2', - 'notification_type': 'email', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': False, - 'current_month_billable_sms': 0, -} - -sms_provider_intl_1 = { - 'id': 'sms_provider_intl_1-id', - 'active': False, - 'priority': 10, - 'display_name': 'SMS Provider Intl 1', - 'identifier': 'sms_provider_intl_1', - 'notification_type': 'sms', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': True, - 'current_month_billable_sms': 0, -} - -sms_provider_intl_2 = { - 'id': 'sms_provider_intl_2-id', - 'active': False, - 'priority': 10, - 'display_name': 'SMS Provider Intl 2', - 'identifier': 'sms_provider_intl_2', - 'notification_type': 'sms', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': True, - 'current_month_billable_sms': 0, -} +@pytest.fixture +def sms_provider_1(): + return { + 'id': 'sms_provider_1-id', + 'active': True, + 'priority': 20, + 'display_name': 'SMS Provider 1', + 'identifier': 'sms_provider_1', + '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, + } @pytest.fixture -def stub_providers(): +def sms_provider_2(): + return { + 'id': 'sms_provider_2-id', + 'active': True, + 'priority': 10, + 'display_name': 'SMS Provider 2', + 'identifier': 'sms_provider_2', + 'notification_type': 'sms', + 'updated_at': None, + 'version': 1, + 'created_by': None, + 'supports_international': False, + 'current_month_billable_sms': 6891, + } + + +@pytest.fixture +def email_provider_1(): + return { + 'id': 'email_provider_1-id', + 'active': True, + 'priority': 1, + 'display_name': 'Email Provider 1', + 'identifier': 'email_provider_1', + 'notification_type': 'email', + 'updated_at': None, + 'version': 1, + 'created_by': None, + 'supports_international': False, + 'current_month_billable_sms': 0, + } + + +@pytest.fixture +def email_provider_2(): + return { + 'id': 'email_provider_2-id', + 'active': True, + 'priority': 2, + 'display_name': 'Email Provider 2', + 'identifier': 'email_provider_2', + 'notification_type': 'email', + 'updated_at': None, + 'version': 1, + 'created_by': None, + 'supports_international': False, + 'current_month_billable_sms': 0, + } + + +@pytest.fixture +def sms_provider_intl_1(): + return { + 'id': 'sms_provider_intl_1-id', + 'active': False, + 'priority': 10, + 'display_name': 'SMS Provider Intl 1', + 'identifier': 'sms_provider_intl_1', + 'notification_type': 'sms', + 'updated_at': None, + 'version': 1, + 'created_by': None, + 'supports_international': True, + 'current_month_billable_sms': 0, + } + + +@pytest.fixture +def sms_provider_intl_2(): + return { + 'id': 'sms_provider_intl_2-id', + 'active': False, + 'priority': 10, + 'display_name': 'SMS Provider Intl 2', + 'identifier': 'sms_provider_intl_2', + 'notification_type': 'sms', + 'updated_at': None, + 'version': 1, + 'created_by': None, + 'supports_international': True, + 'current_month_billable_sms': 0, + } + + +@pytest.fixture +def stub_providers( + sms_provider_1, + sms_provider_2, + email_provider_1, + email_provider_2, + sms_provider_intl_1, + sms_provider_intl_2, +): return { 'provider_details': [ sms_provider_1, @@ -294,6 +318,7 @@ def test_edit_sms_provider_provider_ratio( platform_admin_user, mocker, stub_providers, + sms_provider_1 ): mocker.patch( 'app.provider_client.get_all_providers', @@ -317,15 +342,13 @@ def test_edit_sms_provider_provider_ratio_only_shows_active_providers( platform_admin_user, mocker, stub_providers, + sms_provider_1, ): - sms_provider_1_inactive = copy.deepcopy(sms_provider_1) - sms_provider_1_inactive['active'] = False + sms_provider_1['active'] = False mocker.patch( 'app.provider_client.get_all_providers', - return_value={ - 'provider_details': [sms_provider_1_inactive, sms_provider_2] - } + return_value=stub_providers, ) client_request.login(platform_admin_user) From 9132004ea3c47cd95028da92d20e45186f8f5444 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 17:39:07 +0100 Subject: [PATCH 16/17] DRY-up creating provider fixtures --- tests/app/main/views/test_providers.py | 75 +++++++++++--------------- 1 file changed, 31 insertions(+), 44 deletions(-) diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 274a966d3..8a69e2178 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -7,106 +7,93 @@ from flask import url_for from app.main.views.providers import add_monthly_traffic +def provider_json(overrides): + provider = { + 'id': 'override-me', + 'active': True, + 'priority': 20, + 'display_name': 'Provider', + 'identifier': 'override-me', + 'notification_type': 'sms', + 'updated_at': None, + 'version': 1, + 'created_by_name': None, + 'supports_international': False, + 'current_month_billable_sms': 0, + } + + provider.update(**overrides) + return provider + + @pytest.fixture def sms_provider_1(): - return { + return provider_json({ 'id': 'sms_provider_1-id', - 'active': True, 'priority': 20, 'display_name': 'SMS Provider 1', 'identifier': 'sms_provider_1', '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, - } + }) @pytest.fixture def sms_provider_2(): - return { + return provider_json({ 'id': 'sms_provider_2-id', - 'active': True, 'priority': 10, 'display_name': 'SMS Provider 2', 'identifier': 'sms_provider_2', 'notification_type': 'sms', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': False, 'current_month_billable_sms': 6891, - } + }) @pytest.fixture def email_provider_1(): - return { + return provider_json({ 'id': 'email_provider_1-id', - 'active': True, - 'priority': 1, 'display_name': 'Email Provider 1', 'identifier': 'email_provider_1', 'notification_type': 'email', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': False, - 'current_month_billable_sms': 0, - } + }) @pytest.fixture def email_provider_2(): - return { + return provider_json({ 'id': 'email_provider_2-id', - 'active': True, - 'priority': 2, 'display_name': 'Email Provider 2', 'identifier': 'email_provider_2', 'notification_type': 'email', - 'updated_at': None, - 'version': 1, - 'created_by': None, - 'supports_international': False, - 'current_month_billable_sms': 0, - } + }) @pytest.fixture def sms_provider_intl_1(): - return { + return provider_json({ 'id': 'sms_provider_intl_1-id', 'active': False, - 'priority': 10, 'display_name': 'SMS Provider Intl 1', 'identifier': 'sms_provider_intl_1', 'notification_type': 'sms', - 'updated_at': None, - 'version': 1, - 'created_by': None, 'supports_international': True, - 'current_month_billable_sms': 0, - } + }) @pytest.fixture def sms_provider_intl_2(): - return { + return provider_json({ 'id': 'sms_provider_intl_2-id', 'active': False, - 'priority': 10, 'display_name': 'SMS Provider Intl 2', 'identifier': 'sms_provider_intl_2', 'notification_type': 'sms', - 'updated_at': None, - 'version': 1, - 'created_by': None, 'supports_international': True, - 'current_month_billable_sms': 0, - } + }) @pytest.fixture From 355a74d2023e6b029f7d6d4cea1ff0ae196f79a6 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 11 Apr 2022 14:02:04 +0100 Subject: [PATCH 17/17] Make SMS provider inputs easier to interpret In response to: [^1]. [^1]: https://github.com/alphagov/notifications-admin/pull/4205#discussion_r847204574 --- app/main/forms.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 243534d9b..9f356b61b 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1749,10 +1749,13 @@ class AdminProviderRatioForm(Form): ( provider['identifier'], GovukIntegerField( - provider['display_name'], + f"{provider['display_name']} (%)", validators=[validators.NumberRange( min=0, max=100, message="Must be between 0 and 100" - )] + )], + param_extensions={ + 'classes': "govuk-input--width-3", + } ) ) for provider in providers ]