From 13a962b6bd23948214ede93de2768d38538bb064 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Nov 2018 10:37:33 +0000 Subject: [PATCH 1/3] =?UTF-8?q?Don=E2=80=99t=20hide=20the=20rename=20form?= =?UTF-8?q?=20on=20delete?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s weird that this page changes when you click delete – it looks like you’re going to a different page. It should feel like you’re on the same page, just with the confirmation message. The problem we had before is that the rename form would `POST` to `…/delete`, which would then delete the template instead of updating its name. We can fix this by explicitly setting the `action` attribute on the rename form to always post to `…/manage`, even if the user is currently looking at the `…/delete` page. --- app/main/views/templates.py | 5 ++-- .../templates/manage-template-folder.html | 26 ++++++++----------- tests/app/main/views/test_template_folders.py | 19 +++++++++++--- 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 70a854143..3dc26d475 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -406,7 +406,9 @@ def manage_template_folder(service_id, template_folder_id): def delete_template_folder(service_id, template_folder_id): if not current_service.has_permission('edit_folders'): abort(403) - form = TemplateFolderForm() + form = TemplateFolderForm( + name=current_service.get_template_folder(template_folder_id)['name'] + ) template_folder_path = current_service.get_template_folder_path(template_folder_id) template_folder_name = template_folder_path[-1]["name"] @@ -450,7 +452,6 @@ def delete_template_folder(service_id, template_folder_id): current_service_id=current_service.id, template_folder_id=template_folder_id, template_type="all", - delete_folder=True ) diff --git a/app/templates/views/templates/manage-template-folder.html b/app/templates/views/templates/manage-template-folder.html index b638e8479..2b14dee9f 100644 --- a/app/templates/views/templates/manage-template-folder.html +++ b/app/templates/views/templates/manage-template-folder.html @@ -21,20 +21,16 @@ - {% if not delete_folder %} - {% call form_wrapper() %} - {{ textbox(form.name) }} - {{ page_footer( - 'Save', - delete_link=url_for( - '.delete_template_folder', - service_id=current_service_id, - template_folder_id=template_folder_id - ), - delete_link_text="Delete this folder") }} - {% endcall %} - {% else %} - Back to manage folder page - {% endif %} + {% call form_wrapper(action=url_for('main.manage_template_folder', service_id=current_service.id, template_folder_id=template_folder_id)) %} + {{ textbox(form.name) }} + {{ page_footer( + 'Save', + delete_link=url_for( + '.delete_template_folder', + service_id=current_service_id, + template_folder_id=template_folder_id + ), + delete_link_text="Delete this folder") }} + {% endcall %} {% endblock %} diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 506c80ef3..da3a97b60 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -367,7 +367,7 @@ def test_get_manage_folder_page( assert normalize_spaces(page.select_one('title').text) == ( 'folder_two – Templates – service one – GOV.UK Notify' ) - assert page.select_one('input[name=name]') is not None + assert page.select_one('input[name=name]')['value'] == 'folder_two' delete_link = page.find('a', string="Delete this folder") expected_delete_url = "/services/{}/templates/folders/{}/delete".format(service_one['id'], folder_id) @@ -443,9 +443,20 @@ def test_delete_template_folder_should_request_confirmation( 'Yes, delete' ) - assert len(page.select('label')) == 0 - assert len(page.select('button')) == 1 - assert "Back to manage folder page" in page.text + assert page.select_one('input[name=name]')['value'] == 'sacrifice' + + assert len(page.select('form')) == 2 + assert len(page.select('button')) == 2 + + assert 'action' not in page.select('form')[0] + assert page.select('form button')[0].text == 'Yes, delete' + + assert page.select('form')[1]['action'] == url_for( + 'main.manage_template_folder', + service_id=service_one['id'], + template_folder_id=folder_id, + ) + assert page.select('form button')[1].text == 'Save' def test_delete_template_folder_should_detect_non_empty_folder_on_get( From 4bccce4772bafcae1aa385b385a022951fb3be8d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Nov 2018 10:43:56 +0000 Subject: [PATCH 2/3] Refactor how we get template folder on delete page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s a bit obtuse to look it up from the `template_folder_path`, especially now that we have a specific method on the model that does the exact thing. --- app/main/views/templates.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 3dc26d475..2f8827b57 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -404,18 +404,18 @@ def manage_template_folder(service_id, template_folder_id): @login_required @user_has_permissions('manage_templates') def delete_template_folder(service_id, template_folder_id): + if not current_service.has_permission('edit_folders'): abort(403) - form = TemplateFolderForm( - name=current_service.get_template_folder(template_folder_id)['name'] - ) - template_folder_path = current_service.get_template_folder_path(template_folder_id) - template_folder_name = template_folder_path[-1]["name"] + + template_folder = current_service.get_template_folder(template_folder_id) + + form = TemplateFolderForm(name=template_folder['name']) if len(current_service.get_template_folders_and_templates( template_type="all", template_folder_id=template_folder_id )) > 0: - flash("You must empty this folder before you can delete it".format(template_folder_name), 'info') + flash("You must empty this folder before you can delete it".format(template_folder['name']), 'info') return redirect( url_for( '.choose_template', service_id=service_id, template_type="all", template_folder_id=template_folder_id @@ -432,7 +432,7 @@ def delete_template_folder(service_id, template_folder_id): except HTTPError as e: msg = "Folder is not empty" if e.status_code == 400 and msg in e.message: - flash("You must empty this folder before you can delete it".format(template_folder_name), 'info') + flash("You must empty this folder before you can delete it", 'info') return redirect( url_for( '.choose_template', @@ -444,11 +444,11 @@ def delete_template_folder(service_id, template_folder_id): else: abort(500, e) - flash("Are you sure you want to delete the ‘{}’ folder?".format(template_folder_name), 'delete') + flash("Are you sure you want to delete the ‘{}’ folder?".format(template_folder['name']), 'delete') return render_template( 'views/templates/manage-template-folder.html', form=form, - template_folder_path=template_folder_path, + template_folder_path=current_service.get_template_folder_path(template_folder_id), current_service_id=current_service.id, template_folder_id=template_folder_id, template_type="all", From 77529f419122f7a89ddab378d0320191d1b1b731 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Nov 2018 10:44:09 +0000 Subject: [PATCH 3/3] Redirect to parent after deleting a folder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit You lose your place if, after deleting something that’s deeply nested, you return to the root level. --- app/main/views/templates.py | 2 +- tests/app/main/views/test_template_folders.py | 17 ++++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 2f8827b57..2ae0ba6ae 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -427,7 +427,7 @@ def delete_template_folder(service_id, template_folder_id): template_folder_api_client.delete_template_folder(current_service.id, template_folder_id) return redirect( - url_for('.choose_template', service_id=service_id) + url_for('.choose_template', service_id=service_id, template_folder_id=template_folder['parent_id']) ) except HTTPError as e: msg = "Folder is not empty" diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index da3a97b60..e0cd223c4 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -487,11 +487,15 @@ def test_delete_template_folder_should_detect_non_empty_folder_on_get( ) -def test_delete_folder(client_request, service_one, mock_get_template_folders, mocker): +@pytest.mark.parametrize('parent_folder_id', ( + None, + PARENT_FOLDER_ID, +)) +def test_delete_folder(client_request, service_one, mock_get_template_folders, mocker, parent_folder_id): mock_delete = mocker.patch('app.template_folder_api_client.delete_template_folder') folder_id = str(uuid.uuid4()) mock_get_template_folders.side_effect = [[ - {'id': folder_id, 'name': 'sacrifice', 'parent_id': None}, + {'id': folder_id, 'name': 'sacrifice', 'parent_id': parent_folder_id}, ], []] mocker.patch( 'app.models.service.Service.get_templates', @@ -503,9 +507,12 @@ def test_delete_folder(client_request, service_one, mock_get_template_folders, m 'main.delete_template_folder', service_id=service_one['id'], template_folder_id=folder_id, - _expected_redirect=url_for("main.choose_template", - service_id=service_one['id'], - _external=True) + _expected_redirect=url_for( + "main.choose_template", + service_id=service_one['id'], + template_folder_id=parent_folder_id, + _external=True, + ) ) mock_delete.assert_called_once_with(service_one['id'], folder_id)