Fixed #5752 -- Move pages relative to left or right siblings (#5823)

CHANGELOG.txt

@@ -3,6 +3,9 @@
 * Fixed a bug which prevented certain migrations from running in a multi-db setup.
 * Fixed a bug which prevented the page from being marked as dirty (pending changes)
   when changing the value of the overwrite url field.
+* Fixed a bug where the page tree would not update correctly when a sibling page was moved
+  from left to right or right to left.
+* Improved the ``fix-tree`` command to also fix non-root nodes (pages).
 
 
 === 3.3.3 (2016-10-04) ===

cms/management/commands/subcommands/tree.py

@@ -1,11 +1,29 @@
 # -*- coding: utf-8 -*-
 from __future__ import absolute_import, print_function, unicode_literals
 
+from collections import OrderedDict
+
 from cms.models import Page, CMSPlugin
 
 from .base import SubcommandsCommand
 
 
+def get_descendant_ids(root_id):
+    """
+    Returns the a generator of primary keys which represent
+    descendants of the given page ID (root_id)
+    """
+    # Note this is done because get_descendants() can't be trusted
+    # as the tree can be corrupt.
+    children = Page.objects.filter(parent=root_id).values_list('pk', flat=True)
+
+    for child_id in children.iterator():
+        yield child_id
+
+        for descendant_id in get_descendant_ids(child_id):
+            yield descendant_id
+
+
 class FixTreeCommand(SubcommandsCommand):
     help_string = 'Repairing Materialized Path Tree for Pages'
     command_name = 'fix-tree'
@@ -57,6 +75,80 @@ def handle(self, *args, **options):
             public = page.publisher_public
             page.move(target=public, pos='right')
 
+        for root in root_draft_pages.order_by('site__pk', 'path'):
+            self._update_descendants_tree(root)
+
         self.stdout.write('fixing plugin tree')
         CMSPlugin.fix_tree()
         self.stdout.write('all done')
+
+    def _update_descendants_tree(self, root):
+        descendants_ids = get_descendant_ids(root.pk)
+        public_root_sibling = root.publisher_public
+
+        draft_descendants = (
+            Page
+            .objects
+            .filter(pk__in=descendants_ids)
+            .select_related('parent', 'publisher_public')
+            .order_by('depth', 'path')
+        )
+
+        descendants_by_parent = OrderedDict()
+
+        for descendant in draft_descendants.iterator():
+            parent = descendant.parent_id
+            descendants_by_parent.setdefault(parent, []).append(descendant)
+
+        for tree in descendants_by_parent.values():
+            last_draft = None
+            last_public = None
+            draft_parent = tree[0].parent
+            public_parent = draft_parent.publisher_public
+
+            for draft_page in tree:
+                draft_page.refresh_from_db()
+
+                if last_draft:
+                    # This is not the loop so this is not the first draft
+                    # child. Set this page a sibling of the last processed
+                    # draft page.
+                    draft_page.move(target=last_draft.reload(), pos='right')
+                else:
+                    # This is the first time through the loop so this is the first
+                    # draft child for this parent.
+                    draft_page.move(target=draft_parent.reload(), pos='first-child')
+
+                last_draft = draft_page
+
+                if not draft_page.publisher_public_id:
+                    continue
+
+                public_page = draft_page.publisher_public
+
+                if last_public:
+                    public_target = last_public
+                    public_position = 'right'
+                    last_public = public_page
+                elif public_parent:
+                    # always insert the first public child node found
+                    # as the first child of the public parent
+                    public_target = public_parent
+                    public_position = 'first-child'
+                    last_public = public_page
+                else:
+                    # No public parent has been found
+                    # Insert the node as a sibling to the last root sibling
+                    # Its very unlikely but possible for the root to not have
+                    # a public page. When this happens, use the root draft page
+                    # as sibling.
+                    public_target = public_root_sibling or root
+                    public_position = 'right'
+                    # This page now becomes the last root sibling
+                    public_root_sibling = public_page
+
+                public_page.refresh_from_db()
+                public_page.move(
+                    target=public_target.reload(),
+                    pos=public_position,
+                )

cms/models/pagemodel.py

@@ -1348,29 +1348,50 @@ def _publisher_can_publish(self):
                 raise PublisherCantPublish
         return True
 
-    def get_previous_filtered_sibling(self, **filters):
+    def get_filtered_siblings(self, **filters):
         filters.update({
             'publisher_is_draft': self.publisher_is_draft
         })
         filters.update({
             'site__id': self.site_id
         })
+        return self.get_siblings().filter(**filters)
+
+    def get_previous_filtered_sibling(self, **filters):
+        siblings = self.get_filtered_siblings(path__lt=self.path, **filters)
+
         try:
-            return self.get_siblings().filter(path__lt=self.path, **filters).reverse()[0]
+            sibling = siblings.reverse()[0]
         except IndexError:
-            return None
+            sibling = None
+        return sibling
+
+    def get_left_sibling_pk(self):
+        siblings = self.get_filtered_siblings(path__lt=self.path)
+
+        try:
+            sibling = siblings.values_list('pk', flat=True).reverse()[0]
+        except IndexError:
+            sibling = None
+        return sibling
 
     def get_next_filtered_sibling(self, **filters):
-        filters.update({
-            'publisher_is_draft': self.publisher_is_draft
-        })
-        filters.update({
-            'site__id': self.site_id
-        })
+        siblings = self.get_filtered_siblings(path__gt=self.path, **filters)
+
         try:
-            return self.get_siblings().filter(path__gt=self.path, **filters)[0]
+            sibling = siblings[0]
         except IndexError:
-            return None
+            sibling = None
+        return sibling
+
+    def get_right_sibling_pk(self):
+        siblings = self.get_filtered_siblings(path__gt=self.path)
+
+        try:
+            sibling = siblings.values_list('pk', flat=True)[0]
+        except IndexError:
+            sibling = None
+        return sibling
 
     def _publisher_save_public(self, obj):
         """Mptt specific stuff before the object can be saved, overrides
@@ -1402,32 +1423,43 @@ def _publisher_save_public(self, obj):
         if not public_parent:
             return obj
 
-        # The draft page (self) has been moved under
-        # another page.
+        # The draft page (self) has been moved under another page.
         # Or is already inside another page and it's been moved
         # to a different location in the same page tree.
-        prev_sibling = self.get_previous_filtered_sibling(
-            publisher_public__isnull=False,
+
+        # The sibling page on the left side of the draft page being moved (self)
+        left_sibling = self.get_previous_filtered_sibling(
+            publisher_public__parent=public_parent,
+        )
+
+        # The sibling page on the right side of the draft page being moved (self)
+        right_sibling = self.get_next_filtered_sibling(
             publisher_public__parent=public_parent,
         )
 
-        if prev_sibling:
-            prev_public_sibling = obj.get_previous_filtered_sibling()
-            sibling_changed = prev_sibling.publisher_public != prev_public_sibling
+        if left_sibling:
+            left_public_sibling_pk = obj.get_left_sibling_pk()
+            left_sibling_changed = left_sibling.publisher_public_id != left_public_sibling_pk
+        else:
+            left_sibling_changed = False
+
+        if right_sibling:
+            right_public_sibling_pk = obj.get_right_sibling_pk()
+            right_sibling_changed = right_sibling.publisher_public_id != right_public_sibling_pk
         else:
-            prev_public_sibling = None
-            sibling_changed = False
+            right_sibling_changed = False
 
         first_time_published = not self.publisher_public_id
         parent_change = public_parent != obj.parent
-        tree_change = self.depth != obj.depth or sibling_changed
+        tree_change = self.depth != obj.depth or left_sibling_changed or right_sibling_changed
 
         if first_time_published or parent_change or tree_change:
-            if prev_public_sibling:
-                # We use a sibling on the left side of the current page
-                # to make sure the live page is inserted in the same position
-                # on the tree relative to it's parent (draft or live).
-                obj = obj.move(prev_sibling.publisher_public, pos="right")
+            if left_sibling:
+                # Moving sibling page from left to right
+                obj = obj.move(left_sibling.publisher_public, pos="right")
+            elif right_sibling:
+                # Moving sibling page from right to left
+                obj = obj.move(right_sibling.publisher_public, pos="left")
             else:
                 obj = obj.move(target=public_parent, pos='first-child')
         return obj

cms/templates/admin/cms/page/tree/lazy_child_menu.html

@@ -1,3 +1,3 @@
-{% with page.children.all as children %}
+{% with page.get_children as children %}
     {% include "admin/cms/page/tree/menu.html" %}
 {% endwith %}

cms/templates/admin/cms/page/tree/lazy_menu.html

@@ -1,3 +1,3 @@
-{% with page.children.all as children %}
+{% with page.get_children as children %}
     {% include "admin/cms/page/tree/menu.html" %}
 {% endwith %}

cms/tests/test_management.py

@@ -110,13 +110,84 @@ def test_fix_tree_regression_5641(self):
             (delta, '0001000100010001'),
             (theta, '00010001000100010001'),
             (alpha.publisher_public, '0002'),
-            (delta.publisher_public, '0006'),
-            (theta.publisher_public, '00060001'),
+            (delta.publisher_public, '0003'),
+            (theta.publisher_public, '00030001'),
         ]
 
         for page, path in tree:
             self.assertEqual(page.path, path)
 
+    def test_fix_tree_regression_5752(self):
+        # ref: https://github.com/divio/django-cms/issues/5752
+        # Draft tree
+        #   Home
+        #       Alpha
+        #       Beta
+        #       Delta
+
+        home = create_page("Home", "nav_playground.html", "en", published=True)
+        alpha = create_page(
+            "Alpha",
+            "nav_playground.html",
+            "en",
+            published=True,
+            parent=home.reload(),
+        )
+        beta = create_page(
+            "Beta",
+            "nav_playground.html",
+            "en",
+            published=True,
+            parent=home.reload(),
+        )
+        delta = create_page(
+            "Delta",
+            "nav_playground.html",
+            "en",
+            published=True,
+            parent=home.reload(),
+        )
+
+        self.assertEqual(home.path, '0001')
+        self.assertEqual(alpha.path, '00010001')
+        self.assertEqual(beta.path, '00010002')
+        self.assertEqual(delta.path, '00010003')
+
+        # Public tree (corrupted)
+        #   Home
+        #       Delta
+        #       Beta
+        #       Alpha
+
+        delta.publisher_public.move(
+            target=home.publisher_public.reload(),
+            pos='first-child',
+        )
+        beta.publisher_public.move(
+            target=delta.publisher_public.reload(),
+            pos='right',
+        )
+        alpha.publisher_public.move(
+            target=beta.publisher_public.reload(),
+            pos='right',
+        )
+
+        # We corrupted the public tree above and now assert that is correctly
+        # corrupted.
+        self.assertEqual(home.publisher_public.reload().path, '0002')
+        self.assertEqual(delta.publisher_public.reload().path, '00020001')
+        self.assertEqual(beta.publisher_public.reload().path, '00020002')
+        self.assertEqual(alpha.publisher_public.reload().path, '00020003')
+
+        out = StringIO()
+        management.call_command('cms', 'fix-tree', interactive=False, stdout=out)
+
+        # fix-tree should fix the public tree to match the draft tree
+        self.assertEqual(home.publisher_public.reload().path, '0002')
+        self.assertEqual(alpha.publisher_public.reload().path, '00020001')
+        self.assertEqual(beta.publisher_public.reload().path, '00020002')
+        self.assertEqual(delta.publisher_public.reload().path, '00020003')
+
     @override_settings(INSTALLED_APPS=TEST_INSTALLED_APPS)
     def test_uninstall_apphooks_with_apphook(self):
         with apphooks(SampleApp):