From 42e175392e1248184e3addeccc4081ebb62bb45b Mon Sep 17 00:00:00 2001 From: Tim Heap Date: Tue, 22 Jul 2014 17:11:58 +1000 Subject: [PATCH 01/15] 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 a7212cc3e..20ddead47 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 3db7a71a0..9ce006144 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 c5ad7ea8d..37691f547 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 02/15] 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 c708c452b..66709b897 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 ab231cc7e..eb7668a6d 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 03/15] 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 20ddead47..53810fa3c 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 04/15] 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 9ce006144..65d2930d2 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 05/15] 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 bcea079bb..f4929f292 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 06/15] 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 65d2930d2..dd1bdb52c 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 07/15] 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 dd1bdb52c..1fa436014 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 37691f547..52913beda 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 08/15] 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 3e463523d..1c5ec6a09 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 1fa436014..6b13c61de 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 52913beda..d03d5ae75 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 09/15] `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 1c5ec6a09..137bb207a 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 6b13c61de..2c5bcaa15 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 d03d5ae75..403c79bf3 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) From c68dcff7bc2c07b91ec7b141bd70e896276fc5da Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Tue, 16 Sep 2014 11:55:05 +0100 Subject: [PATCH 10/15] copyediting on parent_page_types documentation --- docs/core_components/pages/theory.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/core_components/pages/theory.rst b/docs/core_components/pages/theory.rst index f4929f292..639ee34f2 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, 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. +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 setting a ``parent_page_types`` class property, you can specify which models are allowed to be parents of this page model. 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 From f28f7f92c2eb2c156c57d2f9ddcf4fcadd76f010 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Tue, 16 Sep 2014 11:55:40 +0100 Subject: [PATCH 11/15] in to -> into --- wagtail/wagtailcore/utils.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/wagtail/wagtailcore/utils.py b/wagtail/wagtailcore/utils.py index 403c79bf3..dcd3cf6d5 100644 --- a/wagtail/wagtailcore/utils.py +++ b/wagtail/wagtailcore/utils.py @@ -10,7 +10,7 @@ def camelcase_to_underscore(str): def resolve_model_string(model_string, default_app=None): """ - Resolve an 'app_label.model_name' string in to an actual model class. + Resolve an 'app_label.model_name' string into an actual model class. If a model class is passed in, just return that. """ if isinstance(model_string, string_types): @@ -22,17 +22,17 @@ def resolve_model_string(model_string, default_app=None): app_label = default_app model_name = model_string else: - raise ValueError("Can not resolve {0!r} in to a model. Model names " + raise ValueError("Can not resolve {0!r} into 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 LookupError("Can not resolve {0!r} in to a model".format(model_string), model_string) + raise LookupError("Can not resolve {0!r} into a model".format(model_string), model_string) return model elif model_string is not None and issubclass(model_string, Model): return model else: - raise LookupError("Can not resolve {0!r} in to a model".format(model_string), model_string) + raise LookupError("Can not resolve {0!r} into a model".format(model_string), model_string) From 4398b64282e4b688a365da65068eb172b42d9674 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Tue, 16 Sep 2014 11:57:03 +0100 Subject: [PATCH 12/15] remove allowed_parent_pages / allowed_subpages. allowed_parent_pages was a (broken) relic of the aborted no-tree add-page interface where you selected page type first and then specify where to put it. In that context, allowed_subpages doesn't make sense because the page you're about to add doesn't have subpages... --- wagtail/wagtailcore/models.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index e9afaddea..126c9c923 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -573,20 +573,6 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed 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): - """ - 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 - """ - return Page.objects.filter(content_type__in=cls.allowed_subpage_types()) - @classmethod def get_verbose_name(cls): """ From b4097dce1d2942c1ae5dcfa53861578ede2d06a5 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Tue, 16 Sep 2014 15:38:28 +0100 Subject: [PATCH 13/15] add unit tests for allowed_subpage_types and allowed_parent_page_types --- wagtail/wagtailcore/tests/test_page_model.py | 38 +++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/wagtail/wagtailcore/tests/test_page_model.py b/wagtail/wagtailcore/tests/test_page_model.py index 5ebb9de62..804b2c4e1 100644 --- a/wagtail/wagtailcore/tests/test_page_model.py +++ b/wagtail/wagtailcore/tests/test_page_model.py @@ -6,9 +6,10 @@ import pytz from django.test import TestCase, Client from django.test.utils import override_settings from django.http import HttpRequest, Http404 +from django.contrib.contenttypes.models import ContentType from wagtail.wagtailcore.models import Page, Site -from wagtail.tests.models import EventPage, EventIndex, SimplePage, PageWithOldStyleRouteMethod +from wagtail.tests.models import EventPage, EventIndex, SimplePage, PageWithOldStyleRouteMethod, BusinessIndex, BusinessSubIndex, BusinessChild, StandardIndex class TestSiteRouting(TestCase): @@ -517,3 +518,38 @@ class TestCopyPage(TestCase): # Check that the revisions weren't removed from old page self.assertEqual(old_christmas_event.specific.revisions.count(), 1, "Revisions were removed from the original page") + + +class TestSubpageTypeBusinessRules(TestCase): + def test_allowed_subpage_types(self): + # SimplePage does not define any restrictions on subpage types + # SimplePage is a valid subpage of SimplePage + self.assertIn(ContentType.objects.get_for_model(SimplePage), SimplePage.allowed_subpage_types()) + # BusinessIndex is a valid subpage of SimplePage + self.assertIn(ContentType.objects.get_for_model(BusinessIndex), SimplePage.allowed_subpage_types()) + # BusinessSubIndex is not valid, because it explicitly omits SimplePage from parent_page_types + self.assertNotIn(ContentType.objects.get_for_model(BusinessSubIndex), SimplePage.allowed_subpage_types()) + + # BusinessChild has an empty subpage_types list, so does not allow anything + self.assertNotIn(ContentType.objects.get_for_model(SimplePage), BusinessChild.allowed_subpage_types()) + self.assertNotIn(ContentType.objects.get_for_model(BusinessIndex), BusinessChild.allowed_subpage_types()) + self.assertNotIn(ContentType.objects.get_for_model(BusinessSubIndex), BusinessChild.allowed_subpage_types()) + + # BusinessSubIndex only allows BusinessChild as subpage type + self.assertNotIn(ContentType.objects.get_for_model(SimplePage), BusinessSubIndex.allowed_subpage_types()) + self.assertIn(ContentType.objects.get_for_model(BusinessChild), BusinessSubIndex.allowed_subpage_types()) + + def test_allowed_parent_page_types(self): + # SimplePage does not define any restrictions on parent page types + # SimplePage is a valid parent page of SimplePage + self.assertIn(ContentType.objects.get_for_model(SimplePage), SimplePage.allowed_parent_page_types()) + # BusinessChild cannot be a parent of anything + self.assertNotIn(ContentType.objects.get_for_model(BusinessChild), SimplePage.allowed_parent_page_types()) + + # StandardIndex does not allow anything as a parent + self.assertNotIn(ContentType.objects.get_for_model(SimplePage), StandardIndex.allowed_parent_page_types()) + self.assertNotIn(ContentType.objects.get_for_model(StandardIndex), StandardIndex.allowed_parent_page_types()) + + # BusinessSubIndex only allows BusinessIndex as a parent + self.assertNotIn(ContentType.objects.get_for_model(SimplePage), BusinessSubIndex.allowed_parent_page_types()) + self.assertIn(ContentType.objects.get_for_model(BusinessIndex), BusinessSubIndex.allowed_parent_page_types()) From fa558b149d5118ff4f4a93325119405d91882403 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Tue, 16 Sep 2014 15:44:42 +0100 Subject: [PATCH 14/15] Fix ability to pass a class object in subpage_types / parent_page_types --- wagtail/tests/models.py | 2 +- wagtail/wagtailcore/utils.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/wagtail/tests/models.py b/wagtail/tests/models.py index 66709b897..53a9b2796 100644 --- a/wagtail/tests/models.py +++ b/wagtail/tests/models.py @@ -404,7 +404,7 @@ class BusinessSubIndex(Page): class BusinessChild(Page): """ Can only be placed under Business indexes, no children allowed """ subpage_types = [] - parent_page_types = ['tests.BusinessIndex', 'tests.BusinessSubIndex'] + parent_page_types = ['tests.BusinessIndex', BusinessSubIndex] class SearchTest(models.Model, index.Indexed): diff --git a/wagtail/wagtailcore/utils.py b/wagtail/wagtailcore/utils.py index dcd3cf6d5..1880684bc 100644 --- a/wagtail/wagtailcore/utils.py +++ b/wagtail/wagtailcore/utils.py @@ -31,8 +31,8 @@ def resolve_model_string(model_string, default_app=None): raise LookupError("Can not resolve {0!r} into a model".format(model_string), model_string) return model - elif model_string is not None and issubclass(model_string, Model): - return model + elif isinstance(model_string, type) and issubclass(model_string, Model): + return model_string else: raise LookupError("Can not resolve {0!r} into a model".format(model_string), model_string) From 4ed76af781168b1ad966b4d39bd9d758228787b6 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Tue, 16 Sep 2014 15:47:40 +0100 Subject: [PATCH 15/15] release note for #491 --- CHANGELOG.txt | 1 + docs/releases/0.7.rst | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 6c748f45a..d30c4bc65 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -6,6 +6,7 @@ Changelog * Added interface for choosing focal point on images * Removed 'content_type' template filter from the project template, as the same thing can be accomplished with self.get_verbose_name|slugify * Page copy operations now also copy the page revision history + * Page models now support a 'parent_page_types' property in addition to 'subpage types', to restrict the types of page they can be created under 0.6 (11.09.2014) ~~~~~~~~~~~~~~~~ diff --git a/docs/releases/0.7.rst b/docs/releases/0.7.rst index 42eea8845..19b89216a 100644 --- a/docs/releases/0.7.rst +++ b/docs/releases/0.7.rst @@ -18,8 +18,8 @@ Minor features ~~~~~~~~~~~~~~ * The ``content_type`` template filter has been removed from the project template, as the same thing can be accomplished with ``self.get_verbose_name|slugify``. - * Page copy operations now also copy the page revision history - + * Page copy operations now also copy the page revision history. + * Page models now support a ``parent_page_types`` property in addition to ``subpage types``, to restrict the types of page they can be created under. Bug fixes ~~~~~~~~~