fix: redirect to existing payment session on paid booking retry#23096
fix: redirect to existing payment session on paid booking retry#23096anikdhabal merged 10 commits intomainfrom
Conversation
- Extract existing payment UID from unpaid bookings - Return paymentUid and paymentId when payment form should be shown - Prevents payment form bypass by redirecting to original payment session - Maintains payment continuity without creating duplicate bookings Fixes issue where canceled payments showed success page on rebooking Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
- Test verifies existing payment UID is returned on booking retry - Ensures no duplicate bookings are created in database - Validates paymentRequired flag is set correctly for unpaid bookings - Covers the payment redirect fix in handleNewBooking.ts Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
WalkthroughThe PR modifies handleNewBooking to treat existing bookings for paid events as payment-aware: it computes requiresPayment from paymentAppData.price, determines if an existing booking is paid, sets paymentRequired accordingly, and returns existing payment identifiers (paymentUid, paymentId) when appropriate. The booking repository's findOriginalRescheduledBooking include now fetches related payment records. Tests add a payment-retry scenario asserting idempotent booking and payment behavior for repeated paid-event booking attempts. Assessment against linked issues
Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
anikdhabal
changed the title
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
dosubot
bot
added
bookings
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/14/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (08/18/25)1 label was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/lib/server/repository/booking.ts (2)
827-833: Limit payment include to id/uid, order desc and take 1 — safe to applyVerified this repo call site: getValidBookingFromEventTypeForAttendee is only used by packages/features/bookings/lib/handleNewBooking.ts, and that caller only reads payment[0].id and payment[0].uid — changing the include to a single, ordered slim payment is safe.
- Files to change:
- packages/lib/server/repository/booking.ts (getValidBookingFromEventTypeForAttendee include block, ~lines 827–833)
Apply this diff:
include: { attendees: true, references: true, user: true, - payment: true, + payment: { + select: { id: true, uid: true }, + orderBy: { id: "desc" }, + take: 1, + }, },
684-688: Limit payment payload and return latest successful payment (deterministic ordering)The current include (payment: true) returns all payment columns in unspecified order. Downstream code expects to find a successful payment and the id/uid — returning a single, deterministic, small object is safer and smaller.
Files to change:
- packages/lib/server/repository/booking.ts — findOriginalRescheduledBooking include block (around the current lines ~684–688).
Apply this diff:
destinationCalendar: true, - payment: true, + payment: { + where: { success: true }, + select: { id: true, uid: true, success: true }, + orderBy: { id: "desc" }, + take: 1, + }, references: true, workflowReminders: true,Notes:
- We filter to successful payments, select only id/uid (and success so existing .find(payment => payment.success) still works), order by id desc and take 1 to get the newest successful payment deterministically.
- There are many other occurrences of include: { payment: true } across the repo — consider a follow-up PR to convert them to explicit select-based nested relations per the “select-only” guideline to avoid large payloads and brittle order-dependent logic.
🧹 Nitpick comments (2)
packages/features/bookings/lib/handleNewBooking.ts (1)
565-571: Edge cases and ordering for firstPayment selection.
firstPayment = existingBooking.payment[0]assumes payments are ordered and that the first one is the correct (active) session to reuse. Without explicit ordering, Prisma won’t guarantee this.- If ever a booking exists with
requiresPayment === true,existingBooking.paid === false, butexistingBooking.payment.length === 0(e.g., a previous session failed before a Payment row was created), current logic would treat it as “paid” and suppress the payment form.Recommendations:
- Pair this with the repository change to order and
take: 1(newest first) so[0]is deterministic.- Consider treating “no payments on unpaid booking” as “needs payment”:
const isPaidBooking = existingBooking.paid;const firstPayment = (requiresPayment && !isPaidBooking && existingBooking.payment.length) ? existingBooking.payment[0] : undefined;If you want, I can provide a precise diff for this block after you confirm expected behavior for the “unpaid but no payments” scenario.
packages/features/bookings/lib/handleNewBooking/test/fresh-booking.test.ts (1)
3283-3402: Solid coverage for “payment retry” behavior; minor nits.This test validates the intended regression fix:
- Reuses the existing booking UID on retry.
- Returns the original payment UID instead of creating a new session.
- Asserts no duplicate Booking rows and exactly one Payment row.
Minor suggestions:
- The test mocks Daily Video and Google Calendar but seeds only the Stripe app in
apps. They’re not exercised here (location is a plain string), so you can remove those extra mocks to reduce noise.- To align with repo guidelines on Prisma usage, optionally replace
include: { payment: true }in the test queries with a minimalselect(though this is less critical for tests).If helpful, I can send an updated test snippet removing unused mocks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/features/bookings/lib/handleNewBooking.ts(3 hunks)packages/features/bookings/lib/handleNewBooking/test/fresh-booking.test.ts(2 hunks)packages/lib/server/repository/booking.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/lib/server/repository/booking.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/test/fresh-booking.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/lib/server/repository/booking.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/test/fresh-booking.test.ts
🧬 Code Graph Analysis (2)
packages/features/bookings/lib/handleNewBooking.ts (1)
packages/features/bookings/lib/getBookingResponsesSchema.ts (1)
bookingResponse(14-14)
packages/features/bookings/lib/handleNewBooking/test/fresh-booking.test.ts (3)
packages/platform/libraries/index.ts (1)
handleNewBooking(35-35)apps/web/test/utils/bookingScenario/bookingScenario.ts (10)
getBooker(2178-2192)getOrganizer(1496-1552)TestData(1215-1487)getGoogleCalendarCredential(1168-1176)getStripeAppCredential(1206-1213)getScenarioData(1554-1640)createBookingScenario(954-985)mockSuccessfulVideoMeetingCreation(2022-2040)mockPaymentApp(2056-2081)mockCalendarToHaveNoBusySlots(1901-1914)apps/web/test/utils/bookingScenario/getMockRequestDataForBooking.ts (1)
getMockRequestDataForBooking(39-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: required
🔇 Additional comments (3)
packages/features/bookings/lib/handleNewBooking.ts (2)
554-554: Good: localizes “requires payment” decision next to existing-booking path.
requiresPaymentderived frompaymentAppData.price > 0is concise and consistent with the rest of the handler.
578-589: Surfacing paymentRequired/paymentUid/paymentId for retry looks correct.
paymentRequiredmaps toshouldShowPaymentForm, which is the right flag to signal the client to redirect back to the payment flow.- Returning
paymentUid/paymentIdenables seamless redirection to the same payment session and avoids duplicate bookings.This should address the “success page on retry” bug.
packages/features/bookings/lib/handleNewBooking/test/fresh-booking.test.ts (1)
10-11: Test infra import LGTM.Importing
prismaMockallows stable assertions against the in-memory DB state.
- Add deleteEventsAndMeetings call back to EventManager.reschedule() when changedOrganizer is true - Add unit test to verify calendar deletion during round robin reassignment - Fixes regression where calendar events weren't deleted from original host's calendar Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
github-actions
bot
added
billing
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/features/ee/round-robin/roundRobinReassignment.test.ts (2)
298-404: Add delegated-credential scenario to harden deletion pathTo prevent regressions for delegated credentials, add a variant where the booking reference has delegationCredentialId set (and possibly credentialId null/0). Assert deleteEventsAndMeetings receives that reference so delegated deletions are exercised.
I can draft the additional test block mirroring this one with a delegated reference if helpful.
298-404: Optional: reset spies between testsMinor isolation improvement: clear/restore spies/mocks after each test to avoid cross-test leakage. setupAndTeardown may cover some of this, but being explicit helps if another test modifies the same spies.
For example:
import { afterEach } from "vitest"; afterEach(() => { vi.clearAllMocks(); vi.restoreAllMocks(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/ee/round-robin/roundRobinReassignment.test.ts(1 hunks)packages/lib/EventManager.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/lib/EventManager.tspackages/features/ee/round-robin/roundRobinReassignment.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/lib/EventManager.tspackages/features/ee/round-robin/roundRobinReassignment.test.ts
🧠 Learnings (1)
📚 Learning: 2025-07-22T11:42:47.623Z
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Applied to files:
packages/features/ee/round-robin/roundRobinReassignment.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
packages/features/ee/round-robin/roundRobinReassignment.test.ts (1)
298-404: Great regression test covering organizer change pre-delete — LGTMThis test asserts the right behaviors (reschedule with changedOrganizer + prior destination calendar, and deleteEventsAndMeetings called with the original host’s calendar reference). It directly guards the new flow.
packages/features/bookings/lib/handleNewBooking/test/fresh-booking.test.ts
Outdated
Show resolved
Hide resolved
Pull request was converted to draft
- Changed from numbered list format to concise single line description - Test functionality remains unchanged and passes successfully Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
E2E results are ready! |
4 participants
Summary
Fixes payment bypass issue where users who cancel payment and retry booking see success page instead of payment form. Now returns existing payment UID to redirect users to original payment session, preventing duplicate bookings while ensuring payment completion.
This PR also adds unit tests to verify the payment redirect fix that ensures users who cancel payment and retry booking get redirected to the existing payment session instead of creating duplicate bookings.
Fixes CAL-6247
Fixes #23065