-
Notifications
You must be signed in to change notification settings - Fork 32
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: Improve default copy method to also copy placeholders and plugins #345
feat: Improve default copy method to also copy placeholders and plugins #345
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #345 +/- ##
==========================================
- Coverage 90.55% 90.54% -0.01%
==========================================
Files 72 72
Lines 2720 2729 +9
Branches 318 321 +3
==========================================
+ Hits 2463 2471 +8
Misses 182 182
- Partials 75 76 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Reviewer's Guide by SourceryThis pull request enhances the default copy method to include copying placeholders and plugins, reducing code duplication across various packages using djangocms-versioning. It also introduces a Updated class diagram for default_copy functionclassDiagram
class default_copy{
+default_copy(original_content)
+content_fields
+new_content
+new_placeholders
+placeholder_fields
+new_placeholder
}
class Placeholder{
-_meta
+objects
+copy_plugins(new_placeholder)
}
default_copy -- Placeholder : creates
note for default_copy "Copies placeholders and plugins by default"
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 - here's some feedback:
Overall Comments:
- The new tests are good, but could be improved by using
self.assertIsInstance
instead of creating a MockContent class. - Consider adding a test case where
copy_relations
exists but is not callable.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…cms-versioning into feat/improve_default_copy
Description
Versioning allows specifying a copy method to allow for copying foreign keys, one-to-one fields in a meaningful way for the specific model.
One of the most relevant relations will be placeholders and their plugins.
To avoid code repetition of largely identical copy methods in any package using djangocms-versioning, I propose to include coyping placeholders in versioned models by default.
This would make custom copy functions obsolete in alias and blog and other content objects to come.
Also, if the verisoned model has a method
copy_relations
it is called on the new object allowing to keep simple copy information within the model.Tests follow.
Related resources
Checklist
master
Slack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Improves the default copy method to also copy placeholders and plugins, and introduces a mechanism to allow versioned models to define custom copy logic.
Enhancements:
copy_relations
method on the new object if it exists, allowing models to define custom copy logic.