diff --git a/app/main/forms.py b/app/main/forms.py index 2e3e6a05e..b50d1aadf 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -702,7 +702,7 @@ class ServicePostageForm(StripWhitespaceForm): ) -class BrandingStyle(RadioField): +class RadioFieldWithNoneOption(RadioField): def post_validate(self, form, validation_stopped): if self.data == 'None': @@ -711,7 +711,7 @@ class BrandingStyle(RadioField): class ServiceSetBranding(StripWhitespaceForm): - branding_style = BrandingStyle( + branding_style = RadioFieldWithNoneOption( 'Branding style', validators=[ DataRequired() @@ -1124,7 +1124,7 @@ class TemplateAndFoldersSelectionForm(Form): ALL_TEMPLATES_FOLDER = { 'name': 'All templates', - 'id': None, + 'id': 'None', } def __init__( @@ -1151,8 +1151,8 @@ class TemplateAndFoldersSelectionForm(Form): def ids_and_names(items, exclude=None): return [ (item['id'], item['name']) for item in items - if item['id'] != exclude + if item['id'] != str(exclude) ] templates_and_folders = MultiCheckboxField('Choose templates or folders') - move_to = RadioField('Choose a folder') + move_to = RadioFieldWithNoneOption('Choose a folder') diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 4062f8c65..d12da3223 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -100,10 +100,10 @@ def start_tour(service_id, template_id): ) -@main.route("/services//templates") -@main.route("/services//templates/folders/") -@main.route("/services//templates/") -@main.route("/services//templates//folders/") +@main.route("/services//templates", methods=['GET', 'POST']) +@main.route("/services//templates/folders/", methods=['GET', 'POST']) +@main.route("/services//templates/", methods=['GET', 'POST']) +@main.route("/services//templates//folders/", methods=['GET', 'POST']) @login_required @user_has_permissions() def choose_template(service_id, template_type='all', template_folder_id=None): @@ -114,13 +114,17 @@ def choose_template(service_id, template_type='all', template_folder_id=None): current_folder_id=template_folder_id, ) + if template_operation('move', templates_and_folders_form): + current_service.move_to_folder( + ids_to_move=templates_and_folders_form.templates_and_folders.data, + move_to=templates_and_folders_form.move_to.data, + ) + return redirect(request.url) + return render_template( 'views/templates/choose.html', current_template_folder_id=template_folder_id, - can_manage_folders=( - current_service.has_permission('edit_folders') and - current_user.has_permissions('manage_templates') - ), + can_manage_folders=can_manage_folders(), template_folder_path=current_service.get_template_folder_path(template_folder_id), template_folders=current_service.get_template_folders(template_folder_id), templates=current_service.get_templates(template_type, template_folder_id), @@ -136,6 +140,17 @@ def choose_template(service_id, template_type='all', template_folder_id=None): ) +def template_operation(operation_name, form): + + if ( + can_manage_folders() and + request.method == 'POST' and + request.form.get('operation') == operation_name and + form.validate_on_submit() + ): + return True + + def get_template_nav_items(template_folder_id): return [ ( @@ -151,6 +166,13 @@ def get_template_nav_items(template_folder_id): ] +def can_manage_folders(): + return ( + current_service.has_permission('edit_folders') and + current_user.has_permissions('manage_templates') + ) + + @main.route("/services//templates/.") @login_required @user_has_permissions() diff --git a/app/models/service.py b/app/models/service.py index 928ffbf1f..a0fe5ba5f 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -331,3 +331,14 @@ class Service(): self.get_templates(template_type, template_folder_id) + self.get_template_folders(template_folder_id) ) + + def move_to_folder(self, ids_to_move, move_to): + + ids_to_move = set(ids_to_move) + + template_folder_api_client.move_to_folder( + service_id=self.id, + folder_id=move_to, + template_ids=ids_to_move & self.all_template_ids, + folder_ids=ids_to_move & self.all_template_folder_ids, + ) diff --git a/app/notify_client/template_folder_api_client.py b/app/notify_client/template_folder_api_client.py index c04a7bada..2b65e2210 100644 --- a/app/notify_client/template_folder_api_client.py +++ b/app/notify_client/template_folder_api_client.py @@ -24,5 +24,24 @@ class TemplateFolderAPIClient(NotifyAdminAPIClient): def get_template_folders(self, service_id): return self.get('/service/{}/template-folder'.format(service_id))['template_folders'] + @cache.delete('service-{service_id}-template-folders') + @cache.delete('service-{service_id}-templates') + def move_to_folder(self, service_id, folder_id, template_ids, folder_ids): + + if folder_id: + url = '/service/{}/template-folder/move-to-folder/{}'.format(service_id, folder_id) + else: + url = '/service/{}/template-folder/move-to-folder'.format(service_id) + + self.post(url, { + 'templates': list(template_ids), + 'folders': list(folder_ids), + }) + + self.redis_client.delete(*map( + 'template-{}-version-None'.format, + template_ids, + )) + template_folder_api_client = TemplateFolderAPIClient() diff --git a/app/templates/components/page-footer.html b/app/templates/components/page-footer.html index 777d8efff..31c1e94e3 100644 --- a/app/templates/components/page-footer.html +++ b/app/templates/components/page-footer.html @@ -16,6 +16,8 @@ diff --git a/requirements-app.txt b/requirements-app.txt index 4bdc06e7f..9b736c2de 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -27,4 +27,4 @@ botocore<1.11.0 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default itsdangerous==0.24 # pyup: <1.0.0 -git+https://github.com/alphagov/notifications-utils.git@30.5.6#egg=notifications-utils==30.5.6 +git+https://github.com/alphagov/notifications-utils.git@30.6.0#egg=notifications-utils==30.6.0 diff --git a/requirements.txt b/requirements.txt index 06cc36bca..869d4ff86 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,7 +29,7 @@ botocore<1.11.0 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default itsdangerous==0.24 # pyup: <1.0.0 -git+https://github.com/alphagov/notifications-utils.git@30.5.6#egg=notifications-utils==30.5.6 +git+https://github.com/alphagov/notifications-utils.git@30.6.0#egg=notifications-utils==30.6.0 ## The following requirements were added by pip freeze: bleach==2.1.3 @@ -43,16 +43,16 @@ docopt==0.6.2 docutils==0.14 et-xmlfile==1.0.1 Flask-Redis==0.3.0 -future==0.17.0 +future==0.17.1 greenlet==0.4.15 html5lib==1.0.1 idna==2.7 jdcal==1.4 Jinja2==2.10 jmespath==0.9.3 -lml==0.0.4 +lml==0.0.6 lxml==4.2.5 -MarkupSafe==1.0 +MarkupSafe==1.1.0 mistune==0.8.3 monotonic==1.5 openpyxl==2.5.9 @@ -66,14 +66,14 @@ python-dateutil==2.7.5 python-json-logger==0.1.8 PyYAML==3.13 redis==2.10.6 -requests==2.20.0 +requests==2.20.1 rsa==3.4.2 s3transfer==0.1.13 six==1.11.0 smartypants==2.0.1 statsd==3.2.2 -texttable==1.4.0 -urllib3==1.24 +texttable==1.5.0 +urllib3==1.24.1 webencodings==0.5.1 Werkzeug==0.14.1 WTForms==2.2.1 diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index b8869bf24..f82512854 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -1,3 +1,4 @@ +import uuid from datetime import datetime from unittest.mock import ANY, Mock @@ -16,6 +17,10 @@ from tests import ( template_json, validate_route_permission, ) +from tests.app.main.views.test_template_folders import ( + CHILD_FOLDER_ID, + PARENT_FOLDER_ID, +) from tests.conftest import ( SERVICE_ONE_ID, SERVICE_TWO_ID, @@ -23,6 +28,7 @@ from tests.conftest import ( active_caseworking_user, active_user_view_permissions, active_user_with_permissions, + fake_uuid, mock_get_service_email_template, mock_get_service_letter_template, mock_get_service_template, @@ -108,12 +114,12 @@ def test_should_show_page_for_choosing_a_template( user, expected_page_title, ): - mocker.patch('app.user_api_client.get_user', return_value=user(fake_uuid)) service_one['permissions'].append('letter') + client_request.login(user(fake_uuid)) page = client_request.get( 'main.choose_template', - service_id=SERVICE_ONE_ID, + service_id=service_one['id'], **extra_args ) @@ -169,8 +175,8 @@ def test_should_show_checkboxes_for_selecting_templates( user, extra_service_permissions, ): - mocker.patch('app.user_api_client.get_user', return_value=user(fake_uuid)) - service_one['permissions'] = service_one['permissions'] + extra_service_permissions + service_one['permissions'] += extra_service_permissions + client_request.login(user(fake_uuid)) page = client_request.get( 'main.choose_template', @@ -188,6 +194,179 @@ def test_should_show_checkboxes_for_selecting_templates( assert TEMPLATE_ONE_ID not in checkboxes[index]['id'] +@pytest.mark.parametrize('user', [ + pytest.param( + active_user_with_permissions + ), + pytest.param( + active_user_view_permissions, + marks=pytest.mark.xfail(raises=AssertionError) + ), + pytest.param( + active_caseworking_user, + marks=pytest.mark.xfail(raises=AssertionError) + ), +]) +@pytest.mark.parametrize('extra_service_permissions', [ + pytest.param( + ['edit_folders'] + ), + pytest.param( + [], + marks=pytest.mark.xfail(raises=AssertionError) + ), +]) +@pytest.mark.parametrize('folder_id, expected_destinations', [ + (None, []), + (fake_uuid(), []), +]) +def test_should_show_radio_buttons_for_move_destination( + client_request, + mocker, + service_one, + mock_get_service_templates, + mock_get_template_folders, + mock_has_no_jobs, + fake_uuid, + user, + extra_service_permissions, + folder_id, + expected_destinations, +): + service_one['permissions'] += extra_service_permissions + client_request.login(user(fake_uuid)) + FOLDER_TWO_ID = str(uuid.uuid4()) + FOLDER_ONE_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}, + {'id': CHILD_FOLDER_ID, 'name': 'folder_one_one', 'parent_id': PARENT_FOLDER_ID}, + {'id': FOLDER_ONE_TWO_ID, 'name': 'folder_one_two', 'parent_id': PARENT_FOLDER_ID}, + ] + page = client_request.get( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + ) + radios = page.select('input[type=radio]') + labels = page.select('label[for^=move_to]') + assert radios == page.select('input[name=move_to]') + + assert [x['value'] for x in radios] == [ + PARENT_FOLDER_ID, FOLDER_TWO_ID, CHILD_FOLDER_ID, FOLDER_ONE_TWO_ID + ] + assert [x.text.strip() for x in labels] == [ + 'folder_one', 'folder_two', 'folder_one_one', 'folder_one_two' + ] + assert page.select_one('button[name=operation]')['value'] == 'move' + + +@pytest.mark.parametrize('user', [ + pytest.param( + active_user_with_permissions + ), + pytest.param( + active_user_view_permissions, + marks=pytest.mark.xfail(raises=AssertionError) + ), + pytest.param( + active_caseworking_user, + marks=pytest.mark.xfail(raises=AssertionError) + ), +]) +@pytest.mark.parametrize('extra_service_permissions', [ + pytest.param( + ['edit_folders'] + ), + pytest.param( + [], + marks=pytest.mark.xfail(raises=AssertionError) + ), +]) +@pytest.mark.parametrize('folder_id, expected_destinations', [ + (None, []), + (fake_uuid(), []), +]) +def test_should_post_move_to_api( + client_request, + service_one, + fake_uuid, + mock_get_service_templates, + mock_get_template_folders, + mock_move_to_template_folder, + user, + extra_service_permissions, + folder_id, + expected_destinations, +): + service_one['permissions'] += extra_service_permissions + client_request.login(user(fake_uuid)) + 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': [ + FOLDER_TWO_ID, + TEMPLATE_ONE_ID, + ], + }, + _expected_status=302, + _expected_redirect=url_for( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + _external=True, + ), + ) + mock_move_to_template_folder.assert_called_once_with( + service_id=SERVICE_ONE_ID, + folder_id=PARENT_FOLDER_ID, + folder_ids={FOLDER_TWO_ID}, + template_ids={TEMPLATE_ONE_ID}, + ) + + +@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 +]) +def test_should_validate_illegal_moves( + client_request, + service_one, + fake_uuid, + mock_get_service_templates, + mock_get_template_folders, + mock_move_to_template_folder, + thing_to_move, +): + 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, + ], + }, + _expected_status=200, + _expected_redirect=None, + ) + assert mock_move_to_template_folder.called is False + + def test_should_not_show_template_nav_if_only_one_type_of_template( client_request, mock_get_template_folders, diff --git a/tests/app/notify_client/test_template_folder_client.py b/tests/app/notify_client/test_template_folder_client.py index 58b7cd60e..14804754a 100644 --- a/tests/app/notify_client/test_template_folder_client.py +++ b/tests/app/notify_client/test_template_folder_client.py @@ -1,6 +1,8 @@ import uuid +from unittest.mock import call import pytest +from orderedset import OrderedSet from app.notify_client.template_folder_api_client import TemplateFolderAPIClient @@ -44,3 +46,60 @@ def test_get_template_folders_calls_correct_api_endpoint(mocker, api_user_active mock_redis_get.assert_called_once_with(redis_key) mock_api_get.assert_called_once_with(expected_url) mock_redis_set.assert_called_once_with(redis_key, '{"a": "b"}', ex=604800) + + +def test_move_templates_and_folders(mocker, api_user_active): + + mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_api_post = mocker.patch('app.notify_client.NotifyAdminAPIClient.post') + + some_service_id = uuid.uuid4() + some_folder_id = uuid.uuid4() + + TemplateFolderAPIClient().move_to_folder( + some_service_id, + some_folder_id, + template_ids=OrderedSet(('a', 'b', 'c')), + folder_ids=OrderedSet(('1', '2', '3')), + ) + + mock_api_post.assert_called_once_with( + '/service/{}/template-folder/move-to-folder/{}'.format( + some_service_id, some_folder_id + ), + { + 'folders': ['1', '2', '3'], + 'templates': ['a', 'b', 'c'], + }, + ) + assert mock_redis_delete.call_args_list == [ + call('service-{}-template-folders'.format(some_service_id)), + call('service-{}-templates'.format(some_service_id)), + call( + 'template-a-version-None', + 'template-b-version-None', + 'template-c-version-None', + ), + ] + + +def test_move_templates_and_folders_to_root(mocker, api_user_active): + + mock_api_post = mocker.patch('app.notify_client.NotifyAdminAPIClient.post') + + some_service_id = uuid.uuid4() + + TemplateFolderAPIClient().move_to_folder( + some_service_id, + None, + template_ids=OrderedSet(('a', 'b', 'c')), + folder_ids=OrderedSet(('1', '2', '3')), + ) + + mock_api_post.assert_called_once_with( + '/service/{}/template-folder/move-to-folder'.format(some_service_id), + { + 'folders': ['1', '2', '3'], + 'templates': ['a', 'b', 'c'], + }, + ) diff --git a/tests/conftest.py b/tests/conftest.py index 8747c7822..0a451b65d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3242,3 +3242,8 @@ def url_for_endpoint_with_token(endpoint, token): @pytest.fixture def mock_get_template_folders(mocker): return mocker.patch('app.template_folder_api_client.get_template_folders', return_value=[]) + + +@pytest.fixture +def mock_move_to_template_folder(mocker): + return mocker.patch('app.template_folder_api_client.move_to_folder')