feature:implement ticket option - one ticket per user #604
Conversation
Reviewer's Guide by SourceryThis PR implements a 'one ticket per user' feature. It adds a setting to the event configuration and enforces the restriction during the cart validation process by checking the attendee's email against existing cart positions and orders. Sequence diagram for cart validation with one-ticket-per-user enabledsequenceDiagram
participant User
participant CartService
participant CartPosition
participant OrderPosition
participant Order
User->>CartService: Adds item to cart
CartService->>CartService: _check_item_constraints()
alt event.settings.one_ticket_per_user is enabled
CartService->>User: Get user email
User->>CartService: Returns user email
CartService->>CartPosition: Check existing cart positions with the same email
CartPosition-->>CartService: Returns existing cart positions
CartService->>OrderPosition: Check existing order positions with the same email
OrderPosition-->>CartService: Returns existing order positions
alt existing cart positions or orders found
CartService-->>User: Raise CartError (one_ticket_per_user)
else no existing cart positions or orders found
CartService->>CartService: Continue with other validations
end
else event.settings.one_ticket_per_user is disabled
CartService->>CartService: Continue with other validations
end
CartService-->>User: Item added to cart (or error)
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @22f1001635 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a database index on
attendee_emailinCartPositionandOrderPositionto improve query performance. - It might be better to perform the email check in a separate service function to keep the cart service focused.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 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.
| @@ -310,6 +312,29 @@ def _check_item_constraints(self, op, current_ops=[]): | |||
| ), self.event.timezone) | |||
| if term_last < self.now_dt: | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the 'one ticket per user' conditional into a helper method to improve readability and reduce redundancy by avoiding duplicate setting checks and reducing nesting complexity within the main function.
Consider extracting the "one ticket per user" conditional into its own helper method to reduce nesting and eliminate the redundant setting check. For example, you could add a helper similar to:
def _validate_one_ticket(self, error_messages):
user_email = self._widget_data.get('email')
if not user_email:
raise CartError(_('Email address is required for ticket purchase'))
user_email = user_email.lower().strip()
existing_positions = CartPosition.objects.filter(
event=self.event,
attendee_email=user_email,
).exclude(
pk__in=[op.position.id for op in self._operations if isinstance(op, self.RemoveOperation)]
)
existing_orders = OrderPosition.objects.filter(
order__event=self.event,
attendee_email=user_email,
order__status__in=[Order.STATUS_PENDING, Order.STATUS_PAID]
)
if existing_positions.exists() or existing_orders.exists():
raise CartError(error_messages['one_ticket_per_user'])Then simplify your main flow by replacing the nested block with a single call:
if self.event.settings.one_ticket_per_user:
self._validate_one_ticket(error_messages)This flattening avoids duplicate setting checks and clarifies the one-ticket validation logic without altering functionality.
There was a problem hiding this comment.
@22f1001635 please consider this suggestion
d50e9d8 to
474c781
Compare
|
How have you tested these changes? |
|
@22f1001635 any updates? |
|
Hi @mariobehling, My laptop display was damaged due to a liquid spill, and I just received it back after repairs only three days ago. I am currently in the process of setting up the required dependencies, software and preparing everything to get right on track You can expect an updated version by 4th May, including tests and some other small changes. I appreciate your patience, and I am really sorry for any inconvenience. |
| super().__init__(*args, **kwargs) | ||
| qs = Organizer.objects.all() | ||
| if not event.settings.attendee_emails_asked: |
There was a problem hiding this comment.
This is causing an error while creating a new event
| <fieldset> | ||
| <legend>{% trans "Ticket restrictions" %}</legend> | ||
| {% bootstrap_field form.one_ticket_per_user layout="control" %} | ||
| </fieldset> |
There was a problem hiding this comment.
I think these changes must be done in event setting not here, this is the tickets setting after creating a event and this will lead to an error in pre-existing events when trying to access /control/event/<org>/<slug>/settings/tickets
| raise CartError(_('Email address is required for ticket purchase')) | ||
| if user_email: | ||
| # Check both cart positions and orders | ||
| existing_positions = CartPosition.objects.filter( |
There was a problem hiding this comment.
This will not work as CartPositions does not have a attendee_email field associated with it and will raise a FieldError
| order__status__in=[Order.STATUS_PENDING, Order.STATUS_PAID] | ||
| ) | ||
|
|
||
| if existing_positions.exists() or existing_orders.exists(): |
There was a problem hiding this comment.
This won't limit the tickets per order but instead will limit the number of orders per user, The user will still be able to add multiple tickets in an order.
This PR enforces a one-ticket-per-user limit when the one_ticket_per_user event setting is enabled. The validation:
No changes to existing behavior when the setting is disabled.
The restriction helps prevent duplicate ticket purchases while maintaining a smooth user experience.
Fixes #259
Summary by Sourcery
Implement a one-ticket-per-user restriction for event ticket purchases when enabled
New Features:
Bug Fixes:
Enhancements: