From 53214937a8886aa6bd5710d06ad48d6dc4b1402d Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 8 Jul 2019 09:44:22 +0100 Subject: [PATCH] Stop allowing the service org type to be changed The service organisation type will either be the same as the org type of the service's organisation or will be set by a user when creating a new service. This removes the ability to change it from the platform admin settings table. --- app/main/forms.py | 4 - app/main/views/service_settings.py | 24 ---- app/navigation.py | 4 - app/templates/views/service-settings.html | 27 +++-- .../set-organisation-type.html | 23 ---- tests/app/main/views/test_service_settings.py | 107 ++++++------------ 6 files changed, 56 insertions(+), 133 deletions(-) delete mode 100644 app/templates/views/service-settings/set-organisation-type.html diff --git a/app/main/forms.py b/app/main/forms.py index 27d836190..5eaa400e0 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -591,10 +591,6 @@ class CreateServiceForm(StripWhitespaceForm): organisation_type = organisation_type() -class OrganisationTypeForm(StripWhitespaceForm): - organisation_type = organisation_type() - - class NewOrganisationForm( RenameOrganisationForm, OrganisationOrganisationTypeForm, diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 39fc331b4..7beab3951 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -36,7 +36,6 @@ from app.main.forms import ( FreeSMSAllowance, InternationalSMSForm, LinkOrganisationsForm, - OrganisationTypeForm, PreviewBranding, RenameServiceForm, SearchByNameForm, @@ -860,29 +859,6 @@ def service_set_letter_contact_block(service_id): ) -@main.route("/services//service-settings/set-organisation-type", methods=['GET', 'POST']) -@user_is_platform_admin -def set_organisation_type(service_id): - - form = OrganisationTypeForm(organisation_type=current_service.organisation_type) - - if form.validate_on_submit(): - free_sms_fragment_limit = current_app.config['DEFAULT_FREE_SMS_FRAGMENT_LIMITS'].get( - form.organisation_type.data) - - current_service.update( - organisation_type=form.organisation_type.data, - ) - billing_api_client.create_or_update_free_sms_fragment_limit(service_id, free_sms_fragment_limit) - - return redirect(url_for('.service_settings', service_id=service_id)) - - return render_template( - 'views/service-settings/set-organisation-type.html', - form=form, - ) - - @main.route("/services//service-settings/set-free-sms-allowance", methods=['GET', 'POST']) @user_is_platform_admin def set_free_sms_allowance(service_id): diff --git a/app/navigation.py b/app/navigation.py index c893f8210..05627813a 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -286,7 +286,6 @@ class HeaderNavigation(Navigation): 'service_verify_reply_to_address_updates', 'services_or_dashboard', 'set_free_sms_allowance', - 'set_organisation_type', 'set_sender', 'set_template_sender', 'show_accounts_or_dashboard', @@ -411,7 +410,6 @@ class MainNavigation(Navigation): 'service_sms_senders', 'set_free_sms_allowance', 'service_set_letter_branding', - 'set_organisation_type', 'submit_request_to_go_live', }, 'api-integration': { @@ -815,7 +813,6 @@ class CaseworkNavigation(Navigation): 'services_or_dashboard', 'set_free_sms_allowance', 'service_set_letter_branding', - 'set_organisation_type', 'set_sender', 'set_template_sender', 'show_accounts_or_dashboard', @@ -1088,7 +1085,6 @@ class OrgNavigation(Navigation): 'services_or_dashboard', 'set_free_sms_allowance', 'service_set_letter_branding', - 'set_organisation_type', 'set_sender', 'set_template_sender', 'show_accounts_or_dashboard', diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 3f76d36da..1f8bf3f43 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -308,16 +308,27 @@ {% call row() %} {{ text_field('Organisation')}} - {{ optional_text_field(current_service.organisation.name) }} + {% call field() %} + {% if current_service.organisation_id %} + + {{ current_service.organisation.name }} + + {% else %} + Not set + {% endif %} + {% if current_service.organisation_type %} +
+ {{ { + 'central': 'Central government', + 'local': 'Local government', + 'nhs': 'NHS', + }.get(current_service.organisation_type) }} +
+ {% endif %} + {% endcall %} {{ edit_field('Change', url_for('.link_service_to_organisation', service_id=current_service.id)) }} {% endcall %} - {% call row() %} - {{ text_field('Organisation type')}} - {{ optional_text_field( - (current_service.organisation_type or '')|title - ) }} - {{ edit_field('Change', url_for('.set_organisation_type', service_id=current_service.id)) }} - {% endcall %} + {% call row() %} {{ text_field('Free text message allowance')}} {{ text_field('{:,}'.format(current_service.free_sms_fragment_limit)) }} diff --git a/app/templates/views/service-settings/set-organisation-type.html b/app/templates/views/service-settings/set-organisation-type.html deleted file mode 100644 index 13a693eb3..000000000 --- a/app/templates/views/service-settings/set-organisation-type.html +++ /dev/null @@ -1,23 +0,0 @@ -{% extends "withnav_template.html" %} -{% from "components/radios.html" import radios %} -{% from "components/page-header.html" import page_header %} -{% from "components/page-footer.html" import page_footer %} -{% from "components/form.html" import form_wrapper %} - -{% block service_page_title %} - Set branding and organisation -{% endblock %} - -{% block maincolumn_content %} - - {{ page_header( - 'Set organisation type', - back_link=url_for('.service_settings', service_id=current_service.id) - ) }} - - {% call form_wrapper() %} - {{ radios(form.organisation_type) }} - {{ page_footer('Save') }} - {% endcall %} - -{% endblock %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 0ad64397a..f20ec3f93 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -60,7 +60,7 @@ def mock_get_service_settings_page_common( (active_user_with_permissions, [ 'Label Value Action', - 'Service name service one Change', + 'Service name Test Service Change', 'Sign-in method Text message code Change', 'Label Value Action', @@ -82,7 +82,7 @@ def mock_get_service_settings_page_common( (platform_admin_user, [ 'Label Value Action', - 'Service name service one Change', + 'Service name Test Service Change', 'Sign-in method Text message code Change', 'Label Value Action', @@ -103,8 +103,7 @@ def mock_get_service_settings_page_common( 'Label Value Action', 'Live Off Change', 'Count in list of live services Yes Change', - 'Organisation Test Organisation Change', - 'Organisation type Central Change', + 'Organisation Test Organisation Central government Change', 'Free text message allowance 250,000 Change', 'Email branding GOV.UK Change', 'Letter branding Not set Change', @@ -117,7 +116,7 @@ def mock_get_service_settings_page_common( def test_should_show_overview( client, mocker, - service_one, + api_user_active, fake_uuid, no_reply_to_email_addresses, no_letter_contact_blocks, @@ -127,12 +126,15 @@ def test_should_show_overview( expected_rows, mock_get_service_settings_page_common, ): - - service_one['permissions'] = ['sms', 'email'] + service_one = service_json(SERVICE_ONE_ID, + users=[api_user_active['id']], + permissions=['sms', 'email'], + organisation_id=ORGANISATION_ID) + mocker.patch('app.service_api_client.get_service', return_value={'data': service_one}) client.login(user(fake_uuid), mocker, service_one) response = client.get(url_for( - 'main.service_settings', service_id=service_one['id'] + 'main.service_settings', service_id=SERVICE_ONE_ID )) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') @@ -141,7 +143,7 @@ def test_should_show_overview( assert len(rows) == len(expected_rows) for index, row in enumerate(expected_rows): assert row == " ".join(rows[index].text.split()) - app.service_api_client.get_service.assert_called_with(service_one['id']) + app.service_api_client.get_service.assert_called_with(SERVICE_ONE_ID) def test_no_go_live_link_for_service_without_organisation( @@ -149,7 +151,6 @@ def test_no_go_live_link_for_service_without_organisation( mocker, no_reply_to_email_addresses, no_letter_contact_blocks, - mock_get_service_organisation, single_sms_sender, platform_admin_user, mock_get_service_settings_page_common, @@ -163,10 +164,35 @@ def test_no_go_live_link_for_service_without_organisation( 'Live No (organisation must be set first)' ) assert normalize_spaces(page.select('tr')[18].text) == ( - 'Organisation Not set Change' + 'Organisation Not set Central government Change' ) +def test_organisation_name_links_to_org_dashboard( + 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_one = service_json(SERVICE_ONE_ID, + permissions=['sms', 'email'], + organisation_id=ORGANISATION_ID) + 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 = response.select('tr')[18] + 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' + + @pytest.mark.parametrize('permissions, expected_rows', [ (['email', 'sms', 'inbound_sms', 'international_sms'], [ @@ -3288,7 +3314,6 @@ def test_should_set_branding_and_organisations( @pytest.mark.parametrize('method', ['get', 'post']) @pytest.mark.parametrize('endpoint', [ - 'main.set_organisation_type', 'main.set_free_sms_allowance', ]) def test_organisation_type_pages_are_platform_admin_only( @@ -3304,64 +3329,6 @@ def test_organisation_type_pages_are_platform_admin_only( ) -def test_should_show_page_to_set_organisation_type( - logged_in_platform_admin_client, -): - response = logged_in_platform_admin_client.get(url_for( - 'main.set_organisation_type', - service_id=SERVICE_ONE_ID - )) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - - labels = page.select('label') - checked_radio_buttons = page.select('input[checked]') - - assert len(checked_radio_buttons) == 1 - assert checked_radio_buttons[0]['value'] == 'central' - - assert len(labels) == 3 - for index, expected in enumerate(( - 'Central government', - 'Local government', - 'NHS', - )): - assert normalize_spaces(labels[index].text) == expected - - -@pytest.mark.parametrize('organisation_type, free_allowance', [ - ('central', 250000), - ('local', 25000), - ('nhs', 25000), - pytest.param('private sector', 1000, marks=pytest.mark.xfail) -]) -def test_should_set_organisation_type( - logged_in_platform_admin_client, - mock_update_service, - organisation_type, - free_allowance, - mock_create_or_update_free_sms_fragment_limit -): - response = logged_in_platform_admin_client.post( - url_for( - 'main.set_organisation_type', - service_id=SERVICE_ONE_ID, - ), - data={ - 'organisation_type': organisation_type, - 'organisation': 'organisation-id' - }, - ) - assert response.status_code == 302 - assert response.location == url_for('main.service_settings', service_id=SERVICE_ONE_ID, _external=True) - - mock_update_service.assert_called_once_with( - SERVICE_ONE_ID, - organisation_type=organisation_type, - ) - mock_create_or_update_free_sms_fragment_limit.assert_called_once_with(SERVICE_ONE_ID, free_allowance) - - def test_should_show_page_to_set_sms_allowance( logged_in_platform_admin_client, mock_get_free_sms_fragment_limit