Add links to specific event components #423
Conversation
…into feature-240-v2
Reviewer's Guide by SourceryThis PR implements a navigation header with links to specific event components (Tickets, Talk, Videos) across multiple pages in the application. The implementation includes adding video access functionality, refactoring the header layout, and ensuring consistent navigation across different views. Class Diagram for Video Access and Plugin ManagementclassDiagram
class EventView {
+get_queryset()
+get_context_data()
+get_plugins(plugin_list)
}
class VideoAccessAuthenticator {
+get(request, *args, **kwargs)
+generate_token_url(request)
}
EventView <|-- VideoAccessAuthenticator
class PluginHelper {
+_get_plugins(plugin_list)
+_is_video_enabled(event)
}
EventView o-- PluginHelper
VideoAccessAuthenticator o-- PluginHelper
File-Level Changes
Assessment against linked issues
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 using template inheritance or includes to reduce duplication of the navigation header code across multiple templates. This would make future maintenance easier.
- The _get_plugins() and _is_video_enabled() functions are duplicated between event.py and context.py. Consider extracting these into a shared utility module to avoid code duplication.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 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.
| @@ -78,12 +84,23 @@ def get_queryset(self): | |||
| query_set = self.filter_form.filter_qs(query_set) | |||
| return query_set | |||
|
|
|||
| def get_plugins(self, plugin_list): | |||
There was a problem hiding this comment.
issue: This get_plugins() method is duplicated with _get_plugins() - should be consolidated into a single utility function
Consider moving this to a shared utility module since it's used in multiple places. This will reduce code duplication and make maintenance easier.
| return [] | ||
| return [p.strip() for p in plugin_list.split(",") if p.strip()] | ||
|
|
||
| def _is_video_enabled(self): |
There was a problem hiding this comment.
issue: This _is_video_enabled() method is duplicated in context.py - should be consolidated
This same logic appears in context.py. Consider extracting to a shared location to avoid maintaining duplicate code.
| border-radius: 0; | ||
| } | ||
| .header-nav.active { | ||
| background-color: $hover-button-color !important; |
There was a problem hiding this comment.
suggestion: Avoid using !important in CSS rules
Instead of using !important, consider restructuring the CSS selectors to achieve the desired specificity through proper hierarchy.
| background-color: $hover-button-color !important; | |
| background-color: $hover-button-color; |
| @@ -406,7 +406,7 @@ def test_paid(self): | |||
| def test_sent_days(self): | |||
| self.event.settings.mail_days_order_expire_warning = 9 | |||
| send_expiry_warnings(sender=self.event) | |||
| assert len(djmail.outbox) == 0 | |||
| assert len(djmail.outbox) == 1 | |||
There was a problem hiding this comment.
issue (testing): Test assertion change needs explanation or additional test cases
The test assertion has been changed from expecting 0 emails to expecting 1 email, but there's no explanation for this change. Either this is fixing a bug (in which case we should add test cases to verify the fix) or this might be an unintended change. Please clarify and add appropriate test coverage.
This PR PR resolves #391
Implement links to specific event components in eventyay-common and tickets control:
NOTE: please note that this PR did not change how URL being generated in template, I just move it into a div tags
If video plugin is not enabled or video plugin settings missing config, will disable 'Video' button
Summary by Sourcery
Add links to specific event components, including video access, in the eventyay-common and tickets control sections. Refactor navigation headers to include a consistent event navigation bar with component links.
New Features:
Enhancements:
Summary by Sourcery
Add links to specific event components, including video access, in the eventyay-common and tickets control sections. Refactor navigation headers to include a consistent event navigation bar with component links.
New Features:
Enhancements: