From 19632ea4abb8e2c8f26fcafa7db74d561a99b0b3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Jul 2018 12:30:04 +0100 Subject: [PATCH] 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): """