diff --git a/app/main/forms.py b/app/main/forms.py index ed249e9eb..29e3813ae 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1135,6 +1135,18 @@ class TemplateFolderForm(StripWhitespaceForm): name = StringField('Folder name', validators=[DataRequired(message='Can’t be empty')]) +def required_for_ops(*operations): + operations = set(operations) + + def validate(form, field): + if form.op not in operations and field.data: + # super weird + raise ValidationError('Must be empty') + if form.op in operations and not field.data: + raise ValidationError('Can’t be empty') + return validate + + class TemplateAndFoldersSelectionForm(Form): ALL_TEMPLATES_FOLDER = { @@ -1155,6 +1167,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.move_to.choices = [ @@ -1164,24 +1177,29 @@ class TemplateAndFoldersSelectionForm(Form): ] def validate(self): - op = request.form.get('operation') + self.op = request.form.get('operation') - self.is_move_op = op in {'move_to_existing_folder', 'move_to_new_folder'} - self.is_add_op = op in {'add_new_folder', 'move_to_new_folder'} + 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'} if not (self.is_add_op or self.is_move_op): return False return super().validate() - def validate_move_to(self, field): - if self.is_move_op and not field.data: - raise ValidationError('Can’t be empty') + def get_folder_name(self): + if self.op == 'add_new_folder': + return self.add_new_folder_name.data + elif self.op == 'move_to_new_folder': + return self.move_to_new_folder_name.data + return None - def validate_new_folder_name(self, field): - if self.is_add_op and not field.data: - raise ValidationError('Can’t be empty') - - templates_and_folders = MultiCheckboxField('Choose templates or folders') - move_to = RadioFieldWithNoneOption('Choose a folder', validators=[Optional()]) - new_folder_name = StringField('Folder name') + templates_and_folders = MultiCheckboxField('Choose templates or folders', validators=[ + required_for_ops('move_to_new_folder', 'move_to_existing_folder') + ]) + move_to = RadioFieldWithNoneOption('Choose a folder', validators=[ + Optional(), + required_for_ops('move_to_new_folder', 'move_to_existing_folder') + ]) + 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')]) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 02db81402..4a2854a9a 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -118,7 +118,9 @@ def choose_template(service_id, template_type='all', template_folder_id=None): current_folder_id=template_folder_id, ) - if request.method == 'POST' and can_manage_folders() and templates_and_folders_form.validate_on_submit(): + if request.method == 'POST' and templates_and_folders_form.validate_on_submit(): + if not can_manage_folders(): + abort(403) return process_folder_management_form(templates_and_folders_form, template_folder_id) return render_template( @@ -145,7 +147,7 @@ def process_folder_management_form(form, current_folder_id): if form.is_add_op: new_folder_id = template_folder_api_client.create_template_folder( current_service.id, - name=form.new_folder_name.data, + name=form.get_folder_name(), parent_id=current_folder_id ) diff --git a/app/templates/views/templates/_move_to.html b/app/templates/views/templates/_move_to.html index f1789dd64..871e26364 100644 --- a/app/templates/views/templates/_move_to.html +++ b/app/templates/views/templates/_move_to.html @@ -3,9 +3,17 @@ {% 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') }} - {{ textbox(templates_and_folders_form.new_folder_name) }} - {{ page_footer('Move to a new folder', button_name='operation', button_value='move_to_new_folder') }} - {{ page_footer('New folder', button_name='operation', button_value='add_new_folder') }} +
+ {{ radios(templates_and_folders_form.move_to) }} + {{ page_footer('Move', button_name='operation', button_value='move_to_existing_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') }} +
+
+ {{ textbox(templates_and_folders_form.add_new_folder_name) }} + {{ page_footer('New folder', button_name='operation', button_value='add_new_folder') }} +
+ {% endif %} diff --git a/pytest.ini b/pytest.ini index a9b6c3bc3..2b0a34ba8 100644 --- a/pytest.ini +++ b/pytest.ini @@ -10,3 +10,6 @@ env = ZENDESK_API_KEY=test STATSD_PREFIX=stats-prefix REDIS_ENABLED=0 +# :thinking_face: +filterwarnings = + ignore::pytest.RemovedInPytest4Warning diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index aca605794..2943a6a6c 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -3,7 +3,6 @@ import uuid import pytest from flask import url_for -from tests import sample_uuid from tests.conftest import ( SERVICE_ONE_ID, TEMPLATE_ONE_ID, @@ -727,13 +726,12 @@ def test_should_show_checkboxes_for_selecting_templates( assert TEMPLATE_ONE_ID not in checkboxes[index]['id'] -@pytest.mark.parametrize('user,extra_service_permissions,should_show_radio_buttons', [ - (active_user_with_permissions, ['edit_folders'], True), - (active_user_with_permissions, [], False), - (active_user_view_permissions, ['edit_folders'], False), - (active_caseworking_user, ['edit_folders'], False), +@pytest.mark.parametrize('user,extra_service_permissions', [ + (active_user_with_permissions, []), + (active_user_view_permissions, ['edit_folders']), + (active_caseworking_user, ['edit_folders']), ]) -def test_should_show_radios_and_buttons_for_move_destination_if_correct_permissions( +def test_should_not_show_radios_and_buttons_for_move_destination_if_incorrect_permissions( client_request, mocker, service_one, @@ -743,12 +741,38 @@ def test_should_show_radios_and_buttons_for_move_destination_if_correct_permissi fake_uuid, user, extra_service_permissions, - should_show_radio_buttons, ): service_one['permissions'] += extra_service_permissions client_request.login(user(fake_uuid)) + page = client_request.get( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + ) + radios = page.select('input[type=radio]') + radio_div = page.find('div', {'id': 'move_to_folder_radios'}) + assert radios == page.select('input[name=move_to]') + + assert not radios + assert not radio_div + assert page.find_all('button', {'name': 'operation'}) == [] + + +def test_should_show_radios_and_buttons_for_move_destination_if_correct_permissions( + client_request, + mocker, + service_one, + mock_get_service_templates, + mock_get_template_folders, + mock_has_no_jobs, + fake_uuid, + active_user_with_permissions +): + service_one['permissions'] += ['edit_folders'] + + client_request.login(active_user_with_permissions) + FOLDER_TWO_ID = str(uuid.uuid4()) FOLDER_ONE_TWO_ID = str(uuid.uuid4()) mock_get_template_folders.return_value = [ @@ -762,26 +786,21 @@ def test_should_show_radios_and_buttons_for_move_destination_if_correct_permissi service_id=SERVICE_ONE_ID, ) radios = page.select('input[type=radio]') - labels = page.select('label[for^=move_to]') + radio_div = page.find('div', {'id': 'move_to_folder_radios'}) assert radios == page.select('input[name=move_to]') - if should_show_radio_buttons: - assert [x['value'] for x in radios] == [ - PARENT_FOLDER_ID, CHILD_FOLDER_ID, FOLDER_ONE_TWO_ID, FOLDER_TWO_ID, - ] - assert [x.text.strip() for x in labels] == [ - 'folder_one', 'folder_one_one', 'folder_one_two', 'folder_two', - ] - assert set(x['value'] for x in page.find_all('button', {'name': 'operation'})) == { - 'unknown', - 'move_to_existing_folder', - 'move_to_new_folder', - 'add_new_folder' - } - else: - assert not radios - assert not labels - assert page.find_all('button', {'name': 'operation'}) == [] + assert [x['value'] for x in radios] == [ + PARENT_FOLDER_ID, CHILD_FOLDER_ID, FOLDER_ONE_TWO_ID, FOLDER_TWO_ID, + ] + assert [x.text.strip() for x in radio_div.select('label')] == [ + 'folder_one', 'folder_one_one', 'folder_one_two', 'folder_two', + ] + assert set(x['value'] for x in page.find_all('button', {'name': 'operation'})) == { + 'unknown', + 'move_to_existing_folder', + 'move_to_new_folder', + 'add_new_folder' + } def test_should_be_able_to_move_to_existing_folder( @@ -855,8 +874,7 @@ def test_should_not_be_able_to_move_to_existing_folder_if_dont_have_permission( TEMPLATE_ONE_ID, ], }, - # it's just returned the form. Should this have a 400 because there were errors? - _expected_status=200 + _expected_status=403 ) assert mock_move_to_template_folder.called is False @@ -881,7 +899,7 @@ def test_should_be_able_to_move_a_sub_item( service_id=SERVICE_ONE_ID, template_folder_id=PARENT_FOLDER_ID, _data={ - 'operation': 'move', + 'operation': 'move_to_existing_folder', 'move_to': 'None', 'templates_and_folders': [GRANDCHILD_FOLDER_ID], }, @@ -895,36 +913,129 @@ def test_should_be_able_to_move_a_sub_item( ) -@pytest.mark.parametrize('thing_to_move', [ - PARENT_FOLDER_ID, # Can’t move a folder inside itself - CHILD_FOLDER_ID, # Can’t move a folder which doesn’t belong to the service +@pytest.mark.parametrize('data', [ + # move to existing, but add new folder name given + { + 'operation': 'move_to_existing_folder', + 'templates_and_folders': [], + 'add_new_folder_name': 'foo', + 'move_to': PARENT_FOLDER_ID + }, + # move to existing, but move to new folder name given + { + 'operation': 'move_to_existing_folder', + 'templates_and_folders': [TEMPLATE_ONE_ID], + 'move_to_new_folder_name': 'foo', + '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 + } ]) -def test_should_validate_illegal_moves( +def test_no_action_if_user_fills_in_ambiguous_fields( client_request, service_one, mock_get_service_templates, mock_get_template_folders, mock_move_to_template_folder, - thing_to_move, + mock_create_template_folder, + data, ): - service_one['permissions'] += 'edit_folders' + service_one['permissions'] += ['edit_folders'] - FOLDER_TWO_ID = str(uuid.uuid4()) - mock_get_template_folders.return_value = [ - {'id': PARENT_FOLDER_ID, 'name': 'folder_one', 'parent_id': None}, - {'id': FOLDER_TWO_ID, 'name': 'folder_two', 'parent_id': None}, - ] client_request.post( 'main.choose_template', service_id=SERVICE_ONE_ID, - _data={ - 'operation': 'move', - 'move_to': PARENT_FOLDER_ID, - 'templates_and_folders': [ - thing_to_move, - ], - }, + _data=data, _expected_status=200, _expected_redirect=None, ) + assert mock_move_to_template_folder.called is False + assert mock_create_template_folder.called is False + + +def test_new_folder_is_created_if_only_new_folder_is_filled_out( + client_request, + service_one, + mock_get_service_templates, + mock_get_template_folders, + mock_move_to_template_folder, + mock_create_template_folder +): + data = { + 'move_to_new_folder_name': '', + 'add_new_folder_name': 'new folder', + 'operation': 'add_new_folder' + } + + service_one['permissions'] += ['edit_folders'] + + client_request.post( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + _data=data, + _expected_status=302, + _expected_redirect=url_for( + 'main.choose_template', + service_id=service_one['id'], + template_folder_id=None, + _external=True, + ), + ) + + assert mock_move_to_template_folder.called is False + mock_create_template_folder.assert_called_once_with( + SERVICE_ONE_ID, + name='new folder', + parent_id=None + ) + + +def test_should_be_able_to_move_to_new_folder( + client_request, + service_one, + mock_get_service_templates, + mock_get_template_folders, + mock_move_to_template_folder, + mock_create_template_folder, +): + service_one['permissions'] += ['edit_folders'] + new_folder_id = mock_create_template_folder.return_value + FOLDER_TWO_ID = str(uuid.uuid4()) + 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}, + ] + + client_request.post( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + template_folder_id=None, + _data={ + 'operation': 'move_to_new_folder', + 'move_to_new_folder_name': 'new folder', + 'templates_and_folders': [ + FOLDER_TWO_ID, + TEMPLATE_ONE_ID, + ], + }, + _expected_status=302, + _expected_redirect=url_for('main.choose_template', service_id=SERVICE_ONE_ID, _external=True), + ) + + mock_create_template_folder.assert_called_once_with( + SERVICE_ONE_ID, + name='new folder', + parent_id=None + ) + mock_move_to_template_folder.assert_called_once_with( + service_id=SERVICE_ONE_ID, + folder_id=new_folder_id, + folder_ids={FOLDER_TWO_ID}, + template_ids={TEMPLATE_ONE_ID}, + ) diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index b47c989ee..be1a2c5e2 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -16,11 +16,7 @@ from tests import ( template_json, validate_route_permission, ) -from tests.app.main.views.test_template_folders import ( - CHILD_FOLDER_ID, - PARENT_FOLDER_ID, - _folder, -) +from tests.app.main.views.test_template_folders import PARENT_FOLDER_ID, _folder from tests.conftest import ( SERVICE_ONE_ID, SERVICE_TWO_ID, diff --git a/tests/conftest.py b/tests/conftest.py index 5c64914fc..0d964d14b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3255,3 +3255,8 @@ def mock_get_template_folders(mocker): @pytest.fixture def mock_move_to_template_folder(mocker): return mocker.patch('app.template_folder_api_client.move_to_folder') + + +@pytest.fixture +def mock_create_template_folder(mocker): + return mocker.patch('app.template_folder_api_client.create_template_folder', return_value=sample_uuid())