feat: merge page with node tree #7947
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…-cms/django-cms into WiP-feat-merge-Page-with-NodeTree
fsbraun
requested changes
Jul 4, 2024
There was a problem hiding this comment.
Hey @jrief! Great work. Looks promising! It's a big change and I have some comments:
- I strongly promote adding (working) compatibility shims for the changed API:
- A node property for
Pagethat returns the page itself. Thereby, custom apps that accesspage.node.somethingwill not break - Remove
node__prefixes fromfilter,order_by,select_relatedinPageQuerySet - Rename
page.parenttopage.page_parentto be compatible with django CMS 4.1 and earlier - Use the
RemovedInDjangoCMS43warning message to mark compatibility code that will need to be removed in the future.
- A node property for
- Add some documentation for developers who need code to run on both django CMS 4.1 and earlier and on django CMS 4.2+ in
docs/upgrade/4.2.0.rstLinks to this documentation should be part of the warnings (https://docs.django-cms.org/en/release-4.2.x/upgrade/4.2.0.html) - On the smaller things, I've noticed that you use both single and double quotes (
'and"). I'd suggest being consistent and opting for double quotes
I'm undecided if we can already remove TreeNode from the models. @marksweb Do you have an opinion on that? Is there a scenario where custom apps use it?
c0aa851 to
91d6756
Compare
…-cms/django-cms into WiP-feat-merge-Page-with-NodeTree
605be06 to
d58a4b8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is the (huge) pull request to merge models
PageandTreeNodeintoPage.There is some additional cleanup to do, because I haven't yet removed all the legacy code for reference.
All unit tests are working.
This pull request shall go into version 4.2.
@fsbraun Please consider to apply it before merging other pull requests, since this presumably will cause a lot of merging conflicts.
Related resources
It is based on the discussion #7873 and we talks about this a few times during
the biweekly meetings.
Checklist
develop-4