From c5196fbf0790b7c0c566209c0d298c8d7190349c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 20 May 2021 13:27:22 +0100 Subject: [PATCH] Allow setting provider for any channel Previously we could only select a provider when using the test channel, but this is also required for others channels when we do tests on the production network with individual MNOs. In order to reduce duplication and improve consistency, I've reused the new broadcast_service_name_tag macro to show the setting. --- .../stylesheets/components/navigation.scss | 2 +- app/main/forms.py | 43 ++++--- app/main/views/service_settings.py | 51 ++++---- app/templates/service_navigation.html | 10 +- app/templates/views/service-settings.html | 6 +- tests/app/main/views/test_broadcast.py | 30 +++-- tests/app/main/views/test_service_settings.py | 117 ++++++++++++------ 7 files changed, 163 insertions(+), 96 deletions(-) diff --git a/app/assets/stylesheets/components/navigation.scss b/app/assets/stylesheets/components/navigation.scss index 1bcf79d19..f4135576f 100644 --- a/app/assets/stylesheets/components/navigation.scss +++ b/app/assets/stylesheets/components/navigation.scss @@ -40,7 +40,7 @@ box-shadow: 0 -3px 0 0 $grey-3; } - &--live { + &--live, &--test { // This uses new Design System colours to match .govuk-tag--red background: #F6D7D2; color: #942514; diff --git a/app/main/forms.py b/app/main/forms.py index ae85a7740..97c1d4776 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2407,24 +2407,29 @@ class ServiceBroadcastChannelForm(StripWhitespaceForm): class ServiceBroadcastNetworkForm(StripWhitespaceForm): + def __init__(self, broadcast_channel, *args, **kwargs): + super().__init__(*args, **kwargs) + self.broadcast_channel = broadcast_channel + + self.network_variant.choices = [ + (f'live-{broadcast_channel}', 'All networks'), + ('', 'A single network'), + ] + + self.network.choices = [ + (f'live-{broadcast_channel}-ee', 'EE'), + (f'live-{broadcast_channel}-o2', 'O2'), + (f'live-{broadcast_channel}-vodafone', 'Vodafone'), + (f'live-{broadcast_channel}-three', 'Three'), + ] network_variant = ServiceBroadcastAccountTypeField( 'Choose a mobile network', thing='a mobile network', - choices=[ - ('live-test', 'All networks'), - ('', 'A single network'), - ] ) network = OptionalServiceBroadcastAccountTypeField( 'Choose a mobile network', thing='a mobile network', - choices=[ - ('live-test-ee', 'EE'), - ('live-test-o2', 'O2'), - ('live-test-vodafone', 'Vodafone'), - ('live-test-three', 'Three'), - ], ) def validate_network(self, field): @@ -2439,14 +2444,16 @@ class ServiceBroadcastAccountTypeForm(StripWhitespaceForm): 'Change cell broadcast service type', thing='which type of account this cell broadcast service is', choices=[ - ("training-test", "Training mode"), - ("live-test-ee", "Test channel (EE)"), - ("live-test-o2", "Test channel (O2)"), - ("live-test-three", "Test channel (Three)"), - ("live-test-vodafone", "Test channel (Vodafone)"), - ("live-test", "Test channel (all networks)"), - ("live-severe", "Live (all networks)"), - ("live-government", "Government channel (all networks)"), + ("training-test", "") + ] + + [ + (f"live-{broadcast_channel}", "") + for broadcast_channel in ["test", "severe", "government"] + ] + + [ + (f"live-{broadcast_channel}-{provider}", "") + for broadcast_channel in ["test", "severe", "government"] + for provider in ["ee", "o2", "three", "vodafone"] ], validators=[DataRequired()] ) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 5153351a3..e4489346b 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -330,15 +330,16 @@ def service_set_broadcast_channel(service_id): ) if form.validate_on_submit(): - if form.channel.data == 'live-test': + if form.channel.service_mode == 'training': return redirect(url_for( - '.service_set_broadcast_network', + '.service_confirm_broadcast_account_type', service_id=current_service.id, + account_type=form.channel.data, )) return redirect(url_for( - '.service_confirm_broadcast_account_type', + '.service_set_broadcast_network', service_id=current_service.id, - account_type=form.channel.data, + broadcast_channel=form.channel.broadcast_channel, )) return render_template( @@ -347,25 +348,33 @@ def service_set_broadcast_channel(service_id): ) -@main.route("/services//service-settings/broadcasts/network", methods=["GET", "POST"]) +@main.route("/services//service-settings/broadcasts/", methods=["GET", "POST"]) @user_is_platform_admin -def service_set_broadcast_network(service_id): - if current_service.allowed_broadcast_provider == 'all': - form = ServiceBroadcastNetworkForm( - network_variant=( - current_service.live, - current_service.broadcast_channel, - current_service.allowed_broadcast_provider, - ), - ) +def service_set_broadcast_network(service_id, broadcast_channel): + # only populate old settings when the channel is unchanged + if current_service.broadcast_channel == broadcast_channel: + if current_service.allowed_broadcast_provider == 'all': + form = ServiceBroadcastNetworkForm( + broadcast_channel=broadcast_channel, + network_variant=( + current_service.live, + current_service.broadcast_channel, + current_service.allowed_broadcast_provider, + ), + ) + else: + form = ServiceBroadcastNetworkForm( + broadcast_channel=broadcast_channel, + network_variant='', + network=( + current_service.live, + current_service.broadcast_channel, + current_service.allowed_broadcast_provider + ) + ) else: form = ServiceBroadcastNetworkForm( - network_variant='', - network=( - current_service.live, - current_service.broadcast_channel, - current_service.allowed_broadcast_provider - ) + broadcast_channel=broadcast_channel ) if form.validate_on_submit(): @@ -382,7 +391,7 @@ def service_set_broadcast_network(service_id): @main.route( - "/services//service-settings/broadcasts/", + "/services//service-settings/broadcasts//confirm", methods=["GET", "POST"] ) @user_is_platform_admin diff --git a/app/templates/service_navigation.html b/app/templates/service_navigation.html index 8fb5bfc19..b8d100447 100644 --- a/app/templates/service_navigation.html +++ b/app/templates/service_navigation.html @@ -17,17 +17,17 @@ {% set margin_class = "" if left_margin else "govuk-!-margin-left-0" %} {% if trial_mode %} Training - {% elif broadcast_channel == 'severe' %} + {% else %} + {% if broadcast_channel == "severe" %} Live - {% elif broadcast_channel == 'test' %} - {{ broadcast_channel|title }} + {% else %} + {{ broadcast_channel|title }} + {% endif %} {% if allowed_broadcast_provider == "all" %} (all networks) {% else %} ({{ allowed_broadcast_provider|format_mobile_network }}) {% endif %} - {% elif broadcast_channel == 'government' %} - Government {% endif %} {% endmacro %} diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 567003446..e70c960f3 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -1,6 +1,7 @@ {% extends "withnav_template.html" %} {% from "components/banner.html" import banner_wrapper %} {% from "components/table.html" import mapping_table, row, settings_row, text_field, optional_text_field, edit_field, field, boolean_field with context %} +{% from "service_navigation.html" import broadcast_service_name_tag %} {% block service_page_title %} Settings @@ -434,10 +435,7 @@ {% if not current_service.broadcast_channel %} Off {% else %} - {% if current_service.live and current_service.broadcast_channel == "government" %}Government{% endif %} - {% if current_service.live and current_service.broadcast_channel == "severe" %}Live{% endif %} - {% if current_service.live and current_service.broadcast_channel == "test" %}Test {% if current_service.allowed_broadcast_provider != "all" %}({{ current_service.allowed_broadcast_provider|format_mobile_network }}){% else %}(all networks){% endif %}{%endif%} - {% if not current_service.live%}Training {% endif%} + {{ broadcast_service_name_tag(current_service.trial_mode, current_service.broadcast_channel, current_service.allowed_broadcast_provider, left_margin=False) }} {% endif %} {% endcall %} {{ edit_field( diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index a9fb0e371..f92748616 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -318,14 +318,14 @@ def test_some_broadcast_tour_pages_show_service_name( 'severe', 'all', '.navigation-service-type.navigation-service-type--live', - 'service one Live Switch service', - 'Live', + 'service one Live (all networks) Switch service', + 'Live (all networks)', ), ( False, 'test', 'all', - '.navigation-service-type.navigation-service-type--live', + '.navigation-service-type.navigation-service-type--test', 'service one Test (all networks) Switch service', 'Test (all networks)', ), @@ -333,7 +333,7 @@ def test_some_broadcast_tour_pages_show_service_name( False, 'test', 'vodafone', - '.navigation-service-type.navigation-service-type--live', + '.navigation-service-type.navigation-service-type--test', 'service one Test (Vodafone) Switch service', 'Test (Vodafone)', ), @@ -342,12 +342,28 @@ def test_some_broadcast_tour_pages_show_service_name( 'government', 'all', '.navigation-service-type.navigation-service-type--government', - 'service one Government Switch service', - 'Government', + 'service one Government (all networks) Switch service', + 'Government (all networks)', + ), + ( + False, + 'government', + 'vodafone', + '.navigation-service-type.navigation-service-type--government', + 'service one Government (Vodafone) Switch service', + 'Government (Vodafone)', + ), + ( + False, + 'severe', + 'vodafone', + '.navigation-service-type.navigation-service-type--live', + 'service one Live (Vodafone) Switch service', + 'Live (Vodafone)', ), ) ) -def test_broadcast_service_shows_live_or_training( +def test_broadcast_service_shows_channel_settings( client_request, service_one, mock_get_no_broadcast_messages, diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 6d2f3f2de..02b468a96 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -163,9 +163,9 @@ def test_platform_admin_sees_only_relevant_settings_for_broadcast_service( SERVICE_ONE_ID, users=[api_user_active['id']], permissions=['broadcast'], + restricted=True, organisation_id=ORGANISATION_ID, contact_link='contact_us@gov.uk', - broadcast_channel="severe", ) mocker.patch('app.service_api_client.get_service', return_value={'data': service_one}) @@ -186,7 +186,7 @@ def test_platform_admin_sees_only_relevant_settings_for_broadcast_service( 'Label Value Action', 'Notes None Change the notes for the service', 'Email authentication Off Change your settings for Email authentication', - 'Emergency alerts Training Change your settings for emergency alerts', + 'Emergency alerts Off Change your settings for emergency alerts', ] assert len(rows) == len(expected_rows) @@ -204,7 +204,10 @@ def test_platform_admin_sees_only_relevant_settings_for_broadcast_service( (True, "live", "test", "ee", "Test (EE)"), (True, "live", "test", "three", "Test (Three)"), (True, "live", "test", "all", "Test (all networks)"), - (True, "live", "severe", "all", "Live"), + (True, "live", "severe", "all", "Live (all networks)"), + (True, "live", "severe", "three", "Live (Three)"), + (True, "live", "government", "all", "Government (all networks)"), + (True, "live", "government", "three", "Government (Three)"), ] ) def test_platform_admin_sees_correct_description_of_broadcast_service_setting( @@ -239,8 +242,8 @@ def test_platform_admin_sees_correct_description_of_broadcast_service_setting( assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') broadcast_setting_row = page.find(string=re.compile("Emergency alerts")).find_parent('tr') - broadcast_setting_description = broadcast_setting_row.select('td')[1].text.strip() - assert broadcast_setting_description == expected_text + broadcast_setting_description = broadcast_setting_row.select('td')[1].text + assert normalize_spaces(broadcast_setting_description) == expected_text def test_no_go_live_link_for_service_without_organisation( @@ -5536,17 +5539,17 @@ def test_get_service_set_broadcast_account_type_has_radio_selected_for_broadcast ( 'live-test', '.service_set_broadcast_network', - {}, + {'broadcast_channel': 'test'}, ), ( 'live-severe', - '.service_confirm_broadcast_account_type', - {'account_type': 'live-severe'}, + '.service_set_broadcast_network', + {'broadcast_channel': 'severe'}, ), ( 'live-government', - '.service_confirm_broadcast_account_type', - {'account_type': 'live-government'}, + '.service_set_broadcast_network', + {'broadcast_channel': 'government'}, ), ] ) @@ -5587,13 +5590,17 @@ def test_get_service_set_broadcast_channel_redirects( "live", "severe", "all", - [], + [ + ("All networks", "live-severe"), + ], ), ( "live", "government", "all", - [], + [ + ("All networks", "live-government"), + ], ), ( "live", @@ -5639,6 +5646,24 @@ def test_get_service_set_broadcast_channel_redirects( ("Vodafone", "live-test-vodafone"), ], ), + ( + "live", + "severe", + "vodafone", + [ + ("A single network", ""), + ("Vodafone", "live-severe-vodafone"), + ], + ), + ( + "live", + "government", + "vodafone", + [ + ("A single network", ""), + ("Vodafone", "live-government-vodafone"), + ], + ), ] ) def test_get_service_set_broadcast_network_has_radio_selected( @@ -5663,6 +5688,7 @@ def test_get_service_set_broadcast_network_has_radio_selected( page = client_request.get( 'main.service_set_broadcast_network', service_id=SERVICE_ONE_ID, + broadcast_channel=broadcast_channel, ) assert [ @@ -5675,40 +5701,38 @@ def test_get_service_set_broadcast_network_has_radio_selected( @pytest.mark.parametrize( - 'data, expected_result', + 'broadcast_channel, provider, choice_type', ( - ( - {'network_variant': 'live-test'}, - 'live-test' - ), - ( - {'network_variant': '', 'network': 'live-test-ee'}, - 'live-test-ee' - ), - ( - {'network_variant': '', 'network': 'live-test-o2'}, - 'live-test-o2' - ), - ( - {'network_variant': '', 'network': 'live-test-three'}, - 'live-test-three' - ), - ( - {'network_variant': '', 'network': 'live-test-vodafone'}, - 'live-test-vodafone' - ), + ('severe', '', 'network_variant'), + ('government', '', 'network_variant'), + ('test', '', 'network_variant'), + ('test', 'o2', 'network'), + ('test', 'ee', 'network'), + ('test', 'three', 'network'), + ('test', 'vodafone', 'network'), + ('government', 'vodafone', 'network'), + ('severe', 'vodafone', 'network'), ), ) def test_post_service_set_broadcast_network( client_request, platform_admin_user, - data, - expected_result, + broadcast_channel, + provider, + choice_type, ): + if choice_type == 'network_variant': + expected_result = f'live-{broadcast_channel}' + data = {'network_variant': expected_result} + else: + expected_result = f'live-{broadcast_channel}-{provider}' + data = {'network_variant': '', 'network': expected_result} + client_request.login(platform_admin_user) client_request.post( 'main.service_set_broadcast_network', service_id=SERVICE_ONE_ID, + broadcast_channel=broadcast_channel, _data=data, _expected_status=302, _expected_redirect=url_for( @@ -5737,6 +5761,7 @@ def test_post_service_set_broadcast_network_makes_you_choose( page = client_request.post( 'main.service_set_broadcast_network', service_id=SERVICE_ONE_ID, + broadcast_channel='all', _data=data, _expected_status=200, ) @@ -5783,12 +5808,22 @@ def test_post_service_set_broadcast_network_makes_you_choose( 'this service.', ]), ('live-severe', [ - 'Live', + 'Live (all networks)', + 'Members of the public will receive alerts sent from this ' + 'service.', + ]), + ('live-severe-vodafone', [ + 'Live (Vodafone)', 'Members of the public will receive alerts sent from this ' 'service.', ]), ('live-government', [ - 'Government', + 'Government (all networks)', + 'Members of the public will receive alerts sent from this ' + 'service, even if they’ve opted out.' + ]), + ('live-government-vodafone', [ + 'Government (Vodafone)', 'Members of the public will receive alerts sent from this ' 'service, even if they’ve opted out.' ]), @@ -5856,14 +5891,15 @@ 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 a mobile network'), +@pytest.mark.parametrize('endpoint, extra_args, expected_error', ( + ('main.service_set_broadcast_channel', {}, 'Error: Select mode or channel'), + ('main.service_set_broadcast_network', {'broadcast_channel': 'all'}, 'Error: Select a mobile network'), )) def test_post_service_set_broadcast_account_type_shows_errors_if_no_radio_selected( platform_admin_client, mocker, endpoint, + extra_args, expected_error, ): set_service_broadcast_settings_mock = mocker.patch('app.service_api_client.set_service_broadcast_settings') @@ -5873,6 +5909,7 @@ def test_post_service_set_broadcast_account_type_shows_errors_if_no_radio_select url_for( endpoint, service_id=SERVICE_ONE_ID, + **extra_args ) ) assert response.status_code == 200