Merge pull request #3919 from alphagov/refactor-broadcast-settings

Refactor broadcast settings forms

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'
+            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(
+    channel = GovukRadiosField(
         'Emergency alerts settings',
         thing='mode or channel',
         choices=[
-            ("training-test", "Training mode"),
+            ("training", "Training mode"),
-            ("live-test", "Test channel"),
+            ("test", "Test channel"),
-            ("live-severe", "Live channel"),
+            ("severe", "Live channel"),
-            ("live-government", "Government channel"),
+            ("government", "Government channel"),
         ],
     )
 
@@ -2411,32 +2394,36 @@ class ServiceBroadcastNetworkForm(StripWhitespaceForm):
         super().__init__(*args, **kwargs)
         self.broadcast_channel = broadcast_channel
 
-        self.network_variant.choices = [
+    all_networks = OnOffField(
-            (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=(
+            (True, 'All networks'),
+            (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
+    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", "")
+            ("training-test-all", "")
-        ] +
-        [
-            (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"]
+            for provider in ["all", "ee", "o2", "three", "vodafone"]
         ],
         validators=[DataRequired()]
     )

app/main/views/service_settings.py

@@ -321,25 +321,27 @@ def service_set_permission(service_id, permission):
 @main.route("/services/<uuid:service_id>/service-settings/broadcasts", methods=["GET", "POST"])
 @user_is_platform_admin
 def service_set_broadcast_channel(service_id):
-    form = ServiceBroadcastChannelForm(
+    if current_service.has_permission('broadcast'):
-        channel=(
+        if current_service.live:
-            current_service.live,
+            channel = current_service.broadcast_channel
-            current_service.broadcast_channel,
+        else:
-            'all',
+            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':
+        provider = current_service.allowed_broadcast_provider
-            form = ServiceBroadcastNetworkForm(
+
-                broadcast_channel=broadcast_channel,
+        form = ServiceBroadcastNetworkForm(
-                network_variant=(
+            broadcast_channel=broadcast_channel,
-                    current_service.live,
+            all_networks=provider == 'all',
-                    current_service.broadcast_channel,
+            network=provider if provider != 'all' else None,
-                    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(
             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(

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) %}
+        {% call select_wrapper(form.all_networks, hide_legend=True) %}
-          {% for option in form.network_variant %}
+          {% for option in form.all_networks %}
-            {{ radio(option, data_target='single-network' if option.data == "" else None) }}
+            {{ radio(option, data_target='single-network' if option.data == False else None) }}
           {% endfor %}
         {% endcall %}
         {% call conditional_radio_panel('single-network') %}

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",
+            "training",
-        ),
-        (
-            "live",
-            "test",
-            "vodafone",
-            "Test channel",
-            "live-test",
-        ),
-        (
-            "live",
-            "severe",
-            "all",
-            "Live channel",
-            "live-severe",
         ),
         (
             "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"),
+                ("All networks", "True"),
-            ],
-        ),
-        (
-            "live",
-            "government",
-            "all",
-            [
-                ("All networks", "live-government"),
-            ],
-        ),
-        (
-            "live",
-            "test",
-            "all",
-            [
-                ("All networks", "live-test"),
             ],
         ),
         (
@@ -5684,53 +5648,8 @@ def test_service_set_broadcast_channel_redirects(
             "test",
             "ee",
             [
-                ("A single network", ""),
+                ("A single network", "False"),
-                ("EE", "live-test-ee"),
+                ("EE", "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"),
             ],
         ),
     ]
@@ -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'),
+        ('severe', {'all_networks': True}, 'live-severe-all'),
-        ('government', '', 'network_variant'),
+        ('government', {'all_networks': True}, 'live-government-all'),
-        ('test', '', 'network_variant'),
+        ('test', {'all_networks': True}, 'live-test-all'),
-        ('test', 'o2', 'network'),
+        ('test', {'all_networks': False, 'network': 'o2'}, 'live-test-o2'),
-        ('test', 'ee', 'network'),
+        ('test', {'all_networks': False, 'network': 'ee'}, 'live-test-ee'),
-        ('test', 'three', 'network'),
+        ('test', {'all_networks': False, 'network': 'three'}, 'live-test-three'),
-        ('test', 'vodafone', 'network'),
+        ('test', {'all_networks': False, 'network': 'vodafone'}, 'live-test-vodafone'),
-        ('government', 'vodafone', 'network'),
+        ('government', {'all_networks': False, 'network': 'vodafone'}, 'live-government-vodafone'),
-        ('severe', 'vodafone', 'network'),
+        ('severe', {'all_networks': False, 'network': 'vodafone'}, 'live-severe-vodafone'),
     ),
 )
 def test_service_set_broadcast_network(
     client_request,
     platform_admin_user,
     broadcast_channel,
-    provider,
+    data,
-    choice_type,
+    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-severe-all", "live", "severe", "all"),
-        ("live-government", "live", "government", "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',
+    'foo-test-ee',
-    'training-foo',
+    'live-foo-all',
-    'live-foo',
     'live-government-foo'
 ))
 def test_service_confirm_broadcast_account_type_errors_for_unknown_type(