chore: Integrate confirmation booking audit#37
Conversation
WalkthroughThis PR introduces audit tracking and action sourcing to booking operations across the platform. Changes propagate Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/web/app/api/verify-booking-token/__tests__/route.test.ts`:
- Around line 68-70: Update the test mock for makeUserActor so its return shape
matches the real implementation: have
vi.mock("@calcom/features/booking-audit/lib/makeActor", ...)
makeUserActor.mockReturnValue({ identifiedBy: "user", userUuid: "test-uuid" });
then update any assertions that previously expected { type, id } (e.g., the
expectation referenced in the review) to assert { identifiedBy, userUuid } (or
compare the userUuid value) so the test mirrors the actual makeUserActor output
and avoids masking shape-related bugs.
In `@apps/web/test/lib/confirm.handler.test.ts`:
- Around line 115-135: The test's ctx.user object is missing the uuid property
so makeUserActor receives undefined; update the test's ctx.user to include uuid:
testUserUuid (or the same fixture value used elsewhere) so that ctx.user.uuid
passed into makeUserActor is defined, ensuring confirmHandler receives a valid
actor; locate the ctx object in the confirm handler test near the
confirmHandler(...) invocation and add the uuid field to the user shaped as
NonNullable<TrpcSessionUser>.
In `@packages/features/bookings/lib/handleConfirmation.ts`:
- Around line 411-416: The acceptedBookings array is using a hardcoded oldStatus
of BookingStatus.ACCEPTED but should record the actual previous status from the
booking parameter; update the assignment that builds acceptedBookings in
handleConfirmation.ts (the block that sets acceptedBookings = [...]) to use
booking.status for oldStatus (e.g., oldStatus: booking.status) so the correct
prior state is captured for non-recurring bookings when processing confirmation.
- Around line 279-285: The audit event is being fired for recurring bookings
before the database updates, so move the fireBookingAcceptedEvent call
(currently invoked with actor, acceptedBookings, organizationId: orgId ?? null,
actionSource, tracingLogger) to after the prisma.booking.update batch that
persists the recurring booking changes; ensure you mirror the non-recurring path
behavior where the event is emitted only after successful DB commits so the
audit reflects the persisted state.
In `@packages/trpc/server/routers/viewer/bookings/confirm.handler.ts`:
- Around line 401-448: The recurring-booking branch wrongly sets
rejectedBookings using post-update data so oldStatus becomes REJECTED; change it
to populate rejectedBookings from the pre-update result
unconfirmedRecurringBookings (use its status field as oldStatus) before calling
prisma.booking.updateMany, mirroring the single-booking path logic in
confirm.handler.ts (references: rejectedBookings, unconfirmedRecurringBookings,
updatedRecurringBookings, rejectionReason); also make the analogous fix where
single-booking rejection populates oldStatus to ensure it uses the pre-update
status.
🧹 Nitpick comments (7)
packages/features/booking-audit/lib/service/__tests__/accepted-action.integration-test.ts (1)
149-152: Assert audit log presence before indexing.Avoid less-informative failures by asserting the expected length before accessing
auditLogs[0].♻️ Proposed tweak
const result = await bookingAuditViewerService.getAuditLogsForBooking({ bookingUid: testData.booking.uid, userId: testData.owner.id, userEmail: testData.owner.email, userTimeZone: "UTC", organizationId: testData.organization.id, }); + expect(result.auditLogs).toHaveLength(1); const auditLog = result.auditLogs[0]; expect(auditLog.actor.displayName).toBe("Test Audit User"); expect(auditLog.actor.displayEmail).toBe(testData.owner.email); expect(auditLog.actor.userUuid).toBe(testData.owner.uuid);packages/app-store/_utils/payments/handlePaymentSuccess.ts (1)
49-51: Reuse the already-parsedappsto avoid shadowing.
You computeappsonce at the top; reusing it in theisConfirmedblock removes duplicate parsing and a shadowed variable.♻️ Suggested refactor
if (isConfirmed) { - const apps = eventTypeAppMetadataOptionalSchema.parse(eventType?.metadata?.apps); const eventManager = new EventManager({ ...userWithCredentials, credentials: allCredentials }, apps); const scheduleResult = areCalendarEventsEnabled ? await eventManager.create(evt) : placeholderCreatedEvent;Also applies to: 83-85
packages/app-store/alby/api/webhook.ts (1)
95-100: Consider sourcingappSlugfrom config for consistency (if available).
Other webhooks use a config-driven slug; this avoids drift if the slug changes.packages/features/booking-audit/lib/actions/RejectedAuditActionService.ts (1)
69-76: Minor: Unnecessary nullish coalescing onfields.status.new.According to
BookingStatusChangeSchema, thenewfield isz.nativeEnum(BookingStatus)(non-nullable), so?? nullon Line 74 is unnecessary. The code works correctly, but this could be simplified.Optional simplification
getDisplayJson({ storedData }: GetDisplayJsonParams): RejectedAuditDisplayData { const { fields } = this.parseStored(storedData); return { rejectionReason: fields.rejectionReason ?? null, previousStatus: fields.status.old ?? null, - newStatus: fields.status.new ?? null, + newStatus: fields.status.new, }; }packages/features/booking-audit/lib/service/BookingAuditProducerService.interface.ts (1)
204-214: Missing JSDoc comment for consistency.The
queueBulkRejectedAuditmethod lacks a JSDoc comment, unlikequeueBulkAcceptedAudit(Line 174-176) andqueueBulkCancelledAudit(Line 189-191) which have descriptive comments.Add JSDoc for consistency
+ /** + * Queues a bulk rejected audit task for multiple bookings + */ queueBulkRejectedAudit(params: { bookings: Array<{ bookingUid: string; data: z.infer<typeof RejectedAuditActionService.latestFieldsSchema>; }>; actor: Actor; organizationId: number | null; source: ActionSource; operationId?: string | null; context?: BookingAuditContext; }): Promise<void>;packages/features/bookings/lib/onBookingEvents/BookingEventHandlerService.ts (1)
359-383: Missing JSDoc comment for consistency with other bulk methods.The
onBulkBookingsRejectedmethod lacks a JSDoc comment, unlikeonBulkBookingsAccepted(Lines 302-305) andonBulkBookingsCancelled(Lines 331-334).Add JSDoc for consistency
+ /** + * Handles bulk booking rejection for recurring bookings + * Creates a single task that will be processed to create multiple audit logs atomically + */ async onBulkBookingsRejected(params: {Otherwise, the implementation correctly follows the established patterns for bulk booking event handling.
apps/web/app/api/link/__tests__/route.test.ts (1)
216-332: Consider asserting exact call counts for confirmHandler.
toHaveBeenCalledWithallows extra calls to slip by; addingtoHaveBeenCalledTimes(1)ortoHaveBeenNthCalledWith(1, …)in these tests would guard against unintended duplicate invocations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apps/api/v2/src/ee/bookings/2024-08-13/services/bookings.service.tsapps/web/app/api/link/__tests__/route.test.tsapps/web/app/api/link/route.tsapps/web/app/api/verify-booking-token/__tests__/route.test.tsapps/web/app/api/verify-booking-token/route.tsapps/web/test/lib/confirm.handler.test.tspackages/app-store/_utils/getAppActor.tspackages/app-store/_utils/payments/handlePaymentSuccess.test.tspackages/app-store/_utils/payments/handlePaymentSuccess.tspackages/app-store/alby/api/webhook.tspackages/app-store/btcpayserver/api/webhook.tspackages/app-store/hitpay/api/webhook.tspackages/app-store/paypal/api/webhook.tspackages/features/booking-audit/lib/actions/RejectedAuditActionService.tspackages/features/booking-audit/lib/service/BookingAuditProducerService.interface.tspackages/features/booking-audit/lib/service/BookingAuditTaskerProducerService.tspackages/features/booking-audit/lib/service/__tests__/accepted-action.integration-test.tspackages/features/booking-audit/lib/types/actionSource.tspackages/features/bookings/lib/handleConfirmation.tspackages/features/bookings/lib/onBookingEvents/BookingEventHandlerService.tspackages/features/ee/payments/api/webhook.tspackages/platform/libraries/bookings.tspackages/prisma/migrations/20260107093019_add_magic_link_source/migration.sqlpackages/prisma/schema.prismapackages/trpc/server/routers/viewer/bookings/_router.tsxpackages/trpc/server/routers/viewer/bookings/confirm.handler.ts
🧰 Additional context used
🧬 Code graph analysis (18)
packages/app-store/_utils/getAppActor.ts (3)
packages/features/booking-audit/lib/dto/types.ts (1)
Actor(50-50)packages/features/booking-audit/lib/makeActor.ts (2)
makeAppActor(61-66)makeAppActorUsingSlug(73-79)packages/features/booking-audit/lib/getAppNameFromSlug.ts (1)
getAppNameFromSlug(13-18)
apps/web/app/api/verify-booking-token/route.ts (2)
packages/platform/libraries/bookings.ts (1)
makeUserActor(22-22)scripts/prepare-local-for-delegation-credentials-testing.js (1)
user(18-20)
apps/web/app/api/link/route.ts (2)
packages/platform/libraries/bookings.ts (1)
makeUserActor(22-22)scripts/prepare-local-for-delegation-credentials-testing.js (1)
user(18-20)
packages/app-store/_utils/payments/handlePaymentSuccess.ts (2)
packages/features/bookings/lib/payment/getBooking.ts (1)
getBooking(28-207)packages/app-store/_utils/getAppActor.ts (1)
getAppActor(22-49)
packages/app-store/btcpayserver/api/webhook.ts (1)
packages/app-store/_utils/payments/handlePaymentSuccess.ts (1)
handlePaymentSuccess(36-270)
packages/app-store/alby/api/webhook.ts (1)
packages/app-store/_utils/payments/handlePaymentSuccess.ts (1)
handlePaymentSuccess(36-270)
apps/web/app/api/verify-booking-token/__tests__/route.test.ts (1)
apps/web/app/api/verify-booking-token/route.ts (2)
GET(140-140)POST(141-141)
packages/features/booking-audit/lib/service/BookingAuditProducerService.interface.ts (3)
packages/features/booking-audit/lib/dto/types.ts (2)
Actor(50-50)BookingAuditContext(69-69)packages/features/booking-audit/lib/types/actionSource.ts (1)
ActionSource(5-5)packages/features/booking-audit/lib/actions/CreatedAuditActionService.ts (1)
CreatedAuditActionService(20-78)
apps/web/test/lib/confirm.handler.test.ts (5)
packages/features/bookings/lib/onBookingEvents/BookingEventHandlerService.ts (4)
onBookingAccepted(146-157)onBulkBookingsAccepted(306-329)onBookingRejected(211-222)onBulkBookingsRejected(360-383)packages/testing/src/lib/bookingScenario/bookingScenario.ts (4)
getOrganizer(1525-1590)TestData(1256-1516)getScenarioData(1592-1680)mockSuccessfulVideoMeetingCreation(2102-2120)packages/trpc/server/routers/viewer/bookings/confirm.handler.ts (1)
confirmHandler(106-558)packages/platform/libraries/bookings.ts (1)
makeUserActor(22-22)packages/features/bookings/di/BookingEventHandlerService.container.ts (1)
getBookingEventHandlerService(10-14)
packages/app-store/paypal/api/webhook.ts (1)
packages/app-store/_utils/payments/handlePaymentSuccess.ts (1)
handlePaymentSuccess(36-270)
apps/web/app/api/link/__tests__/route.test.ts (3)
packages/platform/libraries/index.ts (1)
TRPCError(62-62)apps/web/app/api/link/route.ts (1)
GET(109-109)apps/web/app/api/verify-booking-token/route.ts (1)
GET(140-140)
packages/trpc/server/routers/viewer/bookings/_router.tsx (1)
packages/platform/libraries/bookings.ts (1)
makeUserActor(22-22)
packages/features/booking-audit/lib/actions/RejectedAuditActionService.ts (3)
packages/features/booking-audit/lib/common/changeSchemas.ts (1)
BookingStatusChangeSchema(29-32)packages/features/booking-audit/lib/actions/IAuditActionService.ts (4)
IAuditActionService(49-129)GetDisplayTitleParams(34-37)TranslationWithParams(15-19)GetDisplayJsonParams(29-32)packages/features/booking-audit/lib/actions/AuditActionServiceHelper.ts (1)
AuditActionServiceHelper(11-85)
packages/app-store/_utils/payments/handlePaymentSuccess.test.ts (2)
packages/lib/CalEventParser.ts (1)
getVideoCallUrlFromCalEvent(456-473)packages/app-store/_utils/payments/handlePaymentSuccess.ts (1)
handlePaymentSuccess(36-270)
apps/api/v2/src/ee/bookings/2024-08-13/services/bookings.service.ts (2)
packages/platform/libraries/index.ts (1)
handleMarkNoShow(48-48)packages/features/booking-audit/lib/makeActor.ts (1)
makeUserActor(9-14)
packages/app-store/hitpay/api/webhook.ts (1)
packages/app-store/_utils/payments/handlePaymentSuccess.ts (1)
handlePaymentSuccess(36-270)
packages/features/bookings/lib/handleConfirmation.ts (5)
packages/features/booking-audit/lib/dto/types.ts (1)
Actor(50-50)packages/features/booking-audit/lib/types/actionSource.ts (1)
ActionSource(5-5)packages/features/di/shared/services/logger.service.ts (1)
ISimpleLogger(14-19)packages/features/bookings/di/BookingEventHandlerService.container.ts (1)
getBookingEventHandlerService(10-14)packages/prisma/zod-utils.ts (1)
PlatformClientParams(373-373)
packages/features/ee/payments/api/webhook.ts (2)
packages/app-store/_utils/payments/handlePaymentSuccess.ts (1)
handlePaymentSuccess(36-270)packages/app-store/_utils/getAppActor.ts (1)
getAppActor(22-49)
🔇 Additional comments (38)
packages/features/booking-audit/lib/service/__tests__/accepted-action.integration-test.ts (1)
92-110: No changes needed. The test correctly awaitsonBookingActionbefore querying audit logs, andonBookingActionprocesses all operations inline with full await semantics—no background task queueing occurs. The audit record is persisted synchronously within the await chain, so the test should not be flaky.Likely an incorrect or invalid review comment.
packages/prisma/schema.prisma (1)
2745-2750: Booking audit source extension looks good.
Adds theMAGIC_LINKsource with proper DB mapping.packages/prisma/migrations/20260107093019_add_magic_link_source/migration.sql (1)
1-2: Enum migration aligns with the schema update.
Straightforward enum extension.packages/features/booking-audit/lib/types/actionSource.ts (1)
3-9: Nice split between DB and client-facing action sources.
ValidActionSourceSchemaavoids exposingUNKNOWNwhile keeping DB fallback.packages/app-store/_utils/getAppActor.ts (1)
1-49: Actor fallback strategy is solid.
Always returns an app actor while flagging missing credentials.packages/features/ee/payments/api/webhook.ts (2)
5-10: App-specific payment handling is wired correctly.
PassingappSluginto the shared handler enables consistent app attribution for Stripe payments.Also applies to: 57-62
136-148: Confirmation now carries actionSource + actor as expected.
Good propagation for audit context.packages/app-store/_utils/payments/handlePaymentSuccess.ts (2)
32-43: Parameter object + appSlug plumbing looks good.
The signature makes app context explicit for downstream auditing.
252-254: Audit context propagation in confirmation flow looks good.
PassingactionSourceandactorpreserves webhook attribution.packages/app-store/_utils/payments/handlePaymentSuccess.test.ts (7)
5-5: Enum import aligns with workflow assertions.
15-17: Credential mock keeps the test setup deterministic.
47-52: Platform OAuth repository mock is a good test isolation step.
221-221: Workflow and credit-service stubs stabilize the workflow path.Also applies to: 228-228
233-238: All test invocations now use the new params object withappSlug.Also applies to: 276-281, 313-318, 331-336, 345-350
285-288: Workflow expectations updated to match the new scheduling call.
327-327: Workflow error path is correctly stubbed and asserted.Also applies to: 340-340
packages/app-store/paypal/api/webhook.ts (1)
14-14: App slug is now passed into payment success context.Also applies to: 71-76
packages/app-store/btcpayserver/api/webhook.ts (1)
94-99: Updated handlePaymentSuccess call uses the new params object.packages/app-store/hitpay/api/webhook.ts (1)
12-12: App slug is now included in the payment success payload.Also applies to: 117-122
packages/trpc/server/routers/viewer/bookings/confirm.handler.ts (6)
5-5: Audit/logging imports and scoped logger usage look good.Also applies to: 35-39, 107-107
48-48: Action source and actor are consistently propagated through confirmation.Also applies to: 115-117, 396-397
51-102: Rejection event helper cleanly handles single vs bulk cases.
306-307: Destination calendar fallback to the user calendar is clear and safe.
489-496: fireRejectionEvent is invoked with full org/source context.
32-32: No action needed. The import syntaximport { v4 as uuidv4 } from "uuid"is correct and fully supported by uuid 8.3.2 installed in packages/trpc, with no TypeScript or ESM compatibility issues. This pattern is already used throughout the codebase in 40+ files without problems.apps/web/test/lib/confirm.handler.test.ts (1)
147-250: Comprehensive test coverage for booking audit integration.The tests thoroughly validate:
- Single and bulk booking acceptance/rejection flows
- Correct propagation of
actor,auditData,source, andorganizationId- Status transition tracking (
PENDING→ACCEPTED/REJECTED)operationIdgeneration for bulk operationsAlso applies to: 252-383, 386-488, 490-620
packages/features/bookings/lib/handleConfirmation.ts (1)
41-88: Well-structured helper for firing booking accepted events.The
fireBookingAcceptedEventhelper properly:
- Handles both single and bulk booking scenarios
- Wraps calls in try-catch to prevent audit failures from breaking the main flow
- Generates unique
operationIdfor bulk operations- Uses appropriate logging for errors
packages/features/booking-audit/lib/actions/RejectedAuditActionService.ts (1)
19-22: Clean schema definition following established patterns.The schema properly uses
BookingStatusChangeSchemafor type-safe status tracking and allows nullablerejectionReason. The class structure mirrors other audit action services for consistency.Also applies to: 24-45
packages/features/booking-audit/lib/service/BookingAuditTaskerProducerService.ts (1)
404-419: LGTM - Consistent implementation following established patterns.The
queueBulkRejectedAuditmethod correctly:
- Delegates to
queueBulkTaskwith the appropriate action type- Follows the same pattern as
queueBulkAcceptedAuditandqueueBulkCancelledAudit- Properly spreads params to include all necessary fields
packages/platform/libraries/bookings.ts (1)
22-22: LGTM!Clean re-export of
makeUserActorfrom the booking-audit feature, consistent with the existing export pattern in this file.apps/web/app/api/link/route.ts (1)
94-95: LGTM!Audit context (
actionSourceandactor) is correctly added to the confirm handler input for magic link flows. Theuser.uuidis properly fetched in the select clause at line 59.packages/trpc/server/routers/viewer/bookings/_router.tsx (1)
57-68: LGTM!Audit context is correctly added to the confirm mutation. Using spread operator preserves the original input while adding
actorandactionSource. The "WEBAPP" action source appropriately distinguishes TRPC calls from magic link flows.apps/web/app/api/verify-booking-token/route.ts (1)
119-120: LGTM!Audit context is correctly added to the confirm handler, consistent with the pattern in
apps/web/app/api/link/route.ts. Theuser.uuidis properly fetched in the select clause at line 91.apps/api/v2/src/ee/bookings/2024-08-13/services/bookings.service.ts (2)
1150-1151: LGTM!Audit context correctly added to
confirmBookingwith "API_V2" action source, appropriately distinguishing API v2 calls from web app and magic link flows.
1185-1186: LGTM!Audit context correctly added to
declineBooking, consistent withconfirmBookingimplementation.apps/web/app/api/verify-booking-token/__tests__/route.test.ts (1)
320-431: Good test coverage for booking confirmation flows.The new test suite thoroughly covers:
- Accept action with correct handler arguments
- Error handling when booking not found
- Error handling when user not found
- Reject action via POST with reason and actor propagation
apps/web/app/api/link/__tests__/route.test.ts (2)
81-83: Solid deterministic actor mock.Keeps actor propagation tests stable and focused.
178-180: TRPCError mock formatting looks good.No concerns with the mock change.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| vi.mock("@calcom/features/booking-audit/lib/makeActor", () => ({ | ||
| makeUserActor: vi.fn().mockReturnValue({ type: "user", id: "test-uuid" }), | ||
| })); |
There was a problem hiding this comment.
Mock structure doesn't match actual implementation.
The mock returns { type: "user", id: "test-uuid" }, but based on the actual makeUserActor implementation (see packages/features/booking-audit/lib/makeActor.ts), it returns { identifiedBy: "user", userUuid: "..." }.
While the test is internally consistent with its mock, this discrepancy could mask issues if downstream code relies on the actual shape.
Proposed fix to match actual implementation
vi.mock("@calcom/features/booking-audit/lib/makeActor", () => ({
- makeUserActor: vi.fn().mockReturnValue({ type: "user", id: "test-uuid" }),
+ makeUserActor: vi.fn().mockReturnValue({ identifiedBy: "user", userUuid: "test-uuid" }),
}));Also update the expectation at line 426:
- actor: { type: "user", id: "test-uuid" },
+ actor: { identifiedBy: "user", userUuid: "test-uuid" },🤖 Prompt for AI Agents
In `@apps/web/app/api/verify-booking-token/__tests__/route.test.ts` around lines
68 - 70, Update the test mock for makeUserActor so its return shape matches the
real implementation: have
vi.mock("@calcom/features/booking-audit/lib/makeActor", ...)
makeUserActor.mockReturnValue({ identifiedBy: "user", userUuid: "test-uuid" });
then update any assertions that previously expected { type, id } (e.g., the
expectation referenced in the review) to assert { identifiedBy, userUuid } (or
compare the userUuid value) so the test mirrors the actual makeUserActor output
and avoids masking shape-related bugs.
| const ctx = { | ||
| user: { | ||
| id: organizer.id, | ||
| name: organizer.name, | ||
| timeZone: organizer.timeZone, | ||
| username: organizer.username, | ||
| } as NonNullable<TrpcSessionUser>, | ||
| traceContext: distributedTracing.createTrace("test_confirm_handler"), | ||
| }; | ||
|
|
||
| const res = await confirmHandler({ | ||
| ctx, | ||
| input: { | ||
| bookingId: 101, | ||
| confirmed: true, | ||
| reason: "", | ||
| emailsEnabled: true, | ||
| actor: makeUserActor(ctx.user.uuid), | ||
| actionSource: "WEBAPP", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Missing uuid property in test context causes makeUserActor to receive undefined.
In this test, ctx.user does not include a uuid property (Line 132 uses ctx.user.uuid), unlike the other tests that explicitly set uuid: testUserUuid. This will pass undefined to makeUserActor, which may not be the intended behavior.
Proposed fix
+ const testUserUuid = "test-uuid-101";
const ctx = {
user: {
id: organizer.id,
name: organizer.name,
timeZone: organizer.timeZone,
username: organizer.username,
+ uuid: testUserUuid,
} as NonNullable<TrpcSessionUser>,
traceContext: distributedTracing.createTrace("test_confirm_handler"),
};📝 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 ctx = { | |
| user: { | |
| id: organizer.id, | |
| name: organizer.name, | |
| timeZone: organizer.timeZone, | |
| username: organizer.username, | |
| } as NonNullable<TrpcSessionUser>, | |
| traceContext: distributedTracing.createTrace("test_confirm_handler"), | |
| }; | |
| const res = await confirmHandler({ | |
| ctx, | |
| input: { | |
| bookingId: 101, | |
| confirmed: true, | |
| reason: "", | |
| emailsEnabled: true, | |
| actor: makeUserActor(ctx.user.uuid), | |
| actionSource: "WEBAPP", | |
| }, | |
| }); | |
| const testUserUuid = "test-uuid-101"; | |
| const ctx = { | |
| user: { | |
| id: organizer.id, | |
| name: organizer.name, | |
| timeZone: organizer.timeZone, | |
| username: organizer.username, | |
| uuid: testUserUuid, | |
| } as NonNullable<TrpcSessionUser>, | |
| traceContext: distributedTracing.createTrace("test_confirm_handler"), | |
| }; | |
| const res = await confirmHandler({ | |
| ctx, | |
| input: { | |
| bookingId: 101, | |
| confirmed: true, | |
| reason: "", | |
| emailsEnabled: true, | |
| actor: makeUserActor(ctx.user.uuid), | |
| actionSource: "WEBAPP", | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@apps/web/test/lib/confirm.handler.test.ts` around lines 115 - 135, The test's
ctx.user object is missing the uuid property so makeUserActor receives
undefined; update the test's ctx.user to include uuid: testUserUuid (or the same
fixture value used elsewhere) so that ctx.user.uuid passed into makeUserActor is
defined, ensuring confirmHandler receives a valid actor; locate the ctx object
in the confirm handler test near the confirmHandler(...) invocation and add the
uuid field to the user shaped as NonNullable<TrpcSessionUser>.
| await fireBookingAcceptedEvent({ | ||
| actor, | ||
| acceptedBookings, | ||
| organizationId: orgId ?? null, | ||
| actionSource, | ||
| tracingLogger, | ||
| }); |
There was a problem hiding this comment.
Audit event fired before database commit for recurring bookings.
fireBookingAcceptedEvent is called before the prisma.booking.update calls (Lines 287-346). If the database updates fail, the audit log will have recorded an acceptance that didn't actually persist. Consider moving this call after the successful database updates, similar to how the non-recurring path (Lines 434-442) fires the event after the DB update.
Proposed fix - move event firing after DB update
acceptedBookings = unconfirmedRecurringBookings.map((booking) => ({
oldStatus: booking.status,
uid: booking.uid,
}));
- const teamId = await getTeamIdFromEventType({
- eventType: {
- team: { id: eventType?.teamId ?? null },
- parentId: eventType?.parentId ?? null,
- },
- });
-
- const triggerForUser = !teamId || (teamId && eventType?.parentId);
- const userId = triggerForUser ? booking.userId : null;
- const orgId = await getOrgIdFromMemberOrTeamId({ memberId: userId, teamId });
-
- await fireBookingAcceptedEvent({
- actor,
- acceptedBookings,
- organizationId: orgId ?? null,
- actionSource,
- tracingLogger,
- });
const updateBookingsPromise = unconfirmedRecurringBookings.map((recurringBooking) =>
// ... existing update logic
);
const updatedBookingsResult = await Promise.all(updateBookingsPromise);
updatedBookings = updatedBookings.concat(updatedBookingsResult);
+
+ const teamId = await getTeamIdFromEventType({
+ eventType: {
+ team: { id: eventType?.teamId ?? null },
+ parentId: eventType?.parentId ?? null,
+ },
+ });
+
+ const triggerForUser = !teamId || (teamId && eventType?.parentId);
+ const userId = triggerForUser ? booking.userId : null;
+ const orgId = await getOrgIdFromMemberOrTeamId({ memberId: userId, teamId });
+
+ await fireBookingAcceptedEvent({
+ actor,
+ acceptedBookings,
+ organizationId: orgId ?? null,
+ actionSource,
+ tracingLogger,
+ });🤖 Prompt for AI Agents
In `@packages/features/bookings/lib/handleConfirmation.ts` around lines 279 - 285,
The audit event is being fired for recurring bookings before the database
updates, so move the fireBookingAcceptedEvent call (currently invoked with
actor, acceptedBookings, organizationId: orgId ?? null, actionSource,
tracingLogger) to after the prisma.booking.update batch that persists the
recurring booking changes; ensure you mirror the non-recurring path behavior
where the event is emitted only after successful DB commits so the audit
reflects the persisted state.
| acceptedBookings = [ | ||
| { | ||
| oldStatus: BookingStatus.ACCEPTED, | ||
| uid: booking.uid, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Incorrect oldStatus value for non-recurring bookings.
The oldStatus is hardcoded to BookingStatus.ACCEPTED, but at this point the booking was PENDING before being updated. This should capture the actual previous status from booking.status (which is passed in via the booking parameter at Line 126).
Proposed fix
acceptedBookings = [
{
- oldStatus: BookingStatus.ACCEPTED,
+ oldStatus: booking.status,
uid: booking.uid,
},
];📝 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.
| acceptedBookings = [ | |
| { | |
| oldStatus: BookingStatus.ACCEPTED, | |
| uid: booking.uid, | |
| }, | |
| ]; | |
| acceptedBookings = [ | |
| { | |
| oldStatus: booking.status, | |
| uid: booking.uid, | |
| }, | |
| ]; |
🤖 Prompt for AI Agents
In `@packages/features/bookings/lib/handleConfirmation.ts` around lines 411 - 416,
The acceptedBookings array is using a hardcoded oldStatus of
BookingStatus.ACCEPTED but should record the actual previous status from the
booking parameter; update the assignment that builds acceptedBookings in
handleConfirmation.ts (the block that sets acceptedBookings = [...]) to use
booking.status for oldStatus (e.g., oldStatus: booking.status) so the correct
prior state is captured for non-recurring bookings when processing confirmation.
| let rejectedBookings: { | ||
| uid: string; | ||
| oldStatus: BookingStatus; | ||
| }[] = []; | ||
|
|
||
| if (recurringEventId) { | ||
| // The booking to reject is a recurring event and comes from /booking/upcoming, proceeding to mark all related | ||
| // bookings as rejected. | ||
| await prisma.booking.updateMany({ | ||
| const unconfirmedRecurringBookings = await prisma.booking.findMany({ | ||
| where: { | ||
| recurringEventId, | ||
| status: BookingStatus.PENDING, | ||
| }, | ||
| select: { | ||
| uid: true, | ||
| status: true, | ||
| }, | ||
| }); | ||
|
|
||
| await prisma.booking.updateMany({ | ||
| where: { | ||
| uid: { | ||
| in: unconfirmedRecurringBookings.map((booking) => booking.uid), | ||
| }, | ||
| }, | ||
| data: { | ||
| status: BookingStatus.REJECTED, | ||
| rejectionReason, | ||
| }, | ||
| }); | ||
|
|
||
| const updatedRecurringBookings = await prisma.booking.findMany({ | ||
| where: { | ||
| uid: { | ||
| in: unconfirmedRecurringBookings.map((booking) => booking.uid), | ||
| }, | ||
| }, | ||
| select: { | ||
| uid: true, | ||
| status: true, | ||
| }, | ||
| }); | ||
|
|
||
| rejectedBookings = updatedRecurringBookings.map((recurringBooking) => ({ | ||
| uid: recurringBooking.uid, | ||
| oldStatus: recurringBooking.status, | ||
| })); | ||
| } else { |
There was a problem hiding this comment.
Audit oldStatus for recurring rejections is currently wrong.
rejectedBookings is built after the update, so oldStatus becomes REJECTED instead of the previous value. Capture the pre-update status from unconfirmedRecurringBookings (single-booking path already does this correctly).
🐛 Proposed fix
- const updatedRecurringBookings = await prisma.booking.findMany({
- where: {
- uid: {
- in: unconfirmedRecurringBookings.map((booking) => booking.uid),
- },
- },
- select: {
- uid: true,
- status: true,
- },
- });
-
- rejectedBookings = updatedRecurringBookings.map((recurringBooking) => ({
- uid: recurringBooking.uid,
- oldStatus: recurringBooking.status,
- }));
+ rejectedBookings = unconfirmedRecurringBookings.map((recurringBooking) => ({
+ uid: recurringBooking.uid,
+ oldStatus: recurringBooking.status,
+ }));Also applies to: 468-473
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/bookings/confirm.handler.ts` around lines
401 - 448, The recurring-booking branch wrongly sets rejectedBookings using
post-update data so oldStatus becomes REJECTED; change it to populate
rejectedBookings from the pre-update result unconfirmedRecurringBookings (use
its status field as oldStatus) before calling prisma.booking.updateMany,
mirroring the single-booking path logic in confirm.handler.ts (references:
rejectedBookings, unconfirmedRecurringBookings, updatedRecurringBookings,
rejectionReason); also make the analogous fix where single-booking rejection
populates oldStatus to ensure it uses the pre-update status.
1 participant
Benchmark PR from agentic-review-benchmarks#10
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.