Merge pull request #2923 from alphagov/dont-ask-org-type

Don’t ask for organisation type when we know it

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:

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') }}
 

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,

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',
+        organisation_type=persisted,
-        message_limit=app_.config['DEFAULT_SERVICE_LIMIT'],
+        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',

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
 

tests/conftest.py

@@ -3134,6 +3134,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(
@@ -3141,6 +3142,7 @@ def mock_get_organisation_by_domain(
             name,
             crown=crown,
             agreement_signed=agreement_signed,
+            organisation_type=organisation_type,
         )
 
     return mocker.patch(