From 8655ab7deab70a12b9d40bc77888f6e3bfb1b900 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 6 Apr 2022 12:53:12 +0100 Subject: [PATCH] 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',