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

RFC 75: Live-only specific models #75

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gasman
Copy link
Contributor

@gasman gasman commented Feb 3, 2022

Rendered

This RFC proposes a change to the database representation of unpublished pages, where the database record for the specific page model is omitted. This is a pre-requisite for auto-saving (if we don't want the limitation of only being able to auto-save page revisions that completely pass validation).

Copy link
Contributor

@kaedroho kaedroho left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed RFC!

I think this is a good approach, and the mitigations sound sensible to me. I've added a question but I am happy with this either way.


## Model-level operations

In the proposed new model, non-live pages (including ones submitted for moderation, or awaiting go-live) will be represented as `wagtailcore_page` records with `live` set to False and `content_type` set to `BlogPage`, but with no corresponding `blog_blogpage` record. `wagtailcore_pagerevision` will track changes to the page content as it does now.
Copy link
Contributor

Choose a reason for hiding this comment

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

but with no corresponding blog_blogpage record.

For sites upgrading, would we remove the specific page records for draft pages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, yes - it would be good to have a single consistently-used representation and not have to deal with a special case for 'legacy' data. (Could be a bit tricky to do that in a wagtailcore migration, though, and the reverse migration is another matter altogether...)

blog_page.body += '...updated'
blog_page.save_revision()

(It would really be a good idea for Wagtail to provide a helper function for automated edits combining the above two cases, so that the necessary change happens on draft and / or live revisions as appropriate. But that's out of scope for this development)
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this would be really helpful to have accessible from data migrations. These usually need to update both the live and latest draft versions at the same time.

Comment on lines +34 to +46
Taking a page from draft state to published will require adding a `blog_blogpage` record to the existing `wagtailcore_page` record. Formally supporting this is [currently an open issue in Django](https://code.djangoproject.com/ticket/7623), but this can be achieved as follows:

page = Page.objects.get(id=123)
blogpage = BlogPage(body="some specific content")
blogpage.page_ptr = page
blogpage.path = page.path
blogpage.depth = page.depth
blogpage._state.adding = False
blogpage.save()

The `blogpage._state.adding = False` line is needed to ensure that the uniqueness check on the `path` field excludes the existing page record.

Unpublishing a page would require dropping the `blog_blogpage` record while preserving the `wagtailcore_page` record. This may require a direct SQL query (to be confirmed).
Copy link
Member

Choose a reason for hiding this comment

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

One potential complexity here would be page class inheritance with multiple levels, e.g. Page -> BlogPage -> SpecificKindOfBlogPage -> EvenMoreSpecificBlogPage. Dropping the child data may be able to take advantage of Model.delete(keep_parents=True).

If this ability to safely upcast/downcast pages between model types gets added, it also opens the door to broader possibilities around changing an existing page's type, which would be amazing (see wagtail/wagtail#4949 and wagtail/wagtail#7378).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The keep_parents=True option looks like exactly what we need here - good catch!

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

Successfully merging this pull request may close these issues.

3 participants