Merge pull request #2219 from alphagov/allow-different-types-of-service-contact-detail

Allow service contact details to be a phone number, email or url

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'));

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)

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'

app/templates/components/radios.html

@@ -32,8 +32,8 @@
   </div>
 {% endmacro %}
 
-{% macro radio(option, disable=[], option_hints={}) %}
-  <div class="multiple-choice">
+{% macro radio(option, disable=[], option_hints={}, data_target=None) %}
+  <div class="multiple-choice" {% if data_target %}data-target="{{ data_target }}"{% endif %}>
     <input
       id="{{ option.id }}" name="{{ option.name }}" type="radio" value="{{ option.data }}"
       {% if option.data in disable %}

app/templates/views/service-settings.html

@@ -47,7 +47,7 @@
         {% endcall %}
 
         {% call settings_row(if_has_permission='upload_document') %}
-          {{ text_field('Contact link') }}
+          {{ text_field('Contact details') }}
           {{ text_field(current_service.contact_link, truncate=true) }}
           {{ edit_field(
               'Change',

app/templates/views/service-settings/contact_link.html

@@ -1,24 +1,36 @@
 {% extends "withnav_template.html" %}
 {% from "components/textbox.html" import textbox %}
 {% from "components/page-footer.html" import page_footer %}
+{% from "components/radios.html" import radio, radios_wrapper %}
 
 {% block service_page_title %}
-  {{ '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
 {% endblock %}
 
 {% block maincolumn_content %}
   <div class="grid-row">
     <div class="column-five-sixths">
       <h1 class="heading-large">
-        {{ '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
       </h1>
       <p>
-        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).
       </p>
-      <form method="post">
-        {{ textbox(form.url, width='1-1') }}
+      <form method="post" novalidate>
+
+        {% 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) }}
+            <div class="panel panel-border-narrow js-hidden" id={{data_target}}>
+              {{ textbox(form|attr(option.data), label=' ', width='1-1') }}
+            </div>
+          {% endfor %}
+        {% endcall %}
+
         {{ page_footer(
           'Save',
           back_link=url_for('.service_settings', service_id=current_service.id),

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',

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']

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', [