-
Notifications
You must be signed in to change notification settings - Fork 33
fix: Remove unnecessary _original_manager
usage from toolbar
#477
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
Conversation
Reviewer's GuideUpdate toolbar content lookup to use Django’s default manager and streamline the published page version query. Class diagram for PageContent manager usage updateclassDiagram
class PageContent {
+objects: Manager
-_original_manager: Manager (removed from usage)
}
class cms_toolbars {
_get_published_page_version()
}
PageContent <.. cms_toolbars : used by
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @fsbraun - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `djangocms_versioning/cms_toolbars.py:246` </location>
<code_context>
return
- return PageContent._original_manager.filter(
- page=self.page, language=language, versions__state=PUBLISHED
+ return PageContent.objects.filter(
+ page=self.page, language=language
).select_related("page").first()
</code_context>
<issue_to_address>
Removing the versions__state=PUBLISHED filter may return non-published versions.
Please confirm that returning PageContent objects in any state is intentional, as this may affect logic expecting only published versions.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
_original_manager
usage from toolbar
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #477 +/- ##
==========================================
+ Coverage 90.55% 93.66% +3.10%
==========================================
Files 72 76 +4
Lines 2732 2699 -33
Branches 322 0 -322
==========================================
+ Hits 2474 2528 +54
+ Misses 182 171 -11
+ Partials 76 0 -76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
LGTM 👍🏼
Description
The toolbar accesses
_original_manager
which is the style of djangocms-versioning<2. For consistency and unnecessary code repetition, theobjects
manager should be used for getting published content.Related resources
Checklist
master
Slack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Use the default PageContent manager instead of the outdated
_original_manager
when fetching published content in the CMS toolbarEnhancements:
PageContent._original_manager.filter
withPageContent.objects.filter
in_get_published_page_version
select_related("page")
to optimize the published content query