Post to the API when moving folders and templates

This commit adds logic to: - take the list of selected folders and templates - split it into two lists (of folders and templates) - `POST` that data to the API, to effect the movement of said folders and templates I’ve tried to architect it in such a way that we can easily add more template ‘operations’ in the future, as we add more forms to the choose template page.
2026-08-04 13:41:38 -04:00 · 2018-11-08 14:46:18 +00:00
parent 980d66bdaa
commit cdb5b47c4d
10 changed files with 322 additions and 25 deletions
--- 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')
--- 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/<service_id>/templates")
-@main.route("/services/<service_id>/templates/folders/<template_folder_id>")
-@main.route("/services/<service_id>/templates/<template_type>")
-@main.route("/services/<service_id>/templates/<template_type>/folders/<template_folder_id>")
+@main.route("/services/<service_id>/templates", methods=['GET', 'POST'])
+@main.route("/services/<service_id>/templates/folders/<template_folder_id>", methods=['GET', 'POST'])
+@main.route("/services/<service_id>/templates/<template_type>", methods=['GET', 'POST'])
+@main.route("/services/<service_id>/templates/<template_type>/folders/<template_folder_id>", 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/<service_id>/templates/<template_id>.<filetype>")
@login_required
@user_has_permissions()
--- 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,
+        )
--- 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()
--- a/app/templates/components/page-footer.html
+++ b/app/templates/components/page-footer.html
@@ -16,6 +16,8 @@
      <button
        type="submit"
        class="button{% if destructive %}-destructive{% endif %}"
+        {% if button_name %}name="{{ button_name }}"{% endif %}
+        {% if button_value %}value="{{ button_value }}"{% endif %}
      >
        {{- button_text -}}
      </button>
--- 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
--- 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
--- 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,
--- 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'],
+        },
+    )
--- 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')