From 2b936a00e50e72740810021db49d279371eadabc Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Mon, 4 Apr 2016 17:05:48 +0100 Subject: [PATCH] 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 | 59 +++++++++---------- .../wagtailadmin/tests/test_edit_handlers.py | 23 ++++++++ wagtail/wagtailadmin/tests/test_widgets.py | 23 ++++++-- wagtail/wagtailadmin/widgets.py | 52 +++++++++------- 4 files changed, 100 insertions(+), 57 deletions(-) diff --git a/wagtail/wagtailadmin/edit_handlers.py b/wagtail/wagtailadmin/edit_handlers.py index 7462d388d..2049569e5 100644 --- a/wagtail/wagtailadmin/edit_handlers.py +++ b/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): diff --git a/wagtail/wagtailadmin/tests/test_edit_handlers.py b/wagtail/wagtailadmin/tests/test_edit_handlers.py index 4c7359f48..4dd639bf5 100644 --- a/wagtail/wagtailadmin/tests/test_edit_handlers.py +++ b/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', diff --git a/wagtail/wagtailadmin/tests/test_widgets.py b/wagtail/wagtailadmin/tests/test_widgets.py index 041e22ff5..df91358b5 100644 --- a/wagtail/wagtailadmin/tests/test_widgets.py +++ b/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) diff --git a/wagtail/wagtailadmin/widgets.py b/wagtail/wagtailadmin/widgets.py index 9e791c6a5..8889b0c30 100644 --- a/wagtail/wagtailadmin/widgets.py +++ b/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')