-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: Add management command to create version objects #304
Conversation
Change precedence of user selection
Codecov Report
@@ Coverage Diff @@
## master #304 +/- ##
==========================================
+ Coverage 89.75% 89.87% +0.12%
==========================================
Files 67 68 +1
Lines 2196 2242 +46
Branches 293 299 +6
==========================================
+ Hits 1971 2015 +44
- Misses 170 172 +2
Partials 55 55
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No test coverage, this is by far the leading package for anything created with the CMS, it could be a shame to lose that. Needs tests hence marking as needing changes.
A comment added for the version states.
try: | ||
Version.objects.create( | ||
content=orphan, | ||
state=options["state"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the state should be set, I think this needs to be calculated. The reason I think this is that you could have a grouper object (Page) with many content objects (PageContent), if you blindly set them all to draft you'll get an error where you can only have one draft, it could be that the state machine handles this and forces the existing version to archived, but it could be that the content objects are not in the correct order.
There's some additional complexities when filtering to find what other versions a grouper has because it can have grouping filters such as language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be necessary for a "recovery". I was more thinking about a situation where you added versioning for a first time. Then there should only be one "Version" per grouper.
If we're thinking about recovery I only see one option:
- Only "published", "draft", and "archived" are valid as target states.
- If a draft or published version already exists the target state will have to be "archived"
- If multiple content objects per grouper (and grouping filters) exist without a Version object they are processed from highest pk to lowest pk.
As a result either all recovered Versions will be archived (if "archived" is selected) or all recovered Versions will be archived except the one created latest (by pk) which will be either draft or published.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair, that would be a fair assumption what you have proposed. I was thinking more a corrupt setup.
If there ever as such an occurrence I guess we could point to documentation in the code that this solution is based on that assumption / scenario that you propose. There will be a day where someone comes with such a corrupt setup, maybe they manually deleted Version objects or something (I once did it trying to remove a package, this could be improved by fixing the deletion where you can delete things, a separate issue that I always wanted the time to fix, needs to simply change the source version).
My biggest fear of getting more people involved with V4 was just that, corrupt Version history. Maybe fix it when someone comes with it. I had done something similar although it doesn't handle other contenttypes i.e. Page / Pagecontent only, that was in the 3 to 4 migration package where the version states were kept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm almost there. But the added complexity will require some tests. Will push an update later. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aiky30 I've updated the PR. It will successfully recover missing Version objects even if there are multiple content objects per grouper. It also reflects additional grouping fields. A test is added that runs on models with (Poll) and without (Blog) additional grouping field.
…ersioning into feat/create_versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments regarding the management command tests, the tests normally follow the AAA standard, this makes the test easier to follow. Minor and not blocking.
Description
This PR adds a management command to create (usually draft)
Version
objects for versioned content that lacks aVersion
object. This is relevant if one adds versioning to a project after unversioned content has been created.Related resources
Checklist
master
Slack to find a “pr review buddy” who is going to review my pull request.