From a0f54539ccab1ac8d9850a4d891289b1ad917bfb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 11 May 2021 11:07:30 +0100 Subject: [PATCH] Add a second step for choosing networks Only the test channel has the option to isolate messages to one network. This commits makes the choices less confusing by only showing the network choice to those who have selected the test channel. --- app/main/forms.py | 31 +++- app/main/views/service_settings.py | 41 ++++- app/navigation.py | 3 +- app/templates/views/service-settings.html | 2 +- ...ervice-confirm-broadcast-account-type.html | 2 +- .../service-set-broadcast-channel.html | 32 ++++ .../service-set-broadcast-network.html | 32 ++++ tests/app/main/views/test_service_settings.py | 169 +++++++++++++++--- tests/app/test_navigation.py | 3 +- 9 files changed, 284 insertions(+), 31 deletions(-) create mode 100644 app/templates/views/service-settings/service-set-broadcast-channel.html create mode 100644 app/templates/views/service-settings/service-set-broadcast-network.html diff --git a/app/main/forms.py b/app/main/forms.py index 327873385..223970de8 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2316,7 +2316,7 @@ class ServiceBroadcastAccountTypeField(GovukRadiosField): # (service_mode, broadcast_channel, allowed_broadcast_provider) # to a value to be used in our form such as "live-severe-ee" def process_data(self, value): - if isinstance(value, str): + if not value or isinstance(value, str): return super().process_data(value) (live, broadcast_channel, allowed_broadcast_provider) = value account_type = None @@ -2332,13 +2332,40 @@ class ServiceBroadcastAccountTypeField(GovukRadiosField): # broadcast_channel and provider_restriction to be used by the flask route to send to the # API def post_validate(self, form, validation_stopped): - if not validation_stopped: + if not validation_stopped and self.data: split_values = self.data.split("-") self.service_mode = split_values[0] self.broadcast_channel = split_values[1] self.provider_restriction = split_values[2] if len(split_values) == 3 else 'all' +class ServiceBroadcastChannelForm(StripWhitespaceForm): + channel = ServiceBroadcastAccountTypeField( + 'Emergency alerts settings', + thing='mode or channel', + choices=[ + ("training-test", "Training mode"), + ("live-test", "Test channel"), + ("live-severe", "Live channel"), + ("live-government", "Government channel"), + ], + ) + + +class ServiceBroadcastNetworkForm(StripWhitespaceForm): + network = ServiceBroadcastAccountTypeField( + 'Choose a mobile network', + thing='mobile network', + choices=[ + ("live-test", "All networks"), + ("live-test-ee", "EE only"), + ("live-test-o2", "O2 only"), + ("live-test-vodafone", "Vodafone only"), + ("live-test-three", "Three only"), + ], + ) + + class ServiceBroadcastAccountTypeForm(StripWhitespaceForm): account_type = ServiceBroadcastAccountTypeField( 'Change cell broadcast service type', diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 8473a9e99..0167a2843 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -46,6 +46,8 @@ from app.main.forms import ( RenameServiceForm, SearchByNameForm, ServiceBroadcastAccountTypeForm, + ServiceBroadcastChannelForm, + ServiceBroadcastNetworkForm, ServiceContactDetailsForm, ServiceDataRetentionEditForm, ServiceDataRetentionForm, @@ -318,9 +320,38 @@ def service_set_permission(service_id, permission): @main.route("/services//service-settings/broadcasts", methods=["GET", "POST"]) @user_is_platform_admin -def service_set_broadcast_account_type(service_id): - form = ServiceBroadcastAccountTypeForm( - account_type=( +def service_set_broadcast_channel(service_id): + form = ServiceBroadcastChannelForm( + channel=( + current_service.live, + current_service.broadcast_channel, + 'all', + ) + ) + + if form.validate_on_submit(): + if form.channel.data == 'live-test': + return redirect(url_for( + '.service_set_broadcast_network', + service_id=current_service.id, + )) + return redirect(url_for( + '.service_confirm_broadcast_account_type', + service_id=current_service.id, + account_type=form.channel.data, + )) + + return render_template( + 'views/service-settings/service-set-broadcast-channel.html', + form=form, + ) + + +@main.route("/services//service-settings/broadcasts/network", methods=["GET", "POST"]) +@user_is_platform_admin +def service_set_broadcast_network(service_id): + form = ServiceBroadcastNetworkForm( + network=( current_service.live, current_service.broadcast_channel, current_service.allowed_broadcast_provider @@ -331,11 +362,11 @@ def service_set_broadcast_account_type(service_id): return redirect(url_for( '.service_confirm_broadcast_account_type', service_id=current_service.id, - account_type=form.account_type.data, + account_type=form.network.data, )) return render_template( - 'views/service-settings/service-set-broadcast-account-type.html', + 'views/service-settings/service-set-broadcast-network.html', form=form, ) diff --git a/app/navigation.py b/app/navigation.py index ce0ffe8bb..77e1f59e3 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -243,8 +243,9 @@ class MainNavigation(Navigation): 'service_set_auth_type', 'service_set_channel', 'send_files_by_email_contact_details', - 'service_set_broadcast_account_type', 'service_confirm_broadcast_account_type', + 'service_set_broadcast_channel', + 'service_set_broadcast_network', 'service_set_email_branding', 'service_set_inbound_number', 'service_set_inbound_sms', diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index ea1e3b3dd..4f665378e 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -442,7 +442,7 @@ {% endcall %} {{ edit_field( 'Change', - url_for('.service_set_broadcast_account_type', service_id=current_service.id), + url_for('.service_set_broadcast_channel', service_id=current_service.id), suffix='your settings for Send cell broadcasts' ) }} diff --git a/app/templates/views/service-settings/service-confirm-broadcast-account-type.html b/app/templates/views/service-settings/service-confirm-broadcast-account-type.html index 79fc1bd40..98fb1a52c 100644 --- a/app/templates/views/service-settings/service-confirm-broadcast-account-type.html +++ b/app/templates/views/service-settings/service-confirm-broadcast-account-type.html @@ -13,7 +13,7 @@
{{ page_header( 'Confirm emergency alert settings', - back_link=url_for('.service_set_broadcast_account_type', service_id=current_service.id) + back_link=url_for('.service_set_broadcast_channel', service_id=current_service.id) ) }} {% if form.account_type.service_mode == 'training' %}

diff --git a/app/templates/views/service-settings/service-set-broadcast-channel.html b/app/templates/views/service-settings/service-set-broadcast-channel.html new file mode 100644 index 000000000..b8a07b1d1 --- /dev/null +++ b/app/templates/views/service-settings/service-set-broadcast-channel.html @@ -0,0 +1,32 @@ +{% extends "withnav_template.html" %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/form.html" import form_wrapper %} +{% from "components/back-link/macro.njk" import govukBackLink %} + +{% block service_page_title %} + Emergency alerts settings +{% endblock %} + +{% block maincolumn_content %} + +

+
+ {{ govukBackLink({ + "text": "Back", + "href": url_for('.service_settings', service_id=current_service.id) + }) }} + {% call form_wrapper() %} + {{ form.channel(param_extensions={ + 'fieldset': { + 'legend': { + 'isPageHeading': True, + 'classes': 'govuk-fieldset__legend--l' + } + } + }) }} + {{ page_footer('Continue') }} + {% endcall %} +
+
+ +{% endblock %} diff --git a/app/templates/views/service-settings/service-set-broadcast-network.html b/app/templates/views/service-settings/service-set-broadcast-network.html new file mode 100644 index 000000000..bb0e68561 --- /dev/null +++ b/app/templates/views/service-settings/service-set-broadcast-network.html @@ -0,0 +1,32 @@ +{% extends "withnav_template.html" %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/form.html" import form_wrapper %} +{% from "components/back-link/macro.njk" import govukBackLink %} + +{% block service_page_title %} + Choose a mobile network +{% endblock %} + +{% block maincolumn_content %} + +
+
+ {{ govukBackLink({ + "text": "Back", + "href": url_for('.service_set_broadcast_channel', service_id=current_service.id) + }) }} + {% call form_wrapper() %} + {{ form.network(param_extensions={ + 'fieldset': { + 'legend': { + 'isPageHeading': True, + 'classes': 'govuk-fieldset__legend--l' + } + } + }) }} + {{ page_footer('Continue') }} + {% endcall %} +
+
+ +{% endblock %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 1bca91636..c11f39872 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -5418,24 +5418,20 @@ def test_get_service_set_broadcast_account_type( ): response = platform_admin_client.get( url_for( - 'main.service_set_broadcast_account_type', + 'main.service_set_broadcast_channel', service_id=SERVICE_ONE_ID, ) ) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.select_one('h1').text.strip() == "Change cell broadcast service type" + assert page.select_one('h1').text.strip() == 'Emergency alerts settings' expected_labels = [ "Training mode", - "Test channel (EE)", - "Test channel (O2)", - "Test channel (Three)", - "Test channel (Vodafone)", - "Test channel (all networks)", - "Live (all networks)", - "Government channel (all networks)", + "Test channel", + "Live channel", + "Government channel", ] labels = page.find_all('label', class_="govuk-radios__label") assert len(labels) == len(expected_labels) @@ -5452,7 +5448,7 @@ def test_get_service_set_broadcast_account_type_has_no_radio_selected_for_non_br ): response = platform_admin_client.get( url_for( - 'main.service_set_broadcast_account_type', + 'main.service_set_broadcast_channel', service_id=SERVICE_ONE_ID, ) ) @@ -5475,16 +5471,23 @@ def test_get_service_set_broadcast_account_type_has_no_radio_selected_for_non_br "live", "test", "vodafone", - "Test channel (Vodafone)", - "live-test-vodafone", + "Test channel", + "live-test", ), ( "live", "severe", "all", - "Live (all networks)", + "Live channel", "live-severe", ), + ( + "live", + "government", + "all", + "Government channel", + "live-government", + ), ] ) def test_get_service_set_broadcast_account_type_has_radio_selected_for_broadcast_service( @@ -5507,7 +5510,7 @@ def test_get_service_set_broadcast_account_type_has_radio_selected_for_broadcast response = platform_admin_client.get( url_for( - 'main.service_set_broadcast_account_type', + 'main.service_set_broadcast_channel', service_id=SERVICE_ONE_ID, ) ) @@ -5522,16 +5525,136 @@ def test_get_service_set_broadcast_account_type_has_radio_selected_for_broadcast assert selected_label.text.strip() == expected_text +@pytest.mark.parametrize( + 'account_type,expected_redirect_endpoint,extra_args', + [ + ( + 'training-test', + '.service_confirm_broadcast_account_type', + {'account_type': 'training-test'}, + ), + ( + 'live-test', + '.service_set_broadcast_network', + {}, + ), + ( + 'live-severe', + '.service_confirm_broadcast_account_type', + {'account_type': 'live-severe'}, + ), + ( + 'live-government', + '.service_confirm_broadcast_account_type', + {'account_type': 'live-government'}, + ), + ] +) +def test_get_service_set_broadcast_channel_redirects( + client_request, + platform_admin_user, + mocker, + account_type, + expected_redirect_endpoint, + extra_args, +): + client_request.login(platform_admin_user) + client_request.post( + 'main.service_set_broadcast_channel', + service_id=SERVICE_ONE_ID, + _data={ + 'channel': account_type, + }, + _expected_redirect=url_for( + expected_redirect_endpoint, + service_id=SERVICE_ONE_ID, + _external=True, + **extra_args, + ) + ) + + +@pytest.mark.parametrize( + 'service_mode,broadcast_channel,allowed_broadcast_provider,expected_text,expected_value', + [ + ( + "training", + "test", + "all", + "All networks", + "live-test", + ), + ( + "live", + "test", + "ee", + "EE only", + "live-test-ee", + ), + ( + "live", + "test", + "o2", + "O2 only", + "live-test-o2", + ), + ( + "live", + "test", + "three", + "Three only", + "live-test-three", + ), + ( + "live", + "test", + "vodafone", + "Vodafone only", + "live-test-vodafone", + ), + ] +) +def test_get_service_set_broadcast_network_has_radio_selected( + client_request, + platform_admin_user, + mocker, + service_mode, + broadcast_channel, + allowed_broadcast_provider, + expected_text, + expected_value +): + client_request.login(platform_admin_user) + service_one = service_json( + SERVICE_ONE_ID, + permissions=['broadcast'], + restricted=False, + broadcast_channel=broadcast_channel, + allowed_broadcast_provider=allowed_broadcast_provider, + ) + mocker.patch('app.service_api_client.get_service', return_value={'data': service_one}) + + page = client_request.get( + 'main.service_set_broadcast_network', + service_id=SERVICE_ONE_ID, + ) + selected_radios = page.select('input[checked]') + assert len(selected_radios) == 1 + + selected_radio = selected_radios[0] + assert selected_radio.get('value') == expected_value + selected_label = selected_radio.find_next_sibling('label') + assert normalize_spaces(selected_label.text) == expected_text + + @pytest.mark.parametrize( 'value', ( - 'training-test', 'live-test-ee', 'live-test-o2', 'live-test-three', 'live-test-vodafone', 'live-test', - 'live-severe', pytest.param('foo', marks=pytest.mark.xfail), ), ) @@ -5543,10 +5666,10 @@ def test_post_service_set_broadcast_account_type_confirms( ): client_request.login(platform_admin_user) client_request.post( - 'main.service_set_broadcast_account_type', + 'main.service_set_broadcast_network', service_id=SERVICE_ONE_ID, _data={ - 'account_type': value, + 'network': value, }, _expected_status=302, _expected_redirect=url_for( @@ -5669,21 +5792,27 @@ def test_post_service_set_broadcast_account_type_posts_data_to_api_and_redirects ) +@pytest.mark.parametrize('endpoint, expected_error', ( + ('main.service_set_broadcast_channel', 'Error: Select mode or channel'), + ('main.service_set_broadcast_network', 'Error: Select mobile network'), +)) def test_post_service_set_broadcast_account_type_shows_errors_if_no_radio_selected( platform_admin_client, mocker, + endpoint, + expected_error, ): set_service_broadcast_settings_mock = mocker.patch('app.service_api_client.set_service_broadcast_settings') mock_event_handler = mocker.patch('app.main.views.service_settings.create_broadcast_account_type_change_event') response = platform_admin_client.post( url_for( - 'main.service_set_broadcast_account_type', + endpoint, service_id=SERVICE_ONE_ID, ) ) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert "This field is required" in page.find("span", {"class": "govuk-error-message"}).text + assert expected_error in page.find("span", {"class": "govuk-error-message"}).text assert not set_service_broadcast_settings_mock.called assert not mock_event_handler.called diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index 6df041e79..79b78376d 100644 --- a/tests/app/test_navigation.py +++ b/tests/app/test_navigation.py @@ -245,8 +245,9 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, { 'service_preview_email_branding', 'service_preview_letter_branding', 'service_set_auth_type', - 'service_set_broadcast_account_type', 'service_confirm_broadcast_account_type', + 'service_set_broadcast_channel', + 'service_set_broadcast_network', 'service_set_channel', 'service_set_email_branding', 'service_set_inbound_number',