From 7d66dadcd73a390c8d07d06c270e74644a7e78d0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 May 2021 13:26:13 +0100 Subject: [PATCH 1/7] Add a confirmation checkbox for live broadcasts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We want people to be really sure before sending a live broadcast, not just clicking through the green buttons. This commit adds a checkbox which explains exactly the consequences of what they’re about to do, tailored to the channel they’re on, and the area chosen by the person creating the alert. --- app/main/forms.py | 45 ++++++ app/main/views/broadcast.py | 17 ++- .../views/broadcast/view-message.html | 11 ++ tests/app/main/views/test_broadcast.py | 133 ++++++++++++++++-- 4 files changed, 196 insertions(+), 10 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 065b62a6d..e18b346fd 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1,3 +1,4 @@ +import math import weakref from datetime import datetime, timedelta from itertools import chain @@ -1288,6 +1289,50 @@ class NewBroadcastForm(StripWhitespaceForm): return self.content.data == 'template' +class ConfirmBroadcastForm(StripWhitespaceForm): + + def __init__(self, *args, service_is_live, channel, max_phones, **kwargs): + super().__init__(*args, **kwargs) + + self.confirm.label.text = self.generate_label(channel, max_phones) + + if service_is_live: + self.confirm.validators += (DataRequired(),) + + confirm = GovukCheckboxField("Confirm") + + @staticmethod + def generate_label(channel, max_phones): + if channel == 'test': + return ( + 'I understand this will alert anyone who has switched ' + 'on the test channel' + ) + if channel == 'severe': + return ( + f'I understand this will alert {ConfirmBroadcastForm.format_number_generic(max_phones)} ' + 'of people' + ) + if channel == 'government': + return ( + f'I understand this will alert {ConfirmBroadcastForm.format_number_generic(max_phones)} ' + 'of people, even if they’ve opted out' + ) + + @staticmethod + def format_number_generic(count): + for threshold, message in ( + (1_000_000, 'millions'), + (100_000, 'hundreds of thousands'), + (10_000, 'tens of thousands'), + (1_000, 'thousands'), + (100, 'hundreds'), + (-math.inf, 'an unknown number') + ): + if count >= threshold: + return message + + class BaseTemplateForm(StripWhitespaceForm): name = GovukTextInputField( "Template name", diff --git a/app/main/views/broadcast.py b/app/main/views/broadcast.py index a808e0a29..7e6a8c0d0 100644 --- a/app/main/views/broadcast.py +++ b/app/main/views/broadcast.py @@ -14,6 +14,7 @@ from app.main.forms import ( BroadcastAreaForm, BroadcastAreaFormWithSelectAll, BroadcastTemplateForm, + ConfirmBroadcastForm, NewBroadcastForm, SearchByNameForm, ) @@ -401,6 +402,11 @@ def view_broadcast(service_id, broadcast_message_id): back_link_endpoint, service_id=current_service.id, ), + form=ConfirmBroadcastForm( + service_is_live=current_service.live, + channel=current_service.broadcast_channel, + max_phones=broadcast_message.count_of_phones_likely, + ), ) @@ -414,6 +420,12 @@ def approve_broadcast_message(service_id, broadcast_message_id): service_id=current_service.id, ) + form = ConfirmBroadcastForm( + service_is_live=current_service.live, + channel=current_service.broadcast_channel, + max_phones=broadcast_message.count_of_phones_likely, + ) + if broadcast_message.status != 'pending-approval': return redirect(url_for( '.view_current_broadcast', @@ -421,14 +433,15 @@ def approve_broadcast_message(service_id, broadcast_message_id): broadcast_message_id=broadcast_message.id, )) - broadcast_message.approve_broadcast() - if current_service.trial_mode: + broadcast_message.approve_broadcast() return redirect(url_for( '.broadcast_tour', service_id=current_service.id, step_index=6, )) + elif form.validate_on_submit(): + broadcast_message.approve_broadcast() return redirect(url_for( '.view_current_broadcast', diff --git a/app/templates/views/broadcast/view-message.html b/app/templates/views/broadcast/view-message.html index b9de5bcf2..5a447b032 100644 --- a/app/templates/views/broadcast/view-message.html +++ b/app/templates/views/broadcast/view-message.html @@ -92,6 +92,17 @@ wants to broadcast {{ broadcast_message.template.name }} + {% if current_service.live %} + {{ form.confirm(param_extensions={ + 'formGroup': { + 'classes': 'govuk-!-margin-bottom-4' + } + }) }} + {% else %} +

+ No phones will get this alert. +

+ {% endif %} {{ page_footer( "Start broadcasting now", delete_link=url_for('main.reject_broadcast_message', service_id=current_service.id, broadcast_message_id=broadcast_message.id), diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index 804505afb..022075fd1 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -1710,8 +1710,10 @@ def test_view_pending_broadcast( normalize_spaces(page.select_one('.banner').text) ) == ( 'Test User wants to broadcast Example template ' + 'No phones will get this alert. ' 'Start broadcasting now Reject this alert' ) + assert not page.select('.banner input[type=checkbox]') form = page.select_one('form.banner') assert form['method'] == 'post' @@ -1764,6 +1766,7 @@ def test_view_pending_broadcast_without_template( normalize_spaces(page.select_one('.banner').text) ) == ( 'Test User wants to broadcast No template test ' + 'No phones will get this alert. ' 'Start broadcasting now Reject this alert' ) assert ( @@ -1807,6 +1810,7 @@ def test_view_pending_broadcast_from_api_call( normalize_spaces(page.select_one('.banner').text) ) == ( 'An API call wants to broadcast abc123 ' + 'No phones will get this alert. ' 'Start broadcasting now Reject this alert' ) assert ( @@ -1817,6 +1821,116 @@ def test_view_pending_broadcast_from_api_call( ) +@pytest.mark.parametrize('channel, expected_label_text', ( + ('test', ( + 'I understand this will alert anyone who has switched on the test channel' + )), + ('severe', ( + 'I understand this will alert millions of people' + )), + ('government', ( + 'I understand this will alert millions of people, even if they’ve opted out' + )), +)) +@freeze_time('2020-02-22T22:22:22.000000') +def test_checkbox_to_confirm_non_training_broadcasts( + mocker, + client_request, + service_one, + active_user_with_permissions, + fake_uuid, + channel, + expected_label_text, +): + mocker.patch( + 'app.broadcast_message_api_client.get_broadcast_message', + return_value=broadcast_message_json( + id_=fake_uuid, + service_id=SERVICE_ONE_ID, + template_id=None, + created_by_id=None, + status='pending-approval', + ), + ) + service_one['permissions'] += ['broadcast'] + service_one['restricted'] = False + service_one['allowed_broadcast_provider'] = 'all' + service_one['broadcast_channel'] = channel + + page = client_request.get( + '.view_current_broadcast', + service_id=SERVICE_ONE_ID, + broadcast_message_id=fake_uuid, + ) + + label = page.select_one('form.banner label') + + assert label['for'] == 'confirm' + assert ( + normalize_spaces(page.select_one('form.banner label').text) + ) == expected_label_text + assert page.select_one('form.banner input[type=checkbox]')['name'] == 'confirm' + assert page.select_one('form.banner input[type=checkbox]')['value'] == 'y' + + +@pytest.mark.parametrize('channel', ( + 'test', + 'severe', + 'government', +)) +@pytest.mark.parametrize('post_data', ( + pytest.param({}, marks=pytest.mark.xfail), + {'confirm': 'y'}, +)) +@freeze_time('2020-02-22T22:22:22.000000') +def test_confirm_approve_non_training_broadcasts( + mocker, + client_request, + service_one, + active_user_with_permissions, + fake_uuid, + mock_update_broadcast_message, + mock_update_broadcast_message_status, + channel, + post_data, +): + mocker.patch( + 'app.broadcast_message_api_client.get_broadcast_message', + return_value=broadcast_message_json( + id_=fake_uuid, + service_id=SERVICE_ONE_ID, + template_id=None, + created_by_id=None, + status='pending-approval', + ), + ) + service_one['permissions'] += ['broadcast'] + service_one['restricted'] = False + service_one['allowed_broadcast_provider'] = 'all' + service_one['broadcast_channel'] = channel + + client_request.post( + '.view_current_broadcast', + service_id=SERVICE_ONE_ID, + broadcast_message_id=fake_uuid, + _data=post_data + ) + + mock_update_broadcast_message.assert_called_once_with( + service_id=SERVICE_ONE_ID, + broadcast_message_id=fake_uuid, + data={ + 'starts_at': '2020-02-22T22:22:22', + 'finishes_at': '2020-02-23T02:22:22', + }, + ) + mock_update_broadcast_message_status.assert_called_once_with( + 'broadcasting', + service_id=SERVICE_ONE_ID, + broadcast_message_id=fake_uuid, + ) + + @freeze_time('2020-02-22T22:22:22.000000') def test_cant_approve_own_broadcast( mocker, @@ -1997,33 +2111,34 @@ def test_view_only_user_cant_approve_broadcast( assert not page.select_one('.banner a') -@pytest.mark.parametrize('trial_mode, initial_status, expected_approval, expected_redirect', ( - (True, 'draft', False, partial( +@pytest.mark.parametrize('trial_mode, initial_status, post_data, expected_approval, expected_redirect', ( + (True, 'draft', {}, False, partial( url_for, '.view_current_broadcast', broadcast_message_id=sample_uuid, )), - (True, 'pending-approval', True, partial( + (True, 'pending-approval', {}, True, partial( url_for, '.broadcast_tour', step_index=6, )), - (False, 'pending-approval', True, partial( + (False, 'pending-approval', {}, False, lambda service_id, _external: None), + (False, 'pending-approval', {'confirm': 'y'}, True, partial( url_for, '.view_current_broadcast', broadcast_message_id=sample_uuid, )), - (True, 'rejected', False, partial( + (True, 'rejected', {}, False, partial( url_for, '.view_current_broadcast', broadcast_message_id=sample_uuid, )), - (True, 'broadcasting', False, partial( + (True, 'broadcasting', {}, False, partial( url_for, '.view_current_broadcast', broadcast_message_id=sample_uuid, )), - (True, 'cancelled', False, partial( + (True, 'cancelled', {}, False, partial( url_for, '.view_current_broadcast', broadcast_message_id=sample_uuid, @@ -2039,6 +2154,7 @@ def test_request_approval( mock_update_broadcast_message, mock_update_broadcast_message_status, initial_status, + post_data, expected_approval, trial_mode, expected_redirect, @@ -2064,7 +2180,8 @@ def test_request_approval( _expected_redirect=expected_redirect( service_id=SERVICE_ONE_ID, _external=True, - ) + ), + _data=post_data, ) if expected_approval: From e2ef8cd36ece2125f46ffc512445c9fee2feedab Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 17 May 2021 12:09:43 +0100 Subject: [PATCH 2/7] =?UTF-8?q?Show=20error=20message=20if=20checkbox=20wa?= =?UTF-8?q?sn=E2=80=99t=20checked?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because we were redirecting in all cases the error message wasn’t being shown. This commit changes the endpoint to respond with content (including an error message) if the `POST` is not successful. --- app/main/forms.py | 4 +- app/main/views/broadcast.py | 27 +++-- tests/app/main/views/test_broadcast.py | 132 +++++++++++++++++-------- 3 files changed, 116 insertions(+), 47 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index e18b346fd..ae85a7740 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1297,7 +1297,9 @@ class ConfirmBroadcastForm(StripWhitespaceForm): self.confirm.label.text = self.generate_label(channel, max_phones) if service_is_live: - self.confirm.validators += (DataRequired(),) + self.confirm.validators += ( + DataRequired('You need to confirm that you understand'), + ) confirm = GovukCheckboxField("Confirm") diff --git a/app/main/views/broadcast.py b/app/main/views/broadcast.py index 7e6a8c0d0..be47568fa 100644 --- a/app/main/views/broadcast.py +++ b/app/main/views/broadcast.py @@ -22,6 +22,15 @@ from app.models.broadcast_message import BroadcastMessage, BroadcastMessages from app.utils import service_has_permission, user_has_permissions +def get_back_link_endpoint(): + return { + 'main.view_current_broadcast': '.broadcast_dashboard', + 'main.view_previous_broadcast': '.broadcast_dashboard_previous', + 'main.view_rejected_broadcast': '.broadcast_dashboard_rejected', + 'main.approve_broadcast_message': '.broadcast_dashboard', + }[request.endpoint] + + @main.route('/services//broadcast-tour/') @user_has_permissions() @service_has_permission('broadcast') @@ -389,17 +398,11 @@ def view_broadcast(service_id, broadcast_message_id): broadcast_message_id=broadcast_message.id, )) - back_link_endpoint = { - 'main.view_current_broadcast': '.broadcast_dashboard', - 'main.view_previous_broadcast': '.broadcast_dashboard_previous', - 'main.view_rejected_broadcast': '.broadcast_dashboard_rejected', - }[request.endpoint] - return render_template( 'views/broadcast/view-message.html', broadcast_message=broadcast_message, back_link=url_for( - back_link_endpoint, + get_back_link_endpoint(), service_id=current_service.id, ), form=ConfirmBroadcastForm( @@ -442,6 +445,16 @@ def approve_broadcast_message(service_id, broadcast_message_id): )) elif form.validate_on_submit(): broadcast_message.approve_broadcast() + else: + return render_template( + 'views/broadcast/view-message.html', + broadcast_message=broadcast_message, + back_link=url_for( + get_back_link_endpoint(), + service_id=current_service.id, + ), + form=form, + ) return redirect(url_for( '.view_current_broadcast', diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index 022075fd1..e4e0cf1b9 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -1878,10 +1878,6 @@ def test_checkbox_to_confirm_non_training_broadcasts( 'severe', 'government', )) -@pytest.mark.parametrize('post_data', ( - pytest.param({}, marks=pytest.mark.xfail), - {'confirm': 'y'}, -)) @freeze_time('2020-02-22T22:22:22.000000') def test_confirm_approve_non_training_broadcasts( mocker, @@ -1892,7 +1888,6 @@ def test_confirm_approve_non_training_broadcasts( mock_update_broadcast_message, mock_update_broadcast_message_status, channel, - post_data, ): mocker.patch( 'app.broadcast_message_api_client.get_broadcast_message', @@ -1913,7 +1908,7 @@ def test_confirm_approve_non_training_broadcasts( '.view_current_broadcast', service_id=SERVICE_ONE_ID, broadcast_message_id=fake_uuid, - _data=post_data + _data={'confirm': 'y'} ) mock_update_broadcast_message.assert_called_once_with( @@ -1931,6 +1926,58 @@ def test_confirm_approve_non_training_broadcasts( ) +@pytest.mark.parametrize('channel', ( + 'test', + 'severe', + 'government', +)) +@freeze_time('2020-02-22T22:22:22.000000') +def test_confirm_approve_non_training_broadcasts_errors_if_not_ticked( + mocker, + client_request, + service_one, + active_user_with_permissions, + fake_uuid, + mock_update_broadcast_message, + mock_update_broadcast_message_status, + channel, +): + mocker.patch( + 'app.broadcast_message_api_client.get_broadcast_message', + return_value=broadcast_message_json( + id_=fake_uuid, + service_id=SERVICE_ONE_ID, + template_id=None, + created_by_id=None, + status='pending-approval', + ), + ) + service_one['permissions'] += ['broadcast'] + service_one['restricted'] = False + service_one['allowed_broadcast_provider'] = 'all' + service_one['broadcast_channel'] = channel + + page = client_request.post( + '.view_current_broadcast', + service_id=SERVICE_ONE_ID, + broadcast_message_id=fake_uuid, + _data={}, + _expected_status=200, + ) + + error_message = page.select_one('form.banner .govuk-error-message') + assert error_message['id'] == 'confirm-error' + assert normalize_spaces(error_message.text) == ( + 'Error: You need to confirm that you understand' + ) + assert page.select_one('form.banner input#confirm')['aria-describedby'] == ( + error_message['id'] + ) + + assert mock_update_broadcast_message.called is False + assert mock_update_broadcast_message_status.called is False + + @freeze_time('2020-02-22T22:22:22.000000') def test_cant_approve_own_broadcast( mocker, @@ -2111,39 +2158,44 @@ def test_view_only_user_cant_approve_broadcast( assert not page.select_one('.banner a') -@pytest.mark.parametrize('trial_mode, initial_status, post_data, expected_approval, expected_redirect', ( - (True, 'draft', {}, False, partial( - url_for, - '.view_current_broadcast', - broadcast_message_id=sample_uuid, - )), - (True, 'pending-approval', {}, True, partial( - url_for, - '.broadcast_tour', - step_index=6, - )), - (False, 'pending-approval', {}, False, lambda service_id, _external: None), - (False, 'pending-approval', {'confirm': 'y'}, True, partial( - url_for, - '.view_current_broadcast', - broadcast_message_id=sample_uuid, - )), - (True, 'rejected', {}, False, partial( - url_for, - '.view_current_broadcast', - broadcast_message_id=sample_uuid, - )), - (True, 'broadcasting', {}, False, partial( - url_for, - '.view_current_broadcast', - broadcast_message_id=sample_uuid, - )), - (True, 'cancelled', {}, False, partial( - url_for, - '.view_current_broadcast', - broadcast_message_id=sample_uuid, - )), -)) +@pytest.mark.parametrize( + 'trial_mode, initial_status, post_data, expected_approval, expected_redirect, expected_status', + ( + (True, 'draft', {}, False, partial( + url_for, + '.view_current_broadcast', + broadcast_message_id=sample_uuid, + ), 302), + (True, 'pending-approval', {}, True, partial( + url_for, + '.broadcast_tour', + step_index=6, + ), 302), + (False, 'pending-approval', {}, False, ( + lambda service_id, _external: None + ), 200), + (False, 'pending-approval', {'confirm': 'y'}, True, partial( + url_for, + '.view_current_broadcast', + broadcast_message_id=sample_uuid, + ), 302), + (True, 'rejected', {}, False, partial( + url_for, + '.view_current_broadcast', + broadcast_message_id=sample_uuid, + ), 302), + (True, 'broadcasting', {}, False, partial( + url_for, + '.view_current_broadcast', + broadcast_message_id=sample_uuid, + ), 302), + (True, 'cancelled', {}, False, partial( + url_for, + '.view_current_broadcast', + broadcast_message_id=sample_uuid, + ), 302), + ) +) @freeze_time('2020-02-22T22:22:22.000000') def test_request_approval( mocker, @@ -2158,6 +2210,7 @@ def test_request_approval( expected_approval, trial_mode, expected_redirect, + expected_status, ): mocker.patch( 'app.broadcast_message_api_client.get_broadcast_message', @@ -2181,6 +2234,7 @@ def test_request_approval( service_id=SERVICE_ONE_ID, _external=True, ), + _expected_status=expected_status, _data=post_data, ) From 859674db3829f3359182c2ae074386127302e3f6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 18 May 2021 15:11:42 +0100 Subject: [PATCH 3/7] Remove duplicative test This case was already covered by `test_confirm_approve_non_training_broadcasts_errors_if_not_ticked` --- tests/app/main/views/test_broadcast.py | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index e4e0cf1b9..199e03694 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -2159,41 +2159,38 @@ def test_view_only_user_cant_approve_broadcast( @pytest.mark.parametrize( - 'trial_mode, initial_status, post_data, expected_approval, expected_redirect, expected_status', + 'trial_mode, initial_status, post_data, expected_approval, expected_redirect', ( (True, 'draft', {}, False, partial( url_for, '.view_current_broadcast', broadcast_message_id=sample_uuid, - ), 302), + )), (True, 'pending-approval', {}, True, partial( url_for, '.broadcast_tour', step_index=6, - ), 302), - (False, 'pending-approval', {}, False, ( - lambda service_id, _external: None - ), 200), + )), (False, 'pending-approval', {'confirm': 'y'}, True, partial( url_for, '.view_current_broadcast', broadcast_message_id=sample_uuid, - ), 302), + )), (True, 'rejected', {}, False, partial( url_for, '.view_current_broadcast', broadcast_message_id=sample_uuid, - ), 302), + )), (True, 'broadcasting', {}, False, partial( url_for, '.view_current_broadcast', broadcast_message_id=sample_uuid, - ), 302), + )), (True, 'cancelled', {}, False, partial( url_for, '.view_current_broadcast', broadcast_message_id=sample_uuid, - ), 302), + )), ) ) @freeze_time('2020-02-22T22:22:22.000000') @@ -2210,7 +2207,6 @@ def test_request_approval( expected_approval, trial_mode, expected_redirect, - expected_status, ): mocker.patch( 'app.broadcast_message_api_client.get_broadcast_message', @@ -2234,7 +2230,6 @@ def test_request_approval( service_id=SERVICE_ONE_ID, _external=True, ), - _expected_status=expected_status, _data=post_data, ) From cee2a4cb7fa7db8ff830fb090a0d3ca248cb86ef Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 18 May 2021 15:12:52 +0100 Subject: [PATCH 4/7] Remove check for ARIA role MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is testing something built into GOV.UK Frontend, we don’t need to test it ourselves. --- tests/app/main/views/test_broadcast.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index 199e03694..200009864 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -1970,9 +1970,6 @@ def test_confirm_approve_non_training_broadcasts_errors_if_not_ticked( assert normalize_spaces(error_message.text) == ( 'Error: You need to confirm that you understand' ) - assert page.select_one('form.banner input#confirm')['aria-describedby'] == ( - error_message['id'] - ) assert mock_update_broadcast_message.called is False assert mock_update_broadcast_message_status.called is False From 8db280317fca7b01651e8693198f3e5cc8ba7a3f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 18 May 2021 15:14:06 +0100 Subject: [PATCH 5/7] Rename helper method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is ambiguous, as it's not used in a lot of the views. There’s precendent for naming these kind of methods here: https://github.com/alphagov/notifications-admin/blob/06cc5b58c7367b6e29dc71a5851492f20ef53a55/app/main/views/broadcast.py#L238 --- app/main/views/broadcast.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/main/views/broadcast.py b/app/main/views/broadcast.py index be47568fa..83dab3b44 100644 --- a/app/main/views/broadcast.py +++ b/app/main/views/broadcast.py @@ -22,7 +22,7 @@ from app.models.broadcast_message import BroadcastMessage, BroadcastMessages from app.utils import service_has_permission, user_has_permissions -def get_back_link_endpoint(): +def _get_back_link_from_view_broadcast_endpoint(): return { 'main.view_current_broadcast': '.broadcast_dashboard', 'main.view_previous_broadcast': '.broadcast_dashboard_previous', @@ -402,7 +402,7 @@ def view_broadcast(service_id, broadcast_message_id): 'views/broadcast/view-message.html', broadcast_message=broadcast_message, back_link=url_for( - get_back_link_endpoint(), + _get_back_link_from_view_broadcast_endpoint(), service_id=current_service.id, ), form=ConfirmBroadcastForm( @@ -450,7 +450,7 @@ def approve_broadcast_message(service_id, broadcast_message_id): 'views/broadcast/view-message.html', broadcast_message=broadcast_message, back_link=url_for( - get_back_link_endpoint(), + _get_back_link_from_view_broadcast_endpoint(), service_id=current_service.id, ), form=form, From ef79edba094a01320777c14037e2aef121f80ddc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 18 May 2021 15:16:15 +0100 Subject: [PATCH 6/7] Remove uneccessary parametrize It proves the behaviour isn't dependent on the channel, but there are other variables we could equally prove that for, which we're not testing here. --- tests/app/main/views/test_broadcast.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index 200009864..518a24ed2 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -1926,11 +1926,6 @@ def test_confirm_approve_non_training_broadcasts( ) -@pytest.mark.parametrize('channel', ( - 'test', - 'severe', - 'government', -)) @freeze_time('2020-02-22T22:22:22.000000') def test_confirm_approve_non_training_broadcasts_errors_if_not_ticked( mocker, @@ -1940,7 +1935,6 @@ def test_confirm_approve_non_training_broadcasts_errors_if_not_ticked( fake_uuid, mock_update_broadcast_message, mock_update_broadcast_message_status, - channel, ): mocker.patch( 'app.broadcast_message_api_client.get_broadcast_message', @@ -1955,7 +1949,7 @@ def test_confirm_approve_non_training_broadcasts_errors_if_not_ticked( service_one['permissions'] += ['broadcast'] service_one['restricted'] = False service_one['allowed_broadcast_provider'] = 'all' - service_one['broadcast_channel'] = channel + service_one['broadcast_channel'] = 'severe' page = client_request.post( '.view_current_broadcast', From f10e3661b971940b03b99a566fb3a22052ea81d0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 18 May 2021 15:21:48 +0100 Subject: [PATCH 7/7] Remove duplicative test Duplicated the assertions in `test_request_approval` --- tests/app/main/views/test_broadcast.py | 53 -------------------------- 1 file changed, 53 deletions(-) diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index 518a24ed2..897766940 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -1873,59 +1873,6 @@ def test_checkbox_to_confirm_non_training_broadcasts( assert page.select_one('form.banner input[type=checkbox]')['value'] == 'y' -@pytest.mark.parametrize('channel', ( - 'test', - 'severe', - 'government', -)) -@freeze_time('2020-02-22T22:22:22.000000') -def test_confirm_approve_non_training_broadcasts( - mocker, - client_request, - service_one, - active_user_with_permissions, - fake_uuid, - mock_update_broadcast_message, - mock_update_broadcast_message_status, - channel, -): - mocker.patch( - 'app.broadcast_message_api_client.get_broadcast_message', - return_value=broadcast_message_json( - id_=fake_uuid, - service_id=SERVICE_ONE_ID, - template_id=None, - created_by_id=None, - status='pending-approval', - ), - ) - service_one['permissions'] += ['broadcast'] - service_one['restricted'] = False - service_one['allowed_broadcast_provider'] = 'all' - service_one['broadcast_channel'] = channel - - client_request.post( - '.view_current_broadcast', - service_id=SERVICE_ONE_ID, - broadcast_message_id=fake_uuid, - _data={'confirm': 'y'} - ) - - mock_update_broadcast_message.assert_called_once_with( - service_id=SERVICE_ONE_ID, - broadcast_message_id=fake_uuid, - data={ - 'starts_at': '2020-02-22T22:22:22', - 'finishes_at': '2020-02-23T02:22:22', - }, - ) - mock_update_broadcast_message_status.assert_called_once_with( - 'broadcasting', - service_id=SERVICE_ONE_ID, - broadcast_message_id=fake_uuid, - ) - - @freeze_time('2020-02-22T22:22:22.000000') def test_confirm_approve_non_training_broadcasts_errors_if_not_ticked( mocker,