test: add comprehensive unit tests for no-show fee payment functions#23626
test: add comprehensive unit tests for no-show fee payment functions#23626
Conversation
- Add configurable time threshold (minutes/hours/days) for cancellation fees - Show warnings during booking submission and cancellation - Automatically charge fees when bookings cancelled within threshold - Exempt organizer/admin cancellations from fees - Extend existing no-show fee infrastructure Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
- Use Record<string, unknown> instead of any for metadata type - Add proper type assertions for nested metadata properties - Maintain type safety while avoiding ESLint no-explicit-any warning Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
- Add proper type guard for metadata JsonValue from Prisma - Ensure metadata is object before casting to Record<string, unknown> - Fixes TypeScript error on line 390 in handleCancelBooking.ts Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
| email: attendee.email, | ||
| timeZone: attendee.timeZone, | ||
| language: { | ||
| translate: await getTranslation(attendee.locale ?? "en", "common"), |
There was a problem hiding this comment.
We only need to getTranslations for the first attendee of the booking so we're removing the looped await
| paymentOption: true, | ||
| appId: true, | ||
| success: true, | ||
| data: true, |
There was a problem hiding this comment.
Missing data needed when manually charging a card
| booking: bookingToDelete, | ||
| teamId, | ||
| }); | ||
| try { |
There was a problem hiding this comment.
Wrapping these payment functions in a try/catch since the booking is already cancelled. If these fail we shouldn't throw an error to the user
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/payment/handleNoShowFee.ts (1)
163-164: Fix ReferenceError: attendeesListPromises is undefinedThis will throw at runtime. Use attendeesList[0].
- await sendNoShowFeeChargedEmail(attendeesListPromises[0], evt, eventTypeMetdata); + await sendNoShowFeeChargedEmail(attendeesList[0], evt, eventTypeMetdata);
♻️ Duplicate comments (1)
packages/lib/server/repository/booking.ts (1)
988-989: LGTM: adding payment.data unblocks manual charge logicThis addresses the earlier “Missing data needed when manually charging a card” note.
🧹 Nitpick comments (12)
packages/lib/server/repository/booking.ts (1)
980-991: Confirm data safety and scope for payment.dataThe payment.data blob can contain provider-returned PII/tokens. Please confirm this is only used server-side and never serialized to tRPC/API responses. If it ever crosses boundaries, redact it or project only required subkeys.
packages/features/bookings/lib/handleCancelBooking.ts (1)
458-466: Minor: unify error logging formatElsewhere in this file we often use safeStringify; consider doing the same here for consistency and structured logs.
- } catch (error) { - log.error(`Error processing payment refund for booking ${bookingToDelete.uid}:`, error); + } catch (error) { + log.error( + `Error processing payment refund for booking ${bookingToDelete.uid}:`, + safeStringify({ error }) + ); } ... - } catch (error) { - log.error(`Error processing no-show fee for booking ${bookingToDelete.uid}:`, error); + } catch (error) { + log.error( + `Error processing no-show fee for booking ${bookingToDelete.uid}:`, + safeStringify({ error }) + ); }Also applies to: 468-476
packages/lib/payment/handleNoShowFee.ts (1)
65-66: Nit: variable name typoeventTypeMetdata → eventTypeMetadata for readability.
- const eventTypeMetdata = eventTypeMetaDataSchemaWithTypedApps.parse(booking.eventType?.metadata ?? {}); + const eventTypeMetadata = eventTypeMetaDataSchemaWithTypedApps.parse(booking.eventType?.metadata ?? {});And update its usage accordingly.
packages/lib/payment/processNoShowFeeOnCancellation.test.ts (2)
493-507: Test title contradicts expectationsThe test expects skipping when HOLD is already successful, but the name says “should still be processed”. Rename for clarity.
- it("should handle successful HOLD payment (should still be processed)", async () => { + it("should skip when HOLD payment is already successful", async () => {
24-44: Optional: add invalid metadata parse caseprocessNoShowFeeOnCancellation uses zod.parse; invalid metadata will throw before try/catch. Consider a test asserting it throws (and, optionally, a follow-up in handleCancelBooking to confirm it’s caught and logged).
packages/lib/payment/handleNoShowFee.test.ts (7)
59-67: Use vi.resetAllMocks() for stronger test isolation.clearAllMocks doesn’t reset custom implementations (e.g., getTranslation). Prefer resetAllMocks to avoid leakage between tests.
Apply:
- vi.clearAllMocks(); + vi.resetAllMocks();
185-228: Tighten mock sequencing in org fallback test.You set findPaymentCredentialByAppIdAndUserIdOrTeamId twice; the second resolve isn’t used on this path. Simplify to reduce noise.
Apply:
- vi.mocked(CredentialRepository.findPaymentCredentialByAppIdAndUserIdOrTeamId) - .mockResolvedValueOnce(null) - .mockResolvedValueOnce(mockCredential); + vi.mocked(CredentialRepository.findPaymentCredentialByAppIdAndUserIdOrTeamId) + .mockResolvedValueOnce(null);
246-263: Assert no downstream calls when membership check fails.Guarantee early-exit by asserting no credential lookup or charge attempt.
Apply:
await expect( handleNoShowFee({ booking: teamBooking, payment: mockPayment, }) ).rejects.toThrow("User is not a member of the team"); + expect(CredentialRepository.findPaymentCredentialByAppIdAndUserIdOrTeamId).not.toHaveBeenCalled(); + expect(mockPaymentService.chargeCard).not.toHaveBeenCalled();
265-286: Assert no charge when credentials are missing.Make sure we don’t attempt a payment without credentials.
Apply:
await expect( handleNoShowFee({ booking: bookingWithoutCredential, payment: mockPayment, }) ).rejects.toThrow("No payment credential found"); + expect(mockPaymentService.chargeCard).not.toHaveBeenCalled();
334-347: Assert email isn’t sent when processing fails.If chargeCard returns null, ensure no email goes out.
Apply:
await expect( handleNoShowFee({ booking: mockBooking, payment: mockPayment, }) ).rejects.toThrow("Payment processing failed"); + expect(sendNoShowFeeChargedEmail).not.toHaveBeenCalled();
425-451: Differentiate organizer vs attendee locale fallback.Set organizer to a non-en locale and assert both translation calls (organizer’s locale and attendee fallback en).
Apply:
- const bookingWithAttendeesWithoutLocale = { - ...mockBooking, - attendees: [ + const bookingWithAttendeesWithoutLocale = { + ...mockBooking, + user: { ...mockBooking.user, locale: "fr" }, + attendees: [ { name: "Jane Attendee", email: "attendee@example.com", timeZone: "UTC", locale: null, }, ], }; @@ expect(result).toEqual({ success: true, paymentId: "pay_123" }); - expect(getTranslation).toHaveBeenCalledWith("en", "common"); + expect(getTranslation).toHaveBeenCalledWith("fr", "common"); // organizer + expect(getTranslation).toHaveBeenCalledWith("en", "common"); // attendee fallback
129-146: Strengthen assertions in no-show fee test
- Expect sendNoShowFeeChargedEmail to have been called once; verify it’s invoked with mockBooking.attendees[0].email and an event containing paymentInfo matching mockPayment.
- Assert the dynamic PaymentService constructor is called with the resolved mockCredential.
Apply:
expect(result).toEqual({ success: true, paymentId: "pay_123" }); expect(mockPaymentService.chargeCard).toHaveBeenCalledWith(mockPayment, mockBooking.id); - expect(sendNoShowFeeChargedEmail).toHaveBeenCalled(); + expect(sendNoShowFeeChargedEmail).toHaveBeenCalledTimes(1); + const [attendee, eventArg] = vi.mocked(sendNoShowFeeChargedEmail).mock.calls[0]; + expect(attendee.email).toBe(mockBooking.attendees[0].email); + expect(eventArg.paymentInfo).toEqual({ + amount: mockPayment.amount, + currency: mockPayment.currency, + paymentOption: mockPayment.paymentOption, + }); + const module = await PaymentServiceMap.stripepayment; + expect(module.PaymentService).toHaveBeenCalledWith(mockCredential);
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/features/bookings/lib/handleCancelBooking.ts(1 hunks)packages/lib/payment/handleNoShowFee.test.ts(1 hunks)packages/lib/payment/handleNoShowFee.ts(1 hunks)packages/lib/payment/processNoShowFeeOnCancellation.test.ts(1 hunks)packages/lib/server/repository/booking.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/features/bookings/lib/handleCancelBooking.tspackages/lib/payment/processNoShowFeeOnCancellation.test.tspackages/lib/payment/handleNoShowFee.test.tspackages/lib/server/repository/booking.tspackages/lib/payment/handleNoShowFee.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/features/bookings/lib/handleCancelBooking.tspackages/lib/payment/processNoShowFeeOnCancellation.test.tspackages/lib/payment/handleNoShowFee.test.tspackages/lib/server/repository/booking.tspackages/lib/payment/handleNoShowFee.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/bookings/lib/handleCancelBooking.tspackages/lib/payment/processNoShowFeeOnCancellation.test.tspackages/lib/payment/handleNoShowFee.test.tspackages/lib/server/repository/booking.tspackages/lib/payment/handleNoShowFee.ts
🧠 Learnings (2)
📚 Learning: 2025-08-21T12:28:42.018Z
Learnt from: alishaz-polymath
PR: calcom/cal.com#23247
File: packages/features/webhooks/lib/factory/WebhookPayloadFactory.ts:274-282
Timestamp: 2025-08-21T12:28:42.018Z
Learning: In BookingPaymentInitiatedDTO and other webhook DTOs in packages/features/webhooks/lib/dto/types.ts, the booking field is a restricted structure containing only specific fields (id, eventTypeId, userId) rather than the full database booking object, so there are no security or PII leakage concerns when passing the booking object to buildEventPayload.
Applied to files:
packages/lib/server/repository/booking.ts
📚 Learning: 2025-08-21T13:44:06.805Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.805Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
Applied to files:
packages/lib/server/repository/booking.ts
🧬 Code graph analysis (3)
packages/features/bookings/lib/handleCancelBooking.ts (2)
apps/api/v2/src/lib/logger.bridge.ts (1)
error(77-79)packages/lib/payment/processNoShowFeeOnCancellation.ts (1)
processNoShowFeeOnCancellation(9-77)
packages/lib/payment/processNoShowFeeOnCancellation.test.ts (3)
packages/lib/payment/shouldChargeNoShowCancellationFee.ts (1)
shouldChargeNoShowCancellationFee(5-58)packages/lib/payment/handleNoShowFee.ts (1)
handleNoShowFee(19-175)packages/lib/payment/processNoShowFeeOnCancellation.ts (1)
processNoShowFeeOnCancellation(9-77)
packages/lib/payment/handleNoShowFee.test.ts (4)
packages/app-store/payment.services.generated.ts (1)
PaymentServiceMap(5-12)packages/lib/server/repository/credential.ts (1)
CredentialRepository(29-289)packages/emails/email-manager.ts (1)
sendNoShowFeeChargedEmail(741-748)packages/lib/payment/handleNoShowFee.ts (1)
handleNoShowFee(19-175)
⏰ Context from checks skipped due to timeout of 180000ms. 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 (6)
packages/features/bookings/lib/handleCancelBooking.ts (2)
458-466: Don’t block cancellation on refund failures — goodWrapping the refund in try/catch is the right tradeoff for UX.
468-476: Graceful no-show fee handling — goodCatching errors from no-show fee processing preserves the cancel flow as intended.
packages/lib/payment/handleNoShowFee.ts (1)
141-148: Verify dynamic import usage shapeIf PaymentServiceMap values are functions returning promises, this should be awaited as paymentAppImportFn(). If they are already promises, awaiting the value is correct. Please confirm the generated shape to avoid a subtle runtime bug.
packages/lib/payment/processNoShowFeeOnCancellation.test.ts (1)
24-509: Strong, comprehensive coverageGood spread of success, skip, payment-selection, threshold, error, and metadata edge cases with clear mocks and assertions.
packages/lib/payment/handleNoShowFee.test.ts (2)
15-23: Dynamic import mocking for PaymentServiceMap is correct.Good approach returning a Promise that resolves to a module-like object with PaymentService.
46-54: Prisma and TeamRepository stubs are sufficient.Stubbing prisma and injecting via mocked TeamRepository keeps tests isolated from DB concerns.
| const attendee = booking.attendees[0]; | ||
|
|
||
| for (const attendee of booking.attendees) { | ||
| const attendeeObject = { | ||
| const attendeesList = [ | ||
| { | ||
| name: attendee.name, | ||
| email: attendee.email, | ||
| timeZone: attendee.timeZone, | ||
| language: { | ||
| translate: await getTranslation(attendee.locale ?? "en", "common"), | ||
| locale: attendee.locale ?? "en", | ||
| }, | ||
| }; | ||
|
|
||
| attendeesListPromises.push(attendeeObject); | ||
| } | ||
|
|
||
| const attendeesList = await Promise.all(attendeesListPromises); | ||
| }, | ||
| ]; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against empty attendees array
Accessing booking.attendees[0] without a check risks a crash. Fail fast with a clear error (or define a fallback recipient if product requirements allow).
- const attendee = booking.attendees[0];
+ const attendee = booking.attendees?.[0];
+ if (!attendee) {
+ log.error("No attendees found on booking to process no-show fee email/locale");
+ throw new Error("No attendees found on booking");
+ }📝 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.
| const attendee = booking.attendees[0]; | |
| for (const attendee of booking.attendees) { | |
| const attendeeObject = { | |
| const attendeesList = [ | |
| { | |
| name: attendee.name, | |
| email: attendee.email, | |
| timeZone: attendee.timeZone, | |
| language: { | |
| translate: await getTranslation(attendee.locale ?? "en", "common"), | |
| locale: attendee.locale ?? "en", | |
| }, | |
| }; | |
| attendeesListPromises.push(attendeeObject); | |
| } | |
| const attendeesList = await Promise.all(attendeesListPromises); | |
| }, | |
| ]; | |
| const attendee = booking.attendees?.[0]; | |
| if (!attendee) { | |
| log.error("No attendees found on booking to process no-show fee email/locale"); | |
| throw new Error("No attendees found on booking"); | |
| } | |
| const attendeesList = [ | |
| { | |
| name: attendee.name, | |
| email: attendee.email, | |
| timeZone: attendee.timeZone, | |
| language: { | |
| translate: await getTranslation(attendee.locale ?? "en", "common"), | |
| locale: attendee.locale ?? "en", | |
| }, | |
| }, | |
| ]; |
🤖 Prompt for AI Agents
In packages/lib/payment/handleNoShowFee.ts around lines 72 to 85, the code
accesses booking.attendees[0] without validating the attendees array; add a
guard that checks booking.attendees exists and has at least one element and if
not either throw a clear Error (e.g. "No attendees on booking <id>") or use the
agreed fallback recipient flow; then only construct attendeesList from
booking.attendees[0] after the guard so the function fails fast with a
descriptive message (or proceeds with the defined fallback) to avoid runtime
crashes.
E2E results are ready! |
4 participants
What does this PR do?
This PR adds comprehensive unit tests for two critical no-show fee payment functions:
packages/lib/payment/handleNoShowFee.ts- Core function that processes no-show fee chargespackages/lib/payment/processNoShowFeeOnCancellation.ts- Function that determines when to charge no-show fees on cancellationThe tests provide extensive coverage including successful scenarios, error handling, and edge cases to ensure the no-show fee functionality works correctly across different payment scenarios and user permissions.
Link to Devin run: https://app.devin.ai/sessions/dd255a976d4041f7a1a59b6a9e5f6efe
Requested by: @joeauyeung
Test Coverage Added
handleNoShowFee.test.ts (14 tests)
processNoShowFeeOnCancellation.test.ts (15 tests)
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Run the test suites to verify comprehensive coverage:
Expected Results:
Important Review Points
PaymentServiceMapmocking strategy aligns with actual app-store service loadingChecklist
TZ=UTC