fix: Checkout skips voucher-required ticket validation#13
fix: Checkout skips voucher-required ticket validation#13JacobCoffee wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds validation to prevent checkout when tickets requiring vouchers are in the cart but the voucher has been removed. The changes implement checkout-time re-validation to catch cases where a user adds a voucher-required ticket to their cart, then removes the voucher before completing checkout.
Changes:
- Added validation in checkout service to verify tickets requiring vouchers still have an appropriate voucher at checkout time
- Modified
_revalidate_stockand_revalidate_ticket_stockto accept and validate voucher parameter - Added test case to verify checkout rejection when voucher-required tickets are present without a voucher
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/django_program/registration/services/checkout.py |
Added voucher parameter to stock validation functions and implemented voucher requirement checks during checkout |
tests/test_registration/test_checkout_service.py |
Added test case for voucher-required ticket validation, but contains a critical bug calling non-existent method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| CartService.apply_voucher(cart, "HIDDEN") | ||
| CartService.add_ticket(cart, hidden_ticket, qty=1) | ||
| CartService.remove_voucher(cart) |
There was a problem hiding this comment.
The method CartService.remove_voucher does not exist in the codebase. The CartService class only has the following methods: get_or_create_cart, add_ticket, add_addon, remove_item, update_quantity, apply_voucher, get_summary, and get_summary_from_items. This test will fail when executed. To remove a voucher from a cart, you should directly set cart.voucher = None and save the cart.
| CartService.remove_voucher(cart) | |
| cart.voucher = None | |
| cart.save(update_fields=["voucher", "updated_at"]) |
There was a problem hiding this comment.
Good question — that recommendation was to fix a test bug, not to change product behavior.
Why I recommended that patch
At line 362, the test calls CartService.remove_voucher(cart), but CartService does not define a remove_voucher method, so the test would fail with an attribute error before reaching the checkout assertion. CartService includes methods like get_or_create_cart, add_ticket, add_addon, remove_item, update_quantity, apply_voucher, get_summary, and get_summary_from_items, but no remove_voucher. tests/test_registration/test_checkout_service.pyL360-L365 src/django_program/registration/services/cart.pyL59-L60 src/django_program/registration/services/cart.pyL103-L104 src/django_program/registration/services/cart.pyL166-L167 src/django_program/registration/services/cart.pyL219-L220 src/django_program/registration/services/cart.pyL248-L249 src/django_program/registration/services/cart.pyL288-L289 src/django_program/registration/services/cart.pyL317-L318 src/django_program/registration/services/cart.pyL333-L334
The suggested replacement (cart.voucher = None + save(update_fields=["voucher", "updated_at"])) was the minimal way to simulate “voucher removed after adding ticket,” which is exactly what that test is trying to validate at checkout time. This also mirrors how voucher assignment is persisted in apply_voucher (same fields saved), just in reverse direction. tests/test_registration/test_checkout_service.pyL340-L368 src/django_program/registration/services/cart.pyL312-L314
Commands I used to verify
sed -n '320,390p' tests/test_registration/test_checkout_service.pyrg -n "class CartService|def remove_voucher|def get_or_create_cart|def apply_voucher|def get_summary_from_items" src testssed -n '50,360p' src/django_program/registration/services/cart.pynl -ba tests/test_registration/test_checkout_service.py | sed -n '346,374p'nl -ba src/django_program/registration/services/cart.py | sed -n '50,360p'python - <<'PY' ... duckduckgo query ... PY(web search attempt failed due proxy/tunnel 403)
If you want, I can also propose a cleaner API-level follow-up (adding CartService.remove_voucher) so tests and application code avoid direct model mutation.
There was a problem hiding this comment.
its wild to me you'd introduce a method that doesn't even exist
JacobCoffee
changed the title
1 participant
Codex generated this pull request, but encountered an unexpected error after generation. This is a placeholder PR message.
Codex Task