-
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
Schedule menu in ticket shop should consider schedule state from talk #354
base: development
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request implements changes to handle the schedule state triggered by the Talk system. It introduces a new API endpoint to manage the schedule's public visibility and removes manual schedule/session/speaker link settings, replacing them with automatic links when the schedule is made public. The changes primarily affect the event settings, API views, and frontend templates. Sequence DiagramsequenceDiagram
participant TS as Talk System
participant API as Pretix API
participant E as Event
participant FE as Frontend
TS->>API: POST /schedule-public
API->>API: Validate token & permissions
API->>E: Update talk_schedule_public setting
API-->>TS: Return success/error response
FE->>E: Request event data
E-->>FE: Return event data (including talk_schedule_public)
FE->>FE: Show/hide schedule links based on talk_schedule_public
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 using a more robust authentication method or established library for token validation instead of the custom
check_token_permission
function to enhance security. - The removal of manual schedule, session, and speaker link settings might reduce flexibility for some users. Consider providing an option to override the automatically generated URLs if needed.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 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.
@csrf_exempt | ||
@require_POST | ||
@scopes_disabled() | ||
def talk_schedule_public(request, *args, **kwargs): |
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: Refine exception handling and function structure
Consider catching specific exceptions instead of using bare except clauses. This function is quite long; consider breaking it down into smaller, more focused functions for better readability and maintainability.
def talk_schedule_public(request, *args, **kwargs):
try:
return _process_talk_schedule(request, *args, **kwargs)
except InvalidToken:
return JsonResponse({"error": "Invalid token"}, status=401)
except PermissionDenied:
return JsonResponse({"error": "Permission denied"}, status=403)
except Exception as e:
logger.error(f"Error processing talk schedule: {str(e)}")
return JsonResponse({"error": "Internal server error"}, status=500)
def _process_talk_schedule(request, *args, **kwargs):
@@ -964,6 +964,24 @@ def has_paid_things(self): | |||
return Item.objects.filter(event=self, default_price__gt=0).exists()\ | |||
or ItemVariation.objects.filter(item__event=self, default_price__gt=0).exists() | |||
|
|||
@property | |||
def talk_schedule_url(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.
suggestion: Improve URL construction for talk-related properties
Ensure that settings.TALK_HOSTNAME is properly defined. Consider using Django's reverse() function or a similar URL construction method instead of string concatenation for more robust URL generation.
def talk_schedule_url(self):
from django.urls import reverse
return reverse('talk:schedule', kwargs={'event_slug': self.slug})
implement API handle schedule public state remove schedule,session,speaker link set
This PR related to issue #353
Implement:
Summary by Sourcery
Implement an API to manage schedule state changes from the Talk system and automate the configuration of schedule-related links based on the schedule's visibility status.
New Features:
Enhancements: