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 127cf8358..9f356b61b 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1740,34 +1740,49 @@ 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(Form): + def __init__(self, providers): + self._providers = providers + # hack: https://github.com/wtforms/wtforms/issues/736 + self._unbound_fields = [ + ( + provider['identifier'], + GovukIntegerField( + 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 + ] -class AdminProviderRatioForm(StripWhitespaceForm): - - 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) + def validate(self): + if not super().validate(): + return False - @property - def percentage_right(self): - return 100 - self.percentage_left + 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): diff --git a/app/main/views/providers.py b/app/main/views/providers.py index d08128f7b..08dfa0060 100644 --- a/app/main/views/providers.py +++ b/app/main/views/providers.py @@ -1,13 +1,11 @@ -from collections import defaultdict 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 format_date_numeric, provider_client +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() @@ -44,79 +42,30 @@ 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(): - - 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(ratio=providers[0]['priority']) - - if len(providers) < 2: - abort(400) - - first_provider, second_provider = providers[0:2] + form = AdminProviderRatioForm(providers) 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')) + 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', - 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'], + providers=providers ) -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-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/edit-sms-provider-ratio.html b/app/templates/views/providers/edit-sms-provider-ratio.html index e522d132b..53485cc26 100644 --- a/app/templates/views/providers/edit-sms-provider-ratio.html +++ b/app/templates/views/providers/edit-sms-provider-ratio.html @@ -4,87 +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 %} -
-
-
-
-
- - - {% for day, versions in versions %} -

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

- - {% endfor %} + {% call form_wrapper() %} + {% for field in form %} + {{ field }} + {% endfor %} + {{ page_footer('Save') }} + {% endcall %} {% endblock %} 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='', diff --git a/app/templates/views/providers/providers.html b/app/templates/views/providers/providers.html index 9cf706e1f..3874cd0bd 100644 --- a/app/templates/views/providers/providers.html +++ b/app/templates/views/providers/providers.html @@ -25,17 +25,16 @@ 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) }} {{ 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') }} @@ -48,26 +47,20 @@ 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 %} - {{ 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') }} - - {{ link_field('change', url_for('main.edit_provider', provider_id=item.id)) }} - {% endcall %}

International SMS Providers

@@ -77,21 +70,18 @@ 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 %} - {{ 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') }} diff --git a/tests/app/main/views/test_providers.py b/tests/app/main/views/test_providers.py index 63f490cc5..8a69e2178 100644 --- a/tests/app/main/views/test_providers.py +++ b/tests/app/main/views/test_providers.py @@ -3,99 +3,108 @@ 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 -from tests.conftest import normalize_spaces -sms_provider_1 = { - 'id': '6005e192-4738-4962-beec-ebd982d0b03f', - 'active': True, - '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(), - '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': 10, - '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, -} +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, + } -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_intl_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_intl_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, -} + provider.update(**overrides) + return provider @pytest.fixture -def stub_providers(): +def sms_provider_1(): + return provider_json({ + 'id': 'sms_provider_1-id', + 'priority': 20, + 'display_name': 'SMS Provider 1', + 'identifier': 'sms_provider_1', + 'notification_type': 'sms', + 'updated_at': datetime(2017, 1, 16, 15, 20, 40).isoformat(), + 'created_by_name': 'Test User', + 'current_month_billable_sms': 5020, + }) + + +@pytest.fixture +def sms_provider_2(): + return provider_json({ + 'id': 'sms_provider_2-id', + 'priority': 10, + 'display_name': 'SMS Provider 2', + 'identifier': 'sms_provider_2', + 'notification_type': 'sms', + 'current_month_billable_sms': 6891, + }) + + +@pytest.fixture +def email_provider_1(): + return provider_json({ + 'id': 'email_provider_1-id', + 'display_name': 'Email Provider 1', + 'identifier': 'email_provider_1', + 'notification_type': 'email', + }) + + +@pytest.fixture +def email_provider_2(): + return provider_json({ + 'id': 'email_provider_2-id', + 'display_name': 'Email Provider 2', + 'identifier': 'email_provider_2', + 'notification_type': 'email', + }) + + +@pytest.fixture +def sms_provider_intl_1(): + return provider_json({ + 'id': 'sms_provider_intl_1-id', + 'active': False, + 'display_name': 'SMS Provider Intl 1', + 'identifier': 'sms_provider_intl_1', + 'notification_type': 'sms', + 'supports_international': True, + }) + + +@pytest.fixture +def sms_provider_intl_2(): + return provider_json({ + 'id': 'sms_provider_intl_2-id', + 'active': False, + 'display_name': 'SMS Provider Intl 2', + 'identifier': 'sms_provider_intl_2', + 'notification_type': 'sms', + 'supports_international': True, + }) + + +@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, @@ -108,13 +117,6 @@ def stub_providers(): } -@pytest.fixture -def stub_provider(): - return { - 'provider_details': sms_provider_1 - } - - @pytest.fixture def stub_provider_history(): return { @@ -151,7 +153,7 @@ def stub_provider_history(): } -def test_should_show_all_providers( +def test_view_providers_shows_all_providers( client_request, platform_admin_user, mocker, @@ -181,8 +183,8 @@ 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 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" @@ -192,8 +194,8 @@ 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 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" @@ -203,36 +205,29 @@ def test_should_show_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[1].text.strip() == "1" - assert domestic_email_table_data[2].text.strip() == "True" + 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" - 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[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" - 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[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" - assert table_data[4].text.strip() == "None" def test_add_monthly_traffic(): @@ -264,134 +259,7 @@ 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( +def test_view_provider_shows_version_history( client_request, platform_admin_user, mocker, @@ -432,121 +300,80 @@ 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( +def test_edit_sms_provider_provider_ratio( client_request, platform_admin_user, mocker, stub_providers, + sms_provider_1 ): 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( + '.edit_sms_provider_ratio', ) - client_request.login(platform_admin_user) - page = client_request.get('main.edit_sms_provider_ratio') + inputs = page.select('.govuk-input[type="text"]') + assert len(inputs) == 2 - 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%' - ), - ] + first_input = page.select_one('.govuk-input[name="sms_provider_1"]') + assert first_input.attrs['value'] == str(sms_provider_1['priority']) -@pytest.mark.parametrize('posted_number, expected_calls', [ - ( - '10', - [ - call(sms_provider_1['id'], 10), - call(sms_provider_2['id'], 90), - ], - ), - ( - '80', - [ - call(sms_provider_1['id'], 80), - call(sms_provider_2['id'], 20), - ], - ), -]) -def test_should_update_priority_of_first_two_sms_providers( +def test_edit_sms_provider_provider_ratio_only_shows_active_providers( client_request, platform_admin_user, mocker, - posted_number, + stub_providers, + sms_provider_1, +): + sms_provider_1['active'] = False + + 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) == 1 + + +@pytest.mark.parametrize('post_data, expected_calls', [ + ( + { + 'sms_provider_1': 10, + 'sms_provider_2': 90 + }, + [ + call('sms_provider_1-id', 10), + call('sms_provider_2-id', 90), + ], + ), + ( + { + 'sms_provider_1': 80, + 'sms_provider_2': 20 + }, + [ + call('sms_provider_1-id', 80), + call('sms_provider_2-id', 20), + ], + ), +]) +def test_edit_sms_provider_ratio_submit( + client_request, + platform_admin_user, + mocker, + post_data, expected_calls, stub_providers, ): @@ -554,10 +381,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' ) @@ -565,13 +388,54 @@ def test_should_update_priority_of_first_two_sms_providers( client_request.login(platform_admin_user) client_request.post( '.edit_sms_provider_ratio', - _data={ - 'ratio': posted_number, - }, + _data=post_data, _expected_redirect=url_for( - '.edit_sms_provider_ratio', + '.view_providers', _external=True, ), ) assert mock_update_provider.call_args_list == expected_calls + + +@pytest.mark.parametrize('post_data, expected_error', [ + ( + { + 'sms_provider_1': 90, + 'sms_provider_2': 20 + }, + "Must add up to 100%" + ), + ( + { + 'sms_provider_1': 101, + 'sms_provider_2': 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() 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',