From 42e175392e1248184e3addeccc4081ebb62bb45b Mon Sep 17 00:00:00 2001 From: Tim Heap Date: Tue, 22 Jul 2014 17:11:58 +1000 Subject: [PATCH 1/9] Implement `Page.parent_page_types` Similar to `Page.subpage_types`, but restricts which pages can have a specific page type as a child. Useful for Blog posts being restricted to Blog list pages, or similar things. --- wagtail/wagtailadmin/views/pages.py | 2 +- wagtail/wagtailcore/models.py | 78 +++++++++++++++++++++-------- wagtail/wagtailcore/utils.py | 28 +++++++++++ 3 files changed, 85 insertions(+), 23 deletions(-) diff --git a/wagtail/wagtailadmin/views/pages.py b/wagtail/wagtailadmin/views/pages.py index a7212cc3e2..20ddead472 100644 --- a/wagtail/wagtailadmin/views/pages.py +++ b/wagtail/wagtailadmin/views/pages.py @@ -68,7 +68,7 @@ def add_subpage(request, parent_page_id): if not parent_page.permissions_for_user(request.user).can_add_subpage(): raise PermissionDenied - page_types = sorted(parent_page.clean_subpage_types(), key=lambda pagetype: pagetype.name.lower()) + page_types = sorted(parent_page.allowed_subpage_types(), key=lambda pagetype: pagetype.name.lower()) if len(page_types) == 1: # Only one page type is available - redirect straight to the create form rather than diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index 3db7a71a0e..9ce0061447 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -8,7 +8,7 @@ from six.moves.urllib.parse import urlparse from modelcluster.models import ClusterableModel, get_all_child_relations from django.db import models, connection, transaction -from django.db.models import get_model, Q +from django.db.models import Q from django.http import Http404 from django.core.cache import cache from django.core.handlers.wsgi import WSGIRequest @@ -26,7 +26,7 @@ from django.utils.encoding import python_2_unicode_compatible from treebeard.mp_tree import MP_Node -from wagtail.wagtailcore.utils import camelcase_to_underscore +from wagtail.wagtailcore.utils import camelcase_to_underscore, resolve_model_string from wagtail.wagtailcore.query import PageQuerySet from wagtail.wagtailcore.url_routing import RouteResult @@ -236,6 +236,7 @@ class PageBase(models.base.ModelBase): cls.ajax_template = None cls._clean_subpage_types = None # to be filled in on first call to cls.clean_subpage_types + cls._clean_parent_page_types = None # to be filled in on first call to cls.clean_parent_page_types if not dct.get('is_abstract'): # subclasses are only abstract if the subclass itself defines itself so @@ -513,36 +514,62 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed # if subpage_types is not specified on the Page class, allow all page types as subpages res = get_page_types() else: - res = [] - for page_type in cls.subpage_types: - if isinstance(page_type, string_types): - try: - app_label, model_name = page_type.split(".") - except ValueError: - # If we can't split, assume a model in current app - app_label = cls._meta.app_label - model_name = page_type - - model = get_model(app_label, model_name) - if model: - res.append(ContentType.objects.get_for_model(model)) - else: - raise NameError(_("name '{0}' (used in subpage_types list) is not defined.").format(page_type)) - - else: - # assume it's already a model class - res.append(ContentType.objects.get_for_model(page_type)) + try: + models = [resolve_model_string(model_string, cls._meta.app_label) + for model_string in subpage_types] + except (NameError,) as err: + raise NameError(err.args[0] + ' (used in subpage_types') + res = map(ContentType.objects.get_for_model, models) cls._clean_subpage_types = res return cls._clean_subpage_types + @classmethod + def clean_parent_page_types(cls): + """ + Returns the list of parent page types, with strings converted to + class objects where required + """ + if cls._clean_parent_page_types is None: + parent_page_types = getattr(cls, 'parent_page_types', None) + if parent_page_types is None: + # if parent_page_types is not specified on the Page class, allow all page types as subpages + res = get_page_types() + else: + try: + models = [resolve_model_string(model_string, cls._meta.app_label) + for model_string in parent_page_types] + except NameError as err: + raise NameError(err.args[0] + ' (used in parent_page_types)') + res = map(ContentType.objects.get_for_model, models) + + cls._clean_parent_page_types = res + + return cls._clean_parent_page_types + @classmethod def allowed_parent_page_types(cls): """ Returns the list of page types that this page type can be a subpage of """ - return [ct for ct in get_page_types() if cls in ct.model_class().clean_subpage_types()] + cls_ct = ContentType.objects.get_for_model(cls) + return [ct for ct in cls.clean_parent_page_types() + if cls_ct in ct.model_class().clean_subpage_types()] + + @classmethod + def allowed_subpage_types(cls): + """ + Returns the list of page types that this page type can be a subpage of + """ + # Special case the 'Page' class, such as the Root page or Home page - + # otherwise you can not add initial pages when setting up a site + if cls == Page: + return get_page_types() + + cls_ct = ContentType.objects.get_for_model(cls) + return [ct for ct in cls.clean_subpage_types() + if cls_ct in ct.model_class().clean_parent_page_types()] @classmethod def allowed_parent_pages(cls): @@ -551,6 +578,13 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed """ return Page.objects.filter(content_type__in=cls.allowed_parent_page_types()) + @classmethod + def allowed_subpages(cls): + """ + Returns the list of pages that this page type can be a parent page of + """ + return Page.objects.filter(content_type__in=cls.allowed_subpage_types()) + @classmethod def get_verbose_name(cls): """ diff --git a/wagtail/wagtailcore/utils.py b/wagtail/wagtailcore/utils.py index c5ad7ea8da..37691f547e 100644 --- a/wagtail/wagtailcore/utils.py +++ b/wagtail/wagtailcore/utils.py @@ -1,6 +1,34 @@ import re +from django.db.models import Model, get_model +from django.utils.translation import ugettext_lazy as _ +from six import string_types def camelcase_to_underscore(str): # http://djangosnippets.org/snippets/585/ return re.sub('(((?<=[a-z])[A-Z])|([A-Z](?![A-Z]|$)))', '_\\1', str).lower().strip('_') + + +def resolve_model_string(model_string, default_app): + """ + Resolve an 'app_label.model_name' string in to an actual model class. + If a model class is passed in, just return that. + """ + if isinstance(model_string, string_types): + try: + app_label, model_name = model_string.split(".") + except ValueError: + # If we can't split, assume a model in current app + app_label = default_app + model_name = model_string + + model = get_model(app_label, model_name) + if not model: + raise NameError(_("name '{0}' is not defined.").format(model_string)) + return model + + elif issubclass(model_string, Model): + return model + + else: + raise ValueError(_("Can not resolve '{0!r}' in to a model").format(model_string)) From 4bcacfabf24df6c52f5f2b1dfeb8fd1013d94883 Mon Sep 17 00:00:00 2001 From: Tim Heap Date: Wed, 23 Jul 2014 12:25:07 +1000 Subject: [PATCH 2/9] Add tests for new `Page.parent_page_types` setting --- wagtail/tests/models.py | 12 ++++++- .../wagtailadmin/tests/test_pages_views.py | 31 +++++++++++++------ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/wagtail/tests/models.py b/wagtail/tests/models.py index c708c452b8..66709b8978 100644 --- a/wagtail/tests/models.py +++ b/wagtail/tests/models.py @@ -376,7 +376,9 @@ class ZuluSnippet(models.Model): class StandardIndex(Page): - pass + """ Index for the site, not allowed to be placed anywhere """ + parent_page_types = [] + StandardIndex.content_panels = [ FieldPanel('title', classname="full title"), @@ -387,14 +389,22 @@ StandardIndex.content_panels = [ class StandardChild(Page): pass + class BusinessIndex(Page): + """ Can be placed anywhere, can only have Business children """ subpage_types = ['tests.BusinessChild', 'tests.BusinessSubIndex'] + class BusinessSubIndex(Page): + """ Can be placed under BusinessIndex, and have BusinessChild children """ subpage_types = ['tests.BusinessChild'] + parent_page_types = ['tests.BusinessIndex'] + class BusinessChild(Page): + """ Can only be placed under Business indexes, no children allowed """ subpage_types = [] + parent_page_types = ['tests.BusinessIndex', 'tests.BusinessSubIndex'] class SearchTest(models.Model, index.Indexed): diff --git a/wagtail/wagtailadmin/tests/test_pages_views.py b/wagtail/wagtailadmin/tests/test_pages_views.py index ab231cc7ea..eb7668a6d9 100644 --- a/wagtail/wagtailadmin/tests/test_pages_views.py +++ b/wagtail/wagtailadmin/tests/test_pages_views.py @@ -8,7 +8,11 @@ from django.core import mail from django.core.paginator import Paginator from django.utils import timezone -from wagtail.tests.models import SimplePage, EventPage, EventPageCarouselItem, StandardIndex, BusinessIndex, BusinessChild, BusinessSubIndex, TaggedPage, Advert, AdvertPlacement +from wagtail.tests.models import ( + SimplePage, EventPage, EventPageCarouselItem, + StandardIndex, StandardChild, + BusinessIndex, BusinessChild, BusinessSubIndex, + TaggedPage, Advert, AdvertPlacement) from wagtail.tests.utils import unittest, WagtailTestUtils from wagtail.wagtailcore.models import Page, PageRevision from wagtail.wagtailcore.signals import page_published, page_unpublished @@ -1483,11 +1487,14 @@ class TestSubpageBusinessRules(TestCase, WagtailTestUtils): self.assertEqual(response.status_code, 200) self.assertContains(response, add_subpage_url) - # add_subpage should give us the full set of page types to choose + # add_subpage should give us choices of StandardChild, and BusinessIndex. + # BusinessSubIndex and BusinessChild are not allowed response = self.client.get(add_subpage_url) self.assertEqual(response.status_code, 200) - self.assertContains(response, 'Standard Child') - self.assertContains(response, 'Business Child') + self.assertContains(response, StandardChild.get_verbose_name()) + self.assertContains(response, BusinessIndex.get_verbose_name()) + self.assertNotContains(response, BusinessSubIndex.get_verbose_name()) + self.assertNotContains(response, BusinessChild.get_verbose_name()) def test_business_subpage(self): add_subpage_url = reverse('wagtailadmin_pages_add_subpage', args=(self.business_index.id, )) @@ -1500,8 +1507,10 @@ class TestSubpageBusinessRules(TestCase, WagtailTestUtils): # add_subpage should give us a cut-down set of page types to choose response = self.client.get(add_subpage_url) self.assertEqual(response.status_code, 200) - self.assertNotContains(response, 'Standard Child') - self.assertContains(response, 'Business Child') + self.assertNotContains(response, StandardIndex.get_verbose_name()) + self.assertNotContains(response, StandardChild.get_verbose_name()) + self.assertContains(response, BusinessSubIndex.get_verbose_name()) + self.assertContains(response, BusinessChild.get_verbose_name()) def test_business_child_subpage(self): add_subpage_url = reverse('wagtailadmin_pages_add_subpage', args=(self.business_child.id, )) @@ -1516,12 +1525,16 @@ class TestSubpageBusinessRules(TestCase, WagtailTestUtils): self.assertEqual(response.status_code, 403) def test_cannot_add_invalid_subpage_type(self): - # cannot add SimplePage as a child of BusinessIndex, as SimplePage is not present in subpage_types - response = self.client.get(reverse('wagtailadmin_pages_create', args=('tests', 'simplepage', self.business_index.id))) + # cannot add StandardChild as a child of BusinessIndex, as StandardChild is not present in subpage_types + response = self.client.get(reverse('wagtailadmin_pages_create', args=('tests', 'standardchild', self.business_index.id))) self.assertEqual(response.status_code, 403) # likewise for BusinessChild which has an empty subpage_types list - response = self.client.get(reverse('wagtailadmin_pages_create', args=('tests', 'simplepage', self.business_child.id))) + response = self.client.get(reverse('wagtailadmin_pages_create', args=('tests', 'standardchild', self.business_child.id))) + self.assertEqual(response.status_code, 403) + + # cannot add BusinessChild to StandardIndex, as BusinessChild restricts is parent page types + response = self.client.get(reverse('wagtailadmin_pages_create', args=('tests', 'businesschild', self.standard_index.id))) self.assertEqual(response.status_code, 403) # but we can add a BusinessChild to BusinessIndex From 9aae3a1a23062ae763c66ef413746d9fd14e59dd Mon Sep 17 00:00:00 2001 From: Tim Heap Date: Wed, 23 Jul 2014 12:29:00 +1000 Subject: [PATCH 3/9] Restrict child page types when creating them The list of allowed child page types was restricted, but if you could guess the URL to create a disallowed page type, nothing would stop you creating it. Fixes a failing test case. --- wagtail/wagtailadmin/views/pages.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wagtail/wagtailadmin/views/pages.py b/wagtail/wagtailadmin/views/pages.py index 20ddead472..53810fa3cc 100644 --- a/wagtail/wagtailadmin/views/pages.py +++ b/wagtail/wagtailadmin/views/pages.py @@ -136,7 +136,7 @@ def create(request, content_type_app_name, content_type_model_name, parent_page_ raise Http404 # page must be in the list of allowed subpage types for this parent ID - if content_type not in parent_page.clean_subpage_types(): + if content_type not in parent_page.allowed_subpage_types(): raise PermissionDenied page = page_class(owner=request.user) From 7b89f283db0932c98031a7caf875b8be52b48dd5 Mon Sep 17 00:00:00 2001 From: Tim Heap Date: Wed, 23 Jul 2014 16:45:12 +1000 Subject: [PATCH 4/9] Force `Page._clean_sub/parent_page_types` to be a list --- wagtail/wagtailcore/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index 9ce0061447..65d2930d27 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -519,7 +519,7 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed for model_string in subpage_types] except (NameError,) as err: raise NameError(err.args[0] + ' (used in subpage_types') - res = map(ContentType.objects.get_for_model, models) + res = list(map(ContentType.objects.get_for_model, models)) cls._clean_subpage_types = res @@ -542,7 +542,7 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed for model_string in parent_page_types] except NameError as err: raise NameError(err.args[0] + ' (used in parent_page_types)') - res = map(ContentType.objects.get_for_model, models) + res = list(map(ContentType.objects.get_for_model, models)) cls._clean_parent_page_types = res From 96e907989080bed7d35544dee6376a3a3fc277f4 Mon Sep 17 00:00:00 2001 From: Tim Heap Date: Thu, 24 Jul 2014 10:40:39 +1000 Subject: [PATCH 5/9] Add `Page.parent_page_types` to the docs --- docs/core_components/pages/theory.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/core_components/pages/theory.rst b/docs/core_components/pages/theory.rst index bcea079bb4..f4929f2926 100644 --- a/docs/core_components/pages/theory.rst +++ b/docs/core_components/pages/theory.rst @@ -52,7 +52,7 @@ A Parent node could provide its own function returning its descendant objects. return events -This example makes sure to limit the returned objects to pieces of content which make sense, specifically ones which have been published through Wagtail's admin interface (``live()``) and are children of this node (``descendant_of(self)``). By setting a ``subpage_types`` class property in your model, you can specify which models are allowed to be set as children, but Wagtail will allow any ``Page``-derived model by default. Regardless, it's smart for a parent model to provide an index filtered to make sense. +This example makes sure to limit the returned objects to pieces of content which make sense, specifically ones which have been published through Wagtail's admin interface (``live()``) and are children of this node (``descendant_of(self)``). By setting a ``subpage_types`` class property in your model, you can specify which models are allowed to be set as children, and by settings a ``parent_page_types`` class property, you can specify which models are allowed to parent certain children. Wagtail will allow any ``Page``-derived model by default. Regardless, it's smart for a parent model to provide an index filtered to make sense. Leaves @@ -71,7 +71,7 @@ The model for the leaf could provide a function that traverses the tree in the o # Find closest ancestor which is an event index return self.get_ancestors().type(EventIndexPage).last() -If defined, ``subpage_types`` will also limit the parent models allowed to contain a leaf. If not, Wagtail will allow any combination of parents and leafs to be associated in the Wagtail tree. Like with index pages, it's a good idea to make sure that the index is actually of the expected model to contain the leaf. +If defined, ``subpage_types`` and ``parent_page_types`` will also limit the parent models allowed to contain a leaf. If not, Wagtail will allow any combination of parents and leafs to be associated in the Wagtail tree. Like with index pages, it's a good idea to make sure that the index is actually of the expected model to contain the leaf. Other Relationships From 18909c834f11e5a1682c56088bd40daffb49aa74 Mon Sep 17 00:00:00 2001 From: Tim Heap Date: Thu, 24 Jul 2014 10:49:30 +1000 Subject: [PATCH 6/9] Replace all calls to clean_subpage_types with allowed_subpage_types `clean_subpage_types` should not be part of the public API any more, and `allowed_subpage_types` should replace it in all instances. --- wagtail/wagtailcore/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index 65d2930d27..dd1bdb52c0 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -1057,7 +1057,7 @@ class PagePermissionTester(object): def can_add_subpage(self): if not self.user.is_active: return False - if not self.page.specific_class.clean_subpage_types(): # this page model has an empty subpage_types list, so no subpages are allowed + if not self.page.specific_class.allowed_subpage_types(): # this page model has an empty subpage_types list, so no subpages are allowed return False return self.user.is_superuser or ('add' in self.permissions) @@ -1121,7 +1121,7 @@ class PagePermissionTester(object): """ if not self.user.is_active: return False - if not self.page.specific_class.clean_subpage_types(): # this page model has an empty subpage_types list, so no subpages are allowed + if not self.page.specific_class.allowed_subpage_types(): # this page model has an empty subpage_types list, so no subpages are allowed return False return self.user.is_superuser or ('publish' in self.permissions) From 999b052fe5b674f2928b67cb91762b16e4d5b137 Mon Sep 17 00:00:00 2001 From: Tim Heap Date: Fri, 22 Aug 2014 09:18:00 +1000 Subject: [PATCH 7/9] Fix indentation of docstings --- wagtail/wagtailcore/models.py | 34 +++++++++++++++++----------------- wagtail/wagtailcore/utils.py | 4 ++-- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index dd1bdb52c0..1fa436014a 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -58,15 +58,15 @@ class Site(models.Model): @staticmethod def find_for_request(request): """ - Find the site object responsible for responding to this HTTP - request object. Try: - - unique hostname first - - then hostname and port - - if there is no matching hostname at all, or no matching - hostname:port combination, fall back to the unique default site, - or raise an exception - NB this means that high-numbered ports on an extant hostname may - still be routed to a different hostname which is set as the default + Find the site object responsible for responding to this HTTP + request object. Try: + - unique hostname first + - then hostname and port + - if there is no matching hostname at all, or no matching + hostname:port combination, fall back to the unique default site, + or raise an exception + NB this means that high-numbered ports on an extant hostname may + still be routed to a different hostname which is set as the default """ try: hostname = request.META['HTTP_HOST'].split(':')[0] # KeyError here goes to the final except clause @@ -505,8 +505,8 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed @classmethod def clean_subpage_types(cls): """ - Returns the list of subpage types, with strings converted to class objects - where required + Returns the list of subpage types, with strings converted to class objects + where required """ if cls._clean_subpage_types is None: subpage_types = getattr(cls, 'subpage_types', None) @@ -528,8 +528,8 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed @classmethod def clean_parent_page_types(cls): """ - Returns the list of parent page types, with strings converted to - class objects where required + Returns the list of parent page types, with strings converted to class + objects where required """ if cls._clean_parent_page_types is None: parent_page_types = getattr(cls, 'parent_page_types', None) @@ -551,7 +551,7 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed @classmethod def allowed_parent_page_types(cls): """ - Returns the list of page types that this page type can be a subpage of + Returns the list of page types that this page type can be a subpage of """ cls_ct = ContentType.objects.get_for_model(cls) return [ct for ct in cls.clean_parent_page_types() @@ -560,7 +560,7 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed @classmethod def allowed_subpage_types(cls): """ - Returns the list of page types that this page type can be a subpage of + Returns the list of page types that this page type can be a subpage of """ # Special case the 'Page' class, such as the Root page or Home page - # otherwise you can not add initial pages when setting up a site @@ -574,14 +574,14 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed @classmethod def allowed_parent_pages(cls): """ - Returns the list of pages that this page type can be a subpage of + Returns the list of pages that this page type can be a subpage of """ return Page.objects.filter(content_type__in=cls.allowed_parent_page_types()) @classmethod def allowed_subpages(cls): """ - Returns the list of pages that this page type can be a parent page of + Returns the list of pages that this page type can be a parent page of """ return Page.objects.filter(content_type__in=cls.allowed_subpage_types()) diff --git a/wagtail/wagtailcore/utils.py b/wagtail/wagtailcore/utils.py index 37691f547e..52913bedac 100644 --- a/wagtail/wagtailcore/utils.py +++ b/wagtail/wagtailcore/utils.py @@ -11,8 +11,8 @@ def camelcase_to_underscore(str): def resolve_model_string(model_string, default_app): """ - Resolve an 'app_label.model_name' string in to an actual model class. - If a model class is passed in, just return that. + Resolve an 'app_label.model_name' string in to an actual model class. + If a model class is passed in, just return that. """ if isinstance(model_string, string_types): try: From 7921abe924ed1c97ecf7df171c59f15983155b0e Mon Sep 17 00:00:00 2001 From: Tim Heap Date: Fri, 22 Aug 2014 10:06:09 +1000 Subject: [PATCH 8/9] Fix errors raised with `resolve_model_string` It now raises `ValueError` for a badly formatted string, and `NameError` if it can not find the model. Error messages are not translated. Edit handlers now use the function. --- wagtail/wagtailadmin/edit_handlers.py | 29 +++++++++------------------ wagtail/wagtailcore/models.py | 10 +++++---- wagtail/wagtailcore/utils.py | 20 ++++++++++-------- 3 files changed, 28 insertions(+), 31 deletions(-) diff --git a/wagtail/wagtailadmin/edit_handlers.py b/wagtail/wagtailadmin/edit_handlers.py index 3e463523d4..1c5ec6a098 100644 --- a/wagtail/wagtailadmin/edit_handlers.py +++ b/wagtail/wagtailadmin/edit_handlers.py @@ -19,8 +19,8 @@ from django.core.urlresolvers import reverse from django.utils.translation import ugettext_lazy from wagtail.wagtailcore.models import Page -from wagtail.wagtailcore.utils import camelcase_to_underscore from wagtail.wagtailcore.fields import RichTextArea +from wagtail.wagtailcore.utils import camelcase_to_underscore, resolve_model_string FORM_FIELD_OVERRIDES = {} @@ -483,25 +483,16 @@ class BasePageChooserPanel(BaseChooserPanel): def target_content_type(cls): if cls._target_content_type is None: if cls.page_type: - if isinstance(cls.page_type, string_types): - # translate the passed model name into an actual model class - from django.db.models import get_model - try: - app_label, model_name = cls.page_type.split('.') - except ValueError: - raise ImproperlyConfigured("The page_type passed to PageChooserPanel must be of the form 'app_label.model_name'") + try: + model = resolve_model_string(cls.page_type) + except NameError: + raise ImproperlyConfigured("{0}.page_type must be of the form 'app_label.model_name', given {1!r}".format( + cls.__name__, cls.page_type)) + except ValueError: + raise ImproperlyConfigured("{0}.page_type refers to model {1!r} that has not been installed".format( + cls.__name__, cls.page_type)) - try: - page_type = get_model(app_label, model_name) - except LookupError: - page_type = None - - if page_type is None: - raise ImproperlyConfigured("PageChooserPanel refers to model '%s' that has not been installed" % cls.page_type) - else: - page_type = cls.page_type - - cls._target_content_type = ContentType.objects.get_for_model(page_type) + cls._target_content_type = ContentType.objects.get_for_model(model) else: # TODO: infer the content type by introspection on the foreign key cls._target_content_type = ContentType.objects.get_by_natural_key('wagtailcore', 'page') diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index 1fa436014a..6b13c61de6 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -20,7 +20,7 @@ from django.conf import settings from django.template.response import TemplateResponse from django.utils import timezone from django.utils.translation import ugettext_lazy as _ -from django.core.exceptions import ValidationError +from django.core.exceptions import ValidationError, ImproperlyConfigured from django.utils.functional import cached_property from django.utils.encoding import python_2_unicode_compatible @@ -517,8 +517,9 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed try: models = [resolve_model_string(model_string, cls._meta.app_label) for model_string in subpage_types] - except (NameError,) as err: - raise NameError(err.args[0] + ' (used in subpage_types') + except NameError as err: + raise ImproperlyConfigured("{0}.subpage_types must be a list of 'app_label.model_name' strings, given {1!r}".format( + cls.__name__, err.args[1])) res = list(map(ContentType.objects.get_for_model, models)) cls._clean_subpage_types = res @@ -541,7 +542,8 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed models = [resolve_model_string(model_string, cls._meta.app_label) for model_string in parent_page_types] except NameError as err: - raise NameError(err.args[0] + ' (used in parent_page_types)') + raise ImproperlyConfigured("{0}.parent_page_types must be a list of 'app_label.model_name' strings, given {1!r}".format( + cls.__name__, err.args[1])) res = list(map(ContentType.objects.get_for_model, models)) cls._clean_parent_page_types = res diff --git a/wagtail/wagtailcore/utils.py b/wagtail/wagtailcore/utils.py index 52913bedac..d03d5ae756 100644 --- a/wagtail/wagtailcore/utils.py +++ b/wagtail/wagtailcore/utils.py @@ -1,6 +1,5 @@ import re from django.db.models import Model, get_model -from django.utils.translation import ugettext_lazy as _ from six import string_types @@ -9,7 +8,7 @@ def camelcase_to_underscore(str): return re.sub('(((?<=[a-z])[A-Z])|([A-Z](?![A-Z]|$)))', '_\\1', str).lower().strip('_') -def resolve_model_string(model_string, default_app): +def resolve_model_string(model_string, default_app=None): """ Resolve an 'app_label.model_name' string in to an actual model class. If a model class is passed in, just return that. @@ -18,17 +17,22 @@ def resolve_model_string(model_string, default_app): try: app_label, model_name = model_string.split(".") except ValueError: - # If we can't split, assume a model in current app - app_label = default_app - model_name = model_string + if default_app is not None: + # If we can't split, assume a model in current app + app_label = default_app + model_name = model_string + else: + raise ValueError("Can not resolve {0!r} in to a model. Model names " + "should be in the form app_label.model_name".format( + model_string), model_string) model = get_model(app_label, model_name) if not model: - raise NameError(_("name '{0}' is not defined.").format(model_string)) + raise NameError("Can not resolve {0!r} in to a model".format(model_string), model_string) return model - elif issubclass(model_string, Model): + elif model_string is not None and issubclass(model_string, Model): return model else: - raise ValueError(_("Can not resolve '{0!r}' in to a model").format(model_string)) + raise NameError("Can not resolve {0!r} in to a model".format(model_string), model_string) From b8b79cd151f8c01bf1137e9ab148d62dffd2f2ea Mon Sep 17 00:00:00 2001 From: Tim Heap Date: Fri, 5 Sep 2014 09:59:53 +1000 Subject: [PATCH 9/9] `resolve_model_string` now raises `LookupError` Django 1.7 raises a `LookupError` when looking for a model that does not exist. This brings the code in to line with Django. The `LookupError` from Django is allowed to propagate in 1.7, and a `LookupError` is now raised instead of a `NameError` in 1.6. All code using `resolve_model_string` has been changed to catch the new errors. --- wagtail/wagtailadmin/edit_handlers.py | 2 +- wagtail/wagtailcore/models.py | 4 ++-- wagtail/wagtailcore/utils.py | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/wagtail/wagtailadmin/edit_handlers.py b/wagtail/wagtailadmin/edit_handlers.py index 1c5ec6a098..137bb207a4 100644 --- a/wagtail/wagtailadmin/edit_handlers.py +++ b/wagtail/wagtailadmin/edit_handlers.py @@ -485,7 +485,7 @@ class BasePageChooserPanel(BaseChooserPanel): if cls.page_type: try: model = resolve_model_string(cls.page_type) - except NameError: + except LookupError: raise ImproperlyConfigured("{0}.page_type must be of the form 'app_label.model_name', given {1!r}".format( cls.__name__, cls.page_type)) except ValueError: diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index 6b13c61de6..2c5bcaa158 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -517,7 +517,7 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed try: models = [resolve_model_string(model_string, cls._meta.app_label) for model_string in subpage_types] - except NameError as err: + except LookupError as err: raise ImproperlyConfigured("{0}.subpage_types must be a list of 'app_label.model_name' strings, given {1!r}".format( cls.__name__, err.args[1])) res = list(map(ContentType.objects.get_for_model, models)) @@ -541,7 +541,7 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed try: models = [resolve_model_string(model_string, cls._meta.app_label) for model_string in parent_page_types] - except NameError as err: + except LookupError as err: raise ImproperlyConfigured("{0}.parent_page_types must be a list of 'app_label.model_name' strings, given {1!r}".format( cls.__name__, err.args[1])) res = list(map(ContentType.objects.get_for_model, models)) diff --git a/wagtail/wagtailcore/utils.py b/wagtail/wagtailcore/utils.py index d03d5ae756..403c79bf38 100644 --- a/wagtail/wagtailcore/utils.py +++ b/wagtail/wagtailcore/utils.py @@ -28,11 +28,11 @@ def resolve_model_string(model_string, default_app=None): model = get_model(app_label, model_name) if not model: - raise NameError("Can not resolve {0!r} in to a model".format(model_string), model_string) + raise LookupError("Can not resolve {0!r} in to a model".format(model_string), model_string) return model elif model_string is not None and issubclass(model_string, Model): return model else: - raise NameError("Can not resolve {0!r} in to a model".format(model_string), model_string) + raise LookupError("Can not resolve {0!r} in to a model".format(model_string), model_string)