- 
                Notifications
    
You must be signed in to change notification settings  - Fork 31
 
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 Report❌ 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
EditorNotePlugindoes not inherit formAttributesMixin. This raisesAttributeErrorin functionrenderwhere is the codeinstance.config.items(). There could be more such plugins, so it is advisable to insert a condition in therenderfunction. It would also be inappropriate to addAttributesMixinto a class when the class does not need it.The
renderfunction is missing a test for theEditorNotePluginplugin, 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: