From a601d6e700fbe09aa0936a192d9bdbfc71905daa Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 25 Feb 2020 11:27:43 +0000 Subject: [PATCH] Send files by email on for everyone and only depending on service having contact details set up. Display not set up yet for send files by email row when contact_link not set up --- app/main/views/service_settings.py | 40 +--- app/navigation.py | 12 +- app/templates/views/service-settings.html | 19 +- ...act_link.html => send-files-by-email.html} | 15 +- tests/__init__.py | 2 +- tests/app/main/views/test_find_services.py | 2 +- tests/app/main/views/test_service_settings.py | 194 +++++++----------- 7 files changed, 108 insertions(+), 176 deletions(-) rename app/templates/views/service-settings/{contact_link.html => send-files-by-email.html} (67%) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 633617fdf..0c1f7b534 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -293,28 +293,6 @@ def service_set_permission(service_id, permission): ) -@main.route("/services//service-settings/can-upload-document", methods=['GET', 'POST']) -@user_has_permissions('manage_service') -def service_switch_can_upload_document(service_id): - if current_service.contact_link: - current_service.force_permission('upload_document', on=(not current_service.has_permission('upload_document'))) - return redirect(url_for(".service_settings", service_id=service_id)) - - form = ServiceContactDetailsForm() - - if form.validate_on_submit(): - contact_type = form.contact_details_type.data - - current_service.update( - contact_link=form.data[contact_type] - ) - - current_service.force_permission('upload_document', on=(not current_service.has_permission('upload_document'))) - return redirect(url_for(".service_settings", service_id=service_id)) - - return render_template('views/service-settings/contact_link.html', form=form) - - @main.route("/services//service-settings/archive", methods=['GET', 'POST']) @user_has_permissions('manage_service') def archive_service(service_id): @@ -360,18 +338,20 @@ def resume_service(service_id): return service_settings(service_id) -@main.route("/services//service-settings/contact-link", methods=['GET', 'POST']) +@main.route("/services//service-settings/send-files-by-email", methods=['GET', 'POST']) @user_has_permissions('manage_service') -def service_set_contact_link(service_id): +def send_files_by_email_contact_details(service_id): form = ServiceContactDetailsForm() + contact_details = None if request.method == 'GET': contact_details = current_service.contact_link - contact_type = check_contact_details_type(contact_details) - field_to_update = getattr(form, contact_type) + if contact_details: + 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 + 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 @@ -381,7 +361,9 @@ def service_set_contact_link(service_id): ) return redirect(url_for('.service_settings', service_id=current_service.id)) - return render_template('views/service-settings/contact_link.html', form=form) + return render_template( + 'views/service-settings/send-files-by-email.html', form=form, contact_details=contact_details + ) @main.route("/services//service-settings/set-reply-to-email", methods=['GET']) diff --git a/app/navigation.py b/app/navigation.py index 3f94bbc9f..a1b68816a 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -287,7 +287,7 @@ class HeaderNavigation(Navigation): 'service_preview_letter_branding', 'service_set_auth_type', 'service_set_channel', - 'service_set_contact_link', + 'send_files_by_email_contact_details', 'service_set_email_branding', 'service_set_inbound_number', 'service_set_inbound_sms', @@ -299,7 +299,6 @@ class HeaderNavigation(Navigation): 'service_set_sms_prefix', 'service_settings', 'service_sms_senders', - 'service_switch_can_upload_document', 'service_switch_count_as_live', 'service_switch_live', 'service_set_permission', @@ -433,7 +432,7 @@ class MainNavigation(Navigation): 'service_preview_letter_branding', 'service_set_auth_type', 'service_set_channel', - 'service_set_contact_link', + 'send_files_by_email_contact_details', 'service_set_email_branding', 'service_set_inbound_number', 'service_set_inbound_sms', @@ -593,7 +592,6 @@ class MainNavigation(Navigation): 'service_delete_letter_contact', 'service_delete_sms_sender', 'service_download_agreement', - 'service_switch_can_upload_document', 'service_switch_count_as_live', 'service_switch_live', 'service_set_permission', @@ -865,7 +863,7 @@ class CaseworkNavigation(Navigation): 'service_preview_letter_branding', 'service_set_auth_type', 'service_set_channel', - 'service_set_contact_link', + 'send_files_by_email_contact_details', 'service_set_email_branding', 'service_set_inbound_number', 'service_set_inbound_sms', @@ -876,7 +874,6 @@ class CaseworkNavigation(Navigation): 'service_set_sms_prefix', 'service_settings', 'service_sms_senders', - 'service_switch_can_upload_document', 'service_switch_count_as_live', 'service_switch_live', 'service_set_permission', @@ -1155,7 +1152,7 @@ class OrgNavigation(Navigation): 'service_preview_letter_branding', 'service_set_auth_type', 'service_set_channel', - 'service_set_contact_link', + 'send_files_by_email_contact_details', 'service_set_email_branding', 'service_set_inbound_number', 'service_set_inbound_sms', @@ -1166,7 +1163,6 @@ class OrgNavigation(Navigation): 'service_set_sms_prefix', 'service_settings', 'service_sms_senders', - 'service_switch_can_upload_document', 'service_switch_count_as_live', 'service_switch_live', 'service_set_permission', diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 132aeb574..7b7213e3a 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -45,19 +45,6 @@ ) }} {% endcall %} - - {% call settings_row(if_has_permission='upload_document') %} - {{ text_field('Contact details') }} - {{ text_field(current_service.contact_link, truncate=true) }} - {{ edit_field( - 'Change', - url_for('.service_set_contact_link', - service_id=current_service.id), - permissions=['manage_service'] - ) - }} - {% endcall %} - {% endcall %} {% call mapping_table( @@ -112,10 +99,10 @@ {% call settings_row(if_has_permission='email') %} {{ text_field('Send files by email') }} - {{ boolean_field('upload_document' in current_service.permissions) }} + {{ text_field(current_service.contact_link if current_service.contact_link else "Not set up", truncate=true) }} {{ edit_field( - 'Change', - url_for('.service_switch_can_upload_document', service_id=current_service.id), + 'Manage', + url_for('.send_files_by_email_contact_details', service_id=current_service.id), permissions=['manage_service'], )}} {% endcall %} diff --git a/app/templates/views/service-settings/contact_link.html b/app/templates/views/service-settings/send-files-by-email.html similarity index 67% rename from app/templates/views/service-settings/contact_link.html rename to app/templates/views/service-settings/send-files-by-email.html index d9e61582b..ced8b2f3a 100644 --- a/app/templates/views/service-settings/contact_link.html +++ b/app/templates/views/service-settings/send-files-by-email.html @@ -7,20 +7,25 @@ {% from "components/form.html" import form_wrapper %} {% block service_page_title %} - {{ 'Change' if 'upload_document' in current_service.permissions else 'Add' }} contact details for ‘Download your document’ page + Send files by email {% endblock %} {% block maincolumn_content %}
{{ page_header( - '{} contact details for ‘Download your document’ page'.format('Change' if 'upload_document' in current_service.permissions else 'Add'), + 'Send files by email', back_link=url_for('main.service_settings', service_id=current_service.id) ) }}

- 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). + This is an API-only feature. +

+

+ To send a file by email, follow the instructions in our API documentation. +

+

{% if contact_details %}Change contact details for{% else %}Add contact details to{% endif %} the file download page

+

+ You need to include contact details for your service so your users can get in touch if there’s a problem. For example, if the link to download the file you sent them has expired.

{% call form_wrapper() %} diff --git a/tests/__init__.py b/tests/__init__.py index 06bd8d492..70f04ed12 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -175,7 +175,7 @@ def service_json( 'inbound_api': inbound_api, 'service_callback_api': service_callback_api, 'prefix_sms': prefix_sms, - 'contact_link': None, + 'contact_link': contact_link, 'volume_email': 111111, 'volume_sms': 222222, 'volume_letter': 333333, diff --git a/tests/app/main/views/test_find_services.py b/tests/app/main/views/test_find_services.py index c527ea5b7..f821205ae 100644 --- a/tests/app/main/views/test_find_services.py +++ b/tests/app/main/views/test_find_services.py @@ -42,7 +42,7 @@ def test_find_services_by_name_displays_multiple_services( ) document = client_request.post('main.find_services_by_name', _data={"search": "Tadfield"}, _expected_status=200) - results = document.find_all('a', {'class': 'browse-list-item'}) + results = document.find_all('li', {'class': 'browse-list-item'}) assert len(results) == 2 assert sorted([result.text.strip() for result in results]) == ["Tadfield Air Base", "Tadfield Police"] diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index fc9e4fa67..c418f9a61 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -61,7 +61,7 @@ def mock_get_service_settings_page_common( 'Send emails On Change', 'Reply-to email addresses Not set Manage', 'Email branding GOV.UK Change', - 'Send files by email Off Change', + 'Send files by email contact_us@gov.uk Manage', 'Label Value Action', 'Send text messages On Change', @@ -84,7 +84,7 @@ def mock_get_service_settings_page_common( 'Send emails On Change', 'Reply-to email addresses Not set Manage', 'Email branding GOV.UK Change', - 'Send files by email Off Change', + 'Send files by email contact_us@gov.uk Manage', 'Label Value Action', 'Send text messages On Change', @@ -120,10 +120,13 @@ def test_should_show_overview( expected_rows, mock_get_service_settings_page_common, ): - service_one = service_json(SERVICE_ONE_ID, - users=[api_user_active['id']], - permissions=['sms', 'email'], - organisation_id=ORGANISATION_ID) + service_one = service_json( + SERVICE_ONE_ID, + users=[api_user_active['id']], + permissions=['sms', 'email'], + organisation_id=ORGANISATION_ID, + contact_link='contact_us@gov.uk', + ) mocker.patch('app.service_api_client.get_service', return_value={'data': service_one}) client.login(user, mocker, service_one) @@ -188,6 +191,40 @@ def test_organisation_name_links_to_org_dashboard( assert normalize_spaces(org_row.find('a').text) == 'Test Organisation' +@pytest.mark.parametrize('service_contact_link,expected_text', [ + ('contact.me@gov.uk', 'Send files by email contact.me@gov.uk Manage'), + (None, 'Send files by email Not set up Manage'), +]) +def test_send_files_by_email_row_on_settings_page( + client_request, + platform_admin_user, + no_reply_to_email_addresses, + no_letter_contact_blocks, + single_sms_sender, + mock_get_service_settings_page_common, + mocker, + mock_get_service_organisation, + service_contact_link, + expected_text +): + service_one = service_json( + SERVICE_ONE_ID, + permissions=['sms', 'email'], + organisation_id=ORGANISATION_ID, + contact_link=service_contact_link + ) + + mocker.patch('app.service_api_client.get_service', return_value={'data': service_one}) + + client_request.login(platform_admin_user, service_one) + response = client_request.get( + 'main.service_settings', service_id=SERVICE_ONE_ID + ) + + org_row = find_element_by_tag_and_partial_text(response, tag='tr', string='Send files by email') + assert normalize_spaces(org_row.get_text()) == expected_text + + @pytest.mark.parametrize('permissions, expected_rows', [ (['email', 'sms', 'inbound_sms', 'international_sms'], [ @@ -198,7 +235,7 @@ def test_organisation_name_links_to_org_dashboard( 'Send emails On Change', 'Reply-to email addresses test@example.com Manage', 'Email branding Organisation name Change', - 'Send files by email Off Change', + 'Send files by email Not set up Manage', 'Label Value Action', 'Send text messages On Change', @@ -220,7 +257,7 @@ def test_organisation_name_links_to_org_dashboard( 'Send emails On Change', 'Reply-to email addresses test@example.com Manage', 'Email branding Organisation name Change', - 'Send files by email Off Change', + 'Send files by email Not set up Manage', 'Label Value Action', 'Send text messages On Change', @@ -3714,85 +3751,6 @@ def test_switch_service_enable_international_sms( assert mocked_fn.call_args[0][0] == service_one['id'] -@pytest.mark.parametrize('start_permissions, contact_details, end_permissions', [ - (['upload_document'], 'http://example.com/', []), - ([], '0207 123 4567', ['upload_document']), -]) -def test_service_switch_can_upload_document_changes_permission_if_service_contact_details_exist( - platform_admin_client, - 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, - start_permissions, - contact_details, - end_permissions, -): - service_one['permissions'] = start_permissions - service_one['contact_link'] = contact_details - - response = platform_admin_client.get( - url_for('main.service_switch_can_upload_document', service_id=SERVICE_ONE_ID), - follow_redirects=True - ) - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert normalize_spaces(page.h1.text) == 'Settings' - mock_update_service.assert_called_with(SERVICE_ONE_ID, permissions=end_permissions) - - -def test_service_switch_can_upload_document_turning_permission_on_with_no_contact_details_shows_form( - platform_admin_client, - service_one, - mock_get_service_settings_page_common, - mock_get_service_organisation, - no_reply_to_email_addresses, - no_letter_contact_blocks, - single_sms_sender, -): - response = platform_admin_client.get( - url_for('main.service_switch_can_upload_document', service_id=SERVICE_ONE_ID), - follow_redirects=True - ) - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - - assert 'upload_document' not in service_one['permissions'] - assert normalize_spaces(page.h1.text) == "Add contact details for ‘Download your document’ page" - - -@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_changes_setting( - platform_admin_client, - 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, - contact_details_value, -): - service_one['permissions'] = [] - data = {'contact_details_type': contact_details_type, contact_details_type: contact_details_value} - - response = platform_admin_client.post( - url_for('main.service_switch_can_upload_document', service_id=SERVICE_ONE_ID), - data=data, - follow_redirects=True - ) - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - - assert normalize_spaces(page.h1.text) == 'Settings' - mock_update_service.assert_called_with(SERVICE_ONE_ID, permissions=['upload_document']) - - @pytest.mark.parametrize('user', ( create_platform_admin_user(), create_active_user_with_permissions(), @@ -3996,7 +3954,7 @@ def test_cant_resume_active_service( ('email_address', 'me@example.com'), ('phone_number', '0207 123 4567'), ]) -def test_service_set_contact_link_prefills_the_form_with_the_existing_contact_details( +def test_send_files_by_email_contact_details_prefills_the_form_with_the_existing_contact_details( client_request, service_one, contact_details_type, @@ -4005,7 +3963,7 @@ def test_service_set_contact_link_prefills_the_form_with_the_existing_contact_de service_one['contact_link'] = contact_details_value page = client_request.get( - 'main.service_set_contact_link', service_id=SERVICE_ONE_ID + 'main.send_files_by_email_contact_details', service_id=SERVICE_ONE_ID ) 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 @@ -4016,7 +3974,7 @@ def test_service_set_contact_link_prefills_the_form_with_the_existing_contact_de ('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( +def test_send_files_by_email_contact_details_updates_contact_details_and_redirects_to_settings_page( client_request, service_one, mock_update_service, @@ -4032,7 +3990,7 @@ def test_service_set_contact_link_updates_contact_details_and_redirects_to_setti service_one['contact_link'] = old_value page = client_request.post( - 'main.service_set_contact_link', service_id=SERVICE_ONE_ID, + 'main.send_files_by_email_contact_details', service_id=SERVICE_ONE_ID, _data={ 'contact_details_type': contact_details_type, contact_details_type: new_value, @@ -4044,7 +4002,7 @@ def test_service_set_contact_link_updates_contact_details_and_redirects_to_setti 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( +def test_send_files_by_email_contact_details_uses_the_selected_field_when_multiple_textboxes_contain_data( client_request, service_one, mock_update_service, @@ -4057,7 +4015,7 @@ def test_service_set_contact_link_updates_contact_details_for_the_selected_field service_one['contact_link'] = 'http://www.old-url.com' page = client_request.post( - 'main.service_set_contact_link', service_id=SERVICE_ONE_ID, + 'main.send_files_by_email_contact_details', service_id=SERVICE_ONE_ID, _data={ 'contact_details_type': 'url', 'url': 'http://www.new-url.com', @@ -4071,12 +4029,33 @@ def test_service_set_contact_link_updates_contact_details_for_the_selected_field 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( +@pytest.mark.parametrize( + 'contact_link, subheader, button_selected', + [ + ('contact.me@gov.uk', 'Change contact details for the file download page', True), + (None, 'Add contact details to the file download page', False), + ] +) +def test_send_files_by_email_contact_details_page( + client_request, service_one, active_user_with_permissions, contact_link, subheader, button_selected +): + service_one["contact_link"] = contact_link + page = client_request.get( + 'main.send_files_by_email_contact_details', service_id=SERVICE_ONE_ID + ) + assert normalize_spaces(page.find_all('h2')[1].text) == subheader + if button_selected: + assert 'checked' in page.find('input', {'name': 'contact_details_type', 'value': 'email_address'}).attrs + else: + assert 'checked' not in page.find('input', {'name': 'contact_details_type', 'value': 'email_address'}).attrs + + +def test_send_files_by_email_contact_details_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, + 'main.send_files_by_email_contact_details', service_id=SERVICE_ONE_ID, _data={ 'contact_details_type': None, 'url': '', @@ -4086,7 +4065,7 @@ def test_service_set_contact_link_displays_error_message_when_no_radio_button_se _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" + assert normalize_spaces(page.h1.text) == "Send files by email" @pytest.mark.parametrize('contact_details_type, invalid_value, error', [ @@ -4094,7 +4073,7 @@ def test_service_set_contact_link_displays_error_message_when_no_radio_button_se ('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( +def test_send_files_by_email_contact_details_does_not_update_invalid_contact_details( mocker, client_request, service_one, @@ -4106,7 +4085,7 @@ def test_service_set_contact_link_does_not_update_invalid_contact_details( service_one['permissions'].append('upload_document') page = client_request.post( - 'main.service_set_contact_link', service_id=SERVICE_ONE_ID, + 'main.send_files_by_email_contact_details', service_id=SERVICE_ONE_ID, _data={ 'contact_details_type': contact_details_type, contact_details_type: invalid_value, @@ -4115,24 +4094,7 @@ def test_service_set_contact_link_does_not_update_invalid_contact_details( ) 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( - 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, -): - service_one['permissions'] = ['upload_document'] - page = client_request.get( - 'main.service_settings', - service_id=SERVICE_ONE_ID, - ) - assert 'Contact details' in page.text + assert normalize_spaces(page.h1.text) == "Send files by email" def test_contact_link_is_not_displayed_without_the_upload_document_permission(