fix: display node tree on menu item delete confirmation page (#155)

Fixes a bug which meant not all child nodes were displayed on the
delete confirmation page. Also ensures that the node tree displays
items with the correct nesting to reflect their position in the tree.

Co-authored-by: Michael Collins <[email protected]>

CHANGELOG.rst

@@ -4,6 +4,8 @@ Changelog
 
 Unreleased
 ==========
+* fix: delete confirmation screen displays all nodes to be deleted, with each node nested
+correctly to match the standard django delete confirmation view
 
 1.8.1 (2022-10-21)
 ==================

djangocms_navigation/admin.py

@@ -5,7 +5,7 @@
 from django.contrib import admin, messages
 from django.contrib.admin.options import IS_POPUP_VAR
 from django.contrib.admin.templatetags.admin_urls import add_preserved_filters
-from django.contrib.admin.utils import get_deleted_objects, quote
+from django.contrib.admin.utils import quote
 from django.contrib.admin.views.main import ChangeList
 from django.contrib.contenttypes.models import ContentType
 from django.contrib.sites.shortcuts import get_current_site
@@ -569,21 +569,30 @@ def set_main_navigation_view(self, request, menu_content_id):
             request, "admin/djangocms_navigation/main_navigation_confirmation.html", context
         )
 
-    def _get_to_be_deleted_objects(self, menu_item, request):
-        def _get_related_objects(menu_item):
-            # Since we are using treebeard, the default 'to_be_deleted' functionality from django
-            # does not work, therefore we must construct the list here and
-            # pass it to the delete confirmation.
-            yield menu_item
-            for child_item in menu_item.get_children():
-                yield child_item
-
-        to_be_deleted = list(_get_related_objects(menu_item))
-        get_deleted_objects_additional_context = {"request": request}
-        (deleted_objects, model_count, perms_needed, protected) = get_deleted_objects(
-            to_be_deleted, admin_site=self.admin_site, **get_deleted_objects_additional_context
-        )
-        return deleted_objects
+    def _get_to_be_deleted(self, nodes, node_list):
+        """
+        Recursively fetches the child nodes to be deleted in a structure that represents their nesting, so that the
+        returned node list is structured so that it is rendered in the template with items nested correctly e.g:
+
+        node_list = [
+            child_node,
+            [
+                child_of_child,
+                sibling_of_child_of_child,
+            ],
+            sibling_node,
+        ]
+        """
+        child_node_list = []
+        for node in nodes:
+            child_node_list.append(f"Menu item: {node}")
+            children = node.get_children()
+            self._get_to_be_deleted(children, child_node_list)
+
+        if child_node_list:
+            node_list.append(child_node_list)
+
+        return node_list
 
     def delete_view(self, request, object_id, menu_content_id=None, form_url="", extra_context=None):
 
@@ -620,7 +629,8 @@ def delete_view(self, request, object_id, menu_content_id=None, form_url="", ext
                     return HttpResponseRedirect(version_list_url(menu_content))
 
                 extra_context["menu_name"] = menu_item
-                extra_context["deleted_objects"] = self._get_to_be_deleted_objects(menu_item, request)
+                to_be_deleted = [f"Menu item: {menu_item}"]
+                extra_context["deleted_objects"] = self._get_to_be_deleted(menu_item.get_children(), to_be_deleted)
 
         return super().delete_view(request, object_id, extra_context)
 

tests/test_admin.py

@@ -1249,6 +1249,9 @@ def test_menuitem_delete_view_item_without_children(self):
                 delete_url_single, follow=True, data={"menu_content_id": menu_content.id}
             )
 
+        # check the context contains the expected structure of objects so that it is rendered correctly in the template
+        expected_deleted_objects = [f"Menu item: {child}"]
+        self.assertEqual(confirmation_response.context["deleted_objects"], expected_deleted_objects)
         # Confirmation screen populated with all to-be deleted items
         self.assertContains(
             confirmation_response,
@@ -1278,7 +1281,7 @@ def test_menuitem_delete_view_item_with_children(self):
         menu_content = factories.MenuContentWithVersionFactory(version__created_by=self.user)
         child = factories.ChildMenuItemFactory(parent=menu_content.root)
         child_of_child = factories.ChildMenuItemFactory(parent=child)
-        factories.ChildMenuItemFactory(parent=child_of_child)
+        child_of_child_of_child = factories.ChildMenuItemFactory(parent=child_of_child)
         # Delete an editable node, with children
         delete_url_with_child = reverse(
             "admin:djangocms_navigation_menuitem_delete", args=(menu_content.id, child.id,),
@@ -1292,6 +1295,17 @@ def test_menuitem_delete_view_item_with_children(self):
                 delete_url_with_child, follow=True, data={"menu_content_id": menu_content.id}
             )
 
+        # check the context contains the expected structure of objects so that it is rendered correctly in the template
+        expected_deleted_objects = [
+            f"Menu item: {child}",
+            [
+                f"Menu item: {child_of_child}",
+                [
+                    f"Menu item: {child_of_child_of_child}"
+                ]
+            ]
+        ]
+        self.assertEqual(confirmation_response.context_data["deleted_objects"], expected_deleted_objects)
         # Confirmation screen populated with all to be deleted items
         self.assertContains(
             confirmation_response,
@@ -1303,16 +1317,25 @@ def test_menuitem_delete_view_item_with_children(self):
         )
         self.assertContains(
             confirmation_response,
-            '<ul>\t<li>Menu item: {}</li>'.format(
+            '<ul>\t<li>Menu item: {}\n\t'.format(
                 child
             )
         )
+        # two tabs indicate this item is nested under the previous node
         self.assertContains(
             confirmation_response,
-            '\t<li>Menu item: {}</li></ul>'.format(
+            '<ul>\n\t\t<li>Menu item: {}\n\t\t'.format(
                 child_of_child
             )
         )
+        # three tabs indicates this item is nested under the previous child node
+        # and then the previous <ul> elements are closed
+        self.assertContains(
+            confirmation_response,
+            '<ul>\n\t\t\t<li>Menu item: {}</li>\n\t\t</ul>\n\t\t</li>\n\t</ul>\n\t</li></ul>'.format(
+                child_of_child_of_child
+            )
+        )
 
         content = response.content.decode('utf-8')