From 08e9b35d7a4da8e9a8365b13d47fc716d039d1ff Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 18 Apr 2019 12:44:18 +0100 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20ask=20for=20organisation=20type?= =?UTF-8?q?=20when=20we=20know=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every time someone adds a new service we ask them what kind of organisation they work for. We can look this up based on the user’s email address now. So we should only ask the question if: - we don’t know about the organisation - or we haven’t set what type of organisation it is (this shouldn’t be possible on productions because we’ve populated the column for all existing organisations and it’s impossible to add a new one without setting it --- app/main/views/add_service.py | 13 +++-- app/templates/views/add-service.html | 4 +- tests/__init__.py | 3 +- tests/app/main/views/test_add_service.py | 60 ++++++++++++++++++++---- tests/app/main/views/test_sign_in.py | 4 +- tests/conftest.py | 2 + 6 files changed, 71 insertions(+), 15 deletions(-) diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 8fce160ae..412b57a03 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -1,5 +1,5 @@ from flask import current_app, redirect, render_template, session, url_for -from flask_login import login_required +from flask_login import current_user, login_required from notifications_python_client.errors import HTTPError from app import billing_api_client, service_api_client @@ -47,14 +47,21 @@ def _create_example_template(service_id): @login_required @user_is_gov_user def add_service(): - form = CreateServiceForm() + form = CreateServiceForm( + organisation_type=current_user.default_organisation.organisation_type + ) heading = 'About your service' if form.validate_on_submit(): email_from = email_safe(form.name.data) service_name = form.name.data - service_id, error = _create_service(service_name, form.organisation_type.data, email_from, form) + service_id, error = _create_service( + service_name, + current_user.default_organisation.organisation_type or form.organisation_type.data, + email_from, + form, + ) if error: return render_template('views/add-service.html', form=form, heading=heading) if len(service_api_client.get_active_services({'user_id': session['user_id']}).get('data', [])) > 1: diff --git a/app/templates/views/add-service.html b/app/templates/views/add-service.html index 639016362..b4bc173bc 100644 --- a/app/templates/views/add-service.html +++ b/app/templates/views/add-service.html @@ -21,7 +21,9 @@ {{ textbox(form.name, hint="You can change this later") }} - {{ radios(form.organisation_type) }} + {% if not current_user.default_organisation.organisation_type %} + {{ radios(form.organisation_type) }} + {% endif %} {{ page_footer('Add service') }} diff --git a/tests/__init__.py b/tests/__init__.py index cbe1405b3..c54c5db6d 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -194,6 +194,7 @@ def organisation_json( domains=None, crown=True, agreement_signed=False, + organisation_type='', ): if users is None: users = [] @@ -208,7 +209,7 @@ def organisation_json( 'created_at': created_at or str(datetime.utcnow()), 'email_branding_id': email_branding_id, 'letter_branding_id': letter_branding_id, - 'organisation_type': '', + 'organisation_type': organisation_type, 'crown': crown, 'agreement_signed': agreement_signed, 'agreement_signed_at': None, diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 18d00b199..20c6b8c92 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -2,6 +2,7 @@ import pytest from flask import session, url_for from app.utils import is_gov_user +from tests.conftest import mock_get_organisation_by_domain def test_non_gov_user_cannot_see_add_service_button( @@ -18,14 +19,48 @@ def test_non_gov_user_cannot_see_add_service_button( def test_get_should_render_add_service_template( - client_request + client_request, + mock_get_organisation_by_domain, ): page = client_request.get('main.add_service') - assert 'About your service' in page.text + assert page.select_one('h1').text.strip() == 'About your service' + assert page.select_one('input[name=name]')['value'] == '' + assert [ + label.text.strip() for label in page.select('.multiple-choice label') + ] == [ + 'Central government', + 'Local government', + 'NHS', + ] + assert [ + radio['value'] for radio in page.select('.multiple-choice input') + ] == [ + 'central', + 'local', + 'nhs', + ] +def test_get_should_not_render_radios_if_org_type_known( + client_request, + mocker, +): + mock_get_organisation_by_domain(mocker, organisation_type='central') + page = client_request.get('main.add_service') + assert page.select_one('h1').text.strip() == 'About your service' + assert page.select_one('input[name=name]')['value'] == '' + assert not page.select('.multiple-choice') + + +@pytest.mark.parametrize('inherited, posted, persisted, sms_limit', ( + (None, 'central', 'central', 250000), + ('central', None, 'central', 250000), + ('nhs', None, 'nhs', 25000), + ('local', None, 'local', 25000), + ('central', 'local', 'central', 250000), +)) def test_should_add_service_and_redirect_to_tour_when_no_services( - app_, + mocker, client_request, mock_create_service, mock_create_service_template, @@ -33,12 +68,17 @@ def test_should_add_service_and_redirect_to_tour_when_no_services( api_user_active, mock_create_or_update_free_sms_fragment_limit, mock_get_all_email_branding, + inherited, + posted, + persisted, + sms_limit, ): + mock_get_organisation_by_domain(mocker, organisation_type=inherited) client_request.post( 'main.add_service', _data={ 'name': 'testing the post', - 'organisation_type': 'local', + 'organisation_type': posted, }, _expected_status=302, _expected_redirect=url_for( @@ -51,8 +91,8 @@ def test_should_add_service_and_redirect_to_tour_when_no_services( assert mock_get_services_with_no_services.called mock_create_service.assert_called_once_with( service_name='testing the post', - organisation_type='local', - message_limit=app_.config['DEFAULT_SERVICE_LIMIT'], + organisation_type=persisted, + message_limit=50, restricted=True, user_id=api_user_active.id, email_from='testing.the.post', @@ -67,7 +107,7 @@ def test_should_add_service_and_redirect_to_tour_when_no_services( 101, ) assert session['service_id'] == 101 - mock_create_or_update_free_sms_fragment_limit.assert_called_once_with(101, 25000) + mock_create_or_update_free_sms_fragment_limit.assert_called_once_with(101, sms_limit) @pytest.mark.parametrize('organisation_type, free_allowance', [ @@ -81,6 +121,7 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service( mock_create_service, mock_create_service_template, mock_get_services, + mock_get_organisation_by_domain, api_user_active, organisation_type, free_allowance, @@ -115,7 +156,8 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service( def test_should_return_form_errors_when_service_name_is_empty( - client_request + client_request, + mock_get_organisation_by_domain, ): page = client_request.post( 'main.add_service', @@ -128,7 +170,7 @@ def test_should_return_form_errors_when_service_name_is_empty( def test_should_return_form_errors_with_duplicate_service_name_regardless_of_case( client_request, mock_create_duplicate_service, - mock_get_all_email_branding, + mock_get_organisation_by_domain, ): page = client_request.post( 'main.add_service', diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index 244d4aef9..0284fd531 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -39,7 +39,9 @@ def test_sign_in_explains_other_browser(logged_in_client, api_user_active, mocke def test_doesnt_redirect_to_sign_in_if_no_session_info( - client_request, api_user_active + client_request, + api_user_active, + mock_get_organisation_by_domain, ): assert api_user_active.current_session_id is None diff --git a/tests/conftest.py b/tests/conftest.py index ca6f95287..b89c70b57 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3143,6 +3143,7 @@ def mock_get_organisation_by_domain( name=False, crown=True, agreement_signed=False, + organisation_type='', ): def _get_organisation_by_domain(org_id): return organisation_json( @@ -3150,6 +3151,7 @@ def mock_get_organisation_by_domain( name, crown=crown, agreement_signed=agreement_signed, + organisation_type=organisation_type, ) return mocker.patch(