chore: [Booking Cancellation Refactor - 2] Inject repositories and use them instead of Prisma in cancellation flow#3
chore: [Booking Cancellation Refactor - 2] Inject repositories and use them instead of Prisma in cancellation flow#3tomerqodo wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR refactors the booking cancellation system to use repository-based dependency injection instead of direct Prisma client dependencies. It introduces five new repository classes, adds repository interface contracts, extends existing repositories with bulk and combined operations, updates the BookingCancelService and cancellation handler to use repositories, and adds comprehensive test coverage for seating and cancellation scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/features/bookings/lib/handleCancelBooking.ts`:
- Around line 513-520: The fetch uses bookingToDelete.startTime while the update
used the computed gte cutoff, so you may retrieve bookings that weren't updated;
change the call to
bookingRepository.findManyIncludeWorkflowRemindersAndReferences to use the same
gte variable used in the updateMany (the gte computed earlier based on
cancelSubsequentBookings) instead of bookingToDelete.startTime so both
updateMany and the subsequent fetch operate on the identical date filter
(referencing bookingRepository.updateMany,
bookingRepository.findManyIncludeWorkflowRemindersAndReferences, the gte
variable, bookingToDelete, cancelSubsequentBookings and allRemainingBookings).
In `@packages/features/bookings/repositories/BookingRepository.ts`:
- Around line 1502-1507: The updateMany implementation in BookingRepository does
not return the result expected by IBookingRepository; change the method so it
returns the Promise<{ count: number }>` produced by
prismaClient.booking.updateMany(...)—i.e., call and return the value from
`this.prismaClient.booking.updateMany({ where, data })` (and ensure the method
signature still matches `async updateMany({ where, data }: { where:
BookingWhereInput; data: BookingUpdateData }): Promise<{ count: number }>`).
🧹 Nitpick comments (10)
packages/lib/server/repository/dto/IBookingReferenceRepository.ts (1)
6-16: Consider completing the interface with existing methods.Line 7 has a placeholder comment
// ... Add existing methods as well here. TheBookingReferenceRepositoryclass has static methods likefindDailyVideoReferenceByRoomNameandreplaceBookingReferencesthat aren't reflected in this interface. If these are intentionally excluded (e.g., static methods not part of the instance contract), consider removing or updating the comment to clarify the design decision.packages/features/bookingReference/repositories/BookingReferenceRepository.ts (1)
18-22: Consider simplifying constructor parameter and addressing static method inconsistency.Two observations:
- The
depsobject wrapper is unnecessary when there's only one dependency. Consider simplifying:- private prismaClient: PrismaClient; - constructor(private deps: { prismaClient: PrismaClient }) { - this.prismaClient = deps.prismaClient; - } + constructor(private prismaClient: PrismaClient) {}
- Static methods (lines 24-66) still use the global
prismaimport while instance methods use the injectedprismaClient. This creates inconsistent data access patterns and makes static methods harder to test. Consider migrating static methods to instance methods in a follow-up refactor.packages/lib/server/repository/dto/IBookingRepository.ts (1)
1-1: Use type-only import forBooking.Since
Bookingis only used as a type annotation (line 50), prefer a type-only import to ensure it's erased at compile time and avoid potential bundling issues.Suggested fix
-import { Booking } from "@calcom/prisma/client"; +import type { Booking } from "@calcom/prisma/client";packages/features/bookings/lib/handleCancelBooking/test/handleCancelBooking.test.ts (3)
1163-1282: Consider verifying that all recurring bookings were actually cancelled.The test sets up two recurring booking instances but only asserts on the result object (
result.success,result.bookingId). It doesn't verify that the second booking (id:idOfBookingToBeCancelled + 1) was also cancelled in the database.💡 Suggested enhancement to verify all bookings were cancelled
expect(result.success).toBe(true); expect(result.onlyRemovedAttendee).toBe(false); expect(result.bookingId).toBe(idOfBookingToBeCancelled); + + // Verify both recurring bookings were cancelled + const { prisma } = await import("@calcom/prisma"); + const cancelledBookings = await prisma.booking.findMany({ + where: { recurringEventId }, + select: { id: true, status: true }, + }); + expect(cancelledBookings.every(b => b.status === BookingStatus.CANCELLED)).toBe(true); });
1284-1403: Same consideration: verify subsequent bookings were actually cancelled.Similar to the
allRemainingBookingstest, this test doesn't verify that the subsequent booking was cancelled in the database. The test only checks the result object properties.
1405-1516: Consider asserting that booking references were cleaned up.The test name suggests it should verify "booking reference cleanup during cancellation", but the assertions only check the result object. Consider querying the database to verify the references were actually marked as deleted or handled appropriately.
💡 Suggested enhancement to verify reference cleanup
expect(result.success).toBe(true); expect(result.onlyRemovedAttendee).toBe(false); expect(result.bookingId).toBe(idOfBookingToBeCancelled); + + // Verify booking references were cleaned up + const { prisma } = await import("@calcom/prisma"); + const references = await prisma.bookingReference.findMany({ + where: { bookingId: idOfBookingToBeCancelled }, + }); + // Assert references were marked as deleted or handled appropriately + expect(references.every(ref => ref.deleted !== null)).toBe(true); });packages/features/bookings/di/BookingCancelService.module.ts (1)
2-7: Import path style inconsistency.Line 2 uses a relative import path (
../lib/handleCancelBooking) while lines 3-7 use absolute@calcomaliased paths. Consider using a consistent import style throughout the file.♻️ Suggested fix for consistency
-import { BookingCancelService } from "../lib/handleCancelBooking"; +import { BookingCancelService } from "@calcom/features/bookings/lib/handleCancelBooking";packages/features/profile/repositories/ProfileRepository.ts (1)
1034-1041: Consolidate duplicate profile lookup methods during DI transition.Both
findFirstByUserId(instance method) andfindFirstForUserId(static method) perform identical queries; the only difference is the Prisma client source. The instance method is the intended API (defined inIProfileRepositoryinterface), while the static method is legacy code being phased out (evidenced by the TODO inRegularBookingService.ts). Complete the transition by removing the static method and updating the remaining call sites inonboardingUtils.tsandRegularBookingService.tsto use the instance method via dependency injection.packages/features/bookings/lib/handleCancelBooking.ts (2)
89-95: Consider using repository interfaces instead of concrete types.The
Dependenciestype uses concrete repository classes. For improved testability and flexibility, consider using the repository interfaces (e.g.,IBookingRepository,IProfileRepository,IBookingReferenceRepository) that appear to exist in the codebase based on the PR context.
703-709: Consider consolidating duplicate type definitions.
BookingCancelServiceDependenciesis identical to theDependenciestype defined at Lines 89-95. Consider reusing a single type to avoid duplication and potential drift.♻️ Proposed fix
-type BookingCancelServiceDependencies = { - userRepository: UserRepository; - bookingRepository: BookingRepository; - profileRepository: ProfileRepository; - bookingReferenceRepository: BookingReferenceRepository; - attendeeRepository: PrismaBookingAttendeeRepository; -}; +type BookingCancelServiceDependencies = Dependencies;
| const allUpdatedBookings = await bookingRepository.findManyIncludeWorkflowRemindersAndReferences({ | ||
| where: { | ||
| recurringEventId: bookingToDelete.recurringEventId, | ||
| startTime: { | ||
| gte: new Date(), | ||
| }, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| startTime: true, | ||
| endTime: true, | ||
| references: { | ||
| select: { | ||
| uid: true, | ||
| type: true, | ||
| externalCalendarId: true, | ||
| credentialId: true, | ||
| }, | ||
| gte: bookingToDelete.startTime, | ||
| }, | ||
| workflowReminders: true, | ||
| uid: true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Potential mismatch between update and fetch date filters.
The updateMany at Line 500 uses the gte variable (which may be new Date() when cancelSubsequentBookings is false), but findManyIncludeWorkflowRemindersAndReferences at Line 517 always uses bookingToDelete.startTime. This could result in fetching bookings that weren't actually updated when allRemainingBookings is true but cancelSubsequentBookings is false.
Consider using the same gte variable for consistency:
🐛 Proposed fix
const allUpdatedBookings = await bookingRepository.findManyIncludeWorkflowRemindersAndReferences({
where: {
recurringEventId: bookingToDelete.recurringEventId,
startTime: {
- gte: bookingToDelete.startTime,
+ gte,
},
},
});📝 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 allUpdatedBookings = await bookingRepository.findManyIncludeWorkflowRemindersAndReferences({ | |
| where: { | |
| recurringEventId: bookingToDelete.recurringEventId, | |
| startTime: { | |
| gte: new Date(), | |
| }, | |
| }, | |
| select: { | |
| id: true, | |
| startTime: true, | |
| endTime: true, | |
| references: { | |
| select: { | |
| uid: true, | |
| type: true, | |
| externalCalendarId: true, | |
| credentialId: true, | |
| }, | |
| gte: bookingToDelete.startTime, | |
| }, | |
| workflowReminders: true, | |
| uid: true, | |
| }, | |
| }); | |
| const allUpdatedBookings = await bookingRepository.findManyIncludeWorkflowRemindersAndReferences({ | |
| where: { | |
| recurringEventId: bookingToDelete.recurringEventId, | |
| startTime: { | |
| gte, | |
| }, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@packages/features/bookings/lib/handleCancelBooking.ts` around lines 513 -
520, The fetch uses bookingToDelete.startTime while the update used the computed
gte cutoff, so you may retrieve bookings that weren't updated; change the call
to bookingRepository.findManyIncludeWorkflowRemindersAndReferences to use the
same gte variable used in the updateMany (the gte computed earlier based on
cancelSubsequentBookings) instead of bookingToDelete.startTime so both
updateMany and the subsequent fetch operate on the identical date filter
(referencing bookingRepository.updateMany,
bookingRepository.findManyIncludeWorkflowRemindersAndReferences, the gte
variable, bookingToDelete, cancelSubsequentBookings and allRemainingBookings).
| async updateMany({ where, data }: { where: BookingWhereInput; data: BookingUpdateData }) { | ||
| await this.prismaClient.booking.updateMany({ | ||
| where: where, | ||
| data, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing return statement - method doesn't fulfill interface contract.
The IBookingRepository interface declares updateMany as returning Promise<{ count: number }>, but this implementation doesn't return the result from prismaClient.booking.updateMany(). Callers expecting the count will receive undefined.
🐛 Proposed fix
async updateMany({ where, data }: { where: BookingWhereInput; data: BookingUpdateData }) {
- await this.prismaClient.booking.updateMany({
+ const result = await this.prismaClient.booking.updateMany({
where: where,
data,
});
+ return { count: result.count };
}🤖 Prompt for AI Agents
In `@packages/features/bookings/repositories/BookingRepository.ts` around lines
1502 - 1507, The updateMany implementation in BookingRepository does not return
the result expected by IBookingRepository; change the method so it returns the
Promise<{ count: number }>` produced by
prismaClient.booking.updateMany(...)—i.e., call and return the value from
`this.prismaClient.booking.updateMany({ where, data })` (and ensure the method
signature still matches `async updateMany({ where, data }: { where:
BookingWhereInput; data: BookingUpdateData }): Promise<{ count: number }>`).
2 participants
Benchmark PR from agentic-review-benchmarks#3
Summary by CodeRabbit
Refactor
Tests