mirror of
https://github.com/GSA/notifications-admin.git
synced 2026-09-07 07:18:25 -04:00
Replace platform admin service setting buttons with forms
Most of the existing platform admin buttons on the service settings page used to issue GET requests to switch service settings. This means they weren't protected by CSRF. On top of that as our number of service permissions increases over time a lot of buttons on the page made it hard to work with. To fix these issues we replace most of the buttons with rows in the platform admin settings table. Each setting has a 'Change' link that leads to a page with an On/Off switch form. This removes "research mode" switch completely since we're planning to deprecate it in the future and we don't expect to switch any new services into research mode at the moment. Most service permissions are now handled by a shared endpoint that is parameterized with the permission name. Some permissions that require some additional logic before they can be toggled (like document upload, which requires setting a contact address) have separate initial endpoints that redirect to `set_service_permission`. "Archive", "Suspend" and "Resume" actions are kept as buttons since they display a confirmation banner (which is a CSRF-protected form) and they're not easily represented as an On/Off switch.
This commit is contained in:
Alexey Bezhan
@@ -3,6 +3,8 @@ import functools
|
||||
import pytest
|
||||
from flask import url_for
|
||||
|
||||
from tests.conftest import normalize_spaces
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def get_service_settings_page(
|
||||
@@ -22,59 +24,107 @@ def get_service_settings_page(
|
||||
return functools.partial(client_request.get, 'main.service_settings', service_id=service_one['id'])
|
||||
|
||||
|
||||
@pytest.mark.parametrize('service_fields, endpoint, kwargs, text', [
|
||||
({'restricted': True}, '.service_switch_live', {}, 'Make service live'),
|
||||
({'restricted': False}, '.service_switch_live', {}, 'Revert service to trial mode'),
|
||||
def test_service_set_permission_requires_platform_admin(
|
||||
mocker,
|
||||
client_request,
|
||||
service_one,
|
||||
mock_get_inbound_number_for_service,
|
||||
):
|
||||
client_request.post(
|
||||
'main.service_set_permission', service_id=service_one['id'], permission='upload_document',
|
||||
_data={'enabled': 'True'},
|
||||
_expected_status=403
|
||||
)
|
||||
|
||||
({'research_mode': True}, '.service_switch_research_mode', {}, 'Take service out of research mode'),
|
||||
({'research_mode': False}, '.service_switch_research_mode', {}, 'Put into research mode'),
|
||||
|
||||
@pytest.mark.parametrize('permission, form_data, on', [
|
||||
('upload_document', 'True', True),
|
||||
('upload_document', 'False', False),
|
||||
('precompiled_letter', 'True', True),
|
||||
('precompiled_letter', 'False', False),
|
||||
('inbound_sms', 'True', True),
|
||||
('inbound_sms', 'False', False),
|
||||
('email_auth', 'True', True),
|
||||
('email_auth', 'False', False),
|
||||
])
|
||||
def test_service_set_permission(
|
||||
mocker,
|
||||
logged_in_platform_admin_client,
|
||||
service_one,
|
||||
mock_get_inbound_number_for_service,
|
||||
permission,
|
||||
form_data,
|
||||
on
|
||||
):
|
||||
force_permission = mocker.patch('app.models.service.Service.force_permission')
|
||||
response = logged_in_platform_admin_client.post(
|
||||
url_for('main.service_set_permission', service_id=service_one['id'], permission=permission),
|
||||
data={'enabled': form_data}
|
||||
)
|
||||
assert response.status_code == 302
|
||||
assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True)
|
||||
force_permission.assert_called_with(
|
||||
permission, on=on
|
||||
)
|
||||
|
||||
|
||||
@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': ['letter', 'precompiled_letter']},
|
||||
'.service_switch_can_send_precompiled_letter', {}, 'Stop sending precompiled letters'),
|
||||
'.service_set_permission', {'permission': 'precompiled_letter'}, 'Send precompiled letters On Change'),
|
||||
({'permissions': ['letter']},
|
||||
'.service_switch_can_send_precompiled_letter', {}, 'Allow to send precompiled letters'),
|
||||
'.service_set_permission', {'permission': 'precompiled_letter'}, 'Send precompiled letters Off Change'),
|
||||
|
||||
({'permissions': ['upload_document']},
|
||||
'.service_switch_can_upload_document', {}, 'Stop uploading documents'),
|
||||
'.service_switch_can_upload_document', {}, 'Uploading documents On Change'),
|
||||
({'permissions': []},
|
||||
'.service_switch_can_upload_document', {}, 'Allow to upload documents'),
|
||||
({'permissions': ['sms']}, '.service_set_inbound_number', {'set_inbound_sms': True}, 'Allow inbound sms'),
|
||||
|
||||
({'active': True}, '.archive_service', {}, 'Archive service'),
|
||||
({'active': True}, '.suspend_service', {}, 'Suspend service'),
|
||||
({'active': False}, '.resume_service', {}, 'Resume service'),
|
||||
'.service_switch_can_upload_document', {}, 'Uploading documents Off Change'),
|
||||
({'permissions': ['sms']}, '.service_set_inbound_number', {}, 'Receive inbound SMS Off Change'),
|
||||
])
|
||||
def test_service_setting_toggles_show(get_service_settings_page, service_one, service_fields, endpoint, kwargs, text):
|
||||
button_url = url_for(endpoint, **kwargs, service_id=service_one['id'])
|
||||
service_one.update(service_fields)
|
||||
page = get_service_settings_page()
|
||||
assert page.find('a', {'class': 'button', 'href': button_url}).text.strip() == text
|
||||
assert normalize_spaces(page.find('a', {'href': button_url}).find_parent('tr').text.strip()) == text
|
||||
|
||||
|
||||
@pytest.mark.parametrize('permissions,permissions_text', [
|
||||
('inbound_sms', 'inbound sms'), # no sms parent permission
|
||||
('precompiled_letter', 'precompiled letters'), # no letter parent permission
|
||||
@pytest.mark.parametrize('service_fields, endpoint, kwargs, text', [
|
||||
({'active': True}, '.archive_service', {}, 'Archive service'),
|
||||
({'active': True}, '.suspend_service', {}, 'Suspend service'),
|
||||
({'active': False}, '.resume_service', {}, 'Resume service'),
|
||||
])
|
||||
def test_service_setting_button_toggles(get_service_settings_page, service_one, service_fields, endpoint, kwargs, text):
|
||||
button_url = url_for(endpoint, **kwargs, service_id=service_one['id'])
|
||||
service_one.update(service_fields)
|
||||
page = get_service_settings_page()
|
||||
assert normalize_spaces(page.find('a', {'class': 'button', 'href': button_url}).text.strip()) == text
|
||||
|
||||
|
||||
@pytest.mark.parametrize('permissions,permissions_text,visible', [
|
||||
('sms', 'inbound SMS', True),
|
||||
('inbound_sms', 'inbound SMS', False), # no sms parent permission
|
||||
('letter', 'precompiled letters', True),
|
||||
('precompiled_letter', 'precompiled letters', False), # no letter parent permission
|
||||
# also test no permissions set
|
||||
('', 'inbound sms'),
|
||||
('', 'precompiled letters')
|
||||
('', 'inbound SMS', False),
|
||||
('', 'precompiled letters', False)
|
||||
])
|
||||
def test_service_settings_doesnt_show_option_if_parent_permission_disabled(
|
||||
get_service_settings_page,
|
||||
service_one,
|
||||
permissions,
|
||||
permissions_text
|
||||
permissions_text,
|
||||
visible
|
||||
):
|
||||
service_one['permissions'] = [permissions]
|
||||
page = get_service_settings_page()
|
||||
toggles = page.find_all('a', {'class': 'button'})
|
||||
assert not any(button for button in toggles if permissions_text in button.text)
|
||||
cells = page.find_all('td')
|
||||
assert any(cell for cell in cells if permissions_text in cell.text) is visible
|
||||
|
||||
|
||||
@pytest.mark.parametrize('service_fields, hidden_button_text', [
|
||||
# if no sms permission, inbound sms shouldn't show
|
||||
({'permissions': ['inbound_sms']}, 'Stop inbound sms'),
|
||||
({'permissions': []}, 'Allow inbound sms'),
|
||||
|
||||
# can't archive or suspend inactive service. Can't resume active service.
|
||||
({'active': False}, 'Archive service'),
|
||||
({'active': False}, 'Suspend service'),
|
||||
|
||||
@@ -92,12 +92,16 @@ def mock_get_service_settings_page_common(
|
||||
'Send letters Off Change',
|
||||
|
||||
'Label Value Action',
|
||||
'Live Off Change',
|
||||
'Organisation Org 1 Change',
|
||||
'Organisation type Central Change',
|
||||
'Free text message allowance 250,000 Change',
|
||||
'Email branding GOV.UK Change',
|
||||
'Letter branding Not set Change',
|
||||
'Data retention email Change'
|
||||
'Data retention email Change',
|
||||
'Receive inbound SMS Off Change',
|
||||
'User auth type editing Off Change',
|
||||
'Uploading documents Off Change',
|
||||
|
||||
]),
|
||||
])
|
||||
@@ -382,8 +386,10 @@ def test_switch_service_to_live(
|
||||
mock_update_service,
|
||||
mock_get_inbound_number_for_service
|
||||
):
|
||||
response = logged_in_platform_admin_client.get(
|
||||
url_for('main.service_switch_live', service_id=service_one['id']))
|
||||
response = logged_in_platform_admin_client.post(
|
||||
url_for('main.service_switch_live', service_id=service_one['id']),
|
||||
data={'enabled': 'True'}
|
||||
)
|
||||
assert response.status_code == 302
|
||||
assert response.location == url_for(
|
||||
'main.service_settings',
|
||||
@@ -418,8 +424,11 @@ def test_switch_service_to_restricted(
|
||||
mock_update_service,
|
||||
mock_get_inbound_number_for_service,
|
||||
):
|
||||
response = logged_in_platform_admin_client.get(
|
||||
url_for('main.service_switch_live', service_id=service_one['id']))
|
||||
response = logged_in_platform_admin_client.post(
|
||||
url_for('main.service_switch_live', service_id=service_one['id']),
|
||||
data={'enabled': 'False'}
|
||||
|
||||
)
|
||||
assert response.status_code == 302
|
||||
assert response.location == url_for(
|
||||
'main.service_settings',
|
||||
@@ -1110,7 +1119,6 @@ def test_route_permissions(
|
||||
'main.request_to_go_live',
|
||||
'main.submit_request_to_go_live',
|
||||
'main.service_switch_live',
|
||||
'main.service_switch_research_mode',
|
||||
'main.archive_service',
|
||||
])
|
||||
def test_route_invalid_permissions(
|
||||
@@ -1164,30 +1172,6 @@ def test_route_for_platform_admin(
|
||||
service_one)
|
||||
|
||||
|
||||
@pytest.mark.parametrize('route', [
|
||||
'main.service_switch_live',
|
||||
'main.service_switch_research_mode',
|
||||
])
|
||||
def test_route_for_platform_admin_update_service(
|
||||
mocker,
|
||||
app_,
|
||||
client,
|
||||
platform_admin_user,
|
||||
service_one,
|
||||
mock_get_all_letter_branding,
|
||||
route,
|
||||
):
|
||||
mocker.patch('app.service_api_client.archive_service')
|
||||
validate_route_permission(mocker,
|
||||
app_,
|
||||
"GET",
|
||||
302,
|
||||
url_for(route, service_id=service_one['id']),
|
||||
[],
|
||||
platform_admin_user,
|
||||
service_one)
|
||||
|
||||
|
||||
def test_and_more_hint_appears_on_settings_with_more_than_just_a_single_sender(
|
||||
client_request,
|
||||
service_one,
|
||||
@@ -1910,47 +1894,6 @@ def test_inbound_sms_sender_is_not_editable(
|
||||
) == "GOVUK This phone number receives replies and can’t be changed"
|
||||
|
||||
|
||||
def test_switch_service_to_research_mode(
|
||||
logged_in_platform_admin_client,
|
||||
platform_admin_user,
|
||||
service_one,
|
||||
mocker,
|
||||
):
|
||||
mocker.patch('app.service_api_client.post', return_value=service_one)
|
||||
response = logged_in_platform_admin_client.get(
|
||||
url_for('main.service_switch_research_mode', service_id=service_one['id'])
|
||||
)
|
||||
assert response.status_code == 302
|
||||
assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True)
|
||||
app.service_api_client.post.assert_called_with(
|
||||
'/service/{}'.format(service_one['id']),
|
||||
{
|
||||
'research_mode': True,
|
||||
'created_by': platform_admin_user.id
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
def test_switch_service_from_research_mode_to_normal(
|
||||
logged_in_platform_admin_client,
|
||||
mocker,
|
||||
):
|
||||
service = service_json(
|
||||
research_mode=True
|
||||
)
|
||||
mocker.patch('app.service_api_client.get_service', return_value={"data": service})
|
||||
update_service_mock = mocker.patch('app.service_api_client.update_service', return_value=service)
|
||||
|
||||
response = logged_in_platform_admin_client.get(
|
||||
url_for('main.service_switch_research_mode', service_id=service['id'])
|
||||
)
|
||||
assert response.status_code == 302
|
||||
assert response.location == url_for('main.service_settings', service_id=service['id'], _external=True)
|
||||
update_service_mock.assert_called_with(
|
||||
service['id'], research_mode=False
|
||||
)
|
||||
|
||||
|
||||
def test_shows_research_mode_indicator(
|
||||
logged_in_client,
|
||||
service_one,
|
||||
@@ -2652,10 +2595,9 @@ def test_switch_service_enable_international_sms(
|
||||
|
||||
@pytest.mark.parametrize('start_permissions, contact_details, end_permissions', [
|
||||
(['upload_document'], 'http://example.com/', []),
|
||||
(['upload_document'], None, []),
|
||||
([], '0207 123 4567', ['upload_document']),
|
||||
])
|
||||
def test_service_switch_can_upload_document_changes_the_permission_if_service_contact_details_exist(
|
||||
def test_service_switch_can_upload_document_shows_permission_page_if_service_contact_details_exist(
|
||||
logged_in_platform_admin_client,
|
||||
service_one,
|
||||
mock_update_service,
|
||||
@@ -2676,11 +2618,7 @@ def test_service_switch_can_upload_document_changes_the_permission_if_service_co
|
||||
follow_redirects=True
|
||||
)
|
||||
page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
|
||||
mock_update_service.assert_called_once_with(
|
||||
SERVICE_ONE_ID,
|
||||
permissions=end_permissions,
|
||||
)
|
||||
assert normalize_spaces(page.h1.text) == 'Settings'
|
||||
assert normalize_spaces(page.h1.text) == 'Uploading documents'
|
||||
|
||||
|
||||
def test_service_switch_can_upload_document_turning_permission_on_with_no_contact_details_shows_form(
|
||||
@@ -2707,7 +2645,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_switches_permission(
|
||||
def test_service_switch_can_upload_document_lets_contact_details_be_added_and_shows_permission_page(
|
||||
logged_in_platform_admin_client,
|
||||
service_one,
|
||||
mock_update_service,
|
||||
@@ -2728,8 +2666,7 @@ def test_service_switch_can_upload_document_lets_contact_details_be_added_and_sw
|
||||
)
|
||||
page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
|
||||
|
||||
assert 'upload_document' in mock_update_service.call_args[1]['permissions']
|
||||
assert normalize_spaces(page.h1.text) == 'Settings'
|
||||
assert normalize_spaces(page.h1.text) == 'Uploading documents'
|
||||
|
||||
|
||||
def test_archive_service_after_confirm(
|
||||
|
||||
Reference in New Issue
Block a user