From e8486433611574e81e1fab54c7ffd8c8ba7e8bf5 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Jun 2021 16:01:59 +0100 Subject: [PATCH 1/6] Simplify provider selection with '-all' suffix Previously we had to cope with two forms of the hyphenated string we use to represent a pending change in broadcast account type. Using "all" to mean "all providers" matches the behaviour in the API [1], and means we can remove some complexity. "training-test-all" isn't ideal, since the provider is irrelevant for a training mode service. However, this isn't much worse than the previous "training-test", noting that the channel also has no relevance. We'll iterate this in later commits. [1]: https://github.com/alphagov/notifications-api/blob/8e1a144f87831142d00149d9f62d8cfd6038d187/migrations/versions/0352_broadcast_provider_types.py#L14 --- app/main/forms.py | 24 ++++----- tests/app/main/views/test_service_settings.py | 53 +++++++++---------- 2 files changed, 35 insertions(+), 42 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 1a538002c..ee04143e5 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2369,9 +2369,7 @@ class ServiceBroadcastAccountTypeField(GovukRadiosField): 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}" + account_type += f"-{broadcast_channel}-{allowed_broadcast_provider}" self.data = account_type @@ -2383,7 +2381,7 @@ 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' + self.provider_restriction = split_values[2] class OptionalServiceBroadcastAccountTypeField(ServiceBroadcastAccountTypeField): @@ -2398,10 +2396,10 @@ class ServiceBroadcastChannelForm(StripWhitespaceForm): '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-test-all", "Training mode"), + ("live-test-all", "Test channel"), + ("live-severe-all", "Live channel"), + ("live-government-all", "Government channel"), ], ) @@ -2412,7 +2410,7 @@ class ServiceBroadcastNetworkForm(StripWhitespaceForm): self.broadcast_channel = broadcast_channel self.network_variant.choices = [ - (f'live-{broadcast_channel}', 'All networks'), + (f'live-{broadcast_channel}-all', 'All networks'), ('', 'A single network'), ] @@ -2444,16 +2442,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/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 575c8e84a..44f826d82 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -5537,28 +5537,28 @@ def test_service_set_broadcast_channel_has_no_radio_selected_for_non_broadcast_s "test", "all", "Training mode", - "training-test", + "training-test-all", ), ( "live", "test", "vodafone", "Test channel", - "live-test", + "live-test-all", ), ( "live", "severe", "all", "Live channel", - "live-severe", + "live-severe-all", ), ( "live", "government", "all", "Government channel", - "live-government", + "live-government-all", ), ] ) @@ -5601,22 +5601,22 @@ def test_service_set_broadcast_channel_has_radio_selected_for_broadcast_service( 'account_type,expected_redirect_endpoint,extra_args', [ ( - 'training-test', + 'training-test-all', '.service_confirm_broadcast_account_type', - {'account_type': 'training-test'}, + {'account_type': 'training-test-all'}, ), ( - 'live-test', + 'live-test-all', '.service_set_broadcast_network', {'broadcast_channel': 'test'}, ), ( - 'live-severe', + 'live-severe-all', '.service_set_broadcast_network', {'broadcast_channel': 'severe'}, ), ( - 'live-government', + 'live-government-all', '.service_set_broadcast_network', {'broadcast_channel': 'government'}, ), @@ -5660,7 +5660,7 @@ def test_service_set_broadcast_channel_redirects( "severe", "all", [ - ("All networks", "live-severe"), + ("All networks", "live-severe-all"), ], ), ( @@ -5668,7 +5668,7 @@ def test_service_set_broadcast_channel_redirects( "government", "all", [ - ("All networks", "live-government"), + ("All networks", "live-government-all"), ], ), ( @@ -5676,7 +5676,7 @@ def test_service_set_broadcast_channel_redirects( "test", "all", [ - ("All networks", "live-test"), + ("All networks", "live-test-all"), ], ), ( @@ -5772,9 +5772,9 @@ def test_service_set_broadcast_network_has_radio_selected( @pytest.mark.parametrize( 'broadcast_channel, provider, choice_type', ( - ('severe', '', 'network_variant'), - ('government', '', 'network_variant'), - ('test', '', 'network_variant'), + ('severe', 'all', 'network_variant'), + ('government', 'all', 'network_variant'), + ('test', 'all', 'network_variant'), ('test', 'o2', 'network'), ('test', 'ee', 'network'), ('test', 'three', 'network'), @@ -5790,11 +5790,11 @@ def test_service_set_broadcast_network( provider, choice_type, ): + expected_result = f'live-{broadcast_channel}-{provider}' + 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) @@ -5844,7 +5844,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 +5872,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 +5888,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 +5920,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 +5963,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( From 5ce76b8b3305c606739e0f04e0a0f635b4d4eece Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Jun 2021 16:38:58 +0100 Subject: [PATCH 2/6] Add property to construct account type string This allows us to start decoupling the form fields from the final, hyphenated string, which we'll do in the next commits. Note that I've also removed the conditional that changes the data of the network field as part of validating it. We shouldn't change data in validations, and having the new property directly above makes it clear there's no need for this code. --- app/main/forms.py | 11 +++++++++-- app/main/views/service_settings.py | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index ee04143e5..7f784d0aa 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2430,11 +2430,18 @@ class ServiceBroadcastNetworkForm(StripWhitespaceForm): thing='a mobile network', ) + @property + def account_type(self): + if self.network_variant.data == f'live-{self.broadcast_channel}-all': + provider = 'all' + else: + provider = self.network.provider_restriction + + return f'live-{self.broadcast_channel}-{provider}' + def validate_network(self, field): if not self.network_variant.data and not field.data: raise ValidationError('Select a mobile network') - if self.network_variant.data == 'all': - field.data = '' class ServiceBroadcastAccountTypeForm(StripWhitespaceForm): diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 8006e8492..f9ef5cef2 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -381,7 +381,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( From ef8cab7fa418e2c33db28d9f0986e944f01034c0 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Jun 2021 16:52:36 +0100 Subject: [PATCH 3/6] Simplify network choice form to use boolean radio This follows the same pattern as in other forms [1]. [1]: https://github.com/alphagov/notifications-admin/blob/1b459d66927718937e0f890c6ca00524ebee8035/app/templates/views/organisations/add-gp-organisation.html#L20 --- app/main/forms.py | 16 +++++----- app/main/views/service_settings.py | 14 ++++----- .../service-set-broadcast-network.html | 6 ++-- tests/app/main/views/test_service_settings.py | 30 ++++++++----------- 4 files changed, 28 insertions(+), 38 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 7f784d0aa..09be3f5b9 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2409,11 +2409,6 @@ class ServiceBroadcastNetworkForm(StripWhitespaceForm): super().__init__(*args, **kwargs) self.broadcast_channel = broadcast_channel - self.network_variant.choices = [ - (f'live-{broadcast_channel}-all', 'All networks'), - ('', 'A single network'), - ] - self.network.choices = [ (f'live-{broadcast_channel}-ee', 'EE'), (f'live-{broadcast_channel}-o2', 'O2'), @@ -2421,9 +2416,12 @@ class ServiceBroadcastNetworkForm(StripWhitespaceForm): (f'live-{broadcast_channel}-three', 'Three'), ] - network_variant = ServiceBroadcastAccountTypeField( + all_networks = OnOffField( 'Choose a mobile network', - thing='a mobile network', + choices=( + (True, 'All networks'), + (False, 'A single network') + ), ) network = OptionalServiceBroadcastAccountTypeField( 'Choose a mobile network', @@ -2432,7 +2430,7 @@ class ServiceBroadcastNetworkForm(StripWhitespaceForm): @property def account_type(self): - if self.network_variant.data == f'live-{self.broadcast_channel}-all': + if self.all_networks.data: provider = 'all' else: provider = self.network.provider_restriction @@ -2440,7 +2438,7 @@ class ServiceBroadcastNetworkForm(StripWhitespaceForm): 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') diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index f9ef5cef2..681052047 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -353,23 +353,21 @@ 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': + provider = current_service.allowed_broadcast_provider + + if provider == 'all': form = ServiceBroadcastNetworkForm( broadcast_channel=broadcast_channel, - network_variant=( - current_service.live, - current_service.broadcast_channel, - current_service.allowed_broadcast_provider, - ), + all_networks=True, ) else: form = ServiceBroadcastNetworkForm( broadcast_channel=broadcast_channel, - network_variant='', + all_networks=False, network=( current_service.live, current_service.broadcast_channel, - current_service.allowed_broadcast_provider + provider, ) ) else: 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 44f826d82..82d6d13b4 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -5649,18 +5649,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-all"), + ("All networks", "True"), ], ), ( @@ -5668,7 +5662,7 @@ def test_service_set_broadcast_channel_redirects( "government", "all", [ - ("All networks", "live-government-all"), + ("All networks", "True"), ], ), ( @@ -5676,7 +5670,7 @@ def test_service_set_broadcast_channel_redirects( "test", "all", [ - ("All networks", "live-test-all"), + ("All networks", "True"), ], ), ( @@ -5684,7 +5678,7 @@ def test_service_set_broadcast_channel_redirects( "test", "ee", [ - ("A single network", ""), + ("A single network", "False"), ("EE", "live-test-ee"), ], ), @@ -5693,7 +5687,7 @@ def test_service_set_broadcast_channel_redirects( "test", "o2", [ - ("A single network", ""), + ("A single network", "False"), ("O2", "live-test-o2"), ], ), @@ -5702,7 +5696,7 @@ def test_service_set_broadcast_channel_redirects( "test", "three", [ - ("A single network", ""), + ("A single network", "False"), ("Three", "live-test-three"), ], ), @@ -5711,7 +5705,7 @@ def test_service_set_broadcast_channel_redirects( "test", "vodafone", [ - ("A single network", ""), + ("A single network", "False"), ("Vodafone", "live-test-vodafone"), ], ), @@ -5720,7 +5714,7 @@ def test_service_set_broadcast_channel_redirects( "severe", "vodafone", [ - ("A single network", ""), + ("A single network", "False"), ("Vodafone", "live-severe-vodafone"), ], ), @@ -5729,7 +5723,7 @@ def test_service_set_broadcast_channel_redirects( "government", "vodafone", [ - ("A single network", ""), + ("A single network", "False"), ("Vodafone", "live-government-vodafone"), ], ), @@ -5793,9 +5787,9 @@ def test_service_set_broadcast_network( expected_result = f'live-{broadcast_channel}-{provider}' if choice_type == 'network_variant': - data = {'network_variant': expected_result} + data = {'all_networks': True} else: - data = {'network_variant': '', 'network': expected_result} + data = {'all_networks': False, 'network': expected_result} client_request.login(platform_admin_user) client_request.post( @@ -5817,7 +5811,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']) From b38cdcad6360a37b9385f33f5b206892b25eb7da Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Jun 2021 17:06:15 +0100 Subject: [PATCH 4/6] Simplify network choice to optional radio buttons Previously this field had to mimic the final hyphenated string of the broadcast account type, even though it was only used to select one of its components. The new, shorter choices make it easier to simplify the test for the POST request. I've also deleted a number of test cases for pre-selected radios. This functionality isn't critical, so we don't need to exhaustively test every single possible combination of values. --- app/main/forms.py | 31 +++--- app/main/views/service_settings.py | 20 +--- tests/app/main/views/test_service_settings.py | 94 +++---------------- 3 files changed, 33 insertions(+), 112 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 09be3f5b9..8007001d9 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): @@ -2384,13 +2391,6 @@ class ServiceBroadcastAccountTypeField(GovukRadiosField): self.provider_restriction = split_values[2] -class OptionalServiceBroadcastAccountTypeField(ServiceBroadcastAccountTypeField): - def pre_validate(self, form): - if self.data is None: - return - super().pre_validate(form) - - class ServiceBroadcastChannelForm(StripWhitespaceForm): channel = ServiceBroadcastAccountTypeField( 'Emergency alerts settings', @@ -2409,13 +2409,6 @@ class ServiceBroadcastNetworkForm(StripWhitespaceForm): super().__init__(*args, **kwargs) self.broadcast_channel = broadcast_channel - 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'), - ] - all_networks = OnOffField( 'Choose a mobile network', choices=( @@ -2423,9 +2416,15 @@ class ServiceBroadcastNetworkForm(StripWhitespaceForm): (False, 'A single network') ), ) - network = OptionalServiceBroadcastAccountTypeField( + network = OptionalGovukRadiosField( 'Choose a mobile network', thing='a mobile network', + choices=( + ('ee', 'EE'), + ('o2', 'O2'), + ('vodafone', 'Vodafone'), + ('three', 'Three'), + ), ) @property @@ -2433,7 +2432,7 @@ class ServiceBroadcastNetworkForm(StripWhitespaceForm): if self.all_networks.data: provider = 'all' else: - provider = self.network.provider_restriction + provider = self.network.data return f'live-{self.broadcast_channel}-{provider}' diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 681052047..2ea9de991 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -355,21 +355,11 @@ def service_set_broadcast_network(service_id, broadcast_channel): if current_service.broadcast_channel == broadcast_channel: provider = current_service.allowed_broadcast_provider - if provider == 'all': - form = ServiceBroadcastNetworkForm( - broadcast_channel=broadcast_channel, - all_networks=True, - ) - else: - form = ServiceBroadcastNetworkForm( - broadcast_channel=broadcast_channel, - all_networks=False, - network=( - current_service.live, - current_service.broadcast_channel, - 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 diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 82d6d13b4..2633e548c 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -5657,74 +5657,13 @@ def test_service_set_broadcast_channel_redirects( ("All networks", "True"), ], ), - ( - "live", - "government", - "all", - [ - ("All networks", "True"), - ], - ), - ( - "live", - "test", - "all", - [ - ("All networks", "True"), - ], - ), ( "live", "test", "ee", [ ("A single network", "False"), - ("EE", "live-test-ee"), - ], - ), - ( - "live", - "test", - "o2", - [ - ("A single network", "False"), - ("O2", "live-test-o2"), - ], - ), - ( - "live", - "test", - "three", - [ - ("A single network", "False"), - ("Three", "live-test-three"), - ], - ), - ( - "live", - "test", - "vodafone", - [ - ("A single network", "False"), - ("Vodafone", "live-test-vodafone"), - ], - ), - ( - "live", - "severe", - "vodafone", - [ - ("A single network", "False"), - ("Vodafone", "live-severe-vodafone"), - ], - ), - ( - "live", - "government", - "vodafone", - [ - ("A single network", "False"), - ("Vodafone", "live-government-vodafone"), + ("EE", "ee"), ], ), ] @@ -5764,33 +5703,26 @@ def test_service_set_broadcast_network_has_radio_selected( @pytest.mark.parametrize( - 'broadcast_channel, provider, choice_type', + 'broadcast_channel, data, expected_result', ( - ('severe', 'all', 'network_variant'), - ('government', 'all', 'network_variant'), - ('test', 'all', '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, ): - expected_result = f'live-{broadcast_channel}-{provider}' - - if choice_type == 'network_variant': - data = {'all_networks': True} - else: - data = {'all_networks': False, 'network': expected_result} - client_request.login(platform_admin_user) client_request.post( 'main.service_set_broadcast_network', From 5bfe5f86de286d85df8d88e3ea819eaa6752c8e6 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Jun 2021 17:23:48 +0100 Subject: [PATCH 5/6] Simplify channel selection using radio buttons This takes a similar approach as in the previous commit. Since the "training channel" doesn't really exist, we need some extra code to pre-select it if a service is already in training mode. As in the previous commit, I've removed a few non-critical test cases where we really don't need to test exhaustively. Note that we also need some specific code to avoid pre-selecting an option for non-broadcast services, which only used to work by fluke: we would try to populate the field with (False, None, 'all'), which isn't a valid combination, so nothing was selected. --- app/main/forms.py | 10 +++--- app/main/views/service_settings.py | 22 +++++++------ tests/app/main/views/test_service_settings.py | 32 ++++++------------- 3 files changed, 26 insertions(+), 38 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 8007001d9..b54f78de4 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2392,14 +2392,14 @@ class ServiceBroadcastAccountTypeField(GovukRadiosField): class ServiceBroadcastChannelForm(StripWhitespaceForm): - channel = ServiceBroadcastAccountTypeField( + channel = GovukRadiosField( 'Emergency alerts settings', thing='mode or channel', choices=[ - ("training-test-all", "Training mode"), - ("live-test-all", "Test channel"), - ("live-severe-all", "Live channel"), - ("live-government-all", "Government channel"), + ("training", "Training mode"), + ("test", "Test channel"), + ("severe", "Live channel"), + ("government", "Government channel"), ], ) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 2ea9de991..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( diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 2633e548c..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-all", - ), - ( - "live", - "test", - "vodafone", - "Test channel", - "live-test-all", - ), - ( - "live", - "severe", - "all", - "Live channel", - "live-severe-all", + "training", ), ( "live", "government", "all", "Government channel", - "live-government-all", + "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-all', + 'training', '.service_confirm_broadcast_account_type', {'account_type': 'training-test-all'}, ), ( - 'live-test-all', + 'test', '.service_set_broadcast_network', {'broadcast_channel': 'test'}, ), ( - 'live-severe-all', + 'severe', '.service_set_broadcast_network', {'broadcast_channel': 'severe'}, ), ( - 'live-government-all', + '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, From 9ada8ad11ee85f047894d0e919f446496d27398c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Jun 2021 17:36:55 +0100 Subject: [PATCH 6/6] Remove redundant code to populate account type This field is now only used on the confirmation page of the settings form, where we pre-populate it with the hyphenated string it expects. --- app/main/forms.py | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index b54f78de4..5d714fd53 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2365,21 +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}-{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