chore: [Booking Cancellation Refactor - 2] Inject repositories and use them instead of Prisma in cancellation flow#98
Open
tomerqodo wants to merge 2 commits into
Open
chore: [Booking Cancellation Refactor - 2] Inject repositories and use them instead of Prisma in cancellation flow#98tomerqodo wants to merge 2 commits into
tomerqodo wants to merge 2 commits into
Conversation
Code Review by Qodo
1. Kebab-case repository filenames
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+11
| import { PrismaWriteService } from "@/modules/prisma/prisma-write.service"; | ||
| import { Injectable } from "@nestjs/common"; | ||
|
|
||
| import { PrismaBookingAttendeeRepository as BasePrismaBookingAttendeeRepository } from "@calcom/platform-libraries/repositories"; | ||
|
|
||
| @Injectable() | ||
| export class PrismaBookingAttendeeRepository extends BasePrismaBookingAttendeeRepository { | ||
| constructor(private readonly dbWrite: PrismaWriteService) { | ||
| super(dbWrite.prisma); | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Kebab-case repository filenames 📘 Rule violation ✓ Correctness
• New repository/service files under apps/api/v2/src/lib/** use kebab-case names and dot-suffixes like .repository.ts / .service.ts, which breaks the required naming convention for discoverability. • The checklist requires repository filenames to be Prisma<Entity>Repository.ts and service filenames to be <Entity>Service.ts, with filenames matching exported class names in PascalCase. • Keeping the current filenames increases maintenance cost by making DI bindings and imports harder to locate and standardize.
Agent prompt
## Issue description
New repository/service files do not follow the required naming conventions: repositories must be `Prisma<Entity>Repository.ts` and services must be `<Entity>Service.ts`, with PascalCase filenames matching exported class names. The current kebab-case + `.repository.ts`/`.service.ts` suffixes violate the convention.
## Issue Context
These files are newly introduced in the PR and are referenced by NestJS modules/imports; renaming will require updating the corresponding import paths.
## Fix Focus Areas
- apps/api/v2/src/lib/repositories/prisma-booking-attendee.repository.ts[1-11]
- apps/api/v2/src/lib/repositories/prisma-booking-reference.repository.ts[1-11]
- apps/api/v2/src/lib/repositories/prisma-profile.repository.ts[1-11]
- apps/api/v2/src/lib/services/booking-cancel.service.ts[1-27]
- apps/api/v2/src/lib/modules/booking-cancel.module.ts[1-22]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+15
| import type { PrismaClient } from '@calcom/prisma/client' | ||
|
|
||
| import type { IBookingAttendeeRepository } from '../lib/dto/IBookingAttendeeRepository' | ||
|
|
||
| export class PrismaBookingAttendeeRepository implements IBookingAttendeeRepository { | ||
| constructor(private prismaClient: PrismaClient) {} | ||
|
|
||
| async deleteManyByBookingId(bookingId: number): Promise<void> { | ||
| await this.prismaClient.attendee.deleteMany({ | ||
| where: { | ||
| bookingId, | ||
| }, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Single quotes; missing semicolons 📘 Rule violation ✓ Correctness
• The new PrismaBookingAttendeeRepository file uses single-quoted imports and omits semicolons, deviating from the Biome formatting standards. • The checklist requires consistent formatting rules (including semicolons and standardized quoting) across all TS/JS files. • Leaving formatting non-standard creates inconsistent style and will cause formatter/lint churn across the codebase.
Agent prompt
## Issue description
The new TypeScript file is not formatted per the Biome standard configuration (e.g., single quotes and missing semicolons).
## Issue Context
This repository will be imported across features; formatting should match the repo-wide rules to avoid style drift.
## Fix Focus Areas
- packages/features/bookings/repositories/PrismaBookingAttendeeRepository.ts[1-15]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1502
to
+1507
| async updateMany({ where, data }: { where: BookingWhereInput; data: BookingUpdateData }) { | ||
| await this.prismaClient.booking.updateMany({ | ||
| where: where, | ||
| data, | ||
| }); | ||
| } |
There was a problem hiding this comment.
3. Updatemany contract mismatch 🐞 Bug ✓ Correctness
• BookingRepository.updateMany does not return the Prisma updateMany result, but the new
IBookingRepository interface requires Promise<{count:number}>.
• This is a hard type/contract mismatch that should break TypeScript compilation and also prevents
callers from inspecting affected-row count.
Agent prompt
### Issue description
`BookingRepository.updateMany` currently returns `void`, but the new `IBookingRepository` contract requires `Promise<{ count: number }>`.
### Issue Context
This is a direct interface/implementation mismatch and should fail TypeScript compilation.
### Fix Focus Areas
- packages/features/bookings/repositories/BookingRepository.ts[1502-1507]
- packages/lib/server/repository/dto/IBookingRepository.ts[46-55]
### Suggested change
- Return the value from `this.prismaClient.booking.updateMany(...)` (which includes `{ count }`) and ensure the method signature matches the interface.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
98
to
103
| createdDate: true, | ||
| trialEndsAt: true, | ||
| completedOnboarding: true, | ||
| locale: true, | ||
| timeFormat: true, | ||
| twoFactorSecret: true, | ||
| twoFactorEnabled: true, |
There was a problem hiding this comment.
4. Organizer locale removed 🐞 Bug ✓ Correctness
• UserRepository.findById/findByIdOrThrow uses userSelect which no longer selects locale. • Booking cancellation reads organizer.locale for translations; this will either fail compilation (property missing) or force organizer locale to be unavailable and default to English, degrading localization.
Agent prompt
### Issue description
`UserRepository.userSelect` no longer includes `locale`, but booking cancellation uses `organizer.locale`.
### Issue Context
This likely breaks TS compilation (missing property) and/or causes organizer locale to always fall back to `"en"`.
### Fix Focus Areas
- packages/features/users/repositories/UserRepository.ts[83-122]
- packages/features/bookings/lib/handleCancelBooking.ts[299-371]
### Suggested change
- Re-introduce `locale: true` in `userSelect`, or provide a dedicated `findByIdForBookingCancellation` method/select that includes locale and use it in cancellation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+4
| import { bindModuleToClassOnToken, createModule } from "@calcom/features/di/di"; | ||
| import { DI_TOKENS } from "@calcom/features/di/tokens"; | ||
| import { BookingReferenceRepository } from "@calcom/lib/server/repository/bookingReference"; | ||
| import { moduleLoader as prismaModuleLoader } from "@calcom/prisma/prisma.module"; |
There was a problem hiding this comment.
5. Bad bookingreference import 🐞 Bug ✓ Correctness
• BookingReferenceRepository DI module imports @calcom/lib/server/repository/bookingReference, but the features package doesn’t define path aliases for @calcom/lib/* and the @calcom/lib package does not contain that module. • This is likely to fail module resolution at build/runtime for the features package.
Agent prompt
### Issue description
The DI module imports `BookingReferenceRepository` from `@calcom/lib/server/repository/bookingReference`, but the implementation lives under `packages/features/bookingReference/...`.
### Issue Context
`packages/features/tsconfig.json` does not define a `@calcom/lib/*` path alias, so this import likely relies on the physical module existing within the `@calcom/lib` workspace package.
### Fix Focus Areas
- packages/features/bookings/di/BookingReferenceRepository.module.ts[1-5]
### Suggested change
- Change the import to `@calcom/features/bookingReference/repositories/BookingReferenceRepository` (or another valid canonical path), OR
- Create and maintain a `packages/lib/server/repository/bookingReference` re-export/module if `@calcom/lib/...` is intended to be the stable API.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
497
to
503
| const recurringEventId = bookingToDelete.recurringEventId; | ||
| const gte = cancelSubsequentBookings ? bookingToDelete.startTime : new Date(); | ||
| // Proceed to mark as cancelled all remaining recurring events instances (greater than or equal to right now) | ||
| await prisma.booking.updateMany({ | ||
| await bookingRepository.updateMany({ | ||
| where: { | ||
| recurringEventId, | ||
| startTime: { |
There was a problem hiding this comment.
6. Recurring cancel misses cleanup 🐞 Bug ✓ Correctness
• In recurring cancellation, the code cancels bookings using a lower bound gte that differs depending on the option (now vs booking start). • It then fetches updated bookings using gte: bookingToDelete.startTime regardless, so for allRemainingBookings it can omit cancelled instances between now and bookingToDelete.startTime, leaving webhook triggers and workflow reminders uncleared for those bookings.
Agent prompt
### Issue description
Recurring cancellation uses `gte` for the update step, but fetches updated bookings with a different lower bound (`bookingToDelete.startTime`). This can omit some cancelled bookings from the cleanup loops.
### Issue Context
For `allRemainingBookings`, `gte` is `new Date()`, but the fetch uses `bookingToDelete.startTime`. If the cancelled booking is in the future, instances between now and that start time are cancelled but not included in `updatedBookings`.
### Fix Focus Areas
- packages/features/bookings/lib/handleCancelBooking.ts[492-520]
- packages/features/bookings/lib/handleCancelBooking.ts[648-655]
### Suggested change
- Replace `gte: bookingToDelete.startTime` in the `findManyIncludeWorkflowRemindersAndReferences` call with `gte` (the variable computed above), or otherwise ensure the fetch range matches the cancellation range.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from agentic-review-benchmarks#3