-
Notifications
You must be signed in to change notification settings - Fork 25
fix: AttributeError CMSPlugin (EditorNotePlugin) object has no attribute config #280
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 GuideWrap the plugin render logic in a conditional check to safely handle cases where the plugin instance has no config attribute, preventing AttributeError. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #280 +/- ##
=======================================
Coverage 88.65% 88.66%
=======================================
Files 124 124
Lines 3376 3378 +2
Branches 287 288 +1
=======================================
+ Hits 2993 2995 +2
Misses 265 265
Partials 118 118 ☔ 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.
Hey @zbohm - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
djangocms_frontend/ui_plugin_base.py
Outdated
if hasattr(instance, "config"): | ||
for key, value in instance.config.items(): | ||
if isinstance(value, dict) and set(value.keys()) == {"pk", "model"}: | ||
if key not in instance.__dir__(): # hasattr would return the value in the config dict | ||
setattr(instance.__class__, key, get_related(key)) | ||
if "instance" not in instance.config and isinstance(instance.config, dict): |
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.
suggestion (bug_risk): Guard for config existence may be too broad
hasattr only checks for the attribute's existence, not its type. If config exists but isn't a dict, .items() will fail. Use getattr with a default and check isinstance(config, dict) before iterating.
if hasattr(instance, "config"): | |
for key, value in instance.config.items(): | |
if isinstance(value, dict) and set(value.keys()) == {"pk", "model"}: | |
if key not in instance.__dir__(): # hasattr would return the value in the config dict | |
setattr(instance.__class__, key, get_related(key)) | |
if "instance" not in instance.config and isinstance(instance.config, dict): | |
config = getattr(instance, "config", None) | |
if isinstance(config, dict): | |
for key, value in config.items(): | |
if isinstance(value, dict) and set(value.keys()) == {"pk", "model"}: | |
if key not in instance.__dir__(): | |
setattr(instance.__class__, key, get_related(key)) | |
if "instance" not in config: |
if isinstance(value, dict) and set(value.keys()) == {"pk", "model"}: | ||
if key not in instance.__dir__(): # hasattr would return the value in the config dict | ||
setattr(instance.__class__, key, get_related(key)) |
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.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs
)
if isinstance(value, dict) and set(value.keys()) == {"pk", "model"}: | |
if key not in instance.__dir__(): # hasattr would return the value in the config dict | |
setattr(instance.__class__, key, get_related(key)) | |
if isinstance(value, dict) and set(value.keys()) == {"pk", "model"} and key not in instance.__dir__(): | |
setattr(instance.__class__, key, get_related(key)) | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if
conditions can be combined using
and
is an easy win.
@zbohm Great catch! I added tests to the master branch - you'll just have to merge the |
fe8e86f
to
0a949bb
Compare
@zbohm Can you do two more things?
Thank you! |
0a949bb
to
f265e24
Compare
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.
Great work, @zbohm ! Thank you so much! Have you already joined the Django CMS discord server?
Plugin
EditorNotePlugin
does not inherit formAttributesMixin
. This raisesAttributeError
in functionrender
where is the codeinstance.config.items()
. There could be more such plugins, so it is advisable to insert a condition in therender
function. It would also be inappropriate to addAttributesMixin
to a class when the class does not need it.The
render
function is missing a test for theEditorNotePlugin
plugin, otherwise the bug would have been found already. Unfortunately, I am currently unable to complete the test due to time constraints.Summary by Sourcery
Bug Fixes: