Implement organizer onboarding process | Pricing/Privacy/Terms Pages #434
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… and show it on signup page
Reviewer's Guide by SourceryThis PR implements functionality to manage content pages (Pricing/Privacy/Terms) in the admin area and display them on the login/register page. The implementation uses Quill editor for rich text editing and includes proper content sanitization. Class diagram for the new PageSettingsForm and PageCreate classesclassDiagram
class PageSettingsForm {
+GlobalSettingsObject obj
+String page_name
+clean() I18nFormField
+_clean_content_field(content_field) I18nFormField
+_store_image(image_src) String
}
class PageCreate {
+get_form_kwargs() kwargs
+get_context_data() ctx
+form_valid(form) success
+form_invalid(form) error
+get_success_url() url
}
PageCreate --> PageSettingsForm
note for PageSettingsForm "Handles form logic for page settings"
note for PageCreate "View for creating and updating page content"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @lcduong - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding access controls to the uploaded images storage to prevent unauthorized access. The current implementation stores them with public access which could be a security concern.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues 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.
| settings_dict.update(data) | ||
| return data | ||
|
|
||
| def _clean_content_field(self, content_field): |
There was a problem hiding this comment.
suggestion: Add error handling for HTML parsing
The lxml.html.fragments_fromstring() call should be wrapped in a try-except block to handle malformed HTML gracefully. Invalid input could currently cause uncaught exceptions.
def _clean_content_field(self, content_field):
try:
page_content = self.cleaned_data[content_field]
except (lxml.etree.ParserError, ValueError) as e:
raise forms.ValidationError(_('Invalid HTML content')) from e|
|
||
| return page_content | ||
|
|
||
| def _store_image(self, image_src): |
There was a problem hiding this comment.
🚨 issue (security): Add file size limits and type validation for uploaded images
The image upload functionality should validate file sizes and implement stricter MIME type checking to prevent security issues. Consider adding a cleanup mechanism for old images.
| template_name = 'eventyay_common/pages/show.html' | ||
|
|
||
| def get_page(self, page): | ||
| gs = GlobalSettingsObject() |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the page content flag setting logic to use a loop over page types.
The content flag setting in get_context_data can be simplified by looping over page types. This reduces duplication and makes the code more maintainable:
def get_context_data(self, **kwargs):
ctx = super().get_context_data()
page_title, page_content = self.get_page(page=kwargs.get("page"))
ctx["page_title"] = str(LazyI18nString(page_title))
# ... bleach configuration ...
gs = GlobalSettingsObject()
page_types = ['faq', 'pricing', 'privacy', 'terms']
for page_type in page_types:
ctx[f'{page_type}_content'] = bool(getattr(gs.settings, f'{page_type}_content', False))
return ctxThis change:
- Reduces code duplication
- Makes adding new page types easier
- Maintains identical functionality
- Improves readability
| @@ -523,6 +523,44 @@ def get_admin_navigation(request): | |||
| }, | |||
| ] | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
issue (complexity): Consider using a list-based approach to generate the navigation structure programmatically
The Pages navigation structure contains unnecessary repetition. Consider generating it from a simple list of page definitions:
ADMIN_PAGES = [
('faq', _('FAQ')),
('pricing', _('Pricing')),
('privacy', _('Privacy')),
('terms', _('Terms')),
]
pages_nav = {
'label': _('Pages'),
'url': reverse('control:admin.pages.create', kwargs={'page': ADMIN_PAGES[0][0]}),
'active': False,
'icon': "file-text",
'children': [{
'label': label,
'url': reverse('control:admin.pages.create', kwargs={'page': page_type}),
'active': url.kwargs.get('page') == page_type,
} for page_type, label in ADMIN_PAGES]
}This approach:
- Makes adding new pages trivial
- Reduces chance of copy-paste errors
- Maintains consistent structure
- Keeps all functionality intact
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is part of issue #379 Implement organizer onboarding process
It implement:
Summary by Sourcery
Implement a new admin page for managing Pricing, Privacy, and Terms content, and integrate Quill Editor for rich text editing.
New Features:
Enhancements:
Documentation: