From e8486433611574e81e1fab54c7ffd8c8ba7e8bf5 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Jun 2021 16:01:59 +0100 Subject: [PATCH] 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(