-
Notifications
You must be signed in to change notification settings - Fork 17
feat: CSP support #58
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 GuideThis PR externalizes inline widget CSS/JS into static assets and conditionally inlines them to support CSP, revamps CI workflows (linting with Ruff, expanded test matrix, concurrency), introduces publish pipelines for TestPyPI and PyPI, bumps the package version and supported platforms, and adds project lint/format configs. Class diagram for AttributesWidget CSP supportclassDiagram
class AttributesWidget {
+render(name, value, attrs=None, renderer=None)
+value_from_datadict(data, files, name)
}
class Widget
AttributesWidget --|> Widget
class apps {
+is_installed(app_name)
}
AttributesWidget ..> apps : uses
class _inline_code
AttributesWidget ..> _inline_code : uses
class _read_static_files {
+_read_static_files()
}
_inline_code ..> _read_static_files : calls if not installed
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:
- Loading the JS/CSS files via a hard-coded open() on a relative path is brittle—consider using Django’s staticfiles finders or importlib.resources to reliably locate package assets.
- When
djangocms_attributes_field
is installed you currently suppress inline code but haven’t added a Media class onAttributesWidget
, so the external CSS/JS won’t be enqueued—please defineclass Media
with the correct asset paths. - In your CI matrix (
.github/workflows/test.yml
) thedj52_cms50.txt
entry is duplicated, remove the extra line to avoid running identical test jobs twice.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Loading the JS/CSS files via a hard-coded open() on a relative path is brittle—consider using Django’s staticfiles finders or importlib.resources to reliably locate package assets.
- When `djangocms_attributes_field` is installed you currently suppress inline code but haven’t added a Media class on `AttributesWidget`, so the external CSS/JS won’t be enqueued—please define `class Media` with the correct asset paths.
- In your CI matrix (`.github/workflows/test.yml`) the `dj52_cms50.txt` entry is duplicated, remove the extra line to avoid running identical test jobs twice.
## Individual Comments
### Comment 1
<location> `djangocms_attributes_field/widgets.py:17` </location>
<code_context>
+# inline scripts/styles, but also support projects that historically do not use
+# djangocms_attributes_field as an app, but still want to use the widget.
+
+if apps.is_installed('djangocms_attributes_field'):
+ _inline_code = ""
+else:
</code_context>
<issue_to_address>
The use of apps.is_installed at import time may cause issues in some Django setups.
Checking apps.is_installed at import time can trigger ImportError or AppRegistryNotReady if the app registry isn't ready. Move this check to runtime, such as within the render method, to prevent these issues.
</issue_to_address>
### Comment 2
<location> `djangocms_attributes_field/widgets.py:21` </location>
<code_context>
+ _inline_code = ""
+else:
+ def _read_static_files():
+ with open('./static/djangocms_attributes_field/widget.js', 'r', encoding='utf-8') as f:
+ js_code = f.read()
+ with open('./static/djangocms_attributes_field/widget.css', 'r', encoding='utf-8') as f:
+ css_code = f.read()
+ return css_code, js_code
</code_context>
<issue_to_address>
Hardcoding static file paths may break in various deployment scenarios.
Relative paths like './static/djangocms_attributes_field/widget.js' may not work reliably in production or with collectstatic. Use Django's staticfiles finders to ensure correct file resolution.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
djangocms_attributes_field/static/djangocms_attributes_field/widget.js
Outdated
Show resolved
Hide resolved
djangocms_attributes_field/static/djangocms_attributes_field/widget.js
Outdated
Show resolved
Hide resolved
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #58 +/- ##
==========================================
+ Coverage 90.36% 91.44% +1.08%
==========================================
Files 4 4
Lines 166 187 +21
Branches 26 29 +3
==========================================
+ Hits 150 171 +21
Misses 10 10
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Replace inline CSS and JS by media files if djangocms-attributes-field is installed in
INSTALLED_APPS
. Otherwise keep the inline code for backward compatibility.Related resources
Checklist
master
Slack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Enable Content Security Policy compatibility by extracting inline assets, refactor widget asset loading, update version and metadata, modernize CI with Ruff, and add publish workflows
New Features:
Enhancements:
CI:
Deployment: