From 9453f301d26e44dfff39e46d302ed206fc4731e1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 4 Oct 2017 11:49:32 +0100 Subject: [PATCH] Collect organisation type So that we can default services to their appropriate text allowance, we need to find out what sector they're in. So let's start collecting that from teams as they create new services. I think Central/Local/NHS are the right options, but these can be easily changed if not. --- app/main/forms.py | 19 +++++++++- app/main/views/add_service.py | 23 ++++++------ app/main/views/service_settings.py | 4 +-- app/notify_client/service_api_client.py | 11 +++++- app/templates/views/add-service.html | 5 ++- tests/app/main/views/test_add_service.py | 24 ++++++++++--- .../notify_client/test_service_api_client.py | 35 +++++++++++++++++++ tests/conftest.py | 4 +-- 8 files changed, 104 insertions(+), 21 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index e04b8d148..803698290 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -213,7 +213,7 @@ class TextNotReceivedForm(Form): mobile_number = international_phone_number() -class ServiceNameForm(Form): +class RenameServiceForm(Form): name = StringField( u'Service name', validators=[ @@ -221,6 +221,23 @@ class ServiceNameForm(Form): ]) +class CreateServiceForm(Form): + name = StringField( + u'What’s your service called?', + validators=[ + DataRequired(message='Can’t be empty') + ]) + organisation_type = RadioField( + 'Who runs this service?', + choices=[ + ('central', 'Central government'), + ('local', 'Local government'), + ('nhs', 'NHS'), + ], + validators=[DataRequired()], + ) + + class ConfirmPasswordForm(Form): def __init__(self, validate_password_func, *args, **kwargs): self.validate_password_func = validate_password_func diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 9b7d5b9fd..8019199e6 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -14,7 +14,7 @@ from notifications_python_client.errors import HTTPError from werkzeug.exceptions import abort from app.main import main -from app.main.forms import ServiceNameForm +from app.main.forms import CreateServiceForm from app.notify_client.models import InvitedUser from app import ( @@ -39,13 +39,16 @@ def _add_invited_user_to_service(invited_user): return service_id -def _create_service(service_name, email_from, form): +def _create_service(service_name, organisation_type, email_from, form): try: - service_id = service_api_client.create_service(service_name=service_name, - message_limit=current_app.config['DEFAULT_SERVICE_LIMIT'], - restricted=True, - user_id=session['user_id'], - email_from=email_from) + service_id = service_api_client.create_service( + service_name=service_name, + organisation_type=organisation_type, + message_limit=current_app.config['DEFAULT_SERVICE_LIMIT'], + restricted=True, + user_id=session['user_id'], + email_from=email_from, + ) session['service_id'] = service_id return service_id, None except HTTPError as e: @@ -78,14 +81,14 @@ def add_service(): if not is_gov_user(current_user.email_address): abort(403) - form = ServiceNameForm() - heading = 'Which service do you want to set up notifications for?' + form = CreateServiceForm() + 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, email_from, form) + service_id, error = _create_service(service_name, 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/main/views/service_settings.py b/app/main/views/service_settings.py index 7356f5c53..6278b5818 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -25,7 +25,7 @@ from app.main import main from app.utils import user_has_permissions, email_safe, get_cdn_domain from app.main.forms import ( ConfirmPasswordForm, - ServiceNameForm, + RenameServiceForm, RequestToGoLiveForm, ServiceReplyToEmailForm, ServiceSmsSender, @@ -100,7 +100,7 @@ def service_settings(service_id): @login_required @user_has_permissions('manage_settings', admin_override=True) def service_name_change(service_id): - form = ServiceNameForm() + form = RenameServiceForm() if request.method == 'GET': form.name.data = current_service.get('name') diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index d8c034eaa..aa92b0e1b 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -16,12 +16,21 @@ class ServiceAPIClient(NotifyAdminAPIClient): self.service_id = application.config['ADMIN_CLIENT_USER_NAME'] self.api_key = application.config['ADMIN_CLIENT_SECRET'] - def create_service(self, service_name, message_limit, restricted, user_id, email_from): + def create_service( + self, + service_name, + organisation_type, + message_limit, + restricted, + user_id, + email_from, + ): """ Create a service and return the json. """ data = { "name": service_name, + "organisation_type": organisation_type, "active": True, "message_limit": message_limit, "user_id": user_id, diff --git a/app/templates/views/add-service.html b/app/templates/views/add-service.html index 75dcc3cae..a7c88d2d2 100644 --- a/app/templates/views/add-service.html +++ b/app/templates/views/add-service.html @@ -1,4 +1,5 @@ {% extends "withoutnav_template.html" %} +{% from "components/radios.html" import radios %} {% from "components/textbox.html" import textbox %} {% from "components/page-footer.html" import page_footer %} @@ -12,13 +13,15 @@

- When people receive notifications, who should they be from? + About your service

{{ textbox(form.name, hint="You can change this later") }} + {{ radios(form.organisation_type) }} + {{ page_footer('Add service') }}
diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 703e8ab7a..b40b7a85d 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -20,7 +20,7 @@ def test_get_should_render_add_service_template( ): response = logged_in_client.get(url_for('main.add_service')) assert response.status_code == 200 - assert 'Which service do you want to set up notifications for?' in response.get_data(as_text=True) + assert 'About your service' in response.get_data(as_text=True) def test_should_add_service_and_redirect_to_tour_when_no_services( @@ -33,10 +33,15 @@ def test_should_add_service_and_redirect_to_tour_when_no_services( ): response = logged_in_client.post( url_for('main.add_service'), - data={'name': 'testing the post'}) + data={ + 'name': 'testing the post', + 'organisation_type': 'local', + } + ) 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'], restricted=True, user_id=api_user_active.id, @@ -72,10 +77,15 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service( ): response = logged_in_client.post( url_for('main.add_service'), - data={'name': 'testing the post'}) + data={ + 'name': 'testing the post', + 'organisation_type': 'central', + } + ) assert mock_get_services.called mock_create_service.assert_called_once_with( service_name='testing the post', + organisation_type='central', message_limit=app_.config['DEFAULT_SERVICE_LIMIT'], restricted=True, user_id=api_user_active.id, @@ -99,7 +109,13 @@ def test_should_return_form_errors_with_duplicate_service_name_regardless_of_cas logged_in_client, mock_create_duplicate_service, ): - response = logged_in_client.post(url_for('main.add_service'), data={'name': 'SERVICE ONE'}) + response = logged_in_client.post( + url_for('main.add_service'), + data={ + 'name': 'SERVICE ONE', + 'organisation_type': 'central', + }, + ) assert response.status_code == 200 assert 'This service name is already in use' in response.get_data(as_text=True) diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 9fa6cd4d9..710d2260d 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -43,3 +43,38 @@ def test_client_only_updates_allowed_attributes(mocker): with pytest.raises(TypeError) as error: ServiceAPIClient().update_service('service_id', foo='bar') assert str(error.value) == 'Not allowed to update service attributes: foo' + + +def test_client_creates_service_with_correct_data( + mocker, + active_user_with_permissions, + fake_uuid, +): + client = ServiceAPIClient() + mock_post = mocker.patch.object(client, 'post') + mocker.patch('app.notify_client.current_user', id='123') + + client.create_service( + service_name='My first service', + organisation_type='central_government', + message_limit=1, + restricted=True, + user_id=fake_uuid, + email_from='test@example.com', + ) + mock_post.assert_called_once_with( + '/service', + dict( + # Autogenerated arguments + created_by='123', + active=True, + # ‘service_name’ argument is coerced to ‘name’ + name='My first service', + # The rest pass through with the same names + organisation_type='central_government', + message_limit=1, + restricted=True, + user_id=fake_uuid, + email_from='test@example.com', + ), + ) diff --git a/tests/conftest.py b/tests/conftest.py index c58dad210..2051247fd 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -488,7 +488,7 @@ def mock_get_service_with_letters(mocker, api_user_active): @pytest.fixture(scope='function') def mock_create_service(mocker): - def _create(service_name, message_limit, restricted, user_id, email_from): + def _create(service_name, organisation_type, message_limit, restricted, user_id, email_from): service = service_json( 101, service_name, [user_id], message_limit=message_limit, restricted=restricted, email_from=email_from) return service['id'] @@ -499,7 +499,7 @@ def mock_create_service(mocker): @pytest.fixture(scope='function') def mock_create_duplicate_service(mocker): - def _create(service_name, message_limit, restricted, user_id, email_from): + def _create(service_name, organisation_type, message_limit, restricted, user_id, email_from): json_mock = Mock(return_value={'message': {'name': ["Duplicate service name '{}'".format(service_name)]}}) resp_mock = Mock(status_code=400, json=json_mock) http_error = HTTPError(response=resp_mock, message="Default message")