Do not use ContentTypes in PageChooserPanel

Using ContentTypes can lead to the awkward case where they are used

before the database is migrated, leading to errors on start up. Removing

ContentTypes and using plain Model classes instead makes this

impossible, and also makes the code clearer.



Fixes #2433

wagtail/wagtailadmin/edit_handlers.py

@@ -508,41 +508,40 @@ class BaseChooserPanel(BaseFieldPanel):
 class BasePageChooserPanel(BaseChooserPanel):
     object_type_name = "page"
 
-    _target_content_type = None
-
     @classmethod
     def widget_overrides(cls):
         return {cls.field_name: widgets.AdminPageChooser(
-            content_type=cls.target_content_type(), can_choose_root=cls.can_choose_root)}
+            target_models=cls.target_models(),
+            can_choose_root=cls.can_choose_root)}
 
-    @classmethod
+    @cached_classmethod
+    def target_models(cls):
+        if cls.page_type:
+            target_models = []
+
+            for page_type in cls.page_type:
+                try:
+                    target_models.append(resolve_model_string(page_type))
+                except LookupError:
+                    raise ImproperlyConfigured(
+                        "{0}.page_type must be of the form 'app_label.model_name', given {1!r}".format(
+                            cls.__name__, page_type
+                        )
+                    )
+                except ValueError:
+                    raise ImproperlyConfigured(
+                        "{0}.page_type refers to model {1!r} that has not been installed".format(
+                            cls.__name__, page_type
+                        )
+                    )
+
+            return target_models
+        else:
+            return [cls.model._meta.get_field(cls.field_name).rel.to]
+
+    @cached_classmethod
     def target_content_type(cls):
-        if cls._target_content_type is None:
-            if cls.page_type:
-                target_models = []
-
-                for page_type in cls.page_type:
-                    try:
-                        target_models.append(resolve_model_string(page_type))
-                    except LookupError:
-                        raise ImproperlyConfigured(
-                            "{0}.page_type must be of the form 'app_label.model_name', given {1!r}".format(
-                                cls.__name__, page_type
-                            )
-                        )
-                    except ValueError:
-                        raise ImproperlyConfigured(
-                            "{0}.page_type refers to model {1!r} that has not been installed".format(
-                                cls.__name__, page_type
-                            )
-                        )
-
-                cls._target_content_type = list(ContentType.objects.get_for_models(*target_models).values())
-            else:
-                target_model = cls.model._meta.get_field(cls.field_name).rel.to
-                cls._target_content_type = [ContentType.objects.get_for_model(target_model)]
-
-        return cls._target_content_type
+        return list(ContentType.objects.get_for_models(*cls.target_models()).values())
 
 
 class PageChooserPanel(object):

wagtail/wagtailadmin/tests/test_edit_handlers.py

@@ -536,6 +536,29 @@ class TestPageChooserPanel(TestCase):
 
         self.assertIn(expected_js, result)
 
+    def test_target_models(self):
+        result = PageChooserPanel(
+            'barbecue',
+            'wagtailcore.site'
+        ).bind_to_model(PageChooserModel).target_models()
+        self.assertEqual(result, [Site])
+
+    def test_target_models_malformed_type(self):
+        result = PageChooserPanel(
+            'barbecue',
+            'snowman'
+        ).bind_to_model(PageChooserModel)
+        self.assertRaises(ImproperlyConfigured,
+                          result.target_models)
+
+    def test_target_models_nonexistent_type(self):
+        result = PageChooserPanel(
+            'barbecue',
+            'snowman.lorry'
+        ).bind_to_model(PageChooserModel)
+        self.assertRaises(ImproperlyConfigured,
+                          result.target_models)
+
     def test_target_content_type(self):
         result = PageChooserPanel(
             'barbecue',

wagtail/wagtailadmin/tests/test_widgets.py

@@ -51,12 +51,26 @@ class TestAdminPageChooserWidget(TestCase):
     # def test_render_html_init_with_content_type omitted as HTML does not
     # change when selecting a content type
 
+    def test_render_js_init_with_target_model(self):
+        widget = widgets.AdminPageChooser(target_models=[SimplePage])
+
+        js_init = widget.render_js_init('test-id', 'test', None)
+        self.assertEqual(js_init, "createPageChooser(\"test-id\", [\"tests.simplepage\"], null, false);")
+
+    def test_render_js_init_with_multiple_target_models(self):
+        target_models = [SimplePage, EventPage]
+        widget = widgets.AdminPageChooser(target_models=target_models)
+
+        js_init = widget.render_js_init('test-id', 'test', None)
+        self.assertEqual(
+            js_init, "createPageChooser(\"test-id\", [\"tests.simplepage\", \"tests.eventpage\"], null, false);"
+        )
+
     def test_render_js_init_with_content_type(self):
         content_type = ContentType.objects.get_for_model(SimplePage)
         widget = widgets.AdminPageChooser(content_type=content_type)
 
-        js_init = widget.render_js_init('test-id', 'test', None)
-        self.assertEqual(js_init, "createPageChooser(\"test-id\", [\"tests.simplepage\"], null, false);")
+        self.assertEqual(widget.target_models, [SimplePage])
 
     def test_render_js_init_with_multiple_content_types(self):
         content_types = [
@@ -66,10 +80,7 @@ class TestAdminPageChooserWidget(TestCase):
         ]
         widget = widgets.AdminPageChooser(content_type=content_types)
 
-        js_init = widget.render_js_init('test-id', 'test', None)
-        self.assertEqual(
-            js_init, "createPageChooser(\"test-id\", [\"tests.simplepage\", \"tests.eventpage\"], null, false);"
-        )
+        self.assertEqual(widget.target_models, [SimplePage, EventPage])
 
     def test_render_js_init_with_can_choose_root(self):
         widget = widgets.AdminPageChooser(can_choose_root=True)

wagtail/wagtailadmin/widgets.py

@@ -130,24 +130,38 @@ class AdminPageChooser(AdminChooser):
     choose_another_text = _('Choose another page')
     link_to_chosen_text = _('Edit this page')
 
-    def __init__(self, content_type=None, can_choose_root=False, **kwargs):
+    def __init__(self, target_models=None, content_type=None, can_choose_root=False, **kwargs):
         super(AdminPageChooser, self).__init__(**kwargs)
-        self._content_type = content_type
+
+        self.target_models = list(target_models or [Page])
+
+        if content_type is not None:
+            if target_models is not None:
+                raise ValueError("Can not set both target_models and content_type")
+            if isinstance(content_type, ContentType):
+                self.target_models = [content_type.model_class()]
+            else:
+                self.target_models = [ct.model_class() for ct in content_type]
+
         self.can_choose_root = can_choose_root
 
     @cached_property
     def target_content_types(self):
-        target_content_types = self._content_type or ContentType.objects.get_for_model(Page)
-        # Make sure target_content_types is a list or tuple
-        if not isinstance(target_content_types, (list, tuple)):
-            target_content_types = [target_content_types]
-        return target_content_types
+        return list(ContentType.objects.get_for_models(*self.target_models).values())
+
+    def _get_lowest_common_page_class(self):
+        """
+        Return a Page class that is an ancestor for all Page classes in
+        ``target_models``, and is also a concrete Page class itself.
+        """
+        if len(self.target_models) == 1:
+            # Shortcut for a single page type
+            return self.target_models[0]
+        else:
+            return Page
 
     def render_html(self, name, value, attrs):
-        if len(self.target_content_types) == 1:
-            model_class = self.target_content_types[0].model_class()
-        else:
-            model_class = Page
+        model_class = self._get_lowest_common_page_class()
 
         instance, value = self.get_instance_and_id(model_class, value)
 
@@ -166,22 +180,18 @@ class AdminPageChooser(AdminChooser):
             page = value
         else:
             # Value is an ID look up object
-            if len(self.target_content_types) == 1:
-                model_class = self.target_content_types[0].model_class()
-            else:
-                model_class = Page
-
+            model_class = self._get_lowest_common_page_class()
             page = self.get_instance(model_class, value)
 
         parent = page.get_parent() if page else None
 
-        return "createPageChooser({id}, {content_type}, {parent}, {can_choose_root});".format(
+        return "createPageChooser({id}, {model_names}, {parent}, {can_choose_root});".format(
             id=json.dumps(id_),
-            content_type=json.dumps([
+            model_names=json.dumps([
                 '{app}.{model}'.format(
-                    app=content_type.app_label,
-                    model=content_type.model)
-                for content_type in self.target_content_types
+                    app=model._meta.app_label,
+                    model=model._meta.model_name)
+                for model in self.target_models
             ]),
             parent=json.dumps(parent.id if parent else None),
             can_choose_root=('true' if self.can_choose_root else 'false')