diff --git a/app/main/forms.py b/app/main/forms.py index 1a538002c..5d714fd53 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -881,6 +881,13 @@ class GovukRadiosField(RadioField): return govuk_radios_field_widget(self, field, param_extensions=param_extensions, **kwargs) +class OptionalGovukRadiosField(GovukRadiosField): + def pre_validate(self, form): + if self.data is None: + return + super().pre_validate(form) + + class OnOffField(GovukRadiosField): def __init__(self, label, choices=None, *args, **kwargs): @@ -2358,23 +2365,6 @@ class GoLiveNotesForm(StripWhitespaceForm): class ServiceBroadcastAccountTypeField(GovukRadiosField): - # When receiving Python data, eg when instantiating the form object - # we want to convert it from a tuple of - # (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 not value or isinstance(value, str): - return super().process_data(value) - (live, broadcast_channel, allowed_broadcast_provider) = value - account_type = None - if broadcast_channel: - account_type = "live" if live else "training" - account_type += f"-{broadcast_channel}" - if allowed_broadcast_provider != 'all': - account_type += f"-{allowed_broadcast_provider}" - - self.data = account_type - # After validation we split the value back into its parts of service_mode # broadcast_channel and provider_restriction to be used by the flask route to send to the # API @@ -2383,25 +2373,18 @@ class ServiceBroadcastAccountTypeField(GovukRadiosField): 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 OptionalServiceBroadcastAccountTypeField(ServiceBroadcastAccountTypeField): - def pre_validate(self, form): - if self.data is None: - return - super().pre_validate(form) + self.provider_restriction = split_values[2] class ServiceBroadcastChannelForm(StripWhitespaceForm): - channel = ServiceBroadcastAccountTypeField( + channel = GovukRadiosField( 'Emergency alerts settings', thing='mode or channel', choices=[ - ("training-test", "Training mode"), - ("live-test", "Test channel"), - ("live-severe", "Live channel"), - ("live-government", "Government channel"), + ("training", "Training mode"), + ("test", "Test channel"), + ("severe", "Live channel"), + ("government", "Government channel"), ], ) @@ -2411,32 +2394,36 @@ class ServiceBroadcastNetworkForm(StripWhitespaceForm): 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', - ) - network = OptionalServiceBroadcastAccountTypeField( + all_networks = OnOffField( + 'Choose a mobile network', + choices=( + (True, 'All networks'), + (False, 'A single network') + ), + ) + network = OptionalGovukRadiosField( 'Choose a mobile network', thing='a mobile network', + choices=( + ('ee', 'EE'), + ('o2', 'O2'), + ('vodafone', 'Vodafone'), + ('three', 'Three'), + ), ) + @property + def account_type(self): + if self.all_networks.data: + provider = 'all' + else: + provider = self.network.data + + return f'live-{self.broadcast_channel}-{provider}' + def validate_network(self, field): - if not self.network_variant.data and not field.data: + if not self.all_networks.data and not field.data: raise ValidationError('Select a mobile network') - if self.network_variant.data == 'all': - field.data = '' class ServiceBroadcastAccountTypeForm(StripWhitespaceForm): @@ -2444,16 +2431,12 @@ class ServiceBroadcastAccountTypeForm(StripWhitespaceForm): 'Change cell broadcast service type', thing='which type of account this cell broadcast service is', choices=[ - ("training-test", "") - ] + - [ - (f"live-{broadcast_channel}", "") - for broadcast_channel in ["test", "severe", "government"] + ("training-test-all", "") ] + [ (f"live-{broadcast_channel}-{provider}", "") for broadcast_channel in ["test", "severe", "government"] - for provider in ["ee", "o2", "three", "vodafone"] + for provider in ["all", "ee", "o2", "three", "vodafone"] ], validators=[DataRequired()] ) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 8006e8492..fd45e8ebc 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -321,25 +321,27 @@ def service_set_permission(service_id, permission): @main.route("/services//service-settings/broadcasts", methods=["GET", "POST"]) @user_is_platform_admin def service_set_broadcast_channel(service_id): - form = ServiceBroadcastChannelForm( - channel=( - current_service.live, - current_service.broadcast_channel, - 'all', - ) - ) + if current_service.has_permission('broadcast'): + if current_service.live: + channel = current_service.broadcast_channel + else: + channel = 'training' + else: + channel = None + + form = ServiceBroadcastChannelForm(channel=channel) if form.validate_on_submit(): - if form.channel.service_mode == 'training': + if form.channel.data == 'training': return redirect(url_for( '.service_confirm_broadcast_account_type', service_id=current_service.id, - account_type=form.channel.data, + account_type='training-test-all' )) return redirect(url_for( '.service_set_broadcast_network', service_id=current_service.id, - broadcast_channel=form.channel.broadcast_channel, + broadcast_channel=form.channel.data, )) return render_template( @@ -353,25 +355,13 @@ def service_set_broadcast_channel(service_id): 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 - ) - ) + provider = current_service.allowed_broadcast_provider + + form = ServiceBroadcastNetworkForm( + broadcast_channel=broadcast_channel, + all_networks=provider == 'all', + network=provider if provider != 'all' else None, + ) else: form = ServiceBroadcastNetworkForm( broadcast_channel=broadcast_channel @@ -381,7 +371,7 @@ def service_set_broadcast_network(service_id, broadcast_channel): return redirect(url_for( '.service_confirm_broadcast_account_type', service_id=current_service.id, - account_type=form.network_variant.data or form.network.data, + account_type=form.account_type, )) return render_template( diff --git a/app/templates/views/service-settings/service-set-broadcast-network.html b/app/templates/views/service-settings/service-set-broadcast-network.html index a32f24819..72948e639 100644 --- a/app/templates/views/service-settings/service-set-broadcast-network.html +++ b/app/templates/views/service-settings/service-set-broadcast-network.html @@ -19,9 +19,9 @@ back_link=url_for('.service_set_broadcast_channel', service_id=current_service.id) ) }} {% call form_wrapper() %} - {% call select_wrapper(form.network_variant, hide_legend=True) %} - {% for option in form.network_variant %} - {{ radio(option, data_target='single-network' if option.data == "" else None) }} + {% call select_wrapper(form.all_networks, hide_legend=True) %} + {% for option in form.all_networks %} + {{ radio(option, data_target='single-network' if option.data == False else None) }} {% endfor %} {% endcall %} {% call conditional_radio_panel('single-network') %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 575c8e84a..6da5f1a62 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -5537,28 +5537,14 @@ def test_service_set_broadcast_channel_has_no_radio_selected_for_non_broadcast_s "test", "all", "Training mode", - "training-test", - ), - ( - "live", - "test", - "vodafone", - "Test channel", - "live-test", - ), - ( - "live", - "severe", - "all", - "Live channel", - "live-severe", + "training", ), ( "live", "government", "all", "Government channel", - "live-government", + "government", ), ] ) @@ -5598,25 +5584,25 @@ def test_service_set_broadcast_channel_has_radio_selected_for_broadcast_service( @pytest.mark.parametrize( - 'account_type,expected_redirect_endpoint,extra_args', + 'channel,expected_redirect_endpoint,extra_args', [ ( - 'training-test', + 'training', '.service_confirm_broadcast_account_type', - {'account_type': 'training-test'}, + {'account_type': 'training-test-all'}, ), ( - 'live-test', + 'test', '.service_set_broadcast_network', {'broadcast_channel': 'test'}, ), ( - 'live-severe', + 'severe', '.service_set_broadcast_network', {'broadcast_channel': 'severe'}, ), ( - 'live-government', + 'government', '.service_set_broadcast_network', {'broadcast_channel': 'government'}, ), @@ -5626,7 +5612,7 @@ def test_service_set_broadcast_channel_redirects( client_request, platform_admin_user, mocker, - account_type, + channel, expected_redirect_endpoint, extra_args, ): @@ -5635,7 +5621,7 @@ def test_service_set_broadcast_channel_redirects( 'main.service_set_broadcast_channel', service_id=SERVICE_ONE_ID, _data={ - 'channel': account_type, + 'channel': channel, }, _expected_redirect=url_for( expected_redirect_endpoint, @@ -5649,34 +5635,12 @@ def test_service_set_broadcast_channel_redirects( @pytest.mark.parametrize( 'service_mode,broadcast_channel,allowed_broadcast_provider,expected_selected', [ - ( - "training", - "test", - "all", - [], - ), ( "live", "severe", "all", [ - ("All networks", "live-severe"), - ], - ), - ( - "live", - "government", - "all", - [ - ("All networks", "live-government"), - ], - ), - ( - "live", - "test", - "all", - [ - ("All networks", "live-test"), + ("All networks", "True"), ], ), ( @@ -5684,53 +5648,8 @@ def test_service_set_broadcast_channel_redirects( "test", "ee", [ - ("A single network", ""), - ("EE", "live-test-ee"), - ], - ), - ( - "live", - "test", - "o2", - [ - ("A single network", ""), - ("O2", "live-test-o2"), - ], - ), - ( - "live", - "test", - "three", - [ - ("A single network", ""), - ("Three", "live-test-three"), - ], - ), - ( - "live", - "test", - "vodafone", - [ - ("A single network", ""), - ("Vodafone", "live-test-vodafone"), - ], - ), - ( - "live", - "severe", - "vodafone", - [ - ("A single network", ""), - ("Vodafone", "live-severe-vodafone"), - ], - ), - ( - "live", - "government", - "vodafone", - [ - ("A single network", ""), - ("Vodafone", "live-government-vodafone"), + ("A single network", "False"), + ("EE", "ee"), ], ), ] @@ -5770,33 +5689,26 @@ def test_service_set_broadcast_network_has_radio_selected( @pytest.mark.parametrize( - 'broadcast_channel, provider, choice_type', + 'broadcast_channel, data, expected_result', ( - ('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'), + ('severe', {'all_networks': True}, 'live-severe-all'), + ('government', {'all_networks': True}, 'live-government-all'), + ('test', {'all_networks': True}, 'live-test-all'), + ('test', {'all_networks': False, 'network': 'o2'}, 'live-test-o2'), + ('test', {'all_networks': False, 'network': 'ee'}, 'live-test-ee'), + ('test', {'all_networks': False, 'network': 'three'}, 'live-test-three'), + ('test', {'all_networks': False, 'network': 'vodafone'}, 'live-test-vodafone'), + ('government', {'all_networks': False, 'network': 'vodafone'}, 'live-government-vodafone'), + ('severe', {'all_networks': False, 'network': 'vodafone'}, 'live-severe-vodafone'), ), ) def test_service_set_broadcast_network( client_request, platform_admin_user, broadcast_channel, - provider, - choice_type, + data, + expected_result, ): - 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', @@ -5817,7 +5729,7 @@ def test_service_set_broadcast_network( 'data', ( {}, - {'network_variant': ''}, # Missing choice of MNO + {'all_networks': ''}, # Missing choice of MNO ), ) @pytest.mark.parametrize('broadcast_channel', ['government', 'severe', 'test']) @@ -5844,7 +5756,7 @@ def test_service_set_broadcast_network_makes_you_choose( @pytest.mark.parametrize( 'value, expected_paragraphs', [ - ('training-test', [ + ('training-test-all', [ 'Training', 'No phones will receive alerts sent from this service.', ]), @@ -5872,13 +5784,13 @@ def test_service_set_broadcast_network_makes_you_choose( 'channel on their phones will receive alerts sent from ' 'this service.', ]), - ('live-test', [ + ('live-test-all', [ 'Test', 'Members of the public who have switched on the test ' 'channel on their phones will receive alerts sent from ' 'this service.', ]), - ('live-severe', [ + ('live-severe-all', [ 'Live', 'Members of the public will receive alerts sent from this ' 'service.', @@ -5888,7 +5800,7 @@ def test_service_set_broadcast_network_makes_you_choose( 'Members of the public will receive alerts sent from this ' 'service.', ]), - ('live-government', [ + ('live-government-all', [ 'Government', 'Members of the public will receive alerts sent from this ' 'service, even if they’ve opted out.' @@ -5920,10 +5832,10 @@ def test_service_confirm_broadcast_account_type_confirmation_page( @pytest.mark.parametrize( 'value,service_mode,broadcast_channel,allowed_broadcast_provider', [ - ("training-test", "training", "test", "all"), + ("training-test-all", "training", "test", "all"), ("live-test-vodafone", "live", "test", "vodafone"), - ("live-severe", "live", "severe", "all"), - ("live-government", "live", "government", "all"), + ("live-severe-all", "live", "severe", "all"), + ("live-government-all", "live", "government", "all"), ] ) def test_service_confirm_broadcast_account_type_posts_data_to_api_and_redirects( @@ -5963,9 +5875,8 @@ def test_service_confirm_broadcast_account_type_posts_data_to_api_and_redirects( @pytest.mark.parametrize('account_type', ( - 'foo-severe', - 'training-foo', - 'live-foo', + 'foo-test-ee', + 'live-foo-all', 'live-government-foo' )) def test_service_confirm_broadcast_account_type_errors_for_unknown_type(