From 0728d9b4fb1c3d89400df97050e5d70d10170e51 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 18 Jun 2019 15:26:04 +0100 Subject: [PATCH] Add validation for agreement accepted on behalf of MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the user has selected that they are accepting the agreement on behalf of someone else then we need to make the they provide that person’s details. If they’ve selected that they are accepting the agreement themselves then we have to ignore what they might have put in the ‘on behalf of boxes’ (for example if they filled them out but then changed their mind). --- app/main/forms.py | 27 +++++ app/main/views/agreement.py | 14 +-- tests/app/main/views/test_agreement.py | 149 ++++++++++++++++++++----- 3 files changed, 151 insertions(+), 39 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index d359e6b8a..2dc749e6c 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1466,6 +1466,23 @@ class GoLiveNotesForm(StripWhitespaceForm): class AcceptAgreementForm(StripWhitespaceForm): + @classmethod + def from_organisation(cls, org): + + if org.agreement_signed_on_behalf_of_name and org.agreement_signed_on_behalf_of_email_address: + who = 'someone-else' + elif org.agreement_signed_version: # only set if user has submitted form previously + who = 'me' + else: + who = None + + return cls( + version=org.agreement_signed_version, + who=who, + on_behalf_of_name=org.agreement_signed_on_behalf_of_name, + on_behalf_of_email=org.agreement_signed_on_behalf_of_email_address, + ) + version = StringField( 'Which version of the agreement are you accepting?' ) @@ -1495,6 +1512,16 @@ class AcceptAgreementForm(StripWhitespaceForm): gov_user=False, ) + def __validate_if_nominating(self, field): + if self.who.data == 'someone-else': + if not field.data: + raise ValidationError('Can’t be empty') + else: + field.data = '' + + validate_on_behalf_of_name = __validate_if_nominating + validate_on_behalf_of_email = __validate_if_nominating + def validate_version(self, field): try: float(field.data) diff --git a/app/main/views/agreement.py b/app/main/views/agreement.py index 5c339648b..9e2b8de27 100644 --- a/app/main/views/agreement.py +++ b/app/main/views/agreement.py @@ -33,20 +33,10 @@ def service_agreement(service_id): @login_required def service_accept_agreement(service_id): - org = current_service.organisation - - if not org: + if not current_service.organisation: abort(404) - form = AcceptAgreementForm( - version=org.agreement_signed_version, - who='someone-else' if ( - org.agreement_signed_on_behalf_of_name - and org.agreement_signed_on_behalf_of_email_address - ) else 'me', - on_behalf_of_name=org.agreement_signed_on_behalf_of_name, - on_behalf_of_email=org.agreement_signed_on_behalf_of_email_address, - ) + form = AcceptAgreementForm.from_organisation(current_service.organisation) if form.validate_on_submit(): current_service.organisation.update( diff --git a/tests/app/main/views/test_agreement.py b/tests/app/main/views/test_agreement.py index 9d2d23562..feacb82f3 100644 --- a/tests/app/main/views/test_agreement.py +++ b/tests/app/main/views/test_agreement.py @@ -1,5 +1,6 @@ from functools import partial from io import BytesIO +from unittest.mock import call import pytest from flask import url_for @@ -131,10 +132,12 @@ def test_show_accept_agreement_page( 'I’m accepting the agreement' ) assert page.select('input[name=who]')[0]['value'] == 'me' + assert 'checked' not in page.select('input[name=who]')[0] assert normalize_spaces(page.select_one('label[for=who-1]').text) == ( 'I’m accepting the agreement on behalf of someone else' ) assert page.select('input[name=who]')[1]['value'] == 'someone-else' + assert 'checked' not in page.select('input[name=who]')[1] assert normalize_spaces(page.select_one('label[for=on_behalf_of_name]').text) == ( 'Who are you accepting the agreement on behalf of?' @@ -170,45 +173,142 @@ def test_accept_agreement_page_populates( ('on_behalf_of_name', 'Firstname Lastname'), ('on_behalf_of_email', 'test@example.com'), ] + assert 'checked' not in page.select('input[name=who]')[0] + assert page.select('input[name=who]')[1]['checked'] == '' -def test_accept_agreement_page_validates( - client_request, - mock_get_service_organisation, -): - page = client_request.post( - 'main.service_accept_agreement', - service_id=SERVICE_ONE_ID, - _data={ +@pytest.mark.parametrize('data, expected_errors', ( + ( + { 'version': '', 'who': '', 'on_behalf_of_name': '', 'on_behalf_of_email': '', }, + [ + 'Must be a number', + 'This field is required.', + ], + ), + ( + { + 'version': 'one point two', + 'who': 'me', + 'on_behalf_of_name': '', + 'on_behalf_of_email': '', + }, + [ + 'Must be a number', + ], + ), + ( + { + 'version': '1.2', + 'who': 'someone-else', + 'on_behalf_of_name': '', + 'on_behalf_of_email': '', + }, + [ + 'Can’t be empty', + 'Can’t be empty', + ], + ), + ( + { + 'version': '1.2', + 'who': 'someone-else', + 'on_behalf_of_name': 'Firstname Lastname', + 'on_behalf_of_email': '', + }, + [ + 'Can’t be empty', + ], + ), + ( + { + 'version': '1.2', + 'who': 'someone-else', + 'on_behalf_of_name': '', + 'on_behalf_of_email': 'test@example.com', + }, + [ + 'Can’t be empty', + ], + ), + +)) +def test_accept_agreement_page_validates( + client_request, + mock_get_service_organisation, + data, + expected_errors, +): + page = client_request.post( + 'main.service_accept_agreement', + service_id=SERVICE_ONE_ID, + _data=data, _expected_status=200, ) assert [ error.text.strip() for error in page.select('.error-message') - ] == [ - 'Must be a number', - 'This field is required.', - ] + ] == expected_errors -def test_accept_agreement_page_persists( - client_request, - mock_get_service_organisation, - mock_update_organisation, -): - client_request.post( - 'main.service_accept_agreement', - service_id=SERVICE_ONE_ID, - _data={ +@pytest.mark.parametrize('data, expected_persisted', ( + ( + { 'version': '1.2', 'who': 'someone-else', 'on_behalf_of_name': 'Firstname Lastname', 'on_behalf_of_email': 'test@example.com', }, + call( + '7aa5d4e9-4385-4488-a489-07812ba13383', + agreement_signed_version=1.2, + agreement_signed_on_behalf_of_name='Firstname Lastname', + agreement_signed_on_behalf_of_email_address='test@example.com', + ) + ), + ( + { + 'version': '1.2', + 'who': 'me', + 'on_behalf_of_name': 'Firstname Lastname', + 'on_behalf_of_email': 'test@example.com', + }, + call( + '7aa5d4e9-4385-4488-a489-07812ba13383', + agreement_signed_version=1.2, + agreement_signed_on_behalf_of_name='', + agreement_signed_on_behalf_of_email_address='', + ) + ), + ( + { + 'version': '1.2', + 'who': 'me', + 'on_behalf_of_name': '', + 'on_behalf_of_email': '', + }, + call( + '7aa5d4e9-4385-4488-a489-07812ba13383', + agreement_signed_version=1.2, + agreement_signed_on_behalf_of_name='', + agreement_signed_on_behalf_of_email_address='', + ) + ), +)) +def test_accept_agreement_page_persists( + client_request, + mock_get_service_organisation, + mock_update_organisation, + data, + expected_persisted, +): + client_request.post( + 'main.service_accept_agreement', + service_id=SERVICE_ONE_ID, + _data=data, _expected_status=302, _expected_redirect=url_for( 'main.service_confirm_agreement', @@ -216,12 +316,7 @@ def test_accept_agreement_page_persists( _external=True, ), ) - mock_update_organisation.assert_called_once_with( - '7aa5d4e9-4385-4488-a489-07812ba13383', - agreement_signed_version=1.2, - agreement_signed_on_behalf_of_name='Firstname Lastname', - agreement_signed_on_behalf_of_email_address='test@example.com', - ) + assert mock_update_organisation.call_args_list == [expected_persisted] @pytest.mark.parametrize('name, email, expected_paragraph', (