From b2e6d149588a6af499334ba88041fbff39b6352b Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 17 Feb 2020 15:32:19 +0000 Subject: [PATCH 1/4] Enable users to turn send file by email setting on and off --- app/main/views/service_settings.py | 38 ++++++++++++++++++++--- app/templates/views/service-settings.html | 10 ++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 75de09ad6..f0ddc67d1 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -67,10 +67,16 @@ from app.utils import ( PLATFORM_ADMIN_SERVICE_PERMISSIONS = OrderedDict([ ('inbound_sms', {'title': 'Receive inbound SMS', 'requires': 'sms', 'endpoint': '.service_set_inbound_number'}), ('email_auth', {'title': 'Email authentication'}), - ('upload_document', {'title': 'Send files by email', 'endpoint': '.service_switch_can_upload_document'}), ('upload_letters', {'title': 'Uploading letters', 'requires': 'letter'}), ]) +SERVICE_SETTINGS = OrderedDict([ + ( + 'upload_document', + {'title': 'Send files by email', 'endpoint': '.service_switch_can_upload_document', 'type': 'email'} + ), +]) + @main.route("/services//service-settings") @user_has_permissions('manage_service', 'manage_api_keys') @@ -294,11 +300,35 @@ def service_set_permission(service_id, permission): ) +@main.route("/services//service-settings/", methods=["GET", "POST"]) +@user_has_permissions('manage_service') +def service_set_setting(service_id, setting): + if setting not in SERVICE_SETTINGS: + abort(404) + + title = SERVICE_SETTINGS[setting]['title'] + form = ServiceOnOffSettingForm( + name=title, + enabled=current_service.has_permission(setting) + ) + + if form.validate_on_submit(): + current_service.force_permission(setting, on=form.enabled.data) + + return redirect(url_for(".service_settings", service_id=service_id)) + + return render_template( + 'views/service-settings/set-service-setting.html', + title=title, + form=form, + ) + + @main.route("/services//service-settings/can-upload-document", methods=['GET', 'POST']) -@user_is_platform_admin +@user_has_permissions('manage_service') def service_switch_can_upload_document(service_id): if current_service.contact_link: - return redirect(url_for('.service_set_permission', service_id=service_id, permission='upload_document')) + return redirect(url_for('.service_set_setting', service_id=service_id, setting='upload_document')) form = ServiceContactDetailsForm() @@ -309,7 +339,7 @@ def service_switch_can_upload_document(service_id): contact_link=form.data[contact_type] ) - return redirect(url_for('.service_set_permission', service_id=service_id, permission='upload_document')) + return redirect(url_for('.service_set_setting', service_id=service_id, setting='upload_document')) return render_template('views/service-settings/contact_link.html', form=form) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 3b1db42c0..132aeb574 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -110,6 +110,16 @@ )}} {% endcall %} + {% call settings_row(if_has_permission='email') %} + {{ text_field('Send files by email') }} + {{ boolean_field('upload_document' in current_service.permissions) }} + {{ edit_field( + 'Change', + url_for('.service_switch_can_upload_document', service_id=current_service.id), + permissions=['manage_service'], + )}} + {% endcall %} + {% endcall %} {% call mapping_table( From 02b2a890e8dfd825ea18da9712a0b43a5f850774 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 17 Feb 2020 15:36:46 +0000 Subject: [PATCH 2/4] Get rid of on/off page send file by email setting --- app/main/views/service_settings.py | 37 ++-------------- tests/__init__.py | 4 ++ .../views/accounts/test_choose_accounts.py | 2 +- .../test_service_setting_permissions.py | 8 +--- tests/app/main/views/test_find_services.py | 2 +- tests/app/main/views/test_send.py | 6 +-- tests/app/main/views/test_service_settings.py | 44 +++++++++++-------- tests/app/main/views/test_template_folders.py | 2 +- tests/app/main/views/test_uploads.py | 4 +- 9 files changed, 43 insertions(+), 66 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index f0ddc67d1..633617fdf 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -70,13 +70,6 @@ PLATFORM_ADMIN_SERVICE_PERMISSIONS = OrderedDict([ ('upload_letters', {'title': 'Uploading letters', 'requires': 'letter'}), ]) -SERVICE_SETTINGS = OrderedDict([ - ( - 'upload_document', - {'title': 'Send files by email', 'endpoint': '.service_switch_can_upload_document', 'type': 'email'} - ), -]) - @main.route("/services//service-settings") @user_has_permissions('manage_service', 'manage_api_keys') @@ -300,35 +293,12 @@ def service_set_permission(service_id, permission): ) -@main.route("/services//service-settings/", methods=["GET", "POST"]) -@user_has_permissions('manage_service') -def service_set_setting(service_id, setting): - if setting not in SERVICE_SETTINGS: - abort(404) - - title = SERVICE_SETTINGS[setting]['title'] - form = ServiceOnOffSettingForm( - name=title, - enabled=current_service.has_permission(setting) - ) - - if form.validate_on_submit(): - current_service.force_permission(setting, on=form.enabled.data) - - return redirect(url_for(".service_settings", service_id=service_id)) - - return render_template( - 'views/service-settings/set-service-setting.html', - title=title, - form=form, - ) - - @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: - return redirect(url_for('.service_set_setting', service_id=service_id, setting='upload_document')) + 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() @@ -339,7 +309,8 @@ def service_switch_can_upload_document(service_id): contact_link=form.data[contact_type] ) - return redirect(url_for('.service_set_setting', service_id=service_id, setting='upload_document')) + 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) diff --git a/tests/__init__.py b/tests/__init__.py index d1382c2b3..06bd8d492 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -620,3 +620,7 @@ def assert_url_expected(actual, expected): 'Expected redirect: {}\n' 'Actual redirect: {}' ).format(expected, actual) + + +def find_element_by_tag_and_partial_text(page, tag, string): + return [e for e in page.find_all(tag) if string in e.text][0] diff --git a/tests/app/main/views/accounts/test_choose_accounts.py b/tests/app/main/views/accounts/test_choose_accounts.py index fc5f1a832..1fb550bbf 100644 --- a/tests/app/main/views/accounts/test_choose_accounts.py +++ b/tests/app/main/views/accounts/test_choose_accounts.py @@ -140,7 +140,7 @@ def test_choose_account_should_show_choose_accounts_page_if_no_services( resp = client_request.get('main.choose_account') page = resp.find('main', {'id': 'main-content'}) - links = page.findAll('a') + links = page.find_all('a') assert len(links) == 1 add_service_link = links[0] assert normalize_spaces(page.h1.text) == 'Choose service' diff --git a/tests/app/main/views/service_settings/test_service_setting_permissions.py b/tests/app/main/views/service_settings/test_service_setting_permissions.py index 9dc34903c..c6dc24cfd 100644 --- a/tests/app/main/views/service_settings/test_service_setting_permissions.py +++ b/tests/app/main/views/service_settings/test_service_setting_permissions.py @@ -31,15 +31,13 @@ def test_service_set_permission_requires_platform_admin( mock_get_inbound_number_for_service, ): client_request.post( - 'main.service_set_permission', service_id=service_one['id'], permission='upload_document', + 'main.service_set_permission', service_id=service_one['id'], permission='email_auth', _data={'enabled': 'True'}, _expected_status=403 ) @pytest.mark.parametrize('permission, form_data, on', [ - ('upload_document', 'True', True), - ('upload_document', 'False', False), ('inbound_sms', 'True', True), ('inbound_sms', 'False', False), ('email_auth', 'True', True), @@ -69,10 +67,6 @@ def test_service_set_permission( @pytest.mark.parametrize('service_fields, endpoint, kwargs, text', [ ({'restricted': True}, '.service_switch_live', {}, 'Live Off Change'), ({'restricted': False}, '.service_switch_live', {}, 'Live On Change'), - ({'permissions': ['upload_document']}, - '.service_switch_can_upload_document', {}, 'Send files by email On Change'), - ({'permissions': []}, - '.service_switch_can_upload_document', {}, 'Send files by email Off Change'), ({'permissions': ['sms']}, '.service_set_inbound_number', {}, 'Receive inbound SMS Off Change'), ({'permissions': ['letter']}, '.service_set_permission', {'permission': 'upload_letters'}, 'Uploading letters Off Change'), diff --git a/tests/app/main/views/test_find_services.py b/tests/app/main/views/test_find_services.py index 1b3884930..c527ea5b7 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.select('.browse-list-item a') + results = document.find_all('a', {'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_send.py b/tests/app/main/views/test_send.py index c6ca1dedb..34d62fd69 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -2640,7 +2640,7 @@ def test_check_messages_back_link( ) assert ( - page.findAll('a', {'class': 'govuk-back-link'})[0]['href'] + page.find_all('a', {'class': 'govuk-back-link'})[0]['href'] ) == expected_url(service_id=SERVICE_ONE_ID, template_id=fake_uuid) @@ -2731,7 +2731,7 @@ def test_check_messages_shows_too_many_messages_errors( assert page.find('div', class_='banner-dangerous').find('a').text.strip() == 'trial mode' # remove excess whitespace from element - details = page.find('div', class_='banner-dangerous').findAll('p')[1] + details = page.find('div', class_='banner-dangerous').find_all('p')[1] details = ' '.join([line.strip() for line in details.text.split('\n') if line.strip() != '']) assert details == expected_msg @@ -3383,7 +3383,7 @@ def test_check_notification_shows_preview( assert page.h1.text.strip() == 'Preview of ‘Two week reminder’' assert ( - page.findAll('a', {'class': 'govuk-back-link'})[0]['href'] + page.find_all('a', {'class': 'govuk-back-link'})[0]['href'] ) == url_for( 'main.send_one_off_step', service_id=service_one['id'], diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index ddede014c..fc9e4fa67 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -13,6 +13,7 @@ from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient import app from app.utils import email_safe from tests import ( + find_element_by_tag_and_partial_text, invite_json, organisation_json, sample_uuid, @@ -60,6 +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', 'Label Value Action', 'Send text messages On Change', @@ -82,6 +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', 'Label Value Action', 'Send text messages On Change', @@ -103,7 +106,6 @@ def mock_get_service_settings_page_common( 'Data retention email Change', 'Receive inbound SMS Off Change', 'Email authentication Off Change', - 'Send files by email Off Change', ]), ]) def test_should_show_overview( @@ -152,12 +154,13 @@ def test_no_go_live_link_for_service_without_organisation( page = client_request.get('main.service_settings', service_id=SERVICE_ONE_ID) assert page.find('h1').text == 'Settings' - assert normalize_spaces(page.select('tr')[16].text) == ( - 'Live No (organisation must be set first)' - ) - assert normalize_spaces(page.select('tr')[18].text) == ( - 'Organisation Not set Central government Change' - ) + + is_live = find_element_by_tag_and_partial_text(page, tag='td', string='Live') + assert normalize_spaces(is_live.find_next_sibling().text) == 'No (organisation must be set first)' + + organisation = find_element_by_tag_and_partial_text(page, tag='td', string='Organisation') + assert normalize_spaces(organisation.find_next_siblings()[0].text) == 'Not set Central government' + assert normalize_spaces(organisation.find_next_siblings()[1].text) == 'Change' def test_organisation_name_links_to_org_dashboard( @@ -180,7 +183,7 @@ def test_organisation_name_links_to_org_dashboard( 'main.service_settings', service_id=SERVICE_ONE_ID ) - org_row = response.select('tr')[18] + org_row = find_element_by_tag_and_partial_text(response, tag='tr', string='Organisation') assert org_row.find('a')['href'] == url_for('main.organisation_dashboard', org_id=ORGANISATION_ID) assert normalize_spaces(org_row.find('a').text) == 'Test Organisation' @@ -195,6 +198,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', 'Label Value Action', 'Send text messages On Change', @@ -216,6 +220,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', 'Label Value Action', 'Send text messages On Change', @@ -1917,14 +1922,14 @@ def test_and_more_hint_appears_on_settings_with_more_than_just_a_single_sender( service_id=service_one['id'] ) - def get_row(page, index): + def get_row(page, label): return normalize_spaces( - page.select('tbody tr')[index].text + find_element_by_tag_and_partial_text(page, tag='tr', string=label).text ) - assert get_row(page, 3) == "Reply-to email addresses test@example.com …and 2 more Manage" - assert get_row(page, 6) == "Text message senders Example …and 2 more Manage" - assert get_row(page, 11) == "Sender addresses 1 Example Street …and 2 more Manage" + assert get_row(page, 'Reply-to email addresses') == "Reply-to email addresses test@example.com …and 2 more Manage" + assert get_row(page, 'Text message senders') == "Text message senders Example …and 2 more Manage" + assert get_row(page, 'Sender addresses') == "Sender addresses 1 Example Street …and 2 more Manage" @pytest.mark.parametrize('sender_list_page, index, expected_output', [ @@ -3713,7 +3718,7 @@ def test_switch_service_enable_international_sms( (['upload_document'], 'http://example.com/', []), ([], '0207 123 4567', ['upload_document']), ]) -def test_service_switch_can_upload_document_shows_permission_page_if_service_contact_details_exist( +def test_service_switch_can_upload_document_changes_permission_if_service_contact_details_exist( platform_admin_client, service_one, mock_update_service, @@ -3734,7 +3739,8 @@ def test_service_switch_can_upload_document_shows_permission_page_if_service_con follow_redirects=True ) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert normalize_spaces(page.h1.text) == 'Send files by email' + 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( @@ -3761,7 +3767,7 @@ def test_service_switch_can_upload_document_turning_permission_on_with_no_contac ('email_address', 'old@example.com'), ('phone_number', '0207 12345'), ]) -def test_service_switch_can_upload_document_lets_contact_details_be_added_and_shows_permission_page( +def test_service_switch_can_upload_document_lets_contact_details_be_added_and_changes_setting( platform_admin_client, service_one, mock_update_service, @@ -3773,6 +3779,7 @@ def test_service_switch_can_upload_document_lets_contact_details_be_added_and_sh 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( @@ -3782,7 +3789,8 @@ def test_service_switch_can_upload_document_lets_contact_details_be_added_and_sh ) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert normalize_spaces(page.h1.text) == 'Send files by email' + assert normalize_spaces(page.h1.text) == 'Settings' + mock_update_service.assert_called_with(SERVICE_ONE_ID, permissions=['upload_document']) @pytest.mark.parametrize('user', ( @@ -4958,7 +4966,7 @@ def test_service_settings_links_to_branding_request_page_for_letters( page = client_request.get( '.service_settings', service_id=SERVICE_ONE_ID ) - assert len(page.findAll('a', attrs={'href': '/services/{}/branding-request/letter'.format(SERVICE_ONE_ID)})) == 1 + assert len(page.find_all('a', attrs={'href': '/services/{}/branding-request/letter'.format(SERVICE_ONE_ID)})) == 1 def test_show_service_data_retention( diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index ba15b5d6b..08db7e007 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -538,7 +538,7 @@ def test_get_manage_folder_viewing_permissions_for_users( assert checkboxes[1]['value'] == team_member_2['id'] assert "checked" in checkboxes[1].attrs - assert "Test User" in page.findAll('label', {'for': 'users_with_permission-0'})[0].text + assert "Test User" in page.find_all('label', {'for': 'users_with_permission-0'})[0].text def test_get_manage_folder_viewing_permissions_for_users_not_visible_when_no_manage_settings_permission( diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index 1fd02eda9..cbb7a5396 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -76,11 +76,11 @@ def test_get_upload_hub_page( 'main.upload_letter', service_id=SERVICE_ONE_ID ) - assert page.findAll( + assert page.find_all( 'a', {'class': 'file-list-filename'} )[0].attrs['href'] == '/services/{}/jobs/job_id_1'.format(SERVICE_ONE_ID) - assert page.findAll( + assert page.find_all( 'a', {'class': 'file-list-filename'} )[1].attrs['href'] == '/services/{}/notification/letter_id_1'.format(SERVICE_ONE_ID) From a601d6e700fbe09aa0936a192d9bdbfc71905daa Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 25 Feb 2020 11:27:43 +0000 Subject: [PATCH 3/4] 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( From 230d62ddd615b1a9e33d276422c7c7b41f614391 Mon Sep 17 00:00:00 2001 From: karlchillmaid Date: Wed, 26 Feb 2020 17:00:31 +0000 Subject: [PATCH 4/4] Update guidance page content --- app/templates/views/guidance/send-files-by-email.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/guidance/send-files-by-email.html b/app/templates/views/guidance/send-files-by-email.html index febb10243..11f55bba0 100644 --- a/app/templates/views/guidance/send-files-by-email.html +++ b/app/templates/views/guidance/send-files-by-email.html @@ -8,8 +8,8 @@

Send files by email

-

Contact us if you want to send files by email.

+

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

-

Then follow the instructions to send a file by email in our API documentation.

+

This is an API-only feature.

{% endblock %}