Add delegated ticket payment flow#177
Conversation
📝 WalkthroughWalkthroughThis PR introduces end-to-end ticketing functionality to support paid event registrations. It adds three new database tables for storing ticketing configurations, reservation records, and payment tracking; implements a payment client service for creating checkout sessions; provides callback verification and handling for payment confirmations; and extends the registration and RSVP flows to initiate paid ticketing when tier pricing is present. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Browser as Browser
participant RegRoute as Register<br/>Route
participant RSVPCtrl as RSVP<br/>Controller
participant TicketSvc as Ticketing<br/>Service
participant PaymentClient as Payment<br/>Client
participant PaymentProvider as Payment<br/>Provider
participant DB as Database
participant CallbackCtrl as Callback<br/>Controller
User->>Browser: Select tier & submit
Browser->>RegRoute: POST register
RegRoute->>RSVPCtrl: Handle RSVP submission
alt Tier requires payment
RSVPCtrl->>TicketSvc: startPaidRegistration()
TicketSvc->>DB: Create reservation<br/>(pending_payment)
DB-->>TicketSvc: reservationId
TicketSvc->>PaymentClient: createCheckout()
PaymentClient->>PaymentProvider: POST /v1/ticket-checkouts
PaymentProvider-->>PaymentClient: checkoutId, checkoutUrl
PaymentClient-->>TicketSvc: checkout details
TicketSvc->>DB: Save checkoutId to<br/>reservation
TicketSvc->>DB: Create payment record<br/>(requires_payment)
TicketSvc-->>RSVPCtrl: {requiresPayment:true, checkoutUrl}
RSVPCtrl-->>Browser: { ok: true, checkoutUrl }
Browser->>PaymentProvider: Redirect to checkout
User->>PaymentProvider: Complete payment
PaymentProvider->>CallbackCtrl: POST callback<br/>(ticket_payment.paid)
CallbackCtrl->>DB: Verify signature &<br/>lock reservation
CallbackCtrl->>DB: Validate callback<br/>vs reservation
CallbackCtrl->>DB: Create/update RSVP
CallbackCtrl->>DB: Sync RSVP answers<br/>from snapshot
CallbackCtrl->>DB: Mark reservation<br/>confirmed
CallbackCtrl->>DB: Record payment<br/>(paid)
DB-->>CallbackCtrl: Success
CallbackCtrl-->>PaymentProvider: { ok: true, rsvpId }
else Tier is free
RSVPCtrl->>TicketSvc: startPaidRegistration()
TicketSvc-->>RSVPCtrl: {requiresPayment:false}
RSVPCtrl->>DB: Standard RSVP flow
RSVPCtrl-->>Browser: { ok: true }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/controllers/events/update.ts (1)
320-330:⚠️ Potential issue | 🟠 MajorDon't delete tiers that still have pending ticket reservations.
This guard only checks
rsvps. In the new paid flow, users can have a pendingticket_reservationsrow for a tier before any RSVP exists, so this delete will hit theticket_reservations.tier_id -> event_tiers.idFK and turn a normal event edit into a 500 while someone is mid-checkout.Proposed fix
+import { actors, events, eventOrganizers, eventQuestions, eventTiers, groupMembers, places, rsvpAnswers, rsvps, ticketReservations } from "~/server/db/schema"; @@ for (const et of existingTiers) { if (!submittedTierIds.includes(et.id)) { const [rsvpRow] = await db .select({ count: sql<number>`count(*)::int` }) .from(rsvps) .where(eq(rsvps.tierId, et.id)); + + const [reservationRow] = await db + .select({ count: sql<number>`count(*)::int` }) + .from(ticketReservations) + .where(eq(ticketReservations.tierId, et.id)); - if (rsvpRow.count === 0) { + if (rsvpRow.count === 0 && reservationRow.count === 0) { await db.delete(eventTiers).where(eq(eventTiers.id, et.id)); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/controllers/events/update.ts` around lines 320 - 330, The current deletion loop for existingTiers only checks rsvps before removing an eventTiers row, but must also ensure there are no pending ticket_reservations pointing to that tier; update the logic inside the loop (where it queries rsvps and calls db.delete on eventTiers) to also query ticket_reservations (e.g., count rows where ticket_reservations.tier_id == et.id and status indicates a reservation) and only perform db.delete(eventTiers).where(eq(eventTiers.id, et.id)) when both the rsvps count and the ticket_reservations count are zero.src/server/controllers/events/rsvp.ts (1)
61-75:⚠️ Potential issue | 🟠 MajorPrevent duplicate pending reservations/checkouts for the same user and event.
The preflight conflict check only looks at
rsvps. In the paid path there is no RSVP until the callback confirms payment, so a double-submit or retry here will create a newticket_reservationsrow and a new checkout every time. Because the idempotency key is tied to the new reservation id, the payment service will not dedupe those requests for you.Suggested direction
+// Before startPaidRegistration(), look for an existing active reservation +// for this user/event/tier and either reuse its checkout details or reject +// the second attempt with a 409.You’ll likely also want a partial unique constraint for one active reservation per user/event/tier to make this robust under concurrent requests.
Also applies to: 77-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/controllers/events/rsvp.ts` around lines 61 - 75, The conflict check only looks at rsvps, so in the paid/reservation path (the ticket_reservations flow around the code handling checkout between lines ~77-167) you must also detect existing active/pending reservations to prevent duplicate ticket_reservations and repeated checkouts: add a DB query against ticket_reservations for the same userId, eventId (and tierId if applicable) filtering for statuses like "pending", "checkout", "reserved" (i.e., any non-final states) and return the same 409 response if one exists before creating a new reservation/checkout; additionally add a partial unique constraint at the DB level on (user_id, event_id, tier_id) for those active statuses to make this robust under concurrency and ensure any reservation creation code path (the create reservation / create checkout sections referenced around lines 77-167) handles a unique-constraint violation by returning a conflict rather than creating a duplicate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 10-11: Add a "packageManager" field to package.json to ensure CI
boots pnpm with the correct pinned version from the repo's lockfile; locate
package.json (near the "test" and "test:watch" scripts) and add e.g.
"packageManager": "pnpm@<version>" where <version> is the exact pnpm version
used to create the lockfile (match the version from your pnpm-lock.yaml or
project history) so pnpm/action-setup@v4 can resolve and install consistently in
CI.
In `@src/server/controllers/events/create.ts`:
- Around line 228-240: The controller currently contains business logic: calling
deriveNewEventTicketingSettings and directly persisting via
EventTicketingSettingsRepo.upsert; extract this into a shared service (e.g., a
new function like createOrUpdateEventTicketingSettingsService) that accepts
event.id, tiersToInsert, and DEFAULT_PORTONE_PROVIDER_ACCOUNT_ID, performs the
deriveNewEventTicketingSettings call and the upsert, and returns the persisted
settings; replace the inline block in the create controller with a single call
to that service, and reuse the same service from the update path to avoid
duplicated domain logic.
In `@src/server/controllers/events/update.ts`:
- Around line 400-416: The code unconditionally overwrites
EventTicketingSettings (setting legacy: false) after recomputing from tiers,
which destroys existing external/legacy settings; fix by first loading the
current ticketing row for event.id (e.g., via
EventTicketingSettingsRepo.getByEventId or a db select on the
eventTicketingSettings table), then merge instead of blindly upserting: if an
existing settings row exists and has legacy === true or mode === "external",
preserve its legacy, mode, provider, and providerAccountId values; otherwise
apply the values returned by deriveNewEventTicketingSettings; only set legacy to
false when there was no prior legacy row or when explicitly transitioning off
legacy. Ensure EventTicketingSettingsRepo.upsert is called with the merged
result rather than always forcing legacy: false.
In `@src/server/controllers/ticket-payments/callback.ts`:
- Around line 41-179: The controller is doing business logic (reservation
validation, capacity check, RSVP creation, answers sync, payment persistence)
that should live in the service layer; extract the whole DB transaction block
(the code touching ticketReservations/ticket_reservations,
checkCapacityAndDetermineStatus, rsvps, rsvpAnswers, and ticketPayments) into a
new service method (e.g., TicketPaymentService.processPaidReservation or
processReservationPayment) that returns the created rsvpId or throws
ServiceError on failures, then replace the controller's transaction with a
single call to that service and map ServiceError to HTTP responses in the
controller callback; ensure the service owns all DB operations and the
controller only handles HTTP parsing, auth, and response serialization.
In `@src/server/repositories/ticket-payments.ts`:
- Around line 39-69: Replace the read-then-write in markPaid with a single
atomic upsert: add a DB unique constraint on (reservation_id, checkout_id) in
the migration, then change the implementation that currently calls
findByReservationAndCheckout + insert/update to use a single
insert(...).onConflictDoUpdate(...) on the ticketPayments table (targeting
ticketPayments.reservationId and ticketPayments.checkoutId) and set the conflict
update to apply the paid state and provider fields (status: "paid",
providerPaymentId, providerTxId, paidAt, rawEvent, updatedAt) while ensuring
amount/currency are only provided on insert if desired; remove the separate
db.update(...) path and return the resulting row from the onConflict upsert.
In `@src/server/services/ticket-payment-callback.ts`:
- Around line 32-44: The function parseTicketPaymentPaidCallback currently calls
JSON.parse and immediately dereferences properties which throws on malformed
JSON or non-object values; wrap the JSON.parse in a try/catch and return null on
parse errors, then validate that the parsed value is a non-null object (e.g.
typeof value === "object" && value !== null) before checking properties like
value.type, reservationId, checkoutId, paymentId, provider, amount, currency,
and paidAt in parseTicketPaymentPaidCallback so malformed JSON or null bodies
return null instead of throwing.
In `@src/server/services/ticket-payment-client.ts`:
- Around line 48-58: The createCheckout method currently calls fetchImpl without
an AbortSignal; modify createCheckout to create an AbortController, set a 10s
timeout to call controller.abort(), pass controller.signal into the fetchImpl
options, and ensure you clear the timeout after the fetch completes; catch an
AbortError from the fetch and rethrow it as a TicketPaymentClientError
(preserving context like input.reservationId) so callers can fail fast.
In `@src/server/services/ticketing.ts`:
- Around line 141-190: Before creating a new reservation/checkout, query
existing reservations for (input.userId, input.event.id) using an existing deps
method (e.g., deps.findReservations / deps.getReservationsByUserEvent or add one
if missing) and guard: if any reservation.status === "confirmed" (or other
paid/active statuses) return/reject immediately; if an existing
reservation.status === "pending_payment" reuse that reservation id instead of
calling createReservation (update its expiresAt and/or reuse or create a new
checkout for it), setReservationCheckoutId and createPayment for that
reservation, and return the existing reservation.checkoutId/checkoutUrl;
otherwise proceed to call createReservation/createCheckout as currently
implemented. Ensure you reference and check the statuses "pending_payment" and
"confirmed" and update setReservationCheckoutId/createPayment flows to operate
on the reused reservation id when applicable.
- Around line 104-110: The service layer is throwing a plain TicketingError
which breaks the controller contract; change TicketingError so it extends the
application's ServiceError (import ServiceError) and preserve the existing code
and message fields (e.g., class TicketingError extends ServiceError) so
startPaidRegistration() and other service functions throw a ServiceError
subtype; update any throw sites that construct TicketingError to continue using
the same constructor signature and ensure callers expect a ServiceError for
consistent controller mapping.
---
Outside diff comments:
In `@src/server/controllers/events/rsvp.ts`:
- Around line 61-75: The conflict check only looks at rsvps, so in the
paid/reservation path (the ticket_reservations flow around the code handling
checkout between lines ~77-167) you must also detect existing active/pending
reservations to prevent duplicate ticket_reservations and repeated checkouts:
add a DB query against ticket_reservations for the same userId, eventId (and
tierId if applicable) filtering for statuses like "pending", "checkout",
"reserved" (i.e., any non-final states) and return the same 409 response if one
exists before creating a new reservation/checkout; additionally add a partial
unique constraint at the DB level on (user_id, event_id, tier_id) for those
active statuses to make this robust under concurrency and ensure any reservation
creation code path (the create reservation / create checkout sections referenced
around lines 77-167) handles a unique-constraint violation by returning a
conflict rather than creating a duplicate.
In `@src/server/controllers/events/update.ts`:
- Around line 320-330: The current deletion loop for existingTiers only checks
rsvps before removing an eventTiers row, but must also ensure there are no
pending ticket_reservations pointing to that tier; update the logic inside the
loop (where it queries rsvps and calls db.delete on eventTiers) to also query
ticket_reservations (e.g., count rows where ticket_reservations.tier_id == et.id
and status indicates a reservation) and only perform
db.delete(eventTiers).where(eq(eventTiers.id, et.id)) when both the rsvps count
and the ticket_reservations count are zero.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2e9653f-df4f-410e-8af9-c76dab1e16dc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
drizzle/0049_slimy_puma.sqldrizzle/meta/0049_snapshot.jsondrizzle/meta/_journal.jsonpackage.jsonsrc/routes/events/$eventId/register.tsxsrc/server-entry.tssrc/server/controllers/events/create.tssrc/server/controllers/events/rsvp.tssrc/server/controllers/events/update.tssrc/server/controllers/ticket-payments/callback.tssrc/server/db/schema.tssrc/server/repositories/event-ticketing-settings.tssrc/server/repositories/ticket-payments.tssrc/server/repositories/ticket-reservations.tssrc/server/services/ticket-payment-callback.test.tssrc/server/services/ticket-payment-callback.tssrc/server/services/ticket-payment-client.test.tssrc/server/services/ticket-payment-client.tssrc/server/services/ticketing-domain.test.tssrc/server/services/ticketing-domain.tssrc/server/services/ticketing.test.tssrc/server/services/ticketing.tsvitest.config.ts
| "test": "vitest run", | ||
| "test:watch": "vitest", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd '^pnpm-lock\.yaml$' -x sed -n '1,20p' {}
fd .github/workflows -t f -x rg -n "pnpm|action-setup|packageManager" {}Repository: moim-social/moim
Length of output: 838
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking package.json for packageManager field ==="
rg '"packageManager"' package.json || echo "packageManager field not found in package.json"
echo ""
echo "=== Checking pnpm-lock.yaml header for version hints ==="
head -30 pnpm-lock.yaml | head -30
echo ""
echo "=== Searching GitHub workflows for pnpm/packageManager configuration ==="
fd . '.github/workflows' -t f -x rg -n "pnpm|action-setup|packageManager" {}
echo ""
echo "=== Looking for any version pins in workflows ==="
fd . '.github/workflows' -t f -x cat {}Repository: moim-social/moim
Length of output: 1189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full GitHub workflow file(s) ==="
fd . '.github/workflows' -t f -x cat {}
echo ""
echo "=== Check if pnpm version is specified anywhere in the workflow file ==="
fd . '.github/workflows' -t f -x grep -n "with:" {} || echo "No 'with:' sections found"Repository: moim-social/moim
Length of output: 1525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Extract pnpm version from lockfile metadata ==="
# Look for pnpm version info in the lockfile
head -50 pnpm-lock.yaml | grep -i pnpm || echo "pnpm version not in header"
echo ""
echo "=== Check if there's a .pnpmfile.cjs or other version indicator ==="
fd -e pnpmfile -e 'pnpmrc' || echo "No pnpmfile found"
echo ""
echo "=== Inspect package.json to see dependencies and scripts ==="
sed -n '1,100p' package.jsonRepository: moim-social/moim
Length of output: 2684
Add packageManager to bootstrap pnpm in CI.
The CI workflow uses pnpm/action-setup@v4 without specifying a pnpm version, and package.json lacks a packageManager field. This prevents pnpm from being resolved before install, blocking the entire pipeline. Add the field to package.json with the version pinned in your repository's lockfile.
Suggested change
{
"name": "moim",
"version": "0.3.0",
"private": true,
+ "packageManager": "pnpm@<repo-pinned-version>",
"type": "module",To determine the correct version to pin, check your repository's git history or package manager documentation for the version used when the lockfile was created.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 10 - 11, Add a "packageManager" field to
package.json to ensure CI boots pnpm with the correct pinned version from the
repo's lockfile; locate package.json (near the "test" and "test:watch" scripts)
and add e.g. "packageManager": "pnpm@<version>" where <version> is the exact
pnpm version used to create the lockfile (match the version from your
pnpm-lock.yaml or project history) so pnpm/action-setup@v4 can resolve and
install consistently in CI.
| const ticketingSettings = deriveNewEventTicketingSettings( | ||
| tiersToInsert.map((tier) => ({ priceAmount: "priceAmount" in tier ? tier.priceAmount : null })), | ||
| process.env.DEFAULT_PORTONE_PROVIDER_ACCOUNT_ID, | ||
| ); | ||
| await EventTicketingSettingsRepo.upsert({ | ||
| eventId: event.id, | ||
| mode: ticketingSettings.mode, | ||
| provider: ticketingSettings.provider, | ||
| providerAccountId: ticketingSettings.providerAccountId, | ||
| currency: ticketingSettings.currency, | ||
| enabled: ticketingSettings.enabled, | ||
| legacy: false, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move ticketing-settings derivation/persistence out of the controller.
This block is domain logic plus repository orchestration inside the HTTP handler, and the PR already implies a matching copy in the update path. Extracting a shared service here will keep create/update behavior from drifting.
As per coding guidelines, "Controller functions must not contain business logic — orchestration and validation must be in the service layer".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/controllers/events/create.ts` around lines 228 - 240, The
controller currently contains business logic: calling
deriveNewEventTicketingSettings and directly persisting via
EventTicketingSettingsRepo.upsert; extract this into a shared service (e.g., a
new function like createOrUpdateEventTicketingSettingsService) that accepts
event.id, tiersToInsert, and DEFAULT_PORTONE_PROVIDER_ACCOUNT_ID, performs the
deriveNewEventTicketingSettings call and the upsert, and returns the persisted
settings; replace the inline block in the create controller with a single call
to that service, and reuse the same service from the update path to avoid
duplicated domain logic.
| const currentTiers = await db | ||
| .select({ priceAmount: eventTiers.priceAmount }) | ||
| .from(eventTiers) | ||
| .where(eq(eventTiers.eventId, event.id)); | ||
| const ticketingSettings = deriveNewEventTicketingSettings( | ||
| currentTiers, | ||
| process.env.DEFAULT_PORTONE_PROVIDER_ACCOUNT_ID, | ||
| ); | ||
| await EventTicketingSettingsRepo.upsert({ | ||
| eventId: event.id, | ||
| mode: ticketingSettings.mode, | ||
| provider: ticketingSettings.provider, | ||
| providerAccountId: ticketingSettings.providerAccountId, | ||
| currency: ticketingSettings.currency, | ||
| enabled: ticketingSettings.enabled, | ||
| legacy: false, | ||
| }); |
There was a problem hiding this comment.
Preserve legacy external ticketing when recomputing settings.
deriveNewEventTicketingSettings() only looks at tier prices, but this block overwrites the row unconditionally and forces legacy: false. For events backfilled from external_url as mode: "external", editing tiers here will silently convert them to free/paid and break the external checkout path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/controllers/events/update.ts` around lines 400 - 416, The code
unconditionally overwrites EventTicketingSettings (setting legacy: false) after
recomputing from tiers, which destroys existing external/legacy settings; fix by
first loading the current ticketing row for event.id (e.g., via
EventTicketingSettingsRepo.getByEventId or a db select on the
eventTicketingSettings table), then merge instead of blindly upserting: if an
existing settings row exists and has legacy === true or mode === "external",
preserve its legacy, mode, provider, and providerAccountId values; otherwise
apply the values returned by deriveNewEventTicketingSettings; only set legacy to
false when there was no prior legacy row or when explicitly transitioning off
legacy. Ensure EventTicketingSettingsRepo.upsert is called with the merged
result rather than always forcing legacy: false.
| try { | ||
| let rsvpId: string | null = null; | ||
|
|
||
| await db.transaction(async (tx) => { | ||
| const rows = await tx.execute(sql` | ||
| SELECT | ||
| id, | ||
| event_id AS "eventId", | ||
| tier_id AS "tierId", | ||
| user_id AS "userId", | ||
| status, | ||
| checkout_id AS "checkoutId", | ||
| provider, | ||
| amount, | ||
| currency, | ||
| answers_snapshot AS "answersSnapshot", | ||
| expires_at AS "expiresAt" | ||
| FROM ticket_reservations | ||
| WHERE id = ${payload.reservationId} | ||
| LIMIT 1 | ||
| FOR UPDATE | ||
| `); | ||
| const reservation = rows.rows[0] as ReservationRow | undefined; | ||
|
|
||
| if (!reservation) { | ||
| throw new Response(JSON.stringify({ error: "Reservation not found" }), { status: 404 }); | ||
| } | ||
| if (reservation.status === "confirmed") { | ||
| return; | ||
| } | ||
| if ( | ||
| reservation.status !== "pending_payment" || | ||
| reservation.checkoutId !== payload.checkoutId || | ||
| reservation.provider !== payload.provider || | ||
| reservation.amount !== payload.amount || | ||
| reservation.currency !== payload.currency | ||
| ) { | ||
| throw new Response(JSON.stringify({ error: "Callback does not match reservation" }), { status: 409 }); | ||
| } | ||
| if (reservation.expiresAt <= new Date()) { | ||
| await tx | ||
| .update(ticketReservations) | ||
| .set({ status: "expired", updatedAt: new Date() }) | ||
| .where(eq(ticketReservations.id, reservation.id)); | ||
| throw new Response(JSON.stringify({ error: "Reservation expired" }), { status: 409 }); | ||
| } | ||
| if (!reservation.userId || !reservation.tierId) { | ||
| throw new Response(JSON.stringify({ error: "Reservation cannot create RSVP" }), { status: 409 }); | ||
| } | ||
|
|
||
| const [tier] = await tx | ||
| .select({ capacity: eventTiers.capacity }) | ||
| .from(eventTiers) | ||
| .where(eq(eventTiers.id, reservation.tierId)) | ||
| .limit(1); | ||
|
|
||
| const finalStatus = | ||
| tier?.capacity != null && tier.capacity > 0 | ||
| ? await checkCapacityAndDetermineStatus(tx as any, reservation.tierId, tier.capacity) | ||
| : "accepted"; | ||
|
|
||
| const [rsvp] = await tx | ||
| .insert(rsvps) | ||
| .values({ | ||
| userId: reservation.userId, | ||
| eventId: reservation.eventId, | ||
| tierId: reservation.tierId, | ||
| status: finalStatus, | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: [rsvps.userId, rsvps.eventId], | ||
| targetWhere: sql`user_id IS NOT NULL`, | ||
| set: { status: finalStatus, tierId: reservation.tierId }, | ||
| }) | ||
| .returning({ id: rsvps.id }); | ||
| rsvpId = rsvp.id; | ||
|
|
||
| const answers = Array.isArray(reservation.answersSnapshot) | ||
| ? reservation.answersSnapshot.filter((answer): answer is { questionId: string; answer: string } => ( | ||
| typeof answer === "object" && | ||
| answer != null && | ||
| typeof (answer as { questionId?: unknown }).questionId === "string" && | ||
| typeof (answer as { answer?: unknown }).answer === "string" && | ||
| (answer as { answer: string }).answer.trim().length > 0 | ||
| )) | ||
| : []; | ||
|
|
||
| await tx.delete(rsvpAnswers).where(eq(rsvpAnswers.rsvpId, rsvp.id)); | ||
| if (answers.length > 0) { | ||
| await tx.insert(rsvpAnswers).values( | ||
| answers.map((answer) => ({ | ||
| rsvpId: rsvp.id, | ||
| userId: reservation.userId, | ||
| eventId: reservation.eventId, | ||
| questionId: answer.questionId, | ||
| answer: answer.answer, | ||
| })), | ||
| ); | ||
| } | ||
|
|
||
| await tx | ||
| .update(ticketReservations) | ||
| .set({ status: "confirmed", rsvpId: rsvp.id, updatedAt: new Date() }) | ||
| .where(eq(ticketReservations.id, reservation.id)); | ||
|
|
||
| const [existingPayment] = await tx | ||
| .select({ id: ticketPayments.id }) | ||
| .from(ticketPayments) | ||
| .where(and(eq(ticketPayments.reservationId, reservation.id), eq(ticketPayments.checkoutId, payload.checkoutId))) | ||
| .limit(1); | ||
|
|
||
| const paidAt = new Date(payload.paidAt); | ||
| if (existingPayment) { | ||
| await tx | ||
| .update(ticketPayments) | ||
| .set({ | ||
| status: "paid", | ||
| providerPaymentId: payload.paymentId, | ||
| providerTxId: payload.txId ?? null, | ||
| paidAt, | ||
| rawEvent: payload, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(ticketPayments.id, existingPayment.id)); | ||
| } else { | ||
| await tx.insert(ticketPayments).values({ | ||
| reservationId: reservation.id, | ||
| provider: payload.provider, | ||
| providerPaymentId: payload.paymentId, | ||
| providerTxId: payload.txId ?? null, | ||
| checkoutId: payload.checkoutId, | ||
| status: "paid", | ||
| amount: payload.amount, | ||
| currency: payload.currency, | ||
| paidAt, | ||
| rawEvent: payload, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move the paid-confirmation workflow out of the controller.
This block is doing reservation validation, capacity resolution, RSVP mutation, answer syncing, and payment persistence directly in the endpoint. That makes the controller own business logic instead of just HTTP translation, and it bypasses the repo’s ServiceError mapping pattern.
As per coding guidelines "Controller functions must not contain business logic — orchestration and validation must be in the service layer" and "Controller functions must handle HTTP concerns (auth check, body parsing, status codes, response serialization) and catch ServiceError to map to HTTP responses".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/controllers/ticket-payments/callback.ts` around lines 41 - 179,
The controller is doing business logic (reservation validation, capacity check,
RSVP creation, answers sync, payment persistence) that should live in the
service layer; extract the whole DB transaction block (the code touching
ticketReservations/ticket_reservations, checkCapacityAndDetermineStatus, rsvps,
rsvpAnswers, and ticketPayments) into a new service method (e.g.,
TicketPaymentService.processPaidReservation or processReservationPayment) that
returns the created rsvpId or throws ServiceError on failures, then replace the
controller's transaction with a single call to that service and map ServiceError
to HTTP responses in the controller callback; ensure the service owns all DB
operations and the controller only handles HTTP parsing, auth, and response
serialization.
| const existing = await findByReservationAndCheckout(reservationId, values.checkoutId); | ||
|
|
||
| if (existing) { | ||
| const [row] = await db | ||
| .update(ticketPayments) | ||
| .set({ | ||
| status: "paid", | ||
| providerPaymentId: values.providerPaymentId, | ||
| providerTxId: values.providerTxId ?? null, | ||
| paidAt: values.paidAt, | ||
| rawEvent: values.rawEvent, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(ticketPayments.id, existing.id)) | ||
| .returning(); | ||
| return row; | ||
| } | ||
|
|
||
| return insert({ | ||
| reservationId, | ||
| checkoutId: values.checkoutId, | ||
| provider: values.provider, | ||
| providerPaymentId: values.providerPaymentId, | ||
| providerTxId: values.providerTxId ?? null, | ||
| status: "paid", | ||
| amount: values.amount, | ||
| currency: values.currency, | ||
| paidAt: values.paidAt, | ||
| rawEvent: values.rawEvent, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Make markPaid() an atomic upsert.
This read-then-write sequence is racy. Two callback retries can both observe “no existing row” and insert separate "paid" records for the same reservationId/checkoutId, which corrupts the payment ledger. Please back this with a DB unique constraint and collapse the logic to a single onConflictDoUpdate path.
Proposed fix
export async function markPaid(
reservationId: string,
values: {
@@
},
): Promise<TicketPayment> {
- const existing = await findByReservationAndCheckout(reservationId, values.checkoutId);
-
- if (existing) {
- const [row] = await db
- .update(ticketPayments)
- .set({
- status: "paid",
- providerPaymentId: values.providerPaymentId,
- providerTxId: values.providerTxId ?? null,
- paidAt: values.paidAt,
- rawEvent: values.rawEvent,
- updatedAt: new Date(),
- })
- .where(eq(ticketPayments.id, existing.id))
- .returning();
- return row;
- }
-
- return insert({
- reservationId,
- checkoutId: values.checkoutId,
- provider: values.provider,
- providerPaymentId: values.providerPaymentId,
- providerTxId: values.providerTxId ?? null,
- status: "paid",
- amount: values.amount,
- currency: values.currency,
- paidAt: values.paidAt,
- rawEvent: values.rawEvent,
- });
+ const [row] = await db
+ .insert(ticketPayments)
+ .values({
+ reservationId,
+ checkoutId: values.checkoutId,
+ provider: values.provider,
+ providerPaymentId: values.providerPaymentId,
+ providerTxId: values.providerTxId ?? null,
+ status: "paid",
+ amount: values.amount,
+ currency: values.currency,
+ paidAt: values.paidAt,
+ rawEvent: values.rawEvent,
+ })
+ .onConflictDoUpdate({
+ target: [ticketPayments.reservationId, ticketPayments.checkoutId],
+ set: {
+ status: "paid",
+ providerPaymentId: values.providerPaymentId,
+ providerTxId: values.providerTxId ?? null,
+ paidAt: values.paidAt,
+ rawEvent: values.rawEvent,
+ updatedAt: new Date(),
+ },
+ })
+ .returning();
+ return row;
}This needs the matching unique constraint in the schema/migration on (reservation_id, checkout_id).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/repositories/ticket-payments.ts` around lines 39 - 69, Replace the
read-then-write in markPaid with a single atomic upsert: add a DB unique
constraint on (reservation_id, checkout_id) in the migration, then change the
implementation that currently calls findByReservationAndCheckout + insert/update
to use a single insert(...).onConflictDoUpdate(...) on the ticketPayments table
(targeting ticketPayments.reservationId and ticketPayments.checkoutId) and set
the conflict update to apply the paid state and provider fields (status: "paid",
providerPaymentId, providerTxId, paidAt, rawEvent, updatedAt) while ensuring
amount/currency are only provided on insert if desired; remove the separate
db.update(...) path and return the resulting row from the onConflict upsert.
| export function parseTicketPaymentPaidCallback(body: string): TicketPaymentPaidCallback | null { | ||
| const value = JSON.parse(body) as Partial<TicketPaymentPaidCallback>; | ||
| if ( | ||
| value.type !== "ticket_payment.paid" || | ||
| typeof value.reservationId !== "string" || | ||
| typeof value.checkoutId !== "string" || | ||
| typeof value.paymentId !== "string" || | ||
| typeof value.provider !== "string" || | ||
| typeof value.amount !== "number" || | ||
| typeof value.currency !== "string" || | ||
| typeof value.paidAt !== "string" | ||
| ) { | ||
| return null; |
There was a problem hiding this comment.
Guard JSON parsing before dereferencing the payload.
Line 33 can still throw for malformed JSON, and valid-but-non-object bodies like null will also throw on value.type. That turns a bad callback into a 500 instead of the intended null/400 path.
Proposed fix
export function parseTicketPaymentPaidCallback(body: string): TicketPaymentPaidCallback | null {
- const value = JSON.parse(body) as Partial<TicketPaymentPaidCallback>;
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(body);
+ } catch {
+ return null;
+ }
+ if (!parsed || typeof parsed !== "object") {
+ return null;
+ }
+ const value = parsed as Partial<TicketPaymentPaidCallback>;
if (
value.type !== "ticket_payment.paid" ||
typeof value.reservationId !== "string" ||🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/services/ticket-payment-callback.ts` around lines 32 - 44, The
function parseTicketPaymentPaidCallback currently calls JSON.parse and
immediately dereferences properties which throws on malformed JSON or non-object
values; wrap the JSON.parse in a try/catch and return null on parse errors, then
validate that the parsed value is a non-null object (e.g. typeof value ===
"object" && value !== null) before checking properties like value.type,
reservationId, checkoutId, paymentId, provider, amount, currency, and paidAt in
parseTicketPaymentPaidCallback so malformed JSON or null bodies return null
instead of throwing.
| async createCheckout(input: CreateTicketCheckoutInput): Promise<CreateTicketCheckoutOutput> { | ||
| const response = await this.fetchImpl(new URL("/v1/ticket-checkouts", this.config.baseUrl), { | ||
| method: "POST", | ||
| headers: { | ||
| "Accept": "application/json", | ||
| "Authorization": `Bearer ${this.config.token}`, | ||
| "Content-Type": "application/json", | ||
| "Idempotency-Key": `reservation:${input.reservationId}`, | ||
| }, | ||
| body: JSON.stringify(input), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "ticket-payment-client.ts" | head -5Repository: moim-social/moim
Length of output: 107
🏁 Script executed:
# Check file exists and read it
wc -l src/server/services/ticket-payment-client.tsRepository: moim-social/moim
Length of output: 108
🏁 Script executed:
# Read the full file to understand the implementation
cat -n src/server/services/ticket-payment-client.tsRepository: moim-social/moim
Length of output: 3142
🏁 Script executed:
# Search for ServiceError in the codebase
rg "ServiceError" --type ts --type tsx -A 3 -B 1 | head -40Repository: moim-social/moim
Length of output: 88
🏁 Script executed:
# Check if there are other service layer implementations to see error patterns
fd -e ts -path "*/services/*" | head -10 | xargs -I {} basename {} | sort | uniqRepository: moim-social/moim
Length of output: 229
🏁 Script executed:
# Look for how other services handle external calls
rg "fetch" src/server/services --type ts -A 5 | head -60Repository: moim-social/moim
Length of output: 4417
🏁 Script executed:
# Search for ServiceError definition in the codebase
rg "class ServiceError|interface ServiceError" -A 5Repository: moim-social/moim
Length of output: 275
🏁 Script executed:
# Check what error types are used in other services
rg "class.*Error extends Error" src/server/services -A 3 | head -50Repository: moim-social/moim
Length of output: 890
🏁 Script executed:
# Look at the kakao.ts file to see the pattern
cat -n src/server/services/poi-search/kakao.ts | head -60Repository: moim-social/moim
Length of output: 2146
🏁 Script executed:
# Search for actual ServiceError usage in the services directory
rg "ServiceError" src/server/services -A 2 -B 2Repository: moim-social/moim
Length of output: 42
🏁 Script executed:
# Check if AGENTS.md is actually defining ServiceError or just documenting
head -50 AGENTS.md | rg -A 10 "ServiceError"Repository: moim-social/moim
Length of output: 42
🏁 Script executed:
# Look for where ServiceError is imported/exported from
find src -name "*.ts" -exec rg "export.*ServiceError|import.*ServiceError" {} +Repository: moim-social/moim
Length of output: 42
🏁 Script executed:
# Check what's in AGENTS.md to understand ServiceError
cat AGENTS.md | grep -A 10 "ServiceError"Repository: moim-social/moim
Length of output: 1809
🏁 Script executed:
# Look at how ticket-payment-client is used in the codebase
rg "TicketPaymentClient" -A 3 -B 1 | head -80Repository: moim-social/moim
Length of output: 5972
🏁 Script executed:
# Check if there are any timeout/abort patterns already in services
rg "AbortController|setTimeout.*abort|signal:" src/server/services | head -20Repository: moim-social/moim
Length of output: 42
🏁 Script executed:
# Check how TicketPaymentClient is used in the service layer
cat -n src/server/controllers/events/rsvp.ts | head -50Repository: moim-social/moim
Length of output: 2541
🏁 Script executed:
# Look for any integration of TicketPaymentClient in a service wrapper
rg "TicketPaymentClient\|startPaidRegistration" src/server/services/ticketing.ts -A 5 -B 2 | head -80Repository: moim-social/moim
Length of output: 42
🏁 Script executed:
# Read the ticketing service to see how payment client is integrated
cat -n src/server/services/ticketing.tsRepository: moim-social/moim
Length of output: 6488
🏁 Script executed:
# Check the tests to see if they would pass with the timeout fix
cat -n src/server/services/ticket-payment-client.test.tsRepository: moim-social/moim
Length of output: 3171
Add timeout and abort handling to prevent indefinite hangs on stalled payment service.
The createCheckout fetch call has no timeout or abort signal. If the payment service stalls, this request will hang indefinitely, blocking RSVP submission and consuming request workers. Wrap the fetch in an AbortController with a reasonable timeout (10s) and translate AbortError to TicketPaymentClientError so callers can fail fast.
⏱️ Suggested fix
export class TicketPaymentClient {
private readonly fetchImpl: typeof fetch;
constructor(private readonly config: TicketPaymentClientConfig) {
this.fetchImpl = config.fetchImpl ?? fetch;
}
async createCheckout(input: CreateTicketCheckoutInput): Promise<CreateTicketCheckoutOutput> {
- const response = await this.fetchImpl(new URL("/v1/ticket-checkouts", this.config.baseUrl), {
- method: "POST",
- headers: {
- "Accept": "application/json",
- "Authorization": `Bearer ${this.config.token}`,
- "Content-Type": "application/json",
- "Idempotency-Key": `reservation:${input.reservationId}`,
- },
- body: JSON.stringify(input),
- });
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 10_000);
+
+ let response: Response;
+ try {
+ response = await this.fetchImpl(new URL("/v1/ticket-checkouts", this.config.baseUrl), {
+ method: "POST",
+ headers: {
+ "Accept": "application/json",
+ "Authorization": `Bearer ${this.config.token}`,
+ "Content-Type": "application/json",
+ "Idempotency-Key": `reservation:${input.reservationId}`,
+ },
+ body: JSON.stringify(input),
+ signal: controller.signal,
+ });
+ } catch (error) {
+ if (error instanceof DOMException && error.name === "AbortError") {
+ throw new TicketPaymentClientError("CHECKOUT_CREATE_TIMEOUT");
+ }
+ throw error;
+ } finally {
+ clearTimeout(timeout);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async createCheckout(input: CreateTicketCheckoutInput): Promise<CreateTicketCheckoutOutput> { | |
| const response = await this.fetchImpl(new URL("/v1/ticket-checkouts", this.config.baseUrl), { | |
| method: "POST", | |
| headers: { | |
| "Accept": "application/json", | |
| "Authorization": `Bearer ${this.config.token}`, | |
| "Content-Type": "application/json", | |
| "Idempotency-Key": `reservation:${input.reservationId}`, | |
| }, | |
| body: JSON.stringify(input), | |
| }); | |
| async createCheckout(input: CreateTicketCheckoutInput): Promise<CreateTicketCheckoutOutput> { | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 10_000); | |
| let response: Response; | |
| try { | |
| response = await this.fetchImpl(new URL("/v1/ticket-checkouts", this.config.baseUrl), { | |
| method: "POST", | |
| headers: { | |
| "Accept": "application/json", | |
| "Authorization": `Bearer ${this.config.token}`, | |
| "Content-Type": "application/json", | |
| "Idempotency-Key": `reservation:${input.reservationId}`, | |
| }, | |
| body: JSON.stringify(input), | |
| signal: controller.signal, | |
| }); | |
| } catch (error) { | |
| if (error instanceof DOMException && error.name === "AbortError") { | |
| throw new TicketPaymentClientError("CHECKOUT_CREATE_TIMEOUT"); | |
| } | |
| throw error; | |
| } finally { | |
| clearTimeout(timeout); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/services/ticket-payment-client.ts` around lines 48 - 58, The
createCheckout method currently calls fetchImpl without an AbortSignal; modify
createCheckout to create an AbortController, set a 10s timeout to call
controller.abort(), pass controller.signal into the fetchImpl options, and
ensure you clear the timeout after the fetch completes; catch an AbortError from
the fetch and rethrow it as a TicketPaymentClientError (preserving context like
input.reservationId) so callers can fail fast.
| export class TicketingError extends Error { | ||
| constructor( | ||
| public readonly code: string, | ||
| message = code, | ||
| ) { | ||
| super(message); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use ServiceError for service-layer failures.
startPaidRegistration() is part of the service layer, but its failure type is a plain custom Error. That breaks the controller contract for catching and mapping service errors consistently.
As per coding guidelines "Service layer functions must return typed results and throw ServiceError — never return Response objects".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/services/ticketing.ts` around lines 104 - 110, The service layer
is throwing a plain TicketingError which breaks the controller contract; change
TicketingError so it extends the application's ServiceError (import
ServiceError) and preserve the existing code and message fields (e.g., class
TicketingError extends ServiceError) so startPaidRegistration() and other
service functions throw a ServiceError subtype; update any throw sites that
construct TicketingError to continue using the same constructor signature and
ensure callers expect a ServiceError for consistent controller mapping.
| const reservation = await deps.createReservation({ | ||
| eventId: input.event.id, | ||
| tierId: input.tier.id, | ||
| userId: input.userId, | ||
| provider: setting.provider, | ||
| providerAccountId: setting.providerAccountId, | ||
| amount, | ||
| currency: setting.currency, | ||
| status: "pending_payment", | ||
| answersSnapshot: input.answers, | ||
| expiresAt: deps.addMinutes(deps.now(), 10), | ||
| }); | ||
|
|
||
| const checkout = await deps.createCheckout( | ||
| { | ||
| provider: "portone", | ||
| paymentMethodFamily: "easy_pay", | ||
| reservationId: reservation.id, | ||
| eventId: input.event.id, | ||
| tierId: input.tier.id, | ||
| orderName: `${input.event.title} - ${input.tier.name}`, | ||
| amount, | ||
| currency: setting.currency, | ||
| customer: { | ||
| moimUserId: input.userId, | ||
| ...input.customer, | ||
| }, | ||
| successUrl: `${input.baseUrl}/events/${input.event.id}/register?payment=success`, | ||
| cancelUrl: `${input.baseUrl}/events/${input.event.id}/register?payment=cancel`, | ||
| callbackUrl: input.callbackUrl, | ||
| }, | ||
| `reservation:${reservation.id}`, | ||
| ); | ||
|
|
||
| await deps.setReservationCheckoutId(reservation.id, checkout.checkoutId); | ||
| await deps.createPayment({ | ||
| reservationId: reservation.id, | ||
| checkoutId: checkout.checkoutId, | ||
| provider: setting.provider, | ||
| status: "requires_payment", | ||
| amount, | ||
| currency: setting.currency, | ||
| }); | ||
|
|
||
| return { | ||
| requiresPayment: true, | ||
| reservationId: reservation.id, | ||
| checkoutId: checkout.checkoutId, | ||
| checkoutUrl: checkout.checkoutUrl, | ||
| }; |
There was a problem hiding this comment.
Guard against duplicate active paid registrations before creating a new reservation.
This always creates a fresh reservation and checkout for the same user/event. If the user starts two sessions and both succeed, the callback path upserts RSVPs by (userId, eventId), so both payments can be accepted while only one registration survives. Reuse an existing pending reservation or reject while a pending/confirmed registration already exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/services/ticketing.ts` around lines 141 - 190, Before creating a
new reservation/checkout, query existing reservations for (input.userId,
input.event.id) using an existing deps method (e.g., deps.findReservations /
deps.getReservationsByUserEvent or add one if missing) and guard: if any
reservation.status === "confirmed" (or other paid/active statuses) return/reject
immediately; if an existing reservation.status === "pending_payment" reuse that
reservation id instead of calling createReservation (update its expiresAt and/or
reuse or create a new checkout for it), setReservationCheckoutId and
createPayment for that reservation, and return the existing
reservation.checkoutId/checkoutUrl; otherwise proceed to call
createReservation/createCheckout as currently implemented. Ensure you reference
and check the statuses "pending_payment" and "confirmed" and update
setReservationCheckoutId/createPayment flows to operate on the reused
reservation id when applicable.
Summary
Adds the Moim-side ticket payment domain for paid event tiers, delegating checkout creation and payment confirmation to an external payment service. This includes payment-specific schema, repositories, service logic, RSVP checkout handoff, signed payment callbacks, and return URL handling.
Why
ticket_reservations;rsvpsare created only after a signed paid callback.Commit History
Test Plan
pnpm test -- src/server/services/ticketing-domain.test.ts src/server/services/ticketing.test.ts src/server/services/ticket-payment-client.test.ts src/server/services/ticket-payment-callback.test.tspnpm test -- src/server/services/ticketing.test.tspnpm typecheckcurrently still fails on pre-existing calendar import, binary response BodyInit, and missing@types/pgissues unrelated to this PR.payment-staging.Assisted-By: Codex(gpt-5)
Summary by CodeRabbit
New Features
Tests
Chores