From 10100e51b6f7e9c96ac499587da05a795ea0460a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 28 Nov 2018 13:48:13 +0000 Subject: [PATCH] Let users add new template from the choose page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we’re letting users add new folders directly from the choose page it makes sense that they should also be able to add templates from there. This resolves the problem we saw in user research where people found it hard to know where to go to add a new folder when they were all behind one green button. --- app/main/forms.py | 29 +++- app/main/views/templates.py | 95 ++++++----- app/templates/views/templates/_move_to.html | 33 ++-- tests/app/main/views/test_template_folders.py | 5 +- tests/app/main/views/test_templates.py | 147 +++++++++++++++--- tests/conftest.py | 4 + 6 files changed, 234 insertions(+), 79 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 2a1b5b66f..6607c99a0 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -717,7 +717,14 @@ class FieldWithNoneOption(): class RadioFieldWithNoneOption(FieldWithNoneOption, RadioField): - pass + + def validate(self, *args, **kwargs): + + if self.data == self.NONE_OPTION_VALUE: + self.data = None + return True + + return super().validate(*args, **kwargs) class HiddenFieldWithNoneOption(FieldWithNoneOption, HiddenField): @@ -1174,6 +1181,8 @@ class TemplateAndFoldersSelectionForm(Form): all_template_folders, template_list, current_folder_id, + allow_adding_letter_template, + allow_adding_copy_of_template, *args, **kwargs ): @@ -1183,7 +1192,7 @@ class TemplateAndFoldersSelectionForm(Form): self.templates_and_folders.choices = template_list.as_id_and_name self.op = None - self.is_move_op = self.is_add_op = False + self.is_move_op = self.is_add_folder_op = self.is_add_template_op = False self.move_to.choices = [ (item['id'], item['name']) @@ -1191,13 +1200,21 @@ class TemplateAndFoldersSelectionForm(Form): if item['id'] != str(current_folder_id) ] + self.add_template_by_template_type.choices = filter(None, [ + ('email', 'Email template'), + ('sms', 'Text message template'), + ('letter', 'Letter template') if allow_adding_letter_template else None, + ('copy-existing', 'Copy of an existing template') if allow_adding_copy_of_template else None, + ]) + def validate(self): self.op = request.form.get('operation') self.is_move_op = self.op in {'move_to_existing_folder', 'move_to_new_folder'} - self.is_add_op = self.op in {'add_new_folder', 'move_to_new_folder'} + self.is_add_folder_op = self.op in {'add_new_folder', 'move_to_new_folder'} + self.is_add_template_op = self.op in {'add_template'} - if not (self.is_add_op or self.is_move_op): + if not (self.is_add_folder_op or self.is_move_op or self.is_add_template_op): return False return super().validate() @@ -1218,3 +1235,7 @@ class TemplateAndFoldersSelectionForm(Form): ]) add_new_folder_name = StringField('Folder name', validators=[required_for_ops('add_new_folder')]) move_to_new_folder_name = StringField('Folder name', validators=[required_for_ops('move_to_new_folder')]) + + add_template_by_template_type = RadioFieldWithNoneOption('Add new', validators=[ + required_for_ops('add_template') + ]) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 4a2854a9a..ad81a475e 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -116,6 +116,11 @@ def choose_template(service_id, template_type='all', template_folder_id=None): template_list=template_list, template_type=template_type, current_folder_id=template_folder_id, + allow_adding_letter_template=current_service.has_permission('letter'), + allow_adding_copy_of_template=( + current_service.all_templates or + len(user_api_client.get_service_ids_for_user(current_user)) > 1 + ), ) if request.method == 'POST' and templates_and_folders_form.validate_on_submit(): @@ -144,7 +149,13 @@ def choose_template(service_id, template_type='all', template_folder_id=None): def process_folder_management_form(form, current_folder_id): new_folder_id = None - if form.is_add_op: + if form.is_add_template_op: + return _add_template_by_type( + form.add_template_by_template_type.data, + current_folder_id, + ) + + if form.is_add_folder_op: new_folder_id = template_folder_api_client.create_template_folder( current_service.id, name=form.get_folder_name(), @@ -248,55 +259,59 @@ def add_template_by_type(service_id, template_folder_id=None): form = ChooseTemplateType( include_letters=current_service.has_permission('letter'), include_copy=any(( - service_api_client.count_service_templates(service_id) > 0, + len(current_service.all_templates) > 0, len(user_api_client.get_service_ids_for_user(current_user)) > 1, )), include_folder=current_service.has_permission('edit_folders') ) 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', - 'letter', - 'Body', - service_id, - 'Main heading', - 'normal', - template_folder_id - ) - return redirect(url_for( - '.view_template', - service_id=service_id, - template_id=blank_letter['data']['id'], - )) - - if email_or_sms_not_enabled(form.template_type.data, current_service.permissions): - return redirect(url_for( - '.action_blocked', - service_id=service_id, - notification_type=form.template_type.data, - return_to='add_new_template', - template_id='0' - )) - else: - return redirect(url_for( - '.add_service_template', - service_id=service_id, - template_type=form.template_type.data, - template_folder_id=template_folder_id, - )) + return _add_template_by_type(form.template_type.data, template_folder_id) return render_template('views/templates/add.html', form=form) +def _add_template_by_type(template_type, template_folder_id): + + if template_type == 'copy-existing': + return redirect(url_for( + '.choose_template_to_copy', + service_id=current_service.id, + )) + + if template_type == 'letter': + blank_letter = service_api_client.create_service_template( + 'Untitled', + 'letter', + 'Body', + current_service.id, + 'Main heading', + 'normal', + template_folder_id + ) + return redirect(url_for( + '.view_template', + service_id=current_service.id, + template_id=blank_letter['data']['id'], + )) + + if email_or_sms_not_enabled(template_type, current_service.permissions): + return redirect(url_for( + '.action_blocked', + service_id=current_service.id, + notification_type=template_type, + return_to='add_new_template', + template_id='0' + )) + else: + return redirect(url_for( + '.add_service_template', + service_id=current_service.id, + template_type=template_type, + template_folder_id=template_folder_id, + )) + + @main.route("/services//templates/copy") @login_required @user_has_permissions('manage_templates') diff --git a/app/templates/views/templates/_move_to.html b/app/templates/views/templates/_move_to.html index 10c6b7341..b8e35407e 100644 --- a/app/templates/views/templates/_move_to.html +++ b/app/templates/views/templates/_move_to.html @@ -1,19 +1,20 @@ {% from "components/radios.html" import radios %} {% from "components/page-footer.html" import page_footer %} -{% if templates_and_folders_form.move_to.choices and template_list.templates_to_show %} -
- {{ radios(templates_and_folders_form.move_to) }} - {{ page_footer('Move', button_name='operation', button_value='move_to_existing_folder') }} -
-
-
- Move to a new folder - {{ textbox(templates_and_folders_form.move_to_new_folder_name) }} - {{ page_footer('Move to a new folder', button_name='operation', button_value='move_to_new_folder') }} -
-
+ {% if templates_and_folders_form.move_to.choices and template_list.templates_to_show %} +
+ {{ radios(templates_and_folders_form.move_to) }} + {{ page_footer('Move', button_name='operation', button_value='move_to_existing_folder') }} +
+
+
+ Move to a new folder + {{ textbox(templates_and_folders_form.move_to_new_folder_name) }} + {{ page_footer('Move to a new folder', button_name='operation', button_value='move_to_new_folder') }} +
+
+ {% endif %}
Add a new folder @@ -21,4 +22,10 @@ {{ page_footer('New folder', button_name='operation', button_value='add_new_folder') }}
-{% endif %} +
+
+ Add a new template + {{ radios(templates_and_folders_form.add_template_by_template_type) }} + {{ page_footer('Continue', button_name='operation', button_value='add_template') }} +
+
diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 2943a6a6c..dc65ede19 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -785,7 +785,7 @@ def test_should_show_radios_and_buttons_for_move_destination_if_correct_permissi 'main.choose_template', service_id=SERVICE_ONE_ID, ) - radios = page.select('input[type=radio]') + radios = page.select('#move_to_folder_radios input[type=radio]') radio_div = page.find('div', {'id': 'move_to_folder_radios'}) assert radios == page.select('input[name=move_to]') @@ -799,7 +799,8 @@ def test_should_show_radios_and_buttons_for_move_destination_if_correct_permissi 'unknown', 'move_to_existing_folder', 'move_to_new_folder', - 'add_new_folder' + 'add_new_folder', + 'add_template', } diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index be1a2c5e2..6ade834fc 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -21,6 +21,7 @@ from tests.conftest import ( SERVICE_ONE_ID, SERVICE_TWO_ID, TEMPLATE_ONE_ID, + ElementNotFound, active_caseworking_user, active_user_view_permissions, mock_get_service_email_template, @@ -40,6 +41,7 @@ from tests.conftest import single_letter_contact_block def test_should_show_empty_page_when_no_templates( client_request, service_one, + mock_get_organisations_and_services_for_user, mock_get_service_templates_when_no_templates_exist, mock_get_template_folders, extra_permissions, @@ -128,6 +130,7 @@ def test_should_show_empty_page_when_no_templates( ) def test_should_show_page_for_choosing_a_template( client_request, + mock_get_organisations_and_services_for_user, mock_get_service_templates, mock_get_template_folders, mock_has_no_jobs, @@ -242,6 +245,63 @@ 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(['edit_folders'], [ + 'email', + 'sms', + 'copy-existing', + ], [ + 'Email template', + 'Text message template', + 'Copy of an existing template', + ]), + pytest.param(['edit_folders', 'letter'], [ + 'email', + 'sms', + 'letter', + 'copy-existing', + ], [ + 'Email template', + 'Text message template', + 'Letter template', + 'Copy of an existing template', + ]), + pytest.param( + [], [], [], + marks=pytest.mark.xfail(raises=ElementNotFound) + ), +)) +def test_should_show_new_template_choices_if_service_has_folder_permission( + client_request, + service_one, + mock_get_organisations_and_services_for_user, + mock_get_service_templates, + mock_get_template_folders, + extra_permissions, + expected_values, + expected_labels, +): + service_one['permissions'] += extra_permissions + + page = client_request.get( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + ) + + if not page.select('#add_new_template_form'): + raise ElementNotFound() + + assert normalize_spaces(page.select_one('#add_new_template_form fieldset fieldset legend').text) == ( + 'Add new' + ) + assert [ + choice['value'] for choice in page.select('#add_new_template_form input[type=radio]') + ] == expected_values + assert [ + normalize_spaces(choice.text) for choice in page.select('#add_new_template_form label') + ] == expected_labels + + def test_should_show_page_for_one_template( logged_in_client, mock_get_service_template, @@ -551,15 +611,35 @@ def test_dont_show_preview_letter_templates_for_bad_filetype( assert mock_get_service_template.called is False +@pytest.mark.parametrize('endpoint, data', ( + ('main.add_template_by_type', { + 'template_type': 'copy-existing' + }), + ('main.choose_template', { + 'operation': 'add_template', + 'add_template_by_template_type': 'copy-existing' + }), +)) def test_choosing_to_copy_redirects( client_request, + service_one, mock_get_service_templates, + mock_get_template_folders, mock_get_organisations_and_services_for_user, + endpoint, + data, ): + service_one['permissions'] += ['edit_folders'] client_request.post( - 'main.add_template_by_type', + endpoint, service_id=SERVICE_ONE_ID, - _data={'template_type': 'copy-existing'} + _data=data, + _expected_status=302, + _expected_redirect=url_for( + 'main.choose_template_to_copy', + service_id=SERVICE_ONE_ID, + _external=True, + ), ) @@ -693,29 +773,56 @@ def test_cant_copy_template_from_non_member_service( assert mock_get_service_email_template.call_args_list == [] -@pytest.mark.parametrize('type_of_template', ['email', 'sms']) +@pytest.mark.parametrize('endpoint, data, expected_error', ( + ( + 'main.add_template_by_type', + { + 'template_type': 'email', + }, + "Sending emails has been disabled for your service." + ), + ( + 'main.add_template_by_type', + { + 'template_type': 'sms', + }, + "Sending text messages has been disabled for your service." + ), + ( + 'main.choose_template', + { + 'operation': 'add_template', + 'add_template_by_template_type': 'email', + }, + "Sending emails has been disabled for your service." + ), + ( + 'main.choose_template', + { + 'operation': 'add_template', + 'add_template_by_template_type': 'sms', + }, + "Sending text messages has been disabled for your service." + ), +)) def test_should_not_allow_creation_of_template_through_form_without_correct_permission( - logged_in_client, + client_request, service_one, - mocker, mock_get_service_templates, + mock_get_template_folders, mock_get_organisations_and_services_for_user, - type_of_template, + endpoint, + data, + expected_error, ): - service_one['permissions'] = [] - template_description = {'sms': 'text messages', 'email': 'emails'} - - response = logged_in_client.post(url_for( - '.add_template_by_type', - service_id=service_one['id']), - data={'template_type': type_of_template}, - follow_redirects=True) - - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - - assert response.status_code == 200 - assert page.select('main p')[0].text.strip() == \ - "Sending {} has been disabled for your service.".format(template_description[type_of_template]) + service_one['permissions'] = ['edit_folders'] + page = client_request.post( + endpoint, + service_id=SERVICE_ONE_ID, + _data=data, + _follow_redirects=True, + ) + assert normalize_spaces(page.select('main p')[0].text) == expected_error assert page.select(".page-footer-back-link")[0].text == "Back to add new template" assert page.select(".page-footer-back-link")[0]['href'] == url_for( '.add_template_by_type', diff --git a/tests/conftest.py b/tests/conftest.py index 0d964d14b..824d580d7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -32,6 +32,10 @@ from . import ( ) +class ElementNotFound(Exception): + pass + + @pytest.fixture def app_(request): app = Flask('app')