From 9b06f2da23d67a598c478d72262704709f0c7995 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 4 Oct 2017 11:44:04 +0100 Subject: [PATCH 1/8] Remove redundant init MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This convoluted bit of code was doing the exact same thing as just inheriting from the parent class’s `__init__`. --- app/main/forms.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index cf899cc4f..e04b8d148 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -214,9 +214,6 @@ class TextNotReceivedForm(Form): class ServiceNameForm(Form): - def __init__(self, *args, **kwargs): - super(ServiceNameForm, self).__init__(*args, **kwargs) - name = StringField( u'Service name', validators=[ From 9453f301d26e44dfff39e46d302ed206fc4731e1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 4 Oct 2017 11:49:32 +0100 Subject: [PATCH 2/8] 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") From c51676005650cebad7b9567f390acb0343dd3ee4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 5 Oct 2017 10:46:55 +0100 Subject: [PATCH 3/8] Set free SMS limit depending on organisation type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Different parts of government get billed slightly differently, and there’s differences in how much money we’re allowed to give them. Think these numbers are right, but should be double checked. --- app/config.py | 5 +++++ app/main/views/add_service.py | 1 + app/notify_client/service_api_client.py | 2 ++ tests/app/main/views/test_add_service.py | 14 +++++++++++-- .../notify_client/test_service_api_client.py | 2 ++ tests/conftest.py | 20 +++++++++++++++++-- 6 files changed, 40 insertions(+), 4 deletions(-) diff --git a/app/config.py b/app/config.py index 645f0042f..2cd1bc3fb 100644 --- a/app/config.py +++ b/app/config.py @@ -36,6 +36,11 @@ class Config(object): ASSETS_DEBUG = False AWS_REGION = 'eu-west-1' DEFAULT_SERVICE_LIMIT = 50 + DEFAULT_FREE_SMS_FRAGMENT_LIMITS = { + 'central': 250000, + 'local': 25000, + 'nhs': 25000, + } EMAIL_EXPIRY_SECONDS = 3600 * 24 * 7 # one week HEADER_COLOUR = '#FFBF47' # $yellow HTTP_PROTOCOL = 'http' diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 8019199e6..a6779fc38 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -45,6 +45,7 @@ def _create_service(service_name, organisation_type, email_from, form): service_name=service_name, organisation_type=organisation_type, message_limit=current_app.config['DEFAULT_SERVICE_LIMIT'], + free_sms_fragment_limit=current_app.config['DEFAULT_FREE_SMS_FRAGMENT_LIMITS'].get(organisation_type), restricted=True, user_id=session['user_id'], email_from=email_from, diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index aa92b0e1b..a35044e6c 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -20,6 +20,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): self, service_name, organisation_type, + free_sms_fragment_limit, message_limit, restricted, user_id, @@ -31,6 +32,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data = { "name": service_name, "organisation_type": organisation_type, + "free_sms_fragment_limit": free_sms_fragment_limit, "active": True, "message_limit": message_limit, "user_id": user_id, diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index b40b7a85d..ee584a27a 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -1,3 +1,4 @@ +import pytest from flask import url_for, session from app.utils import is_gov_user @@ -43,6 +44,7 @@ def test_should_add_service_and_redirect_to_tour_when_no_services( service_name='testing the post', organisation_type='local', message_limit=app_.config['DEFAULT_SERVICE_LIMIT'], + free_sms_fragment_limit=25000, restricted=True, user_id=api_user_active.id, email_from='testing.the.post' @@ -67,6 +69,11 @@ def test_should_add_service_and_redirect_to_tour_when_no_services( ) +@pytest.mark.parametrize('organisation_type, free_allowance', [ + ('central', 250 * 1000), + ('local', 25 * 1000), + ('nhs', 25 * 1000), +]) def test_should_add_service_and_redirect_to_dashboard_when_existing_service( app_, logged_in_client, @@ -74,19 +81,22 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service( mock_create_service_template, mock_get_services, api_user_active, + organisation_type, + free_allowance, ): response = logged_in_client.post( url_for('main.add_service'), data={ 'name': 'testing the post', - 'organisation_type': 'central', + 'organisation_type': organisation_type, } ) assert mock_get_services.called mock_create_service.assert_called_once_with( service_name='testing the post', - organisation_type='central', + organisation_type=organisation_type, message_limit=app_.config['DEFAULT_SERVICE_LIMIT'], + free_sms_fragment_limit=free_allowance, restricted=True, user_id=api_user_active.id, email_from='testing.the.post' diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 710d2260d..a77090a5f 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -57,6 +57,7 @@ def test_client_creates_service_with_correct_data( client.create_service( service_name='My first service', organisation_type='central_government', + free_sms_fragment_limit=2, message_limit=1, restricted=True, user_id=fake_uuid, @@ -72,6 +73,7 @@ def test_client_creates_service_with_correct_data( name='My first service', # The rest pass through with the same names organisation_type='central_government', + free_sms_fragment_limit=2, message_limit=1, restricted=True, user_id=fake_uuid, diff --git a/tests/conftest.py b/tests/conftest.py index 2051247fd..27bcad9a1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -488,7 +488,15 @@ def mock_get_service_with_letters(mocker, api_user_active): @pytest.fixture(scope='function') def mock_create_service(mocker): - def _create(service_name, organisation_type, message_limit, restricted, user_id, email_from): + def _create( + service_name, + organisation_type, + message_limit, + free_sms_fragment_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 +507,15 @@ def mock_create_service(mocker): @pytest.fixture(scope='function') def mock_create_duplicate_service(mocker): - def _create(service_name, organisation_type, message_limit, restricted, user_id, email_from): + def _create( + service_name, + organisation_type, + message_limit, + free_sms_fragment_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") From ff6b2b01dd64c21b1dcbdab9697f419485cce786 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 5 Oct 2017 11:08:22 +0100 Subject: [PATCH 4/8] Show org type and free allowance on settings page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As a platform admin it’s useful to be able to see what each service has set for these attributes. --- app/templates/views/service-settings.html | 13 +++++++++++++ tests/__init__.py | 4 ++++ tests/app/main/views/test_service_settings.py | 2 ++ 3 files changed, 19 insertions(+) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index d9845437d..169033a44 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -191,6 +191,19 @@ field_headings_visible=False, caption_visible=False ) %} + {% call row() %} + {{ text_field('Organisation type')}} + {{ text_field( + current_service.organisation_type|title or 'Not set', + status='' if current_service.organisation_type else 'default' + ) }} + {{ text_field() }} + {% endcall %} + {% call row() %} + {{ text_field('Free text message allowance')}} + {{ text_field('{:,}'.format(current_service.free_sms_fragment_limit)) }} + {{ text_field() }} + {% endcall %} {% call row() %} {{ text_field('Email branding' )}} {% call field() %} diff --git a/tests/__init__.py b/tests/__init__.py index d39311897..b48e2e33e 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -54,6 +54,8 @@ def service_json( letter_contact_block=None, inbound_api=None, permissions=['email', 'sms'], + organisation_type='central', + free_sms_fragment_limit=250000, ): if users is None: users = [] @@ -66,12 +68,14 @@ def service_json( 'name': name, 'users': users, 'message_limit': message_limit, + 'free_sms_fragment_limit': free_sms_fragment_limit, 'active': active, 'restricted': restricted, 'email_from': email_from, 'reply_to_email_address': reply_to_email_address, 'sms_sender': sms_sender, 'research_mode': research_mode, + 'organisation_type': organisation_type, 'organisation': organisation, 'branding': branding, 'created_at': created_at or str(datetime.utcnow()), diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index b8cf44e9c..3f5f64feb 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -65,6 +65,8 @@ from tests.conftest import ( 'Send letters Off Change', 'Label Value Action', + 'Organisation type Central', + 'Free text message allowance 250,000', 'Email branding GOV.UK Change', 'Letter branding HM Government Change', From 521b5b6cf1a5e6a93e3d23be1da0ad2e259dd1e9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 5 Oct 2017 11:21:00 +0100 Subject: [PATCH 5/8] =?UTF-8?q?Add=20=E2=80=98optional=20text=E2=80=99=20m?= =?UTF-8?q?acro?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This pattern of value or grey text saying `None` is common enough in our app that we should codify it into a pattern. Which this commit does. --- app/templates/components/table.html | 7 +++++++ app/templates/views/service-settings.html | 14 +++++--------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/app/templates/components/table.html b/app/templates/components/table.html index aaf0d635a..938f5b040 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -92,6 +92,13 @@ {% endcall %} {%- endmacro %} +{% macro optional_text_field(text, default=None) -%} + {{ text_field( + text or default, + status='' if text else 'default' + ) }} +{%- endmacro %} + {% macro link_field(text, link) -%} {% call field() %} {{ text }} diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 169033a44..a94c0e994 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -1,7 +1,7 @@ {% extends "withnav_template.html" %} {% from "components/banner.html" import banner_wrapper %} {% from "components/browse-list.html" import browse_list %} -{% from "components/table.html" import mapping_table, row, text_field, edit_field, field, boolean_field %} +{% from "components/table.html" import mapping_table, row, text_field, optional_text_field, edit_field, field, boolean_field %} {% block service_page_title %} Settings @@ -90,7 +90,7 @@ {% if (current_user.has_permissions([], admin_override=True) or not can_receive_inbound) and not can_receive_inbound %} {{ edit_field('Change', url_for('.service_set_sms_sender', service_id=current_service.id, set_inbound_sms=False)) }} {% else %} - {{ text_field('') }} + {{ text_field('') }} {% endif %} {% endcall %} @@ -110,10 +110,7 @@ {% if can_receive_inbound %} {% call row() %} {{ text_field('API endpoint for received text messages') }} - {{ text_field( - 'None' if not inbound_api_url else inbound_api_url, - status='' if inbound_api_url else 'default' - ) }} + {{ optional_text_field(inbound_api_url) }} {{ edit_field('Change', url_for('.service_set_inbound_api', service_id=current_service.id)) }} {% endcall %} {% endif %} @@ -193,9 +190,8 @@ ) %} {% call row() %} {{ text_field('Organisation type')}} - {{ text_field( - current_service.organisation_type|title or 'Not set', - status='' if current_service.organisation_type else 'default' + {{ optional_text_field( + (current_service.organisation_type or '')|title ) }} {{ text_field() }} {% endcall %} From 8cc8a0bf059c1d7e66ad83be8cda8bc49f9f587e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 5 Oct 2017 11:23:17 +0100 Subject: [PATCH 6/8] Change None to Not set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ‘None’ is a bit Pythony. ‘Not set’ is a bit more human. --- app/main/views/service_settings.py | 4 ++-- app/templates/components/table.html | 2 +- app/templates/views/service-settings.html | 4 ++-- tests/app/main/views/test_service_settings.py | 8 ++++---- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 6278b5818..171bc8cec 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -73,12 +73,12 @@ def service_settings(service_id): reply_to_email_addresses = service_api_client.get_reply_to_email_addresses(service_id) reply_to_email_address_count = len(reply_to_email_addresses) default_reply_to_email_address = next( - (x['email_address'] for x in reply_to_email_addresses if x['is_default']), "None" + (x['email_address'] for x in reply_to_email_addresses if x['is_default']), "Not set" ) letter_contact_details = service_api_client.get_letter_contacts(service_id) letter_contact_details_count = len(letter_contact_details) default_letter_contact_block = next( - (Field(x['contact_block'], html='escape') for x in letter_contact_details if x['is_default']), "None" + (Field(x['contact_block'], html='escape') for x in letter_contact_details if x['is_default']), "Not set" ) return render_template( 'views/service-settings.html', diff --git a/app/templates/components/table.html b/app/templates/components/table.html index 938f5b040..5fddff5b8 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -92,7 +92,7 @@ {% endcall %} {%- endmacro %} -{% macro optional_text_field(text, default=None) -%} +{% macro optional_text_field(text, default='Not set') -%} {{ text_field( text or default, status='' if text else 'default' diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index a94c0e994..6585bd31f 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -135,8 +135,8 @@ {% if 'letter' in current_service.permissions %} {% call row() %} {{ text_field('Sender addresses') }} - {% call field(status='default' if default_letter_contact_block == "None" else '') %} - {{ default_letter_contact_block | string | nl2br | safe if default_letter_contact_block else 'None'}} + {% call field(status='default' if default_letter_contact_block == "Not set" else '') %} + {{ default_letter_contact_block | string | nl2br | safe if default_letter_contact_block else 'Not set'}} {% if letter_contact_details_count > 1 %}
{{ '…and %d more' | format(letter_contact_details_count - 1) }} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 3f5f64feb..1efc246cb 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -34,7 +34,7 @@ from tests.conftest import ( 'Label Value Action', 'Send emails On Change', - 'Email reply to addresses None Change', + 'Email reply to addresses Not set Change', 'Label Value Action', 'Send text messages On Change', @@ -53,7 +53,7 @@ from tests.conftest import ( 'Label Value Action', 'Send emails On Change', - 'Email reply to addresses None Change', + 'Email reply to addresses Not set Change', 'Label Value Action', 'Send text messages On Change', @@ -115,7 +115,7 @@ def test_should_show_overview( 'Text message sender 0781239871', 'International text messages On Change', 'Receive text messages On Change', - 'API endpoint for received text messages None Change', + 'API endpoint for received text messages Not set Change', 'Label Value Action', 'Send letters Off Change', @@ -252,7 +252,7 @@ def test_letter_contact_block_shows_none_if_not_set( page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') div = page.find_all('tr')[8].find_all('td')[1].div - assert div.text.strip() == 'None' + assert div.text.strip() == 'Not set' assert 'default' in div.attrs['class'][0] From d438109630c15da173d6bc525771ac5cc75773b0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 5 Oct 2017 12:06:40 +0100 Subject: [PATCH 7/8] Add page to change organisation type If a user picks the wrong thing, we should be able to override it when they go live. --- app/main/forms.py | 26 ++++--- app/main/views/service_settings.py | 23 ++++++ app/templates/views/service-settings.html | 2 +- .../set-organisation-type.html | 22 ++++++ tests/app/main/views/test_service_settings.py | 72 ++++++++++++++++++- 5 files changed, 133 insertions(+), 12 deletions(-) create mode 100644 app/templates/views/service-settings/set-organisation-type.html diff --git a/app/main/forms.py b/app/main/forms.py index 803698290..5b5273b99 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -141,6 +141,18 @@ def sms_code(): message='Code not found')]) +def organisation_type(): + return RadioField( + 'Who runs this service?', + choices=[ + ('central', 'Central government'), + ('local', 'Local government'), + ('nhs', 'NHS'), + ], + validators=[DataRequired()], + ) + + class LoginForm(Form): email_address = StringField('Email address', validators=[ Length(min=5, max=255), @@ -227,15 +239,11 @@ class CreateServiceForm(Form): 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()], - ) + organisation_type = organisation_type() + + +class OrganisationTypeForm(Form): + organisation_type = organisation_type() class ConfirmPasswordForm(Form): diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 171bc8cec..c4c788920 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -34,6 +34,7 @@ from app.main.forms import ( LetterBranding, ServiceInboundApiForm, InternationalSMSForm, + OrganisationTypeForm, ) from app import user_api_client, current_service, organisations_client, inbound_number_client from notifications_utils.formatters import formatted_list @@ -574,6 +575,28 @@ def service_set_letter_contact_block(service_id): ) +@main.route("/services//service-settings/set-organisation-type", methods=['GET', 'POST']) +@login_required +@user_has_permissions(admin_override=True) +def set_organisation_type(service_id): + + form = OrganisationTypeForm(organisation_type=current_service.get('organisation_type')) + + if form.validate_on_submit(): + service_api_client.update_service( + service_id, + organisation_type=form.organisation_type.data, + ) + return redirect(url_for('.service_settings', service_id=service_id)) + + form.organisation_type.data = current_service.get('organisation_type') + + return render_template( + 'views/service-settings/set-organisation-type.html', + form=form, + ) + + @main.route("/services//service-settings/set-branding-and-org", methods=['GET', 'POST']) @login_required @user_has_permissions(admin_override=True) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 6585bd31f..77033ba94 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -193,7 +193,7 @@ {{ optional_text_field( (current_service.organisation_type or '')|title ) }} - {{ text_field() }} + {{ edit_field('Change', url_for('.set_organisation_type', service_id=current_service.id)) }} {% endcall %} {% call row() %} {{ text_field('Free text message allowance')}} diff --git a/app/templates/views/service-settings/set-organisation-type.html b/app/templates/views/service-settings/set-organisation-type.html new file mode 100644 index 000000000..afa9474ed --- /dev/null +++ b/app/templates/views/service-settings/set-organisation-type.html @@ -0,0 +1,22 @@ +{% extends "withnav_template.html" %} +{% from "components/radios.html" import radios, branding_radios %} +{% from "components/page-footer.html" import page_footer %} + +{% block service_page_title %} + Set branding and organisation +{% endblock %} + +{% block maincolumn_content %} + +

Set organisation type

+
+ {{ radios(form.organisation_type) }} + {{ page_footer( + 'Save', + back_link=url_for('.service_settings', service_id=current_service.id), + back_link_text='Back to settings' + ) }} +
+
+ +{% endblock %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 1efc246cb..d0fa1d07d 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -22,7 +22,7 @@ from tests.conftest import ( get_non_default_reply_to_email_address, get_default_letter_contact_block, get_non_default_letter_contact_block, - SERVICE_ONE_ID + SERVICE_ONE_ID, ) @@ -65,7 +65,7 @@ from tests.conftest import ( 'Send letters Off Change', 'Label Value Action', - 'Organisation type Central', + 'Organisation type Central Change', 'Free text message allowance 250,000', 'Email branding GOV.UK Change', 'Letter branding HM Government Change', @@ -1362,6 +1362,74 @@ def test_should_set_branding_and_organisations( ) +@pytest.mark.parametrize('method', ['get', 'post']) +def test_organisation_type_pages_are_platform_admin_only( + client_request, + method, +): + getattr(client_request, method)( + 'main.set_organisation_type', + service_id=SERVICE_ONE_ID, + _expected_status=403, + _test_page_title=False, + ) + + +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', [ + 'central', + 'local', + 'nhs', + pytest.mark.xfail('private sector'), +]) +def test_should_set_organisaton_type( + logged_in_platform_admin_client, + mock_update_service, + organisation_type, +): + 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, + ) + + def test_switch_service_enable_letters( logged_in_platform_admin_client, service_one, From d9104ba59f47becfab6ff402ab36b051ec846281 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 5 Oct 2017 13:49:43 +0100 Subject: [PATCH 8/8] Add page to change SMS allowance There may be exceptions to the rule, so we should have a way of overriding this manually. --- app/main/forms.py | 9 ++++ app/main/views/service_settings.py | 23 +++++++- app/notify_client/service_api_client.py | 4 +- app/templates/views/service-settings.html | 2 +- .../set-free-sms-allowance.html | 21 ++++++++ tests/app/main/views/test_service_settings.py | 53 +++++++++++++++++-- 6 files changed, 105 insertions(+), 7 deletions(-) create mode 100644 app/templates/views/service-settings/set-free-sms-allowance.html diff --git a/app/main/forms.py b/app/main/forms.py index 5b5273b99..b6a5aaf38 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -246,6 +246,15 @@ class OrganisationTypeForm(Form): organisation_type = organisation_type() +class FreeSMSAllowance(Form): + free_sms_allowance = IntegerField( + 'Numbers of text message fragments per year', + validators=[ + DataRequired(message='Can’t be empty') + ] + ) + + class ConfirmPasswordForm(Form): def __init__(self, validate_password_func, *args, **kwargs): self.validate_password_func = validate_password_func diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index c4c788920..1dbe5ba0f 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -35,6 +35,7 @@ from app.main.forms import ( ServiceInboundApiForm, InternationalSMSForm, OrganisationTypeForm, + FreeSMSAllowance, ) from app import user_api_client, current_service, organisations_client, inbound_number_client from notifications_utils.formatters import formatted_list @@ -589,14 +590,32 @@ def set_organisation_type(service_id): ) return redirect(url_for('.service_settings', service_id=service_id)) - form.organisation_type.data = current_service.get('organisation_type') - return render_template( 'views/service-settings/set-organisation-type.html', form=form, ) +@main.route("/services//service-settings/set-free-sms-allowance", methods=['GET', 'POST']) +@login_required +@user_has_permissions(admin_override=True) +def set_free_sms_allowance(service_id): + + form = FreeSMSAllowance(free_sms_allowance=current_service['free_sms_fragment_limit']) + + if form.validate_on_submit(): + service_api_client.update_service( + service_id, + free_sms_fragment_limit=form.free_sms_allowance.data, + ) + return redirect(url_for('.service_settings', service_id=service_id)) + + return render_template( + 'views/service-settings/set-free-sms-allowance.html', + form=form, + ) + + @main.route("/services//service-settings/set-branding-and-org", methods=['GET', 'POST']) @login_required @user_has_permissions(admin_override=True) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index a35044e6c..49bccdfdd 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -104,7 +104,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): 'organisation', 'letter_contact_block', 'dvla_organisation', - 'permissions' + 'permissions', + 'organisation_type', + 'free_sms_fragment_limit', } if disallowed_attributes: raise TypeError('Not allowed to update service attributes: {}'.format( diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 77033ba94..c85a3920b 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -198,7 +198,7 @@ {% call row() %} {{ text_field('Free text message allowance')}} {{ text_field('{:,}'.format(current_service.free_sms_fragment_limit)) }} - {{ text_field() }} + {{ edit_field('Change', url_for('.set_free_sms_allowance', service_id=current_service.id)) }} {% endcall %} {% call row() %} {{ text_field('Email branding' )}} diff --git a/app/templates/views/service-settings/set-free-sms-allowance.html b/app/templates/views/service-settings/set-free-sms-allowance.html new file mode 100644 index 000000000..58ff3b36d --- /dev/null +++ b/app/templates/views/service-settings/set-free-sms-allowance.html @@ -0,0 +1,21 @@ +{% extends "withnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/page-footer.html" import page_footer %} + +{% block service_page_title %} + Emails +{% endblock %} + +{% block maincolumn_content %} + +
+

Free text message allowance

+ {{ textbox(form.free_sms_allowance) }} + {{ page_footer( + 'Save', + back_link=url_for('.service_settings', service_id=current_service.id), + back_link_text='Back to settings' + ) }} +
+ +{% endblock %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index d0fa1d07d..e07b27215 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -66,7 +66,7 @@ from tests.conftest import ( 'Label Value Action', 'Organisation type Central Change', - 'Free text message allowance 250,000', + 'Free text message allowance 250,000 Change', 'Email branding GOV.UK Change', 'Letter branding HM Government Change', @@ -1363,12 +1363,17 @@ 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( client_request, method, + endpoint, ): getattr(client_request, method)( - 'main.set_organisation_type', + endpoint, service_id=SERVICE_ONE_ID, _expected_status=403, _test_page_title=False, @@ -1406,7 +1411,7 @@ def test_should_show_page_to_set_organisation_type( 'nhs', pytest.mark.xfail('private sector'), ]) -def test_should_set_organisaton_type( +def test_should_set_organisation_type( logged_in_platform_admin_client, mock_update_service, organisation_type, @@ -1430,6 +1435,48 @@ def test_should_set_organisaton_type( ) +def test_should_show_page_to_set_sms_allowance( + logged_in_platform_admin_client, +): + response = logged_in_platform_admin_client.get(url_for( + 'main.set_free_sms_allowance', + service_id=SERVICE_ONE_ID + )) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert normalize_spaces(page.select_one('label').text) == 'Numbers of text message fragments per year' + + +@pytest.mark.parametrize('given_allowance, expected_api_argument', [ + ('1', 1), + ('250000', 250000), + pytest.mark.xfail(('foo', 'foo')), +]) +def test_should_set_sms_allowance( + logged_in_platform_admin_client, + mock_update_service, + given_allowance, + expected_api_argument, +): + response = logged_in_platform_admin_client.post( + url_for( + 'main.set_free_sms_allowance', + service_id=SERVICE_ONE_ID, + ), + data={ + 'free_sms_allowance': given_allowance, + }, + ) + 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, + free_sms_fragment_limit=expected_api_argument, + ) + + def test_switch_service_enable_letters( logged_in_platform_admin_client, service_one,