From bf5ccd044bba3b275add194e809322be41256bb0 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 10 Aug 2020 15:12:07 +0100 Subject: [PATCH 1/4] Services can only create templates of types they have turned on --- app/main/forms.py | 8 ++--- tests/app/main/views/test_templates.py | 48 +++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 8a4f09e2c..a013733a4 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1997,12 +1997,8 @@ class TemplateAndFoldersSelectionForm(Form): ] self.add_template_by_template_type.choices = list(filter(None, [ - # We want to show email and text message to everyone, - # whether or not the service has them switched on. The - # option to add letter or broadcast templates should only - # be shown to services which have that permission - ('email', 'Email'), - ('sms', 'Text message'), + ('email', 'Email') if 'email' in available_template_types else None, + ('sms', 'Text message') if 'sms' in available_template_types else None, ('letter', 'Letter') if 'letter' in available_template_types else None, ('broadcast', 'Broadcast') if 'broadcast' in available_template_types else None, ('copy-existing', 'Copy an existing template') if allow_adding_copy_of_template else None, diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index e67ffa759..aa2b4ad62 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -287,8 +287,8 @@ def test_should_show_live_search_if_service_has_lots_of_folders( assert count_of_templates == 4 -@pytest.mark.parametrize('extra_permissions, expected_values, expected_labels', ( - pytest.param([], [ +@pytest.mark.parametrize('service_permissions, expected_values, expected_labels', ( + pytest.param(['email', 'sms'], [ 'email', 'sms', 'copy-existing', @@ -297,7 +297,12 @@ def test_should_show_live_search_if_service_has_lots_of_folders( 'Text message', 'Copy an existing template', ]), - pytest.param(['letter'], [ + pytest.param(['broadcast'], [ + 'broadcast', + ], [ + 'Broadcast', + ]), + pytest.param(['email', 'sms', 'letter'], [ 'email', 'sms', 'letter', @@ -314,11 +319,11 @@ def test_should_show_new_template_choices_if_service_has_folder_permission( service_one, mock_get_service_templates, mock_get_template_folders, - extra_permissions, + service_permissions, expected_values, expected_labels, ): - service_one['permissions'] += extra_permissions + service_one['permissions'] = service_permissions page = client_request.get( 'main.choose_template', @@ -339,6 +344,39 @@ def test_should_show_new_template_choices_if_service_has_folder_permission( ] == expected_labels +@pytest.mark.parametrize("permissions,are_data_attrs_added", [ + (['sms'], True), + (['email'], True), + (['letter'], True), + (['broadcast'], True), + (['sms', 'email'], False), +]) +def test_should_add_data_attributes_for_services_that_only_allow_one_type_of_notifications( + client_request, + service_one, + mock_get_service_templates, + mock_get_template_folders, + permissions, + are_data_attrs_added +): + service_one['permissions'] = permissions + + page = client_request.get( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + ) + + if not page.select('#add_new_template_form'): + raise ElementNotFound() + + if are_data_attrs_added: + assert page.find(id='add_new_template_form').attrs['data-channel'] == permissions[0] + assert page.find(id='add_new_template_form').attrs['data-service'] == SERVICE_ONE_ID + else: + assert page.find(id='add_new_template_form').attrs.get('data-channel') is None + assert page.find(id='add_new_template_form').attrs.get('data-service') is None + + def test_should_show_page_for_one_template( client_request, mock_get_service_template, From bdfc0adcc02cb5fe3e17bd0b54f857758e31c532 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 10 Aug 2020 18:14:36 +0100 Subject: [PATCH 2/4] New template button creates new template for broadcast services --- app/assets/javascripts/templateFolderForm.js | 12 +++++++++--- app/templates/views/templates/_move_to.html | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index 22a7f1a68..a9f2a4bfd 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -153,13 +153,19 @@ return changed; }; + this.$broadcastService = (document.querySelector('div[id=add_new_template_form]')).getAttribute("data-broadcast") + this.actionButtonClicked = function(event) { event.preventDefault(); this.currentState = $(event.currentTarget).val(); - if (this.stateChanged()) { - this.render(); - } + if (event.currentTarget.value === 'add-new-template' && this.$broadcastService) { + return window.location = "/services/" + this.$broadcastService + "/templates/add-broadcast"; + } else { + if (this.stateChanged()) { + this.render(); + }; + }; }; this.selectionStatus = { diff --git a/app/templates/views/templates/_move_to.html b/app/templates/views/templates/_move_to.html index f4d2ee494..09a69e186 100644 --- a/app/templates/views/templates/_move_to.html +++ b/app/templates/views/templates/_move_to.html @@ -28,7 +28,7 @@ {{ page_footer('Add new folder', button_name='operation', button_value='add-new-folder') }} -
+
{{ radios(templates_and_folders_form.add_template_by_template_type) }}
From 36c1ffa7be8cf79d7b8b53f7007943fc2f0c549a Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 11 Aug 2020 15:44:17 +0100 Subject: [PATCH 3/4] New template button takes user to new template page for all services that only allow sending one type of notifications --- app/assets/javascripts/templateFolderForm.js | 11 +++--- app/main/views/templates.py | 7 ++++ app/templates/views/templates/_move_to.html | 2 +- app/utils.py | 2 + tests/javascripts/templateFolderForm.test.js | 39 ++++++++++++++++++-- 5 files changed, 51 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index a9f2a4bfd..c7d804f4e 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -153,19 +153,20 @@ return changed; }; - this.$broadcastService = (document.querySelector('div[id=add_new_template_form]')).getAttribute("data-broadcast") + this.$singleNotificationChannel = (document.querySelector('div[id=add_new_template_form]')).getAttribute("data-channel"); + this.$singleChannelService = (document.querySelector('div[id=add_new_template_form]')).getAttribute("data-service"); this.actionButtonClicked = function(event) { event.preventDefault(); this.currentState = $(event.currentTarget).val(); - if (event.currentTarget.value === 'add-new-template' && this.$broadcastService) { - return window.location = "/services/" + this.$broadcastService + "/templates/add-broadcast"; + if (event.currentTarget.value === 'add-new-template' && this.$singleNotificationChannel) { + window.location = "/services/" + this.$singleChannelService + "/templates/add-" + this.$singleNotificationChannel; } else { if (this.stateChanged()) { this.render(); - }; - }; + } + } }; this.selectionStatus = { diff --git a/app/main/views/templates.py b/app/main/views/templates.py index fcd394f4e..384abda56 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -31,6 +31,7 @@ from app.models.service import Service from app.models.template_list import TemplateList, TemplateLists from app.template_previews import TemplatePreview, get_page_count_for_letter from app.utils import ( + NOTIFICATION_TYPES, get_template, should_skip_template_page, user_has_permissions, @@ -130,6 +131,11 @@ def choose_template(service_id, template_type='all', template_folder_id=None): ) option_hints = {template_folder_id: 'current folder'} + single_notification_channel = None + notification_channels = list(set(current_service.permissions).intersection(NOTIFICATION_TYPES)) + if len(notification_channels) == 1: + single_notification_channel = notification_channels[0] + if request.method == 'POST' and templates_and_folders_form.validate_on_submit(): if not current_user.has_permissions('manage_templates'): abort(403) @@ -168,6 +174,7 @@ def choose_template(service_id, template_type='all', template_folder_id=None): templates_and_folders_form=templates_and_folders_form, move_to_children=templates_and_folders_form.move_to.children(), user_has_template_folder_permission=user_has_template_folder_permission, + single_notification_channel=single_notification_channel, option_hints=option_hints ) diff --git a/app/templates/views/templates/_move_to.html b/app/templates/views/templates/_move_to.html index 09a69e186..103054d69 100644 --- a/app/templates/views/templates/_move_to.html +++ b/app/templates/views/templates/_move_to.html @@ -28,7 +28,7 @@ {{ page_footer('Add new folder', button_name='operation', button_value='add-new-folder') }}
-
+
{{ radios(templates_and_folders_form.add_template_by_template_type) }}
diff --git a/app/utils.py b/app/utils.py index 39ec1f771..c22b4c4a8 100644 --- a/app/utils.py +++ b/app/utils.py @@ -60,6 +60,8 @@ FAILURE_STATUSES = ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure', 'virus-scan-failed', 'validation-failed'] REQUESTED_STATUSES = SENDING_STATUSES + DELIVERED_STATUSES + FAILURE_STATUSES +NOTIFICATION_TYPES = ["sms", "email", "letter", "broadcast"] + with open('{}/email_domains.txt'.format( os.path.dirname(os.path.realpath(__file__)) diff --git a/tests/javascripts/templateFolderForm.test.js b/tests/javascripts/templateFolderForm.test.js index cf880c4bb..0e6612374 100644 --- a/tests/javascripts/templateFolderForm.test.js +++ b/tests/javascripts/templateFolderForm.test.js @@ -1,6 +1,6 @@ const helpers = require('./support/helpers'); -function setFixtures (hierarchy) { +function setFixtures (hierarchy, newTemplateDataModules = "") { const foldersCheckboxesHTML = function (filter) { let count = 0; @@ -27,7 +27,7 @@ function setFixtures (hierarchy) { }(); - function controlsHTML () { + function controlsHTML (newTemplateDataModules) { return `
@@ -78,7 +78,7 @@ function setFixtures (hierarchy) {
-
+
@@ -127,7 +127,7 @@ function setFixtures (hierarchy) { document.body.innerHTML = `
${helpers.templatesAndFoldersCheckboxes(hierarchy)} - ${controlsHTML()} + ${controlsHTML(newTemplateDataModules)}
`; }; @@ -309,6 +309,37 @@ describe('TemplateFolderForm', () => { }); + describe("Click 'New template' for single channel service", () => { + test("should redirect to new template page", () => { + setFixtures(hierarchy, "data-channel='sms' data-service='123'") + templateFolderForm = document.querySelector('form[data-module=template-folder-form]'); + + // start module + window.GOVUK.modules.start(); + + formControls = templateFolderForm.querySelector('#sticky_template_forms'); + + // reset sticky JS mocks called when the module starts + resetStickyMocks(); + // add listener for url change + const descriptor1 = Object.getOwnPropertyDescriptor(window, 'location'); + delete window.location + + const mockCallback = jest.fn(x => {}); + + Object.defineProperty(window, 'location', { + set: mockCallback + }); + // click + helpers.triggerEvent(formControls.querySelector('[value=add-new-template]'), 'click'); + // expect url to change + expect(mockCallback).toHaveBeenCalledWith("/services/123/templates/add-sms") + + setFixtures(hierarchy) + resetStickyMocks() + }); + }) + describe("Clicking 'New template'", () => { beforeEach(() => { From 95078d2d519142bfce2d4ce5eb8a86c9c6745bfa Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 17 Aug 2020 13:01:33 +0100 Subject: [PATCH 4/4] Change page title on new broadcast template page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since broadcast services can only have one type of template we probably don’t need to disambiguate what kind of template you’re creating. And you’ve just come from a page where the button says ‘New template’, without the choice of radios after, so it’s nice for the page title to match that. --- .../views/edit-broadcast-template.html | 4 ++-- tests/app/main/views/test_templates.py | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/app/templates/views/edit-broadcast-template.html b/app/templates/views/edit-broadcast-template.html index 5b97e9a6f..f88bdb44a 100644 --- a/app/templates/views/edit-broadcast-template.html +++ b/app/templates/views/edit-broadcast-template.html @@ -5,13 +5,13 @@ {% from "components/form.html" import form_wrapper %} {% block service_page_title %} - {{ heading_action }} broadcast template + {{ heading_action }} template {% endblock %} {% block maincolumn_content %} {{ page_header( - '{} broadcast template'.format(heading_action), + '{} template'.format(heading_action), back_link=url_for('main.view_template', service_id=current_service.id, template_id=template.id) if template else url_for('main.choose_template', service_id=current_service.id, template_folder_id=template_folder_id) ) }} diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index aa2b4ad62..13f859a78 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -2052,6 +2052,26 @@ def test_route_invalid_permissions( service_one) +@pytest.mark.parametrize('template_type, expected', ( + ('email', 'New email template'), + ('sms', 'New text message template'), + ('broadcast', 'New template'), +)) +def test_add_template_page_title( + client_request, + service_one, + template_type, + expected, +): + service_one['permissions'] += [template_type] + page = client_request.get( + '.add_service_template', + service_id=SERVICE_ONE_ID, + template_type=template_type, + ) + assert normalize_spaces(page.select_one('h1').text) == expected + + def test_can_create_email_template_with_emoji( client_request, mock_create_service_template