Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Proxy page models #1736

Closed
wants to merge 2 commits into from
Closed

Conversation

kaedroho
Copy link
Contributor

This PR includes some tweaks to Wagtail core to allow users to define new page types using proxy models instead of multi-table inheritance. The implementation is currently only for demonstration purposes (not much time).

There's two use-cases I think this would be useful for

  • Defining new page types that extend another (which don't need model changes) without introducing a third level of MTI
  • Creating page types that don't use multi-table inheritance at all (I can see this becoming much more useful when we introduce an abstract base page class)

Currently (Wagtail 1.1), if you define a proxy page model. Wagtail would add another entry into the "add subpage" view, except it's an exact duplicate of the parent class.

proxy-pages

Implementation

To implement this, I've simply gone through wagtailcore/models.py and added for_concrete_model=False to each call to get_for_model. This fixes the issues with "add subpage" and allows pages to be created for these models.

By default, proxy models objects attribute returns all instances of their parent class, so I've added a ProxyPageManager which filters the pages to only include instances of the proxy model, emulating behaviour of multi-table inheritance. (this manager is currently forced on the proxy model, but I'd like to make this opt-in).

Example

Finally, here's the code I created to test this:

from django.db import models

from wagtail.wagtailcore.models import Page
from wagtail.wagtailadmin.edit_handlers import FieldPanel


class BlogEntryPage(Page):
    date = models.DateTimeField()
    body = models.TextField()

    content_panels = Page.content_panels + [
        FieldPanel('date'),
        FieldPanel('body'),
    ]


class AwesomeBlogEntryPage(BlogEntryPage):
    class Meta:
        proxy = True

Both content types can be created in the admin:

proxy_models_2

Pages appear to be two distinct types to the user:

proxy_models_3

And in the shell:

# Proxy models appear in their parent classes objects
>>> BlogEntryPage.objects.all()
[<BlogEntryPage: blog>, <BlogEntryPage: Awesome blog entry>]

# When using PageProxyManager, only instances of the proxy are included in the proxy models objects
>>> AwesomeBlogEntryPage.objects.all()
[<AwesomeBlogEntryPage: blog>]

Underneath, there is one database table for both models (they are told apart by the content_type field)

@@ -242,6 +242,11 @@ def specific(self):
return self.get_queryset().specific()


class ProxyPageManager(PageManager):
def get_queryset(self):
return super().get_queryset().type(self.model)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling .type(self.model) is not enough here because that will return all the objects for the concrete model. This does work however:

return super(ProxyPageManager, self).get_queryset().filter(content_type=ContentType.objects.get_for_model(self.model, for_concrete_model=False))

Or even PageQuerySet.type could take a for_concrete_models=False argument too, which would make it return proxy objects instead of concrete ones. And I wonder if returning proxy objects by default would make sense: I think that's what people would expect but might have some effects on some internal wagtail code (and it's not what django does by default either).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super().get_queryset() should return pages with the proxy model class and type(self.model) should filter them to only include items that are members of that proxy model.

But you're right, I missed for_concrete_models=False in the .type() implementation causing that type filter to select pages with the parent type (but still return instances of the proxy model).. Doh!

.type() just filters the queryset and cannot change the type. So for example, running Page.objects.type(BlogPage) will return a queryset of Page objects.

@kaedroho
Copy link
Contributor Author

kaedroho commented Dec 1, 2015

This could be used to simplify the page types rules with the root page. We could create a new RootPage model that is a proxy of Page (so no new table would be created). This would act as a marker for the "root" page in the tree. For example:

from wagtail.wagtailcore.models import Page, RootPage

class HomePage(Page):
    # Homepages can only be created at the root level
    parent_page_types = [RootPage]

@gasman
Copy link
Collaborator

gasman commented Dec 1, 2015

This could be used to simplify the page types rules with the root page. We could create a new RootPage model that is a proxy of Page (so no new table would be created).

I like this idea! This fits nicely with the is_creatable = False mechanism to stop the new page type from cluttering up the 'add subpage' list - and unlike tinkering with the initial 'welcome' page, it's something we can do neatly in a migration without causing problems with existing sites.

I wonder if we're subverting the purpose of proxy models a bit here - if they're meant to be wrappers that you can freely apply to any instance of the concrete model, rather than an object specifically identifying itself as an instance of the proxy model. Clearly the latter works, though, and it's almost certainly the more useful behaviour for Wagtail to adopt (because allowing page types to appear as multiple types simultaneously is bound to create bigger problems) - so happy to go with this.

@jhrr
Copy link

jhrr commented Jun 1, 2017

Hi all, any update on if/when this feature might land?

@kaedroho kaedroho changed the title [RFC] Proxy page models [WIP] Proxy page models Jun 16, 2017
@timonweb
Copy link
Contributor

Any update on this? What kind of help may be needed here?

@robslotboom
Copy link

robslotboom commented Mar 23, 2018

I’m playing with a proxy Page model in Wagtail 2.0.
To make it work I had to add the following code to the model class:

def __init__(self, *args, **kwargs):
    super().__init__(*args, **kwargs)
    if not self.id:
        content_type = ContentType.objects.get_for_model(self, for_concrete_model=False)
        self.content_type = content_type

@allcaps
Copy link
Member

allcaps commented Jun 29, 2018

If anyone likes to push this further. TODO:

  • Conflicts must be resolved
  • Allow users to opt-in to using this manager (see comment in the code)
  • Add tests
  • Add docs

@probabble
Copy link

I'd be interested in helping to move this forward, it would be so nice to specify different Python behavior for pages without having to create new DB entities...

@robslotboom
Copy link

@allcaps

Can you tell me where the code can be found?

@allcaps
Copy link
Member

allcaps commented Jun 30, 2018

@timonweb
Copy link
Contributor

Any update?

@ababic
Copy link
Contributor

ababic commented Dec 26, 2018

Hi all. I just wanted to highlight that using for_concrete_model=False actually results in the creation of a new ContentType, which might not be desired.

I don't think this is such an issue for Page models, because the Django permission system isn't being used, but this does create a bit of an odd situation, where Permission objects exist for the proxy model, but remain tied to the concrete model's ContentType.

@ababic
Copy link
Contributor

ababic commented Jan 2, 2019

@kaedroho / @gasman, in relation to my last comment, I've created a new issue (#4973) that aims to provide a more consistent approach for supporting proxy models throughout (as pages, snippets, or other models registered via contrib.modeladmin). Would appreciate your thoughts on that.

@ababic
Copy link
Contributor

ababic commented Jun 10, 2019

Superseded by #5202

@brylie
Copy link
Contributor

brylie commented Sep 8, 2021

We could create a new RootPage model that is a proxy of Page (so no new table would be created). This would act as a marker for the "root" page in the tree.

@kaedroho and @gasman, I've incorporated this idea into RFC 68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants