Merge pull request #5831 from alphagov/add-banner-delete-file

Add and improve banners shown when deleting files

Author: quis

app/main/views/template_email_files.py

@@ -1,4 +1,4 @@
-from flask import abort, redirect, render_template, request, url_for
+from flask import abort, flash, redirect, render_template, request, url_for
 from notifications_utils.field import PlainTextField
 from notifications_utils.insensitive_dict import InsensitiveSet
 
@@ -81,32 +81,30 @@ def manage_a_template_email_file(service_id, template_id, template_email_file_id
         must_be_of_type="email",
     )
     delete = bool(request.args.get("delete"))
-    pending = bool(request.args.get("pending", False))
     template_email_file = TemplateEmailFile.get_by_id(
         template_email_file_id=template_email_file_id, service_id=service_id, template_id=template_id
     )
-    if request.method == "POST":
-        if delete:
-            new_content = PlainTextField(
-                template.content,
-                {template_email_file.filename: ""},
-                html="passthrough",
-            )
-            service_api_client.update_service_template(
-                service_id=service_id,
-                template_id=template_id,
-                content=str(new_content).strip(),
-                archive_email_file_ids=[template_email_file.id],
-            )
-            return redirect(url_for("main.view_template", service_id=current_service.id, template_id=template.id))
+    if request.method == "POST" and delete:
+        new_content = PlainTextField(
+            template.content,
+            {template_email_file.filename: ""},
+            html="passthrough",
+        )
+        service_api_client.update_service_template(
+            service_id=service_id,
+            template_id=template_id,
+            content=str(new_content).strip(),
+            archive_email_file_ids=[template_email_file.id],
+        )
+        flash(f"‘{template_email_file.filename}’ has been removed", "default_with_tick")
+        return redirect(url_for("main.view_template", service_id=current_service.id, template_id=template.id))
 
     return render_template(
         "views/templates/email-template-files/email_file.html",
         template_email_file=template_email_file,
         service_id=service_id,
         template_id=template_id,
         delete=delete,
-        pending=pending,
     )
 
 

app/main/views/templates.py

@@ -775,8 +775,10 @@ def edit_service_template(service_id, template_id, language=None):  # noqa
                 template.template_type == "letter" and template.welsh_page_count and language != "welsh"
             )
             if template_change.email_files_removed:
+                multiple_files_removed = len(template_change.email_filenames_removed) > 1
                 flash(
-                    f"Files for {formatted_list(template_change.email_filenames_removed)} have been removed from the template.",  # noqa
+                    f"{formatted_list(template_change.email_filenames_removed)} "
+                    f"{'have' if multiple_files_removed else 'has'} been removed",
                     "default_with_tick",
                 )
             return redirect(

tests/app/main/views/test_template_email_files.py

@@ -224,20 +224,22 @@ def test_post_delete_to_manage_a_template_email_file_updates_and_redirects(
     mock_update_service_template = mocker.patch(
         "app.notify_client.service_api_client.ServiceAPIClient.update_service_template"
     )
-    client_request.post(
+    page = client_request.post(
         "main.manage_a_template_email_file",
         service_id=service_one["id"],
         template_id=fake_uuid,
         template_email_file_id=test_data_for_a_template_email_file["id"],
         delete=True,
-        _expected_redirect=url_for("main.view_template", service_id=service_one["id"], template_id=fake_uuid),
+        _follow_redirects=True,
     )
     mock_update_service_template.assert_called_once_with(
         service_id=service_one["id"],
         template_id=fake_uuid,
         content="This template contains an email file",
         archive_email_file_ids=[test_data_for_a_template_email_file["id"]],
     )
+    assert normalize_spaces(page.select_one("h1.folder-heading")) == "sample template"
+    assert normalize_spaces(page.select_one(".banner-default-with-tick")) == "‘test_file_1.csv’ has been removed"
 
 
 def test_manage_a_template_email_file_raises_404_for_invalid_template_email_file_id(

tests/app/main/views/test_templates.py

@@ -3751,7 +3751,34 @@ def test_should_redirect_when_saving_a_template_email(
     )
 
 
-def test_edit_service_template_archives_email_files(client_request, fake_uuid, mocker):
+@pytest.mark.parametrize(
+    "new_content, expected_file_ids_to_archive, expected_banner_text",
+    (
+        (
+            "For the appointment, you will just need ((map.pdf))",
+            [
+                "00000000-0000-4000-8000-000000000001",
+                "00000000-0000-4000-8000-000000000002",
+            ],
+            "‘invite.pdf’ and ‘form.pdf’ have been removed",
+        ),
+        (
+            "For the appointment, you will just need ((form.pdf)) and ((map.pdf))",
+            [
+                "00000000-0000-4000-8000-000000000001",
+            ],
+            "‘invite.pdf’ has been removed",
+        ),
+    ),
+)
+def test_edit_service_template_archives_email_files(
+    client_request,
+    fake_uuid,
+    mocker,
+    new_content,
+    expected_file_ids_to_archive,
+    expected_banner_text,
+):
     # mock out template with email files
     email_template = create_template(
         template_id=fake_uuid,
@@ -3784,8 +3811,6 @@ def test_edit_service_template_archives_email_files(client_request, fake_uuid, m
     )
     mocker.patch("app.service_api_client.get_service_template", return_value={"data": email_template})
 
-    new_content = "For the appointment, you will just need ((map.pdf))"
-
     mock_update_service_template = mocker.patch("notifications_python_client.base.BaseAPIClient.request")
 
     page = client_request.post(
@@ -3806,21 +3831,15 @@ def test_edit_service_template_archives_email_files(client_request, fake_uuid, m
         "/service/596364a0-858e-42c8-9062-a8fe822260eb/template/6ce466d0-fd6a-11e5-82f5-e0accb9d11a6",
         data={
             "created_by": "6ce466d0-fd6a-11e5-82f5-e0accb9d11a6",
-            "content": "For the appointment, you will just need ((map.pdf))",
+            "content": new_content,
             "subject": "Your ((thing)) is due soon",
             "name": "sample template",
             "has_unsubscribe_link": False,
-            "archive_email_file_ids": [
-                "00000000-0000-4000-8000-000000000001",
-                "00000000-0000-4000-8000-000000000002",
-            ],
+            "archive_email_file_ids": expected_file_ids_to_archive,
         },
     )
 
-    assert (
-        normalize_spaces(page.select(".banner-default-with-tick")[0].text)
-        == "Files for ‘invite.pdf’ and ‘form.pdf’ have been removed from the template."
-    )
+    assert normalize_spaces(page.select(".banner-default-with-tick")[0].text) == expected_banner_text
 
 
 def test_should_redirect_when_saving_a_template_letter(