From e2ef8cd36ece2125f46ffc512445c9fee2feedab Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 17 May 2021 12:09:43 +0100 Subject: [PATCH] =?UTF-8?q?Show=20error=20message=20if=20checkbox=20wasn?= =?UTF-8?q?=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, )