From 09878fcfd87f5b5ea67dd8d178667bce344c02e0 Mon Sep 17 00:00:00 2001 From: Tim Heap Date: Mon, 4 Apr 2016 12:26:15 +1000 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 | 65 ++++++++++--------- .../wagtailadmin/tests/test_edit_handlers.py | 56 ++++++++++++++-- wagtail/wagtailadmin/tests/test_widgets.py | 42 +++++++++--- wagtail/wagtailadmin/widgets.py | 64 ++++++++++++------ 4 files changed, 159 insertions(+), 68 deletions(-) diff --git a/wagtail/wagtailadmin/edit_handlers.py b/wagtail/wagtailadmin/edit_handlers.py index 9e64024216..eb80205234 100644 --- a/wagtail/wagtailadmin/edit_handlers.py +++ b/wagtail/wagtailadmin/edit_handlers.py @@ -1,5 +1,7 @@ from __future__ import absolute_import, unicode_literals +import warnings + import django from django import forms from django.contrib.contenttypes.models import ContentType @@ -11,6 +13,7 @@ from django.utils.six import text_type from django.utils.translation import ugettext_lazy from wagtail.utils.decorators import cached_classmethod +from wagtail.utils.deprecation import RemovedInWagtail17Warning from wagtail.wagtailadmin import widgets from wagtail.wagtailcore.models import Page from wagtail.wagtailcore.utils import camelcase_to_underscore, resolve_model_string @@ -506,41 +509,43 @@ 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 + warnings.warn( + '{cls}.target_content_type() is deprecated. Use {cls}.target_models() instead'.format(cls=cls.__name__), + category=RemovedInWagtail17Warning) + 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 e5de7f3b4a..a2b59fdc28 100644 --- a/wagtail/wagtailadmin/tests/test_edit_handlers.py +++ b/wagtail/wagtailadmin/tests/test_edit_handlers.py @@ -1,5 +1,6 @@ from __future__ import absolute_import, unicode_literals +import warnings from datetime import date import mock @@ -12,6 +13,7 @@ from wagtail.tests.testapp.forms import ValidatedPageForm from wagtail.tests.testapp.models import ( EventPage, EventPageChooserModel, EventPageSpeaker, PageChooserModel, SimplePage, ValidatedPage) from wagtail.tests.utils import WagtailTestUtils +from wagtail.utils.deprecation import RemovedInWagtail17Warning from wagtail.wagtailadmin.edit_handlers import ( FieldPanel, InlinePanel, ObjectList, PageChooserPanel, RichTextFieldPanel, TabbedInterface, extract_panel_definitions_from_model_class, get_form_for_model) @@ -528,28 +530,68 @@ class TestPageChooserPanel(TestCase): self.assertIn(expected_js, result) - def test_target_content_type(self): + def test_target_models(self): result = PageChooserPanel( 'barbecue', 'wagtailcore.site' - ).bind_to_model(PageChooserModel).target_content_type()[0] - self.assertEqual(result.name, 'site') + ).bind_to_model(PageChooserModel).target_models() + self.assertEqual(result, [Site]) - def test_target_content_type_malformed_type(self): + def test_target_models_malformed_type(self): result = PageChooserPanel( 'barbecue', 'snowman' ).bind_to_model(PageChooserModel) self.assertRaises(ImproperlyConfigured, - result.target_content_type) + result.target_models) - def test_target_content_type_nonexistent_type(self): + def test_target_models_nonexistent_type(self): result = PageChooserPanel( 'barbecue', 'snowman.lorry' ).bind_to_model(PageChooserModel) self.assertRaises(ImproperlyConfigured, - result.target_content_type) + result.target_models) + + def test_target_content_type(self): + with warnings.catch_warnings(record=True) as ws: + warnings.simplefilter('always') + + result = PageChooserPanel( + 'barbecue', + 'wagtailcore.site' + ).bind_to_model(PageChooserModel).target_content_type()[0] + self.assertEqual(result.name, 'site') + + self.assertEqual(len(ws), 1) + self.assertIs(ws[0].category, RemovedInWagtail17Warning) + + def test_target_content_type_malformed_type(self): + with warnings.catch_warnings(record=True) as ws: + warnings.simplefilter('always') + + result = PageChooserPanel( + 'barbecue', + 'snowman' + ).bind_to_model(PageChooserModel) + self.assertRaises(ImproperlyConfigured, + result.target_content_type) + + self.assertEqual(len(ws), 1) + self.assertIs(ws[0].category, RemovedInWagtail17Warning) + + def test_target_content_type_nonexistent_type(self): + with warnings.catch_warnings(record=True) as ws: + warnings.simplefilter('always') + + result = PageChooserPanel( + 'barbecue', + 'snowman.lorry' + ).bind_to_model(PageChooserModel) + self.assertRaises(ImproperlyConfigured, + result.target_content_type) + self.assertEqual(len(ws), 1) + self.assertIs(ws[0].category, RemovedInWagtail17Warning) class TestInlinePanel(TestCase, WagtailTestUtils): diff --git a/wagtail/wagtailadmin/tests/test_widgets.py b/wagtail/wagtailadmin/tests/test_widgets.py index 1fcba82bb0..7aee1ab0f7 100644 --- a/wagtail/wagtailadmin/tests/test_widgets.py +++ b/wagtail/wagtailadmin/tests/test_widgets.py @@ -1,9 +1,12 @@ from __future__ import absolute_import, unicode_literals +import warnings + from django.contrib.contenttypes.models import ContentType from django.test import TestCase from wagtail.tests.testapp.models import EventPage, SimplePage +from wagtail.utils.deprecation import RemovedInWagtail17Warning from wagtail.wagtailadmin import widgets from wagtail.wagtailcore.models import Page @@ -52,26 +55,45 @@ 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_content_type(self): - content_type = ContentType.objects.get_for_model(SimplePage) - widget = widgets.AdminPageChooser(content_type=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_content_types(self): - content_types = [ - # Not using get_for_models as we need deterministic ordering - ContentType.objects.get_for_model(SimplePage), - ContentType.objects.get_for_model(EventPage), - ] - widget = widgets.AdminPageChooser(content_type=content_types) + 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): + with warnings.catch_warnings(record=True) as ws: + warnings.simplefilter('always') + content_type = ContentType.objects.get_for_model(SimplePage) + widget = widgets.AdminPageChooser(content_type=content_type) + + self.assertEqual(len(ws), 1) + self.assertIs(ws[0].category, RemovedInWagtail17Warning) + self.assertEqual(widget.target_models, [SimplePage]) + + def test_render_js_init_with_multiple_content_types(self): + with warnings.catch_warnings(record=True) as ws: + warnings.simplefilter('always') + content_types = [ + # Not using get_for_models as we need deterministic ordering + ContentType.objects.get_for_model(SimplePage), + ContentType.objects.get_for_model(EventPage), + ] + widget = widgets.AdminPageChooser(content_type=content_types) + + self.assertEqual(len(ws), 1) + self.assertIs(ws[0].category, RemovedInWagtail17Warning) + 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 469ae917b0..e80f6b820d 100644 --- a/wagtail/wagtailadmin/widgets.py +++ b/wagtail/wagtailadmin/widgets.py @@ -2,6 +2,7 @@ from __future__ import absolute_import, unicode_literals import itertools import json +import warnings from functools import total_ordering from django.contrib.contenttypes.models import ContentType @@ -16,6 +17,7 @@ from django.utils.html import format_html from django.utils.translation import ugettext_lazy as _ from taggit.forms import TagWidget +from wagtail.utils.deprecation import RemovedInWagtail17Warning from wagtail.utils.widgets import WidgetWithScript from wagtail.wagtailcore import hooks from wagtail.wagtailcore.models import Page @@ -135,24 +137,48 @@ 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") + warnings.warn( + 'The content_type argument for AdminPageChooser() is deprecated. Use the target_models argument instead', + category=RemovedInWagtail17Warning) + 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 + warnings.warn( + 'AdminPageChooser.target_content_types is deprecated. Use AdminPageChooser.target_models instead', + category=RemovedInWagtail17Warning) + 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: + base = self.target_models[0] + others = self.target_models[1:] + for cls in base.__mro__: + if all(cls in other.__mro__ for other in others): + return cls 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) @@ -171,22 +197,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')