Fix `BaseChooser.get_instance` for non-existing pks (#10362)

Fixes #10348

e.g. when trying to move an existing page when `parent_page_types`
is added after the page is created under a different parent page type than the one allowed

CHANGELOG.txt

@@ -4,6 +4,7 @@ Changelog
 5.1 (xx.xx.xxxx) - IN DEVELOPMENT
 ~~~~~~~~~~~~~~~~
 
+ * Fix: Prevent choosers from failing when initial value is an unrecognised ID, e.g. when moving a page from a location where `parent_page_types` would disallow it (Dan Braghis)
  * Docs: Document how to add non-ModelAdmin views to a `ModelAdminGroup` (Onno Timmerman)
  * Maintenance: Switch to ruff for flake8 / isort code checking (Oliver Parker)
 

docs/releases/5.1.md

@@ -19,7 +19,7 @@ depth: 1
 
 ### Bug fixes
 
- * ...
+ * Prevent choosers from failing when initial value is an unrecognised ID, e.g. when moving a page from a location where `parent_page_types` would disallow it (Dan Braghis)
 
 ### Documentation
 

wagtail/admin/tests/pages/test_move_page.py

@@ -9,57 +9,53 @@ from django.urls import reverse
 
 from wagtail.models import Page
 from wagtail.signals import post_page_move, pre_page_move
-from wagtail.test.testapp.models import SimplePage
+from wagtail.test.testapp.models import BusinessSubIndex, SimplePage
 from wagtail.test.utils import WagtailTestUtils
 
 
 class TestPageMove(WagtailTestUtils, TestCase):
     fixtures = ["test.json"]
 
-    def setUp(self):
+    @classmethod
+    def setUpTestData(cls):
         # Find root page
-        self.root_page = Page.objects.get(id=2)
+        cls.root_page = Page.objects.get(id=2)
 
         # Create three sections
-        self.section_a = SimplePage(
-            title="Section A", slug="section-a", content="hello"
-        )
-        self.root_page.add_child(instance=self.section_a)
+        cls.section_a = SimplePage(title="Section A", slug="section-a", content="hello")
+        cls.root_page.add_child(instance=cls.section_a)
 
-        self.section_b = SimplePage(
-            title="Section B", slug="section-b", content="hello"
-        )
-        self.root_page.add_child(instance=self.section_b)
+        cls.section_b = SimplePage(title="Section B", slug="section-b", content="hello")
+        cls.root_page.add_child(instance=cls.section_b)
 
-        self.section_c = SimplePage(
-            title="Section C", slug="section-c", content="hello"
-        )
-        self.root_page.add_child(instance=self.section_c)
+        cls.section_c = SimplePage(title="Section C", slug="section-c", content="hello")
+        cls.root_page.add_child(instance=cls.section_c)
 
         # Add test page A into section A
-        self.test_page_a = SimplePage(
+        cls.test_page_a = SimplePage(
             title="Hello world!", slug="hello-world", content="hello"
         )
-        self.section_a.add_child(instance=self.test_page_a)
+        cls.section_a.add_child(instance=cls.test_page_a)
 
         # Add test page B into section C
-        self.test_page_b = SimplePage(
+        cls.test_page_b = SimplePage(
             title="Hello world!", slug="hello-world", content="hello"
         )
-        self.section_c.add_child(instance=self.test_page_b)
+        cls.section_c.add_child(instance=cls.test_page_b)
 
         # Add unpublished page to the root with a child page
-        self.unpublished_page = SimplePage(
+        cls.unpublished_page = SimplePage(
             title="Unpublished", slug="unpublished", content="hello"
         )
         sub_page = SimplePage(title="Sub Page", slug="sub-page", content="child")
-        self.root_page.add_child(instance=self.unpublished_page)
-        self.unpublished_page.add_child(instance=sub_page)
+        cls.root_page.add_child(instance=cls.unpublished_page)
+        cls.unpublished_page.add_child(instance=sub_page)
 
         # unpublish pages last (used to validate the edit only permission)
-        self.unpublished_page.unpublish()
+        cls.unpublished_page.unpublish()
         sub_page.unpublish()
 
+    def setUp(self):
         # Login
         self.user = self.login()
 
@@ -68,6 +64,7 @@ class TestPageMove(WagtailTestUtils, TestCase):
             reverse("wagtailadmin_pages:move", args=(self.test_page_a.id,))
         )
         self.assertEqual(response.status_code, 200)
+        self.assertContains(response, self.section_a.title)
 
     def test_page_move_bad_permissions(self):
         # Remove privileges from user
@@ -241,3 +238,25 @@ class TestPageMove(WagtailTestUtils, TestCase):
         self.assertEqual(
             Page.objects.get(id=self.test_page_a.id).get_parent().id, self.section_b.id
         )
+
+    def test_page_move_after_parent_page_types_changes_to_different_parent_model(self):
+        # Test for issue #10348
+        # While BusinesSubIndex cannot be created under a SimplePage, we can
+        # still create it under a SimplePage invoking django-treebeard's add_child
+        # which works great for our purposes.
+        self.assertFalse(BusinessSubIndex.can_exist_under(self.section_a))
+        page = self.section_a.add_child(
+            instance=BusinessSubIndex(
+                title="Business Sub Index",
+                slug="business-sub-index",
+                live=True,
+                has_unpublished_changes=False,
+            )
+        )
+
+        response = self.client.get(reverse("wagtailadmin_pages:move", args=(page.id,)))
+        self.assertEqual(response.status_code, 200)
+
+        form = response.context["move_form"]
+        self.assertEqual(form.fields["new_parent_page"].initial.pk, self.section_a.pk)
+        self.assertNotContains(response, self.section_a.title)

wagtail/admin/tests/test_widgets.py

@@ -12,15 +12,16 @@ from wagtail.test.testapp.models import EventPage, RestaurantTag, SimplePage
 
 
 class TestAdminPageChooserWidget(TestCase):
-    def setUp(self):
-        self.root_page = Page.objects.get(id=2)
+    @classmethod
+    def setUpTestData(cls):
+        cls.root_page = Page.objects.get(id=2)
 
         # Add child page
-        self.child_page = SimplePage(
+        cls.child_page = SimplePage(
             title="foobarbaz",
             content="hello",
         )
-        self.root_page.add_child(instance=self.child_page)
+        cls.root_page.add_child(instance=cls.child_page)
 
     def test_not_hidden(self):
         widget = widgets.AdminPageChooser()
@@ -164,6 +165,14 @@ class TestAdminPageChooserWidget(TestCase):
             html,
         )
 
+    def test_get_instance(self):
+        widget = widgets.AdminPageChooser(target_models=[SimplePage])
+        self.assertIsNone(widget.get_instance(None))
+        self.assertIsNone(widget.get_instance(self.root_page.id))
+        self.assertIsNone(widget.get_instance(self.child_page.id + 100))
+        self.assertEqual(widget.get_instance(self.child_page), self.child_page)
+        self.assertEqual(widget.get_instance(self.child_page.id), self.child_page)
+
 
 class TestAdminDateInput(TestCase):
     def test_adapt(self):

wagtail/admin/widgets/chooser.py

@@ -118,7 +118,10 @@ class BaseChooser(widgets.Input):
         elif isinstance(value, self.model_class):
             return value
         else:  # assume instance ID
-            return self.model_class.objects.get(pk=value)
+            try:
+                return self.model_class.objects.get(pk=value)
+            except self.model_class.DoesNotExist:
+                return None
 
     def get_display_title(self, instance):
         """

wagtail/test/testapp/models.py

@@ -1280,7 +1280,7 @@ class BusinessSubIndex(Page):
     # BusinessNowherePage is 'incorrectly' added here as a possible child.
     # The rules on BusinessNowherePage prevent it from being a child here though.
     subpage_types = ["tests.BusinessChild", "tests.BusinessNowherePage"]
-    parent_page_types = ["tests.BusinessIndex", "tests.BusinessChild"]
+    parent_page_types = ["tests.BusinessIndex"]
 
 
 class BusinessChild(Page):