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)