Uh oh!
There was an error while loading. Please reload this page.
refactor(api): split booking into services, guests and typed responses - #302
Merged
Conversation
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
harshtandiyaforce-pushed
the
refactor/api-booking
branch
from
July 29, 2026 16:38
681ebb1 to
2d93bc3Compareharshtandiyaforce-pushed
the
refactor/api-tickets
branch
from
July 29, 2026 16:38
10feeae to
0a2790cCompareContributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
harshtandiyaforce-pushed
the
refactor/api-tickets
branch
from
July 29, 2026 16:45
0a2790c to
af8bbadCompareharshtandiyaforce-pushed
the
refactor/api-booking
branch
from
July 29, 2026 16:45
2d93bc3 to
9999c82CompareContributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
harshtandiyaforce-pushed
the
refactor/api-tickets
branch
from
July 29, 2026 16:53
af8bbad to
dfe6e8fCompareharshtandiyaforce-pushed
the
refactor/api-booking
branch
from
July 29, 2026 16:53
9999c82 to
2b651a7CompareContributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
harshtandiyaforce-pushed
the
refactor/api-booking
branch
from
July 29, 2026 17:00
2b651a7 to
8d2c94aCompareharshtandiyaforce-pushed
the
refactor/api-tickets
branch
from
July 29, 2026 17:00
dfe6e8f to
481ea93CompareContributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
harshtandiyaforce-pushed
the
refactor/api-tickets
branch
from
July 29, 2026 17:17
481ea93 to
0d32345Compareharshtandiyaforce-pushed
the
refactor/api-booking
branch
from
July 29, 2026 17:17
8d2c94a to
eb3aad8CompareContributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
booking/__init__.py held 676 lines: six endpoints, a 200-line process_booking taking 16 flat parameters, guest OTP delivery and verification, HMAC access tokens, and three read payloads assembled inline. process_booking becomes BookingRequest + BookingService, with the body split across resolve_user, verify_guest, append_utm_parameters, append_attendees and the three finalize branches. Because a BaseModel parameter reshapes the wire payload, BookingForm.vue nests its payload under "booking" — all three paths (plain, OTP, offline) already funnel through submitBooking, so that is one call site. Attendee rows stay list[dict]: ticket type names arrive as ints and typing them would make frappe's arg validation reject the payload. Only four named errors, not one per throw. The OTP failures carry text the dashboard branches on — BookingForm.vue closes the modal on the "Too many" and "expired" fragments — so those three keep their exact messages, and RegistrationsClosed is a 409. Every other throw stays a generic 417 with the message it already had. APIResponse.__json__ no longer calls model_dump(). pydantic 2.13's polymorphic serialization raises "'None' is not an instance of SchemaSerializer" for any frappe._dict sitting in an Any field, because _dict.__getattr__ is dict.get and so fakes a __pydantic_serializer__; no model_dump flag avoids it. __json__ now returns the raw field values and deep-converts only nested APIResponse models, leaving rows and documents to json_handler as before. Earlier domains never hit this — their query rows were re-wrapped in typed models. create_add_on_doc moves from tickets/ to booking/, its only caller, and OFFLINE_PAYMENT_METHOD now lives beside the service that writes it. send_guest_booking_otp and process_booking are methods=["POST"], continuing the sweep started in forms. The three doctype test modules keep their ~30 flat call sites; a three-line shim in each wraps the endpoint, taking kwargs in and returning __json__() out. test_buzz_event's patches follow are_registrations_closed into services. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
get_booking_details had no permission check at all: any logged-in user could read any booking by id, including every attendee's email and the ticket QR payloads. Its sibling get_booking_confirmation has always checked. The guard sits in build_booking_details, which every caller routes through. The owner-or-permission shape matches create_cancellation_request, and the exception and message are the ones get_booking_confirmation already throws, so no new error class and no dashboard change. Nothing legitimate is tightened out: the only routes into the page are BookingsList, which filters by user: session.user, and the post-booking redirect for the booking just made. Three tests cover it — owner reads, another user gets a PermissionError, a System Manager reads any booking. Verified the middle one fails with the guard removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
harshtandiyaforce-pushed
the
refactor/api-booking
branch
from
July 29, 2026 18:12
eb3aad8 to
705f754CompareContributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin main
git worktree add -d .worktree/backport-302-to-main origin/main
cd .worktree/backport-302-to-main
git switch --create backport-302-to-main
git cherry-pick -x 6de3cd0a3b5b7c484d18d26e912ac0f2c22900d3 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #301 — read that first. Last domain PR; PR 7 (the
require_type_annotated_api_methodsflip) is next.booking/__init__.py676 lines → 49, overservices.py,guests.py,event_data.py,details.py,coupons.py,schemas.py,exceptions.py.process_booking(200 lines, 16 flat params) becomesBookingRequest+BookingService.Second commit fixes an auth hole found while splitting:
get_booking_detailshad no permission check — any logged-in user could read any booking by id, attendee emails and ticket QR payloads included. Reproduced against a real booking, then blocked. Guard mirrorscreate_cancellation_request; message and exception are the onesget_booking_confirmationalready threw.Things the diff won't tell you:
APIResponse.__json__no longer callsmodel_dump()— shared plumbing, so flag it if you'd rather it went back into refactor: split buzz/api into domain packages #295. pydantic 2.13 raises'None' is not an instance of 'SchemaSerializer'for anyfrappe._dictin anAnyfield:_dict.__getattr__isdict.get, so it fakes a__pydantic_serializer__. Nomodel_dumpflag avoids it. Earlier domains re-wrapped their rows in typed models and never hit it.process_bookingreshapes the wire payload. Client now posts{"booking": {...}};BookingForm.vue:1293nests it. Plain/OTP/offline all funnel throughsubmitBooking, so it's one call site.BookingForm.vue:1325closes the modal on the"Too many"and"expired"fragments — so those keep exact text. Everything else stays a generic 417 with its existing message.list[dict]: ticket type names arrive as ints, and typing them makes frappe's arg validation reject the payload.create_add_on_docmoved out oftickets/(only caller was here).send_guest_booking_otpandprocess_bookingpinned to POST.get_booking_details.venueis now always present asnull—BookingEventInfo.vuealreadyv-if-guards it.Checks: 234 Python tests green across 12 modules (19 new in
test_booking.py); all reads byte-identical vs a pre-change snapshot over 8 events × 2 users, a booking and 6 coupons; ruff + semgrep clean;yarn typecheckand unit tests pass; E2E 17/17 headed (event-booking,offline-payment). Not run: the two guest OTP specs pass in CI but not locally — they read the OTP off the response, which needsfrappe.in_teston the server, and they fail identically on the base branch.