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 %} + {% set data_target = option.data.replace('_', '-') ~ "-type" %} + + {{ radio(option, data_target=data_target) }} +
+ {{ textbox(form|attr(option.data), label=' ', width='1-1') }} +
+ {% 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', [