-
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
Fix join online video button after remove customer feature #433
base: development
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis PR refactors the online video joining functionality to use email-based identification instead of the previously removed customer feature. The implementation adds a new API endpoint for customer order validation and updates the token generation logic to use encoded email addresses as identifiers. Sequence diagram for the new CustomerOrderCheckView API endpointsequenceDiagram
actor User
participant API as CustomerOrderCheckView
participant DB as Database
User->>API: POST /<organizer>/<event>/ticket-check
API->>DB: Get Organizer by slug
DB-->>API: Organizer
API->>DB: Get Event by slug and organizer
DB-->>API: Event
API->>DB: Get User by email
DB-->>API: User
API->>DB: Get Orders by event and user email
DB-->>API: Order List
alt Order List is empty
API-->>User: 404 Customer has no orders for this event
else Order List is not empty
alt Order status is 'p'
API-->>User: 200 Customer has paid orders
else Order status is not 'p'
API-->>User: 400 Customer did not paid orders
end
end
Updated class diagram for the event viewsclassDiagram
class CustomerOrderCheckView {
+post(request, *args, **kwargs)
}
class EventViewMixin {
+get_context_data(**kwargs)
+validate_access(request, *args, **kwargs)
+generate_token_url(request, order_position, order)
}
CustomerOrderCheckView <|-- APIView
EventViewMixin <|-- APIView
note for CustomerOrderCheckView "New class for handling order checks using email as identifier"
note for EventViewMixin "Refactored to use user email instead of customer for access validation and token generation"
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 standardizing error responses to use either Response or JsonResponse consistently throughout CustomerOrderCheckView for better maintainability.
- Please add documentation explaining the choice of case-insensitive email matching (iexact) and any security considerations of using email as an identifier.
Here's what I looked at during the review
- 🟡 General issues: 1 issue 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.
return JsonResponse(status=404, data={"error": "Customer not found."}) | ||
|
||
# Get all orders of customer which belong to this event | ||
order_list = (Order.objects.filter(Q(event=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.
suggestion (performance): Consider adding a case-insensitive index on email field to optimize iexact queries
This applies to both CustomerOrderCheckView and validate_access method queries
This PR to rework join online video button feature in ticket shop.
As before, the token to join online video using customer's id for identifier in video, but since customer feature is removed, we will using User's email or Order's email as User identifier in video.
Summary by Sourcery
Rework the 'join online video' button feature by replacing the customer identifier with the user's email or order's email. Introduce a new API view to check customer orders for an event and refactor the video plugin check for better code clarity.
Bug Fixes:
Enhancements: