From 19632ea4abb8e2c8f26fcafa7db74d561a99b0b3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Jul 2018 12:30:04 +0100 Subject: [PATCH 1/2] Add option to copy existing template when adding Sometimes when setting up a service you might have a few very similar templates, in which only a small amount of content. Or you might even have a few of services, which are used by different teams but have similar templates. Copy and pasting, especially from one service to another, is a pain. This commit makes it easier by allowing users to copy an existing template when choosing to add a new one, instead of starting from scratch. --- app/assets/stylesheets/_grids.scss | 8 ++ app/main/forms.py | 5 +- app/main/views/templates.py | 64 +++++++++- app/navigation.py | 8 ++ app/notify_client/user_api_client.py | 16 +++ app/templates/views/edit-email-template.html | 5 +- app/templates/views/edit-letter-template.html | 5 +- app/templates/views/edit-sms-template.html | 5 +- app/templates/views/templates/copy.html | 36 ++++++ tests/app/main/views/test_letters.py | 6 +- tests/app/main/views/test_templates.py | 118 ++++++++++++++++++ tests/conftest.py | 27 +++- 12 files changed, 293 insertions(+), 10 deletions(-) create mode 100644 app/templates/views/templates/copy.html diff --git a/app/assets/stylesheets/_grids.scss b/app/assets/stylesheets/_grids.scss index 84cbbe94c..a4b7fdbcb 100644 --- a/app/assets/stylesheets/_grids.scss +++ b/app/assets/stylesheets/_grids.scss @@ -81,6 +81,14 @@ margin-bottom: $gutter * 2; } +.left-gutter { + padding-left: $gutter; +} + +.left-gutter-4-3 { + padding-left: $gutter * 4 / 3; +} + .align-with-heading { display: block; text-align: center; diff --git a/app/main/forms.py b/app/main/forms.py index f0f7d790a..78c2d7dd2 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -837,14 +837,15 @@ class ChooseTemplateType(StripWhitespaceForm): ] ) - def __init__(self, include_letters=False, *args, **kwargs): + def __init__(self, include_letters=False, include_copy=False, *args, **kwargs): super().__init__(*args, **kwargs) self.template_type.choices = filter(None, [ ('email', 'Email'), ('sms', 'Text message'), - ('letter', 'Letter') if include_letters else None + ('letter', 'Letter') if include_letters else None, + ('copy-existing', 'Copy of an existing template') if include_copy else None, ]) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 1029b5fd8..5430b180a 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -9,7 +9,12 @@ from notifications_python_client.errors import HTTPError from notifications_utils.formatters import nl2br from notifications_utils.recipients import first_column_headings -from app import current_service, service_api_client, template_statistics_client +from app import ( + current_service, + service_api_client, + template_statistics_client, + user_api_client, +) from app.main import main from app.main.forms import ( ChooseTemplateType, @@ -202,11 +207,21 @@ def view_template_version_preview(service_id, template_id, version, filetype): def add_template_by_type(service_id): form = ChooseTemplateType( - include_letters='letter' in current_service['permissions'] + include_letters='letter' in current_service['permissions'], + include_copy=any(( + service_api_client.count_service_templates(service_id), + len(user_api_client.get_service_ids_for_user(current_user)) > 1, + )), ) if form.validate_on_submit(): + if form.template_type.data == 'copy-existing': + return redirect(url_for( + '.choose_template_to_copy', + service_id=service_id, + )) + if form.template_type.data == 'letter': blank_letter = service_api_client.create_service_template( 'Untitled', @@ -240,6 +255,51 @@ def add_template_by_type(service_id): return render_template('views/templates/add.html', form=form) +@main.route("/services//templates/copy") +@login_required +@user_has_permissions('manage_templates') +def choose_template_to_copy(service_id): + return render_template( + 'views/templates/copy.html', + services=[{ + 'name': service['name'], + 'id': service['id'], + 'templates': [ + template for template in + service_api_client.get_service_templates(service['id'])['data'] + if template['template_type'] in current_service['permissions'] + ], + } for service in user_api_client.get_services_for_user(current_user)], + ) + + +@main.route("/services//templates/copy/", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_templates') +def copy_template(service_id, template_id): + + if not user_api_client.user_belongs_to_service( + current_user, request.args.get('from_service') + ): + abort(403) + + template = service_api_client.get_service_template( + request.args.get('from_service'), + str(template_id), + )['data'] + template['template_content'] = template['content'] + template['name'] = 'Copy of ‘{}’'.format(template['name']) + form = form_objects[template['template_type']](**template) + + return render_template( + 'views/edit-{}-template.html'.format(template['template_type']), + form=form, + template_type=template['template_type'], + heading_action='Add', + services=user_api_client.get_service_ids_for_user(current_user), + ) + + @main.route("/services//templates/action-blocked///") @login_required @user_has_permissions('manage_templates') diff --git a/app/navigation.py b/app/navigation.py index 076af3d12..54b13e6b7 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -125,6 +125,7 @@ class HeaderNavigation(Navigation): 'choose_account', 'choose_service', 'choose_template', + 'choose_template_to_copy', 'confirm_edit_organisation_name', 'confirm_redact_template', 'conversation', @@ -132,6 +133,7 @@ class HeaderNavigation(Navigation): 'conversation_reply_with_template', 'conversation_updates', 'cookies', + 'copy_template', 'create_api_key', 'delete_service_template', 'delivery_and_failure', @@ -291,8 +293,10 @@ class MainNavigation(Navigation): 'check_messages', 'check_notification', 'choose_template', + 'choose_template_to_copy', 'confirm_redact_template', 'conversation_reply', + 'copy_template', 'delete_service_template', 'edit_service_template', 'send_messages', @@ -544,6 +548,7 @@ class CaseworkNavigation(Navigation): 'check_notification', 'choose_account', 'choose_service', + 'choose_template_to_copy', 'confirm_edit_organisation_name', 'confirm_redact_template', 'conversation', @@ -551,6 +556,7 @@ class CaseworkNavigation(Navigation): 'conversation_reply_with_template', 'conversation_updates', 'cookies', + 'copy_template', 'create_api_key', 'create_email_branding', 'delete_service_template', @@ -771,12 +777,14 @@ class OrgNavigation(Navigation): 'choose_account', 'choose_service', 'choose_template', + 'choose_template_to_copy', 'confirm_redact_template', 'conversation', 'conversation_reply', 'conversation_reply_with_template', 'conversation_updates', 'cookies', + 'copy_template', 'create_api_key', 'create_email_branding', 'delete_service_template', diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 02b9bfd5b..7a898ea21 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -1,3 +1,5 @@ +from itertools import chain + from notifications_python_client.errors import HTTPError from app.notify_client import NotifyAdminAPIClient, cache @@ -209,3 +211,17 @@ class UserApiClient(NotifyAdminAPIClient): def get_organisations_and_services_for_user(self, user): endpoint = '/user/{}/organisations-and-services'.format(user.id) return self.get(endpoint) + + def get_services_for_user(self, user): + orgs_and_services_for_user = self.get_organisations_and_services_for_user(user) + return orgs_and_services_for_user['services_without_organisations'] + next(chain( + org['services'] for org in orgs_and_services_for_user['organisations'] + ), []) + + def get_service_ids_for_user(self, user): + return { + service['id'] for service in self.get_services_for_user(user) + } + + def user_belongs_to_service(self, user, service_id): + return service_id in self.get_service_ids_for_user(user) diff --git a/app/templates/views/edit-email-template.html b/app/templates/views/edit-email-template.html index bc6010a29..6603d79cf 100644 --- a/app/templates/views/edit-email-template.html +++ b/app/templates/views/edit-email-template.html @@ -13,7 +13,10 @@ {{ heading_action }} email template -
+
{{ textbox(form.name, width='1-1', hint='Your recipients won’t see this', rows=10) }} diff --git a/app/templates/views/edit-letter-template.html b/app/templates/views/edit-letter-template.html index 804e40339..0fa49c587 100644 --- a/app/templates/views/edit-letter-template.html +++ b/app/templates/views/edit-letter-template.html @@ -12,7 +12,10 @@ {{ heading_action }} letter template - +
{{ textbox(form.name, width='1-1', hint='Your recipients won’t see this', rows=10) }} diff --git a/app/templates/views/edit-sms-template.html b/app/templates/views/edit-sms-template.html index e7578aee0..63c929b8e 100644 --- a/app/templates/views/edit-sms-template.html +++ b/app/templates/views/edit-sms-template.html @@ -13,7 +13,10 @@ {{ heading_action }} text message template - +
{{ textbox(form.name, width='1-1', hint='Your recipients won’t see this') }} diff --git a/app/templates/views/templates/copy.html b/app/templates/views/templates/copy.html new file mode 100644 index 000000000..8690b4b2c --- /dev/null +++ b/app/templates/views/templates/copy.html @@ -0,0 +1,36 @@ +{% from "components/message-count-label.html" import message_count_label %} + +{% extends "withnav_template.html" %} + +{% block service_page_title %} + Copy an existing template +{% endblock %} + +{% block maincolumn_content %} + +
+

Copy an existing template

+
+ + +{% endblock %} diff --git a/tests/app/main/views/test_letters.py b/tests/app/main/views/test_letters.py index 94bba13bd..09597c427 100644 --- a/tests/app/main/views/test_letters.py +++ b/tests/app/main/views/test_letters.py @@ -56,17 +56,19 @@ def test_letters_lets_in_without_permission( @pytest.mark.parametrize('permissions, choices', [ ( ['email', 'sms', 'letter'], - ['Email', 'Text message', 'Letter'] + ['Email', 'Text message', 'Letter', 'Copy of an existing template'] ), ( ['email', 'sms'], - ['Email', 'Text message'] + ['Email', 'Text message', 'Copy of an existing template'] ), ]) def test_given_option_to_add_letters_if_allowed( logged_in_client, service_one, mocker, + mock_get_service_templates, + mock_get_organisations_and_services_for_user, permissions, choices, ): diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 464a15c22..0cead3cb2 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -18,6 +18,8 @@ from tests import ( ) from tests.conftest import ( SERVICE_ONE_ID, + SERVICE_TWO_ID, + TEMPLATE_ONE_ID, active_caseworking_user, active_user_view_permissions, mock_get_service_email_template, @@ -394,11 +396,127 @@ def test_dont_show_preview_letter_templates_for_bad_filetype( assert mock_get_service_template.called is False +def test_choosing_to_copy_redirects( + client_request, + mock_get_service_templates, + mock_get_organisations_and_services_for_user, +): + client_request.post( + 'main.add_template_by_type', + service_id=SERVICE_ONE_ID, + _data={'template_type': 'copy-existing'} + ) + + +def test_choose_a_template_to_copy( + client_request, + mock_get_service_templates, + mock_get_non_empty_organisations_and_services_for_user, +): + page = client_request.get( + 'main.choose_template_to_copy', + service_id=SERVICE_ONE_ID, + ) + + assert normalize_spaces( + page.select_one('main nav').text + ) == normalize_spaces( + 'Service 1 ' + ' sms_template_one ' + ' Text message template' + ' sms_template_two Text message template' + ' email_template_one Email template' + ' email_template_two Email template ' + 'Service 2 ' + ' sms_template_one' + ' Text message template' + ' sms_template_two' + ' Text message template' + ' email_template_one' + ' Email template' + ' email_template_two' + ' Email template ' + 'Org 1 service 1 ' + ' sms_template_one' + ' Text message template' + ' sms_template_two' + ' Text message template' + ' email_template_one' + ' Email template' + ' email_template_two' + ' Email template ' + 'Org 1 service 2 ' + ' sms_template_one' + ' Text message template' + ' sms_template_two' + ' Text message template' + ' email_template_one' + ' Email template' + ' email_template_two' + ' Email template' + ) + + assert page.select_one('main nav a')['href'] == url_for( + 'main.copy_template', + service_id=SERVICE_ONE_ID, + template_id=TEMPLATE_ONE_ID, + from_service=SERVICE_TWO_ID, + ) + + +def test_load_edit_template_with_copy_of_template( + client_request, + mock_get_service_email_template, + mock_get_non_empty_organisations_and_services_for_user, +): + page = client_request.get( + 'main.copy_template', + service_id=SERVICE_ONE_ID, + template_id=TEMPLATE_ONE_ID, + from_service=SERVICE_TWO_ID, + ) + + assert page.select_one('form')['method'] == 'post' + assert page.select_one('form')['action'] == url_for( + 'main.add_service_template', + service_id=SERVICE_ONE_ID, + template_type='email', + ) + + assert page.select_one('input')['value'] == ( + 'Copy of ‘Two week reminder’' + ) + assert page.select_one('textarea').text == ( + 'Your ((thing)) is due soon' + ) + mock_get_service_email_template.assert_called_once_with( + SERVICE_TWO_ID, + TEMPLATE_ONE_ID, + ) + + +def test_cant_copy_template_from_non_member_service( + client_request, + mock_get_service_email_template, + mock_get_organisations_and_services_for_user, +): + client_request.get( + 'main.copy_template', + service_id=SERVICE_ONE_ID, + template_id=TEMPLATE_ONE_ID, + from_service=SERVICE_TWO_ID, + _expected_status=403, + ) + assert mock_get_service_email_template.call_args_list == [] + + @pytest.mark.parametrize('type_of_template', ['email', 'sms']) def test_should_not_allow_creation_of_template_through_form_without_correct_permission( logged_in_client, service_one, mocker, + mock_get_service_templates, + mock_get_organisations_and_services_for_user, type_of_template, ): service_one['permissions'] = [] diff --git a/tests/conftest.py b/tests/conftest.py index 513c2fe39..6aafd004e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -680,6 +680,7 @@ def mock_update_service_raise_httperror_duplicate_name(mocker): SERVICE_ONE_ID = "596364a0-858e-42c8-9062-a8fe822260eb" SERVICE_TWO_ID = "147ad62a-2951-4fa1-9ca0-093cd1a52c52" ORGANISATION_ID = "c011fa40-4cbe-4524-b415-dde2f421bd9c" +TEMPLATE_ONE_ID = "b22d7d94-2197-4a7d-a8e7-fd5f9770bf48" @pytest.fixture(scope='function') @@ -949,7 +950,7 @@ def mock_update_service_template_400_content_too_big(mocker): @pytest.fixture(scope='function') def mock_get_service_templates(mocker): - uuid1 = str(generate_uuid()) + uuid1 = TEMPLATE_ONE_ID uuid2 = str(generate_uuid()) uuid3 = str(generate_uuid()) uuid4 = str(generate_uuid()) @@ -3013,6 +3014,30 @@ def mock_get_organisations_and_services_for_user(mocker, organisation_one, api_u ) +@pytest.fixture +def mock_get_non_empty_organisations_and_services_for_user(mocker, organisation_one, api_user_active): + + def _make_services(name): + return [{ + 'name': '{} {}'.format(name, i), + 'id': SERVICE_TWO_ID, + } for i in range(1, 3)] + + def _get_orgs_and_services(user_id): + return { + 'organisations': [ + {'name': 'Org 1', 'services': _make_services('Org 1 service')}, + {'name': 'Org 2', 'services': _make_services('Org 2 service')}, + ], + 'services_without_organisations': _make_services('Service') + } + + return mocker.patch( + 'app.user_api_client.get_organisations_and_services_for_user', + side_effect=_get_orgs_and_services + ) + + @pytest.fixture def mock_create_event(mocker): """ From 2070314cb1cfe9cfeed607f54314d32aa5ee0052 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Jul 2018 15:08:02 +0100 Subject: [PATCH 2/2] Fix missing organisation services from user search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The list of services this page was looking at only included those not belonging to an organisation. On production this excludes services we’ve added to organisations to make the management of those services easier (eg ‘GDS’ and ‘DVLA’). --- app/main/views/find_users.py | 4 ++-- app/templates/admin_template.html | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/main/views/find_users.py b/app/main/views/find_users.py index eff9830f5..71a25f66f 100644 --- a/app/main/views/find_users.py +++ b/app/main/views/find_users.py @@ -30,9 +30,9 @@ def find_users_by_email(): @user_is_platform_admin def user_information(user_id): user = user_api_client.get_user(user_id) - services = user_api_client.get_organisations_and_services_for_user(user) + services = user_api_client.get_services_for_user(user) return render_template( 'views/find-users/user-information.html', user=user, - services=services['services_without_organisations'], + services=services, ) diff --git a/app/templates/admin_template.html b/app/templates/admin_template.html index ae715f45f..bdd34ba41 100644 --- a/app/templates/admin_template.html +++ b/app/templates/admin_template.html @@ -109,7 +109,7 @@ {% block footer_support_links %}