-
Notifications
You must be signed in to change notification settings - Fork 43
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
Implement feature to allow Admin to create/update FAQ content #412
Conversation
Reviewer's Guide by SourceryThis PR implements a feature that allows admin users to create and update FAQ content through a new admin page. The implementation includes integration of the Quill rich text editor for content management, with support for text formatting, images, and other rich content features. Class diagram for PageSettingsForm and PageCreateclassDiagram
class PageSettingsForm {
+I18nFormField faq_title
+I18nFormField faq_content
+_store_image(image_src) ContentFile
+clean_faq_content() I18nFormField
+clean() dict
}
class PageCreate {
+get_context_data() dict
+form_valid(form) HttpResponse
+form_invalid(form) HttpResponse
+get_success_url() str
}
PageCreate --> PageSettingsForm
note for PageSettingsForm "Handles FAQ title and content with image storage"
note for PageCreate "Admin page for creating and updating FAQ content"
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 @lcduong - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding size limits for content and image uploads to prevent abuse of storage resources. You may want to validate both the overall content size and individual image sizes before saving.
- The current image upload flow of converting data URIs to files works but could be inefficient for large images. Consider implementing direct file uploads instead, which would be more efficient and give you better control over the upload process.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 2 issues found
- 🟢 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.
"image/webp": "webp", | ||
} | ||
|
||
def _store_image(self, image_src): |
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.
🚨 issue (security): Image storage needs additional validation and configurable paths
The image storage should validate file size and dimensions, use configurable paths instead of hardcoded ones, and implement rate limiting to prevent abuse.
return default_storage.url(stored_name) | ||
return None | ||
|
||
def clean_faq_content(self): |
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.
issue: Add error handling for HTML parsing
The HTML parsing should be wrapped in try/except to handle malformed HTML gracefully and provide user-friendly error messages.
page_title, page_content = self.get_page(page=kwargs.get("page")) | ||
ctx["page_title"] = str(LazyI18nString(page_title)) | ||
|
||
attributes = dict(bleach.ALLOWED_ATTRIBUTES) |
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 (security): Tighten bleach HTML sanitization configuration
The bleach configuration should be more restrictive. Consider limiting allowed attributes further and adding URL validation for links and images.
attributes = {
'a': ['href', 'title', 'rel'],
'img': ['src', 'alt', 'title'],
'p': ['style'],
'span': ['style'],
'div': ['style']
}
We do not need an implementation of an FAQ at the moment. We need an implementation of the actual logic of the business processes. Therefore no need to work on this PR at the moment. |
Duplicate with #379 |
This issue part of #383
Summary by Sourcery
Implement a new feature allowing admins to create and update FAQ content using a rich text editor. This includes a new settings page accessible only to admins and the integration of the Quill editor for content management.
New Features:
Enhancements:
Documentation: