-
Notifications
You must be signed in to change notification settings - Fork 47
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
Eventyay Common: Create an event dashboard #484
base: development
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new event dashboard to Eventyay Common. It provides an overview of event information and actions, including tickets, talks, and videos. The dashboard is accessible from the event page and includes widgets for managing different aspects of the event. Class diagram for the event dashboard views and navigationclassDiagram
class EventDashboard {
+event_index(request, organizer, event)
+event_index_widgets_lazy(request, organizer, event)
+rearrange(widgets: list)
}
class Navigation {
+get_event_navigation(request)
+get_global_navigation(request)
}
class Context {
+_default_context(request)
}
EventDashboard --> Navigation: uses
EventDashboard --> Context: uses
note for EventDashboard "Main dashboard controller"
note for Navigation "Handles navigation structure"
note for Context "Provides context data"
File-Level Changes
Assessment against linked issues
Possibly linked issues
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 @HungNgien - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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.
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 @HungNgien - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue 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.
return JsonResponse({'widgets': widgets}) | ||
|
||
|
||
def event_index(request, organizer, event): |
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 (complexity): Consider extracting permission checks to a dedicated function and simplifying query building by inverting the content type logic to improve readability and testability.
The event_index function's complexity can be reduced by extracting permission checks and simplifying query building:
- Extract permissions to a dedicated function:
def get_event_permissions(user, organizer, event, request):
return {
'can_view_orders': user.has_event_permission(organizer, event, 'can_view_orders', request=request),
'can_change_orders': user.has_event_permission(organizer, event, 'can_change_orders', request=request),
'can_change_settings': user.has_event_permission(organizer, event, 'can_change_event_settings', request=request),
'can_view_vouchers': user.has_event_permission(organizer, event, 'can_view_vouchers', request=request),
}
- Simplify query building by inverting the content type logic:
def build_log_query(event, permissions):
qs = event.logentry_set.all().select_related(
'user', 'content_type', 'api_token', 'oauth_application', 'device'
).exclude(action_type__in=OVERVIEW_BANLIST)
allowed_types = []
if permissions['can_view_orders']:
allowed_types.append(ContentType.objects.get_for_model(Order))
if permissions['can_view_vouchers']:
allowed_types.append(ContentType.objects.get_for_model(Voucher))
if permissions['can_change_settings']:
allowed_types.extend([
ContentType.objects.get_for_model(model)
for model in [Item, ItemCategory, Quota, Question]
])
return qs.filter(content_type__in=allowed_types).order_by('-datetime')
This refactoring maintains all functionality while making the code more maintainable and easier to test.
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.
Please add screenshots with all components enabled and add spacing below the menu in the mobile view.
What happens if the a component is not enabled, e.g. talk or video? What happens when you click on the "dashboard" options in that case?
There should probably be a pop up "You need to enable this component first. You can do this here."
Or what is your suggestion?
Create dashboard the common event
This PR resolves #455
Summary by Sourcery
New Features:
Summary by Sourcery
New Features: