diff --git a/app/main/forms.py b/app/main/forms.py index 2a1b5b66f..d46815e8d 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -707,12 +707,21 @@ class ServicePostageForm(StripWhitespaceForm): class FieldWithNoneOption(): - # This needs to match the data the browser will post from - # - NONE_OPTION_VALUE = 'None' + # This is a special value that is specific to our forms. This is + # more expicit than casting `None` to a string `'None'` which can + # have unexpected edge cases + NONE_OPTION_VALUE = '__NONE__' + # When receiving Python data, eg when instantiating the form object + # we want to convert that data to our special value, so that it gets + # recognised as being one of the valid choices + def process_data(self, value): + self.data = self.NONE_OPTION_VALUE if value is None else value + + # After validation we want to convert it back to a Python `None` for + # use elsewhere, eg posting to the API def post_validate(self, form, validation_stopped): - if self.data == self.NONE_OPTION_VALUE: + if self.data == self.NONE_OPTION_VALUE and not validation_stopped: self.data = None @@ -1174,6 +1183,8 @@ class TemplateAndFoldersSelectionForm(Form): all_template_folders, template_list, current_folder_id, + allow_adding_letter_template, + allow_adding_copy_of_template, *args, **kwargs ): @@ -1183,21 +1194,32 @@ 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 + + if current_folder_id is None: + current_folder_id = RadioFieldWithNoneOption.NONE_OPTION_VALUE self.move_to.choices = [ (item['id'], item['name']) for item in ([self.ALL_TEMPLATES_FOLDER] + all_template_folders) - if item['id'] != str(current_folder_id) + if item['id'] != current_folder_id ] + self.add_template_by_template_type.choices = list(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 +1240,8 @@ 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=[ + Optional(), + required_for_ops('add_template') + ]) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 4a2854a9a..5e4617ba9 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(), @@ -247,56 +258,60 @@ 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(user_api_client.get_service_ids_for_user(current_user)) > 1, - )), + include_copy=( + current_service.all_templates or + 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_service_settings.py b/tests/app/main/views/test_service_settings.py index 611ab24e5..5ebb9cb93 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -2188,12 +2188,12 @@ def test_set_postage_saves( @pytest.mark.parametrize('current_branding, expected_values, expected_labels', [ (None, [ - 'None', '1', '2', '3', '4', '5', + '__NONE__', '1', '2', '3', '4', '5', ], [ 'GOV.UK', 'org 1', 'org 2', 'org 3', 'org 4', 'org 5' ]), ('5', [ - '5', 'None', '1', '2', '3', '4', + '5', '__NONE__', '1', '2', '3', '4', ], [ 'org 5', 'GOV.UK', 'org 1', 'org 2', 'org 3', 'org 4', ]), @@ -2314,7 +2314,8 @@ def test_should_preview_email_branding( @pytest.mark.parametrize('posted_value, submitted_value', ( ('1', '1'), - ('None', None), + ('__NONE__', None), + pytest.param('None', None, marks=pytest.mark.xfail(raises=AssertionError)), )) def test_should_set_branding_and_organisations( logged_in_platform_admin_client, diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 2943a6a6c..6f0ff26d2 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', } @@ -900,7 +901,7 @@ def test_should_be_able_to_move_a_sub_item( template_folder_id=PARENT_FOLDER_ID, _data={ 'operation': 'move_to_existing_folder', - 'move_to': 'None', + 'move_to': '__NONE__', 'templates_and_folders': [GRANDCHILD_FOLDER_ID], }, _expected_status=302, @@ -928,13 +929,36 @@ def test_should_be_able_to_move_a_sub_item( 'move_to_new_folder_name': 'foo', 'move_to': PARENT_FOLDER_ID }, + # move to existing, but no templates to move + { + 'operation': 'move_to_existing_folder', + 'templates_and_folders': [], + 'move_to_new_folder_name': '', + 'move_to': PARENT_FOLDER_ID + }, # move to new, but nothing selected to move { 'operation': 'move_to_new_folder', 'templates_and_folders': [], 'move_to_new_folder_name': 'foo', 'move_to': None - } + }, + # add a new template, but also select move destination + { + 'operation': 'add_template', + 'templates_and_folders': [], + 'move_to_new_folder_name': '', + 'move_to': PARENT_FOLDER_ID, + 'add_template_by_template_type': 'email', + }, + # add a new template, but also move to root folder + { + 'operation': 'add_template', + 'templates_and_folders': [], + 'move_to_new_folder_name': '', + 'move_to': '__NONE__', + 'add_template_by_template_type': 'email', + }, ]) def test_no_action_if_user_fills_in_ambiguous_fields( client_request, @@ -945,9 +969,14 @@ def test_no_action_if_user_fills_in_ambiguous_fields( mock_create_template_folder, data, ): - service_one['permissions'] += ['edit_folders'] + service_one['permissions'] += ['edit_folders', 'letter'] - client_request.post( + mock_get_template_folders.return_value = [ + {'id': PARENT_FOLDER_ID, 'name': 'parent folder', 'parent_id': None}, + {'id': FOLDER_TWO_ID, 'name': 'folder_two', 'parent_id': None}, + ] + + page = client_request.post( 'main.choose_template', service_id=SERVICE_ONE_ID, _data=data, @@ -958,6 +987,26 @@ def test_no_action_if_user_fills_in_ambiguous_fields( assert mock_move_to_template_folder.called is False assert mock_create_template_folder.called is False + assert page.select_one('button[value={}]'.format(data['operation'])) + + assert [ + 'email', + 'sms', + 'letter', + 'copy-existing', + ] == [ + radio['value'] + for radio in page.select('#add_new_template_form input[type=radio]') + ] + + assert [ + FOLDER_TWO_ID, + PARENT_FOLDER_ID, + ] == [ + radio['value'] + for radio in page.select('#move_to_folder_radios input[type=radio]') + ] + def test_new_folder_is_created_if_only_new_folder_is_filled_out( client_request, 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 9309b9710..927d2e614 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')