-
Notifications
You must be signed in to change notification settings - Fork 6
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
Ticket drop time UI support, unapproved club event visibility changes #750
base: master
Are you sure you want to change the base?
Conversation
…ing details for non-dropped events, add ticket_drop_time serializer validation, make events belonging to unapproved clubs visible to club leaders specifically, standardize spelling of "publicly"
…ged following a ticket being added to a user's cart
…clubs which require reapproval
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #750 +/- ##
==========================================
+ Coverage 71.95% 71.99% +0.04%
==========================================
Files 32 32
Lines 6953 6967 +14
==========================================
+ Hits 5003 5016 +13
- Misses 1950 1951 +1 ☔ View full report in Codecov by Sentry. |
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.
Thanks for picking this up (and sorry for getting to this so late). Left some comments on the backend changes. Frontend changes LGTM!
{"detail": "Related club does not exist", "success": False}, | ||
status=status.HTTP_404_NOT_FOUND, | ||
) | ||
elif not club.approved and not club.ghost: |
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.
I think not None
evaluates to true in Python. Is it possible for club.approved = None
and club.ghost = None
(and if so, do we want this condition evaluating to true)?
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.
Yep, if a club has no approved value, then it's either new, in which case ghost would also be none and the condition should evaluate to true (e.g. the club shouldn't be able to sell tickets), or approved previously but recently changed, in which case the club should be able to sell tickets, ghost would be true, and this condition would be false.
if ( | ||
"ticket_drop_time" in request.data | ||
and Ticket.objects.filter(event=event, owner__isnull=False).exists() | ||
): | ||
raise DRFValidationError( | ||
detail="""Ticket drop times cannot be edited | ||
after tickets have been sold.""" | ||
) |
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.
Will there ever be a case where tickets have been issued to people, and then the drop time is changed? I think yes (imagine a club leader who's issued tickets to their members, before they're generally available to the public).
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.
Maybe this condition can only be triggered if ticket drop time has also already elapsed? I think I was mostly just uncomfortable with the idea of ticket drop times being pushed back retroactively after people have bought tickets, but didn't consider the ticket issuance case.
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.
I thinking only checking this once the ticket drop time has elapsed makes sense.
tickets_to_replace = cart.tickets.filter( | ||
Q(owner__isnull=False) | ||
| Q(event__club__archived=True) | ||
| Q(holder__isnull=False) | ||
| Q(event__end_time__lt=now) | ||
| ( | ||
Q(event__ticket_drop_time__gt=timezone.now()) | ||
& Q(event__ticket_drop_time__isnull=False) | ||
) |
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.
Not sure this is needed. Users shouldn't be able to add tickets to their cart if the ticket drop time hasn't elapsed yet. (The logic's in ClubEventViewSet.add_to_cart()
.)
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.
This is handling the edge case where people have added tickets to their cart under a certain ticket drop time, but the drop time has since been updated (assuming that existing cartholders shouldn't be "grandfathered" into the old ticket drop time).
Q(event__ticket_drop_time__lt=timezone.now()) | ||
| Q(event__ticket_drop_time__isnull=True), | ||
event__club__archived=False, |
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.
Same note as above here. Will there ever be a case where a ticket is in a user's cart, but the drop time hasn't elapsed?
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.
Responded above.
af5e57c
to
8d0adca
Compare
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.
LGTM Julian! Left some comments, thanks for picking this PR back up.
if ticket_drop_time is not None and ticket_drop_time >= end_time: | ||
raise serializers.ValidationError( | ||
"Your ticket drop time must be before the event ends!" | ||
) |
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.
Thinking about this again: would it make more sense for the ticket drop time to be before the event's start time? I can't think of a scenario where the event would start but tickets would drop midway through it.
if ( | ||
"ticket_drop_time" in request.data | ||
and Ticket.objects.filter(event=event, owner__isnull=False).exists() | ||
): | ||
raise DRFValidationError( | ||
detail="""Ticket drop times cannot be edited | ||
after tickets have been sold.""" | ||
) |
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.
I thinking only checking this once the ticket drop time has elapsed makes sense.
@@ -5241,6 +5297,8 @@ def cart(self, request, *args, **kwargs): | |||
continue | |||
|
|||
available_tickets = Ticket.objects.filter( | |||
Q(event__ticket_drop_time__lt=timezone.now()) |
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.
Nit: should be less than or equal to.
@@ -605,6 +647,50 @@ def test_add_to_cart_before_ticket_drop(self): | |||
# Tickets should not be added to cart before drop time | |||
self.assertEqual(resp.status_code, 403, resp.content) | |||
|
|||
def test_add_to_cart_unapproved_club(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.
It'd be nice to test the case where a user adds a ticket that's dropped, the ticket drop time gets pushed back, and the user attempts to check out. But maybe it's extra, feel free to skip.
ticket_drop_time
validation in event serializer based on event end time/directory
which have been approved at some point