From 4520065af4ddcc3033799e7f7a1333425072f10e Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 13 Nov 2018 15:49:25 +0000 Subject: [PATCH 1/6] Add a delete folder link to manage folder page and basic delete endpoint --- app/main/views/templates.py | 9 +++++++++ .../views/templates/manage-template-folder.html | 10 +++++++++- tests/app/main/views/test_template_folders.py | 6 +++++- 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 5fd6e2e7f..2f6716ba3 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -390,9 +390,18 @@ def manage_template_folder(service_id, template_folder_id): 'views/templates/manage-template-folder.html', form=form, template_folder_path=current_service.get_template_folder_path(template_folder_id), + current_service_id=current_service.id, + template_folder_id=template_folder_id ) +@main.route("/services//templates/folders//delete", methods=['POST']) +def delete_template_folder(service_id, template_folder_id): + if not current_service.has_permission('edit_folders'): + abort(403) + pass + + @main.route("/services//templates/add-", methods=['GET', 'POST']) @main.route("/services//templates/folders//add-", methods=['GET', 'POST']) diff --git a/app/templates/views/templates/manage-template-folder.html b/app/templates/views/templates/manage-template-folder.html index 8d9c82bc9..dd6e7db03 100644 --- a/app/templates/views/templates/manage-template-folder.html +++ b/app/templates/views/templates/manage-template-folder.html @@ -25,7 +25,15 @@ Templates {% call form_wrapper() %} {{ textbox(form.name) }} - {{ page_footer('Save') }} + {{ 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 036aece4f..2574994b9 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -324,7 +324,11 @@ def test_get_manage_folder_page(client_request, service_one, mock_get_template_f service_id=service_one['id'], template_folder_id=folder_id ) - assert page.select_one('input[name=name]')['value'] == 'folder_two' + assert page.select_one('input[name=name]') is not None + delete_link = page.find('a', string="Delete this folder") + expected_delete_url = "/services/{}/templates/folders/{}/delete".format(service_one['id'], folder_id) + + assert expected_delete_url in delete_link["href"] def test_manage_folder_page_404s(client_request, service_one, mock_get_template_folders): From b5c78dc55293de467db245270109262cb973d703 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 13 Nov 2018 16:20:18 +0000 Subject: [PATCH 2/6] Add delete_folder method to template folder api client --- app/notify_client/template_folder_api_client.py | 4 ++++ .../test_template_folder_client.py | 17 +++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/app/notify_client/template_folder_api_client.py b/app/notify_client/template_folder_api_client.py index 0a8952e9e..d6fad053b 100644 --- a/app/notify_client/template_folder_api_client.py +++ b/app/notify_client/template_folder_api_client.py @@ -50,5 +50,9 @@ class TemplateFolderAPIClient(NotifyAdminAPIClient): {"name": name} ) + @cache.delete('service-{service_id}-template-folders') + def delete_template_folder(self, service_id, template_folder_id): + self.post('/service/{}/template-folder/{}/delete'.format(service_id, template_folder_id)) + template_folder_api_client = TemplateFolderAPIClient() diff --git a/tests/app/notify_client/test_template_folder_client.py b/tests/app/notify_client/test_template_folder_client.py index 7a6a121c5..27422ea86 100644 --- a/tests/app/notify_client/test_template_folder_client.py +++ b/tests/app/notify_client/test_template_folder_client.py @@ -121,3 +121,20 @@ def test_update_template_folder_calls_correct_api_endpoint(mocker, api_user_acti mock_post.assert_called_once_with(expected_url, data) mock_redis_delete.assert_called_once_with('service-{}-template-folders'.format(some_service_id)) + + +def test_delete_template_folder_calls_correct_api_endpoint(mocker, api_user_active): + mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + + some_service_id = uuid.uuid4() + template_folder_id = uuid.uuid4() + expected_url = '/service/{}/template-folder/{}/delete'.format(some_service_id, template_folder_id) + + client = TemplateFolderAPIClient() + + mock_post = mocker.patch('app.notify_client.template_folder_api_client.TemplateFolderAPIClient.post') + + client.delete_template_folder(some_service_id, template_folder_id) + + mock_post.assert_called_once_with(expected_url) + mock_redis_delete.assert_called_once_with('service-{}-template-folders'.format(some_service_id)) From 91d309816b3f5eaf6afb6c69bffded7c1bdb330d Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 13 Nov 2018 17:56:47 +0000 Subject: [PATCH 3/6] Build delete folder endpoint that calls template_folders_api_client --- app/main/views/templates.py | 16 +++++++++++++-- app/navigation.py | 4 ++++ .../template_folder_api_client.py | 2 +- tests/app/main/views/test_template_folders.py | 20 +++++++++++++++++++ .../test_template_folder_client.py | 6 +++--- 5 files changed, 42 insertions(+), 6 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 2f6716ba3..e120938d0 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -349,6 +349,8 @@ def action_blocked(service_id, notification_type, return_to, template_id): @main.route("/services//templates/add-folder", methods=['GET', 'POST']) @main.route("/services//templates/folders//add-folder", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_templates') def add_template_folder(service_id, template_folder_id=None): if not current_service.has_permission('edit_folders'): abort(403) @@ -370,6 +372,8 @@ def add_template_folder(service_id, template_folder_id=None): @main.route("/services//templates/folders//manage", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_templates') def manage_template_folder(service_id, template_folder_id): if not current_service.has_permission('edit_folders'): abort(403) @@ -395,11 +399,19 @@ def manage_template_folder(service_id, template_folder_id): ) -@main.route("/services//templates/folders//delete", methods=['POST']) +@main.route("/services//templates/folders//delete", methods=['GET', 'POST']) +@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) - pass + form = TemplateFolderForm() + + template_folder_api_client.delete_template_folder(current_service.id, template_folder_id) + + return redirect( + url_for('.choose_template', service_id=service_id) + ) @main.route("/services//templates/add-", methods=['GET', 'POST']) diff --git a/app/navigation.py b/app/navigation.py index 7bae1f3af..fdb13eef6 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -142,6 +142,7 @@ class HeaderNavigation(Navigation): 'create_api_key', 'data_retention', 'delete_service_template', + 'delete_template_folder', 'delivery_and_failure', 'delivery_status_callback', 'design_content', @@ -409,6 +410,7 @@ class MainNavigation(Navigation): 'cookies', 'create_email_branding', 'data_retention', + 'delete_template_folder', 'delivery_and_failure', 'design_content', 'documentation', @@ -590,6 +592,7 @@ class CaseworkNavigation(Navigation): 'create_email_branding', 'data_retention', 'delete_service_template', + 'delete_template_folder', 'delivery_and_failure', 'delivery_status_callback', 'design_content', @@ -827,6 +830,7 @@ class OrgNavigation(Navigation): 'create_email_branding', 'data_retention', 'delete_service_template', + 'delete_template_folder', 'delivery_and_failure', 'delivery_status_callback', 'design_content', diff --git a/app/notify_client/template_folder_api_client.py b/app/notify_client/template_folder_api_client.py index d6fad053b..8dae6ef61 100644 --- a/app/notify_client/template_folder_api_client.py +++ b/app/notify_client/template_folder_api_client.py @@ -52,7 +52,7 @@ class TemplateFolderAPIClient(NotifyAdminAPIClient): @cache.delete('service-{service_id}-template-folders') def delete_template_folder(self, service_id, template_folder_id): - self.post('/service/{}/template-folder/{}/delete'.format(service_id, template_folder_id)) + self.delete('/service/{}/template-folder/{}'.format(service_id, template_folder_id), {}) template_folder_api_client = TemplateFolderAPIClient() diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 2574994b9..ecfe7dae6 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -376,3 +376,23 @@ def test_rename_folder(client_request, service_one, mock_get_template_folders, m folder_id, name="new beautiful name" ) + + +def test_delete_folder(client_request, service_one, mock_get_template_folders, mocker): + mock_delete = mocker.patch('app.template_folder_api_client.delete_template_folder') + folder_id = str(uuid.uuid4()) + mock_get_template_folders.return_value = [ + {'id': folder_id, 'name': 'folder_two', 'parent_id': None}, + ] + service_one['permissions'] += ['edit_folders'] + + client_request.post( + '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) + ) + + mock_delete.assert_called_once_with(service_one['id'], folder_id) diff --git a/tests/app/notify_client/test_template_folder_client.py b/tests/app/notify_client/test_template_folder_client.py index 27422ea86..4057797ca 100644 --- a/tests/app/notify_client/test_template_folder_client.py +++ b/tests/app/notify_client/test_template_folder_client.py @@ -128,13 +128,13 @@ def test_delete_template_folder_calls_correct_api_endpoint(mocker, api_user_acti some_service_id = uuid.uuid4() template_folder_id = uuid.uuid4() - expected_url = '/service/{}/template-folder/{}/delete'.format(some_service_id, template_folder_id) + expected_url = '/service/{}/template-folder/{}'.format(some_service_id, template_folder_id) client = TemplateFolderAPIClient() - mock_post = mocker.patch('app.notify_client.template_folder_api_client.TemplateFolderAPIClient.post') + mock_delete = mocker.patch('app.notify_client.template_folder_api_client.TemplateFolderAPIClient.delete') client.delete_template_folder(some_service_id, template_folder_id) - mock_post.assert_called_once_with(expected_url) + mock_delete.assert_called_once_with(expected_url, {}) mock_redis_delete.assert_called_once_with('service-{}-template-folders'.format(some_service_id)) From d61022e3e6c9e43efbbebd91bb010b19d1a18f51 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 14 Nov 2018 12:16:57 +0000 Subject: [PATCH 4/6] Add flash messages and error handling to delete folder flow --- app/main/views/templates.py | 26 ++++++++++++++++--- tests/app/main/views/test_template_folders.py | 2 +- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index e120938d0..5bd013158 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -406,11 +406,31 @@ def delete_template_folder(service_id, template_folder_id): if not current_service.has_permission('edit_folders'): abort(403) form = TemplateFolderForm() + template_folder_path = current_service.get_template_folder_path(template_folder_id) + template_folder_name = template_folder_path[-1]["name"] + if request.method == 'POST': + try: + template_folder_api_client.delete_template_folder(current_service.id, template_folder_id) - template_folder_api_client.delete_template_folder(current_service.id, template_folder_id) + flash("'{}' folder has been removed".format(template_folder_name), 'info') + return redirect( + url_for('.choose_template', service_id=service_id) + ) + except HTTPError as e: + msg = "Folder is not empty" + if e.status_code == 400 and msg in e.message: + flash("'{}' folder is not empty".format(template_folder_name), 'info') + return redirect(url_for('.choose_template', service_id=service_id)) + else: + abort(500, e) - return redirect( - url_for('.choose_template', service_id=service_id) + flash("Are you sure you want to remove '{}' folder?".format(template_folder_name), 'remove') + return render_template( + 'views/templates/manage-template-folder.html', + form=form, + template_folder_path=template_folder_path, + current_service_id=current_service.id, + template_folder_id=template_folder_id ) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index ecfe7dae6..f0f4ffdd4 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -382,7 +382,7 @@ def test_delete_folder(client_request, service_one, mock_get_template_folders, m mock_delete = mocker.patch('app.template_folder_api_client.delete_template_folder') folder_id = str(uuid.uuid4()) mock_get_template_folders.return_value = [ - {'id': folder_id, 'name': 'folder_two', 'parent_id': None}, + {'id': folder_id, 'name': 'sacrifice', 'parent_id': None}, ] service_one['permissions'] += ['edit_folders'] From 2a1328a17e88049c85c693331b9dcefb27227660 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 14 Nov 2018 15:57:23 +0000 Subject: [PATCH 5/6] Check if folder is empty before calling the api --- app/main/views/templates.py | 8 ++++++++ tests/app/main/views/test_template_folders.py | 8 ++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 5bd013158..e27942342 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -408,6 +408,14 @@ def delete_template_folder(service_id, template_folder_id): form = TemplateFolderForm() template_folder_path = current_service.get_template_folder_path(template_folder_id) template_folder_name = template_folder_path[-1]["name"] + if len(current_service.get_template_folders_and_templates( + template_type="all", template_folder_id=template_folder_id + )) > 0: + flash("'{}' folder is not empty".format(template_folder_name), 'info') + return redirect(url_for( + '.manage_template_folder', service_id, template_folder_id + )) + if request.method == 'POST': try: template_folder_api_client.delete_template_folder(current_service.id, template_folder_id) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index f0f4ffdd4..9aa01d1f3 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -381,9 +381,13 @@ def test_rename_folder(client_request, service_one, mock_get_template_folders, m def test_delete_folder(client_request, service_one, mock_get_template_folders, mocker): mock_delete = mocker.patch('app.template_folder_api_client.delete_template_folder') folder_id = str(uuid.uuid4()) - mock_get_template_folders.return_value = [ + mock_get_template_folders.side_effect = [[ {'id': folder_id, 'name': 'sacrifice', 'parent_id': None}, - ] + ], []] + mocker.patch( + 'app.models.service.Service.get_templates', + return_value=[], + ) service_one['permissions'] += ['edit_folders'] client_request.post( From f54c2c0dc701611dd52aebf167350eb1743cd2de Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 14 Nov 2018 17:07:23 +0000 Subject: [PATCH 6/6] Update content of deletion messages following review --- app/main/views/templates.py | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index e27942342..058ef0771 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -408,31 +408,40 @@ def delete_template_folder(service_id, template_folder_id): form = TemplateFolderForm() template_folder_path = current_service.get_template_folder_path(template_folder_id) template_folder_name = template_folder_path[-1]["name"] + if len(current_service.get_template_folders_and_templates( template_type="all", template_folder_id=template_folder_id )) > 0: - flash("'{}' folder is not empty".format(template_folder_name), 'info') - return redirect(url_for( - '.manage_template_folder', service_id, template_folder_id - )) + 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 + ) + ) if request.method == 'POST': try: template_folder_api_client.delete_template_folder(current_service.id, template_folder_id) - flash("'{}' folder has been removed".format(template_folder_name), 'info') return redirect( url_for('.choose_template', service_id=service_id) ) except HTTPError as e: msg = "Folder is not empty" if e.status_code == 400 and msg in e.message: - flash("'{}' folder is not empty".format(template_folder_name), 'info') - return redirect(url_for('.choose_template', service_id=service_id)) + 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 + ) + ) else: abort(500, e) - flash("Are you sure you want to remove '{}' folder?".format(template_folder_name), 'remove') + 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,