-
Notifications
You must be signed in to change notification settings - Fork 97
feature:implement ticket option - one ticket per user #604
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
base: development
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @22f1001635 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a database index on
attendee_email
inCartPosition
andOrderPosition
to 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@22f1001635 please consider this suggestion
d50e9d8
to
474c781
Compare
How have you tested these changes? |
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: