Fix permission error when sorting pages having page type restrictions

CHANGELOG.txt

@@ -44,6 +44,7 @@ Changelog
 ~~~~~~~~~~~~~~~~~~~
 
  * Fix: Update django-treebeard dependency to 4.5.1 or above (Serafeim Papastefanos)
+ * Fix: Fix permission error when sorting pages having page type restrictions (Thijs Kramer)
 
 
 2.16.1 (11.02.2022)

docs/releases/2.16.2.md

@@ -12,3 +12,4 @@ depth: 1
 ### Bug fixes
 
  * Update django-treebeard dependency to 4.5.1 or above (Serafeim Papastefanos)
+ * Fix permission error when sorting pages having page type restrictions (Thijs Kramer)

wagtail/actions/move_page.py

@@ -25,25 +25,19 @@ class MovePageAction:
         self.pos = pos
         self.user = user
 
-    def check(self, skip_permission_checks=False):
+    def check(self, parent_after, skip_permission_checks=False):
         if self.user and not skip_permission_checks:
-            if not self.page.permissions_for_user(self.user).can_move_to(self.target):
+            if not self.page.permissions_for_user(self.user).can_move_to(parent_after):
                 raise MovePagePermissionError(
                     "You do not have permission to move the page to the target specified."
                 )
 
-    def _move_page(self, page, target, pos=None):
+    def _move_page(self, page, target, parent_after):
         from wagtail.models import Page
 
-        # Determine old and new parents
-        parent_before = page.get_parent()
-        if pos in ("first-child", "last-child", "sorted-child"):
-            parent_after = target
-        else:
-            parent_after = target.get_parent()
-
         # Determine old and new url_paths
         # Fetching new object to avoid affecting `page`
+        parent_before = page.get_parent()
         old_page = Page.objects.get(id=page.id)
         old_url_path = old_page.url_path
         new_url_path = old_page.set_url_path(parent=parent_after)
@@ -62,7 +56,7 @@ class MovePageAction:
         # Only commit when all descendants are properly updated
         with transaction.atomic():
             # Allow treebeard to update `path` values
-            MP_MoveHandler(page, target, pos).process()
+            MP_MoveHandler(page, target, self.pos).process()
 
             # Treebeard's move method doesn't actually update the in-memory instance,
             # so we need to work with a freshly loaded one now
@@ -103,6 +97,11 @@ class MovePageAction:
         logger.info('Page moved: "%s" id=%d path=%s', page.title, page.id, new_url_path)
 
     def execute(self, skip_permission_checks=False):
-        self.check(skip_permission_checks=skip_permission_checks)
+        if self.pos in ("first-child", "last-child", "sorted-child"):
+            parent_after = self.target
+        else:
+            parent_after = self.target.get_parent()
 
-        return self._move_page(self.page, self.target, pos=self.pos)
+        self.check(parent_after, skip_permission_checks=skip_permission_checks)
+
+        return self._move_page(self.page, self.target, parent_after)

wagtail/admin/tests/pages/test_move_page.py

@@ -196,12 +196,6 @@ class TestPageMove(TestCase, WagtailTestUtils):
             )
         )
 
-    def test_page_set_page_position(self):
-        response = self.client.get(
-            reverse("wagtailadmin_pages:set_page_position", args=(self.test_page_a.id,))
-        )
-        self.assertEqual(response.status_code, 200)
-
     def test_before_move_page_hook(self):
         def hook_func(request, page, destination):
             self.assertIsInstance(request, HttpRequest)

wagtail/admin/tests/pages/test_reorder_page.py

@@ -0,0 +1,142 @@
+from django.test import TestCase
+from django.urls import reverse
+
+from wagtail.core.models import Page
+from wagtail.test.testapp.models import BusinessChild, BusinessIndex, SimplePage
+from wagtail.tests.utils import WagtailTestUtils
+
+
+class TestPageReorder(TestCase, WagtailTestUtils):
+    fixtures = ["test.json"]
+
+    def __init__(self, methodName: str = ...) -> None:
+        super().__init__(methodName)
+
+    def setUp(self):
+        # Find root page
+        self.root_page = Page.objects.get(id=2)
+
+        # root
+        # |- simple_index (SimplePage)
+        # |  |- simple_child_1 (SimplePage)
+        # |  |- simple_child_2 (SimplePage)
+        # |  |- simple_child_3 (SimplePage)
+
+        self.index_page = SimplePage(title="Simple", slug="simple", content="hello")
+        self.root_page.add_child(instance=self.index_page)
+
+        self.child_1 = SimplePage(
+            title="Child 1 of SimplePage", slug="child-1", content="hello"
+        )
+        self.index_page.add_child(instance=self.child_1)
+        self.child_2 = SimplePage(
+            title="Child 2 of SimplePage", slug="child-2", content="hello"
+        )
+        self.index_page.add_child(instance=self.child_2)
+        self.child_3 = SimplePage(
+            title="Child 3 of SimplePage", slug="child-3", content="hello"
+        )
+        self.index_page.add_child(instance=self.child_3)
+
+        # Login
+        self.user = self.login()
+
+    def test_page_set_page_position_get_request_with_simple_page(self):
+        """
+        Test that GET requests to set_page_position view don't alter the page order.
+        """
+        response = self.client.get(
+            reverse("wagtailadmin_pages:set_page_position", args=(self.child_1.id,))
+        )
+        self.assertEqual(response.status_code, 200)
+
+        # Ensure page order does not change:
+        child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+        self.assertListEqual(list(child_slugs), ["child-1", "child-2", "child-3"])
+
+    def test_page_set_page_position_without_position_argument_moves_it_to_the_end(self):
+        response = self.client.post(
+            reverse("wagtailadmin_pages:set_page_position", args=(self.child_1.id,))
+        )
+        self.assertEqual(response.status_code, 200)
+
+        # check if child_1 is the last child page:
+        child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+        self.assertListEqual(list(child_slugs), ["child-2", "child-3", "child-1"])
+
+    def test_page_move_page_position_up(self):
+        """Moves child 3 to the first position."""
+        response = self.client.post(
+            reverse("wagtailadmin_pages:set_page_position", args=(self.child_3.id,))
+            + "?position=0"
+        )
+        self.assertEqual(response.status_code, 200)
+        # check if child_3 is the first child page:
+        child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+        self.assertListEqual(list(child_slugs), ["child-3", "child-1", "child-2"])
+
+    def test_page_move_page_position_down(self):
+        """
+        Moves child 3 to the first position."""
+        response = self.client.post(
+            reverse("wagtailadmin_pages:set_page_position", args=(self.child_1.id,))
+            + "?position=1"
+        )
+        self.assertEqual(response.status_code, 200)
+        # check if child_1 is the second child page:
+        child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+        self.assertListEqual(list(child_slugs), ["child-2", "child-1", "child-3"])
+
+    def test_page_move_page_position_to_the_same_position(self):
+        """
+        Moves child 3 to the first position."""
+        response = self.client.post(
+            reverse("wagtailadmin_pages:set_page_position", args=(self.child_1.id,))
+            + "?position=0"
+        )
+        self.assertEqual(response.status_code, 200)
+        # Ensure page order does not change:
+        child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+        self.assertListEqual(list(child_slugs), ["child-1", "child-2", "child-3"])
+
+    def test_page_set_page_position_with_invalid_target_position(self):
+        response = self.client.post(
+            reverse("wagtailadmin_pages:set_page_position", args=(self.child_3.id,))
+            + "?position=99"
+        )
+        self.assertEqual(response.status_code, 200)
+
+        # Ensure page order does not change:
+        child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+        self.assertListEqual(list(child_slugs), ["child-1", "child-2", "child-3"])
+
+
+class TestPageReorderWithParentPageRestrictions(TestPageReorder):
+    """
+    This testCase is the same as the TestPageReorder class above, but with a different
+    page type: BusinessChild has a parent_page_types restriction.
+    This ensures that this restriction doesn't affect the ability to reorder pages.
+    """
+
+    def setUp(self):
+        # Find root page
+        self.root_page = Page.objects.get(id=2)
+
+        # root
+        # |- index_page (BusinessIndex)
+        # |  |- child_1 (BusinessChild)
+        # |  |- child_2 (BusinessChild)
+        # |  |- child_3 (BusinessChild)
+
+        self.index_page = BusinessIndex(title="Simple", slug="simple")
+        self.root_page.add_child(instance=self.index_page)
+
+        self.child_1 = BusinessChild(title="Child 1 of BusinessIndex", slug="child-1")
+        self.index_page.add_child(instance=self.child_1)
+        self.child_2 = BusinessChild(title="Child 2 of BusinessIndex", slug="child-2")
+        self.index_page.add_child(instance=self.child_2)
+        self.child_3 = BusinessChild(title="Child 3 of BusinessIndex", slug="child-3")
+        self.index_page.add_child(instance=self.child_3)
+
+        # Login
+        self.user = self.login()