From 979fd9bbe49764c774279f03d9c65914c98b4f15 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 9 Aug 2018 11:59:05 +0100 Subject: [PATCH 1/2] Allow service contact details to be phone number, email or url Service contact details are needed if the upload document permission is enabled - this used to be a link but services can now choose to use a link, email address or phone number. The form to add or change service contact details now gives these options and validates the data according to the type of contact details provided. When validating phone numbers we can't use the existing validation because we want to allow landlines too, so there is a basic check that the phone number is the right length and doesn't include certain characters. --- app/assets/javascripts/main.js | 3 + app/main/forms.py | 39 ++++- app/main/views/service_settings.py | 30 +++- app/templates/components/radios.html | 4 +- app/templates/views/service-settings.html | 2 +- .../views/service-settings/contact_link.html | 32 +++- gulpfile.babel.js | 1 + .../main/test_service_contact_details_form.py | 92 ++++++++++++ tests/app/main/views/test_service_settings.py | 141 +++++++++++++----- 9 files changed, 291 insertions(+), 53 deletions(-) create mode 100644 tests/app/main/test_service_contact_details_form.py diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index 59cf901c8..c04229cac 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -2,6 +2,9 @@ $(() => $("time.timeago").timeago()); $(() => GOVUK.stickAtTopWhenScrolling.init()); +var showHideContent = new GOVUK.ShowHideContent(); +showHideContent.init(); + $(() => GOVUK.modules.start()); $(() => $('.error-message').eq(0).parent('label').next('input').trigger('focus')); diff --git a/app/main/forms.py b/app/main/forms.py index 9f968cd6a..746809b8c 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -10,6 +10,7 @@ from notifications_utils.columns import Columns from notifications_utils.formatters import strip_whitespace from notifications_utils.recipients import ( InvalidPhoneError, + normalise_phone_number, validate_phone_number, ) from wtforms import ( @@ -592,13 +593,41 @@ class ProviderForm(StripWhitespaceForm): priority = IntegerField('Priority', [validators.NumberRange(min=1, max=100, message="Must be between 1 and 100")]) -class ServiceContactLinkForm(StripWhitespaceForm): - url = StringField( - "URL", - validators=[DataRequired(message='Can’t be empty'), - URL(message='Must be a valid URL')] +class ServiceContactDetailsForm(StripWhitespaceForm): + contact_details_type = RadioField( + 'Type of contact details', + choices=[ + ('url', 'Link'), + ('email_address', 'Email address'), + ('phone_number', 'Phone number'), + ], + validators=[DataRequired()] ) + url = StringField("URL") + email_address = EmailField("Email address") + phone_number = StringField("Phone number") + + def validate(self): + + if self.contact_details_type.data == 'url': + self.url.validators = [DataRequired(), URL(message='Must be a valid URL')] + + elif self.contact_details_type.data == 'email_address': + self.email_address.validators = [DataRequired(), Length(min=5, max=255), ValidEmail()] + + elif self.contact_details_type.data == 'phone_number': + # we can't use the existing phone number validation functions here since we want to allow landlines + def valid_phone_number(self, num): + try: + normalise_phone_number(num.data) + return True + except InvalidPhoneError: + raise ValidationError('Must be a valid phone number') + self.phone_number.validators = [DataRequired(), Length(min=5, max=20), valid_phone_number] + + return super().validate() + class ServiceReplyToEmailForm(StripWhitespaceForm): email_address = email_address(label='Email reply to address', gov_user=False) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 539ea21cd..a180fdcba 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -34,7 +34,7 @@ from app.main.forms import ( OrganisationTypeForm, RenameServiceForm, RequestToGoLiveForm, - ServiceContactLinkForm, + ServiceContactDetailsForm, ServiceDataRetentionEditForm, ServiceDataRetentionForm, ServiceEditInboundNumberForm, @@ -337,7 +337,7 @@ def service_switch_can_send_precompiled_letter(service_id): @login_required @user_is_platform_admin def service_switch_can_upload_document(service_id): - form = ServiceContactLinkForm() + form = ServiceContactDetailsForm() # If turning the permission off, or turning it on and the service already has a contact_link, # don't show the form to add the link @@ -346,9 +346,11 @@ def service_switch_can_upload_document(service_id): return redirect(url_for('.service_settings', service_id=service_id)) if form.validate_on_submit(): + contact_type = form.contact_details_type.data + service_api_client.update_service( current_service.id, - contact_link=form.url.data + contact_link=form.data[contact_type] ) switch_service_permissions(service_id, 'upload_document') return redirect(url_for('.service_settings', service_id=service_id)) @@ -397,15 +399,22 @@ def resume_service(service_id): @login_required @user_has_permissions('manage_service') def service_set_contact_link(service_id): - form = ServiceContactLinkForm() + form = ServiceContactDetailsForm() if request.method == 'GET': - form.url.data = current_service.get('contact_link') + contact_details = current_service.get('contact_link') + contact_type = check_contact_details_type(contact_details) + field_to_update = getattr(form, contact_type) + + form.contact_details_type.data = contact_type + field_to_update.data = contact_details if form.validate_on_submit(): + contact_type = form.contact_details_type.data + service_api_client.update_service( current_service.id, - contact_link=form.url.data + contact_link=form.data[contact_type] ) return redirect(url_for('.service_settings', service_id=current_service.id)) @@ -1058,3 +1067,12 @@ def convert_dictionary_to_wtforms_choices_format(dictionary, value, label): return [ (item[value], item[label]) for item in dictionary ] + + +def check_contact_details_type(contact_details): + if contact_details.startswith('http'): + return 'url' + elif '@' in contact_details: + return 'email_address' + else: + return 'phone_number' diff --git a/app/templates/components/radios.html b/app/templates/components/radios.html index fe05defc0..26c9e512f 100644 --- a/app/templates/components/radios.html +++ b/app/templates/components/radios.html @@ -32,8 +32,8 @@ {% endmacro %} -{% macro radio(option, disable=[], option_hints={}) %} -
+{% macro radio(option, disable=[], option_hints={}, data_target=None) %} +

- {{ 'Change link on' if 'upload_document' in current_service.permissions else 'Add link for' }} ‘Download your document’ page + {{ 'Change' if 'upload_document' in current_service.permissions else 'Add' }} contact details for ‘Download your document’ page

- When you send users a document to download, you need to include a link to your service + When you send users a document to download, you need to include the contact details for your service on the download page. This is so users can contact you if there’s a problem (for example, if the link to download the document has expired).

-
- {{ textbox(form.url, width='1-1') }} + + + {% call radios_wrapper(form.contact_details_type, hide_legend=true) %} + {% for option in form.contact_details_type %} + {% if option.data == 'url' %} + {{ radio(option, data_target="url-type") }} +
+ {{ textbox(form.url, label=' ', width='1-1') }} +
+ {% elif option.data == 'email_address' %} + {{ radio(option, data_target="email-address-type") }} +
+ {{ textbox(form.email_address, label=' ', width='1-1') }} +
+ {% elif option.data == 'phone_number' %} + {{ radio(option, data_target="phone-number-type") }} +
+ {{ textbox(form.phone_number, label=' ', width='1-1') }} +
+ {% endif %} + {% endfor %} + {% endcall %} + {{ page_footer( 'Save', back_link=url_for('.service_settings', service_id=current_service.id), diff --git a/gulpfile.babel.js b/gulpfile.babel.js index 823e237f0..c8c43188c 100644 --- a/gulpfile.babel.js +++ b/gulpfile.babel.js @@ -58,6 +58,7 @@ gulp.task('copy:govuk_template:fonts', () => gulp.src(paths.template + 'assets/s gulp.task('javascripts', () => gulp .src([ paths.toolkit + 'javascripts/govuk/modules.js', + paths.toolkit + 'javascripts/govuk/show-hide-content.js', paths.toolkit + 'javascripts/govuk/stop-scrolling-at-footer.js', paths.toolkit + 'javascripts/govuk/stick-at-top-when-scrolling.js', paths.src + 'javascripts/detailsPolyfill.js', diff --git a/tests/app/main/test_service_contact_details_form.py b/tests/app/main/test_service_contact_details_form.py new file mode 100644 index 000000000..26a3e2367 --- /dev/null +++ b/tests/app/main/test_service_contact_details_form.py @@ -0,0 +1,92 @@ +import pytest + +from app.main.forms import ServiceContactDetailsForm + + +def test_form_fails_validation_with_no_radio_buttons_selected(app_): + with app_.test_request_context(method='POST', data={}): + form = ServiceContactDetailsForm() + + assert not form.validate_on_submit() + assert len(form.errors) == 1 + assert form.errors['contact_details_type'] == ['Not a valid choice'] + + +@pytest.mark.parametrize('selected_radio_button, selected_text_box, text_box_data', [ + ('email_address', 'url', 'http://www.example.com'), + ('phone_number', 'url', 'http://www.example.com'), + ('url', 'email_address', 'user@example.com'), + ('phone_number', 'email_address', 'user@example.com'), + ('url', 'phone_number', '0207 123 4567'), + ('email_address', 'phone_number', '0207 123 4567'), +]) +def test_form_fails_validation_when_radio_button_selected_and_text_box_filled_in_do_not_match( + app_, + selected_radio_button, + selected_text_box, + text_box_data +): + data = {'contact_details_type': selected_radio_button, selected_text_box: text_box_data} + + with app_.test_request_context(method='POST', data=data): + form = ServiceContactDetailsForm() + + assert not form.validate_on_submit() + assert len(form.errors) == 1 + assert form.errors[selected_radio_button] == ['This field is required.'] + + +@pytest.mark.parametrize('selected_field, url, email_address, phone_number', [ + ('url', 'http://www.example.com', 'invalid-email.com', 'phone'), + ('email_address', 'www.invalid-url.com', 'me@example.com', 'phone'), + ('phone_number', 'www.invalid-url.com', 'invalid-email.com', '0207 123 4567'), +]) +def test_form_only_validates_the_field_which_matches_the_selected_radio_button( + app_, + selected_field, + url, + email_address, + phone_number, +): + data = {'contact_details_type': selected_field, + 'url': url, + 'email_address': email_address, + 'phone_number': phone_number} + + with app_.test_request_context(method='POST', data=data): + form = ServiceContactDetailsForm() + + assert form.validate_on_submit() + + +def test_form_url_validation_fails_with_invalid_url_field(app_): + data = {'contact_details_type': 'url', 'url': 'www.example.com'} + + with app_.test_request_context(method='POST', data=data): + form = ServiceContactDetailsForm() + + assert not form.validate_on_submit() + assert len(form.errors) == 1 + assert len(form.errors['url']) == 1 + + +def test_form_email_validation_fails_with_invalid_email_address_field(app_): + data = {'contact_details_type': 'email_address', 'email_address': '1@co'} + + with app_.test_request_context(method='POST', data=data): + form = ServiceContactDetailsForm() + + assert not form.validate_on_submit() + assert len(form.errors) == 1 + assert len(form.errors['email_address']) == 2 + + +def test_form_phone_number_validation_fails_with_invalid_phone_number_field(app_): + data = {'contact_details_type': 'phone_number', 'phone_number': '1235 A'} + + with app_.test_request_context(method='POST', data=data): + form = ServiceContactDetailsForm() + + assert not form.validate_on_submit() + assert len(form.errors) == 1 + assert form.errors['phone_number'] == ['Must be a valid phone number'] diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index a7d14221a..e20604f4f 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -2060,12 +2060,12 @@ def test_switch_service_enable_international_sms( assert mocked_fn.call_args[0][0] == service_one['id'] -@pytest.mark.parametrize('start_permissions, contact_link, end_permissions', [ +@pytest.mark.parametrize('start_permissions, contact_details, end_permissions', [ (['upload_document'], 'http://example.com/', []), (['upload_document'], None, []), - ([], 'http://example.com/', ['upload_document']), + ([], '0207 123 4567', ['upload_document']), ]) -def test_service_switch_can_upload_document_changes_the_permission_if_not_adding_the_permission_without_a_contact_link( +def test_service_switch_can_upload_document_changes_the_permission_if_service_contact_details_exist( logged_in_platform_admin_client, service_one, mock_update_service, @@ -2075,11 +2075,11 @@ def test_service_switch_can_upload_document_changes_the_permission_if_not_adding no_letter_contact_blocks, single_sms_sender, start_permissions, - contact_link, + contact_details, end_permissions, ): service_one['permissions'] = start_permissions - service_one['contact_link'] = contact_link + service_one['contact_link'] = contact_details response = logged_in_platform_admin_client.get( url_for('main.service_switch_can_upload_document', service_id=SERVICE_ONE_ID), @@ -2090,10 +2090,10 @@ def test_service_switch_can_upload_document_changes_the_permission_if_not_adding SERVICE_ONE_ID, permissions=end_permissions, ) - assert page.h1.text.strip() == 'Settings' + assert normalize_spaces(page.h1.text) == 'Settings' -def test_service_switch_can_upload_document_turning_permission_on_with_no_contact_link_shows_link_form( +def test_service_switch_can_upload_document_turning_permission_on_with_no_contact_details_shows_form( logged_in_platform_admin_client, service_one, mock_get_service_settings_page_common, @@ -2109,10 +2109,15 @@ def test_service_switch_can_upload_document_turning_permission_on_with_no_contac page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert 'upload_document' not in service_one['permissions'] - assert page.h1.text.strip() == "Add link for ‘Download your document’ page" + assert normalize_spaces(page.h1.text) == "Add contact details for ‘Download your document’ page" -def test_service_switch_can_upload_document_lets_contact_link_be_added_and_switches_permission( +@pytest.mark.parametrize('contact_details_type, contact_details_value', [ + ('url', 'http://example.com/'), + ('email_address', 'old@example.com'), + ('phone_number', '0207 12345'), +]) +def test_service_switch_can_upload_document_lets_contact_details_be_added_and_switches_permission( logged_in_platform_admin_client, service_one, mock_update_service, @@ -2121,16 +2126,20 @@ def test_service_switch_can_upload_document_lets_contact_link_be_added_and_switc no_reply_to_email_addresses, no_letter_contact_blocks, single_sms_sender, + contact_details_type, + contact_details_value, ): + data = {'contact_details_type': contact_details_type, contact_details_type: contact_details_value} + response = logged_in_platform_admin_client.post( url_for('main.service_switch_can_upload_document', service_id=SERVICE_ONE_ID), - data={'url': 'http://example.com/'}, + data=data, follow_redirects=True ) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert 'upload_document' in mock_update_service.call_args[1]['permissions'] - assert page.h1.text.strip() == 'Settings' + assert normalize_spaces(page.h1.text) == 'Settings' def test_archive_service_after_confirm( @@ -2297,16 +2306,60 @@ def test_cant_resume_active_service( assert 'Resume service' not in {a.text for a in page.find_all('a', class_='button')} -def test_service_set_contact_link_displays_the_contact_link(client_request, service_one): - service_one['contact_link'] = 'http://example.com/' +@pytest.mark.parametrize('contact_details_type, contact_details_value', [ + ('url', 'http://example.com/'), + ('email_address', 'me@example.com'), + ('phone_number', '0207 123 4567'), +]) +def test_service_set_contact_link_prefills_the_form_with_the_existing_contact_details( + client_request, + service_one, + contact_details_type, + contact_details_value, +): + service_one['contact_link'] = contact_details_value page = client_request.get( 'main.service_set_contact_link', service_id=SERVICE_ONE_ID ) - assert page.find('input').get('value') == 'http://example.com/' + assert page.find('input', attrs={'name': 'contact_details_type', 'value': contact_details_type}).has_attr('checked') + assert page.find('input', {'id': contact_details_type}).get('value') == contact_details_value -def test_service_set_contact_link_updates_contact_link_and_redirects_to_settings_page( +@pytest.mark.parametrize('contact_details_type, old_value, new_value', [ + ('url', 'http://example.com/', 'http://new-link.com/'), + ('email_address', 'old@example.com', 'new@example.com'), + ('phone_number', '0207 12345', '0207 56789'), +]) +def test_service_set_contact_link_updates_contact_details_and_redirects_to_settings_page( + client_request, + service_one, + mock_update_service, + mock_get_service_settings_page_common, + mock_get_service_organisation, + no_reply_to_email_addresses, + no_letter_contact_blocks, + single_sms_sender, + contact_details_type, + old_value, + new_value, +): + service_one['contact_link'] = old_value + + page = client_request.post( + 'main.service_set_contact_link', service_id=SERVICE_ONE_ID, + _data={ + 'contact_details_type': contact_details_type, + contact_details_type: new_value, + }, + _follow_redirects=True + ) + + assert page.h1.text == 'Settings' + mock_update_service.assert_called_once_with(SERVICE_ONE_ID, contact_link=new_value) + + +def test_service_set_contact_link_updates_contact_details_for_the_selected_field_when_multiple_textboxes_contain_data( client_request, service_one, mock_update_service, @@ -2316,48 +2369,68 @@ def test_service_set_contact_link_updates_contact_link_and_redirects_to_settings no_letter_contact_blocks, single_sms_sender, ): - service_one['contact_link'] = 'http://example.com/' + service_one['contact_link'] = 'http://www.old-url.com' page = client_request.post( 'main.service_set_contact_link', service_id=SERVICE_ONE_ID, _data={ - 'url': 'http://new-link.com/', + 'contact_details_type': 'url', + 'url': 'http://www.new-url.com', + 'email_address': 'me@example.com', + 'phone_number': '0207 123 4567' }, _follow_redirects=True ) assert page.h1.text == 'Settings' - mock_update_service.assert_called_once_with( - SERVICE_ONE_ID, - contact_link='http://new-link.com/', + mock_update_service.assert_called_once_with(SERVICE_ONE_ID, contact_link='http://www.new-url.com') + + +def test_service_set_contact_link_displays_error_message_when_no_radio_button_selected( + client_request, + service_one +): + page = client_request.post( + 'main.service_set_contact_link', service_id=SERVICE_ONE_ID, + _data={ + 'contact_details_type': None, + 'url': '', + 'email_address': '', + 'phone_number': '', + }, + _follow_redirects=True ) + assert normalize_spaces(page.find('span', class_='error-message').text) == 'Not a valid choice' + assert normalize_spaces(page.h1.text) == "Add contact details for ‘Download your document’ page" -@pytest.mark.parametrize('new_link', ['', 'not/a-valid/link']) -def test_service_set_contact_link_does_not_update_invalid_link( +@pytest.mark.parametrize('contact_details_type, invalid_value, error', [ + ('url', 'invalid.com/', 'Must be a valid URL'), + ('email_address', 'me@co', 'Enter a valid email address'), + ('phone_number', 'abcde', 'Must be a valid phone number'), +]) +def test_service_set_contact_link_does_not_update_invalid_contact_details( mocker, client_request, service_one, - mock_get_service_settings_page_common, - mock_get_service_organisation, - no_reply_to_email_addresses, - no_letter_contact_blocks, - single_sms_sender, - new_link, + contact_details_type, + invalid_value, + error, ): - update_mock = mocker.patch('app.service_api_client.update_service') service_one['contact_link'] = 'http://example.com/' + service_one['permissions'].append('upload_document') page = client_request.post( 'main.service_set_contact_link', service_id=SERVICE_ONE_ID, _data={ - 'url': new_link, + 'contact_details_type': contact_details_type, + contact_details_type: invalid_value, }, _follow_redirects=True ) - assert page.h1.text.strip() == "Add link for ‘Download your document’ page" - update_mock.assert_not_called() + assert normalize_spaces(page.find('span', class_='error-message').text) == error + assert normalize_spaces(page.h1.text) == "Change contact details for ‘Download your document’ page" def test_contact_link_is_displayed_with_upload_document_permission( @@ -2374,7 +2447,7 @@ def test_contact_link_is_displayed_with_upload_document_permission( page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert response.status_code == 200 - assert 'Contact link' in page.text + assert 'Contact details' in page.text def test_contact_link_is_not_displayed_without_the_upload_document_permission( @@ -2390,7 +2463,7 @@ def test_contact_link_is_not_displayed_without_the_upload_document_permission( page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert response.status_code == 200 - assert 'Contact link' not in page.text + assert 'Contact details' not in page.text @pytest.mark.parametrize('endpoint, permissions, expected_p', [ From a978a2fbe55756983398f6f9801275736124e44f Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 13 Aug 2018 12:24:52 +0100 Subject: [PATCH 2/2] Refactor contact link template --- .../views/service-settings/contact_link.html | 22 +++++-------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/app/templates/views/service-settings/contact_link.html b/app/templates/views/service-settings/contact_link.html index 1e70939e1..7b1463899 100644 --- a/app/templates/views/service-settings/contact_link.html +++ b/app/templates/views/service-settings/contact_link.html @@ -22,22 +22,12 @@ {% call radios_wrapper(form.contact_details_type, hide_legend=true) %} {% for option in form.contact_details_type %} - {% if option.data == 'url' %} - {{ radio(option, data_target="url-type") }} -
- {{ textbox(form.url, label=' ', width='1-1') }} -
- {% elif option.data == 'email_address' %} - {{ radio(option, data_target="email-address-type") }} -
- {{ textbox(form.email_address, label=' ', width='1-1') }} -
- {% elif option.data == 'phone_number' %} - {{ radio(option, data_target="phone-number-type") }} -
- {{ textbox(form.phone_number, label=' ', width='1-1') }} -
- {% endif %} + {% set data_target = option.data.replace('_', '-') ~ "-type" %} + + {{ radio(option, data_target=data_target) }} +
+ {{ textbox(form|attr(option.data), label=' ', width='1-1') }} +
{% endfor %} {% endcall %}