fix: Make BOOKING_CANCELLED` webhook payload consistent for regular cancellation and request-reschedule flow #38
Conversation
WalkthroughThis pull request extends webhook payloads for booking cancellations with additional metadata fields (eventTypeId, title, length, iCalSequence) and introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/bookings/lib/handleSeats/cancel/cancelAttendeeSeat.ts (1)
162-168: HardcodedrequestReschedule: trueappears incorrect for a generic seat cancellation.Cancelling an attendee seat is not inherently a reschedule request. The webhook documentation shows
requestReschedule: falsefor regular cancellations. This should befalseby default, or ideally passed as a parameter from the caller to distinguish actual reschedule-triggered cancellations from regular ones.🐛 Proposed fix
const payload: EventPayloadType = { ...evt, ...eventTypeInfo, status: "CANCELLED", smsReminderNumber: bookingToDelete.smsReminderNumber || undefined, - requestReschedule: true, + requestReschedule: false, };Alternatively, if this function can be called from reschedule flows, consider adding a parameter to control this value.
🤖 Fix all issues with AI agents
In
`@packages/features/webhooks/lib/factory/versioned/v2021-10-20/BookingPayloadBuilder.ts`:
- Around line 45-50: In BookingPayloadBuilder, the extra.requestReschedule
default is incorrect; change the logic that sets extra.requestReschedule
(currently dto.requestReschedule ?? true) so it defaults to false when
dto.requestReschedule is undefined, i.e. use dto.requestReschedule ?? false (or
equivalent) so only explicit reschedule cancellations set it true; update the
assignment in the block that constructs extra (fields cancelledBy,
cancellationReason, requestReschedule) to reflect this.
🧹 Nitpick comments (1)
packages/features/bookings/lib/handleCancelBooking.ts (1)
413-430: Make the webhook feature flag parsing more robust.
Line 413-415:ENABLE_WEBHOOKS !== "false"treats"FALSE"/"0"as enabled. Consider normalizing to avoid surprising behavior in different env setups.♻️ Suggested tweak
- const webhooksEnabled = process.env.ENABLE_WEBHOOKS !== "false"; + const webhooksEnabled = !["false", "0", "no"].includes( + (process.env.ENABLE_WEBHOOKS ?? "").toLowerCase() + );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/developing/guides/automation/webhooks.mdxpackages/features/bookings/lib/handleCancelBooking.tspackages/features/bookings/lib/handleSeats/cancel/cancelAttendeeSeat.tspackages/features/bookings/repositories/BookingRepository.tspackages/features/webhooks/lib/dto/types.tspackages/features/webhooks/lib/factory/base/BaseBookingPayloadBuilder.tspackages/features/webhooks/lib/factory/versioned/v2021-10-20/BookingPayloadBuilder.tspackages/features/webhooks/lib/sendPayload.tspackages/features/webhooks/lib/service/BookingWebhookService.tspackages/features/webhooks/lib/types/params.tspackages/lib/server/service/BookingWebhookFactory.tspackages/lib/server/service/__tests__/BookingWebhookFactory.test.tspackages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/features/bookings/lib/handleCancelBooking.ts (2)
packages/features/booking-audit/lib/makeActor.ts (2)
buildActorEmail(84-86)getUniqueIdentifier(88-90)packages/features/webhooks/lib/WebhookService.ts (1)
sendPayload(21-37)
🔇 Additional comments (22)
packages/features/bookings/repositories/BookingRepository.ts (5)
142-179: Looks good: conditional filters remain precise.The rrTimestampBasis switch and virtual queue filters keep the query scoped correctly.
328-328: LGTM (constructor change).
659-672: LGTM: pending bookings query is well scoped.
1659-1664: Good addition: richer eventType and iCal fields.Also applies to: 1687-1689
1919-1925: LGTM (recorded status updater).docs/developing/guides/automation/webhooks.mdx (1)
314-315: LGTM!The documentation correctly shows
requestReschedule: falsefor a regular booking cancellation example, which aligns with the semantic meaning of the field.packages/features/webhooks/lib/dto/types.ts (2)
42-58: LGTM!The DTO extensions for
iCalSequenceandrequestRescheduleare well-typed and appropriately optional, supporting the new webhook payload fields.
571-588: LGTM!The
requestReschedulefield addition toEventPayloadTypealigns with the broader payload consistency changes.packages/features/webhooks/lib/types/params.ts (1)
40-66: LGTM!The parameter type extensions for
iCalSequenceandrequestRescheduleare consistent with the DTO definitions and enable proper propagation through the cancellation flow.packages/lib/server/service/__tests__/BookingWebhookFactory.test.ts (1)
88-116: LGTM!The test correctly validates the expanded payload structure, including the new
eventTitle,eventTypeId,length,requestReschedule,iCalSequence, andstatusfields. This provides good coverage for the payload contract.packages/features/webhooks/lib/sendPayload.ts (1)
95-100: PropagaterequestReschedulein the base event payload type.
Line 99: This is the right place to surface the new flag so downstream payload builders stay consistent.packages/features/bookings/lib/handleCancelBooking.ts (5)
57-62: Audit actor helpers import is appropriate.
Line 57-62: Centralizing email/identifier generation for audit actors keeps the attribution logic consistent.
110-120: Guest actor email generation looks sound.
Line 110-120: Using a generated identifier avoids trusting user-supplied emails for audit attribution.
154-160: Good guardrail for missing action source.
Line 154-160: Logging UNKNOWN actionSource helps track callers that skip metadata.
374-379: Team payload block is consistent.
Line 374-379: Team metadata is still conditionally attached only for team events.
734-734: Constructor addition is fine.
Line 734: Keeping dependencies injected via constructor is consistent with the DI direction in this file.packages/features/webhooks/lib/service/BookingWebhookService.ts (1)
105-120:requestReschedulepropagation is correct.
Line 119: Forwarding the flag from params into the DTO matches the new payload shape.packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts (2)
106-106: Safer event defaulting.
Line 106: Defaulting toeventType ?? {}avoids undefined access while preserving type intent.
274-279: Cancelled payload enrichment looks consistent.
Line 274-279: Adding event metadata plusrequestReschedule: truealigns the reschedule cancellation with the new webhook contract.packages/features/webhooks/lib/factory/base/BaseBookingPayloadBuilder.ts (1)
14-18: Extra data map update is appropriate.
Line 14-18: AddingrequestRescheduleto the BOOKING_CANCELLED extra data keeps type coverage aligned with the new payload.packages/lib/server/service/BookingWebhookFactory.ts (2)
52-57: Cancelled payload interface expansion is sound.
Line 52-57: Optional metadata fields match the new cancellation payload needs.
130-135: Defaults and status assignment are consistent.
Line 130-135: Emittingstatus: "CANCELLED"plus null/false defaults keeps payloads stable for consumers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| extra: { | ||
| cancelledBy: dto.cancelledBy, | ||
| cancellationReason: dto.cancellationReason, | ||
| requestReschedule: dto.requestReschedule ?? true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Default value for requestReschedule should be false, not true.
A booking cancellation where requestReschedule is not explicitly set should default to false, indicating a regular cancellation. The current default of true contradicts the documentation which shows requestReschedule: false for standard cancellations. Only reschedule-triggered cancellations should explicitly set this to true.
🐛 Proposed fix
extra: {
cancelledBy: dto.cancelledBy,
cancellationReason: dto.cancellationReason,
- requestReschedule: dto.requestReschedule ?? true,
+ requestReschedule: dto.requestReschedule ?? false,
},📝 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.
| extra: { | |
| cancelledBy: dto.cancelledBy, | |
| cancellationReason: dto.cancellationReason, | |
| requestReschedule: dto.requestReschedule ?? true, | |
| }, | |
| }); | |
| extra: { | |
| cancelledBy: dto.cancelledBy, | |
| cancellationReason: dto.cancellationReason, | |
| requestReschedule: dto.requestReschedule ?? false, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In
`@packages/features/webhooks/lib/factory/versioned/v2021-10-20/BookingPayloadBuilder.ts`
around lines 45 - 50, In BookingPayloadBuilder, the extra.requestReschedule
default is incorrect; change the logic that sets extra.requestReschedule
(currently dto.requestReschedule ?? true) so it defaults to false when
dto.requestReschedule is undefined, i.e. use dto.requestReschedule ?? false (or
equivalent) so only explicit reschedule cancellations set it true; update the
assignment in the block that constructs extra (fields cancelledBy,
cancellationReason, requestReschedule) to reflect this.
Benchmark PR from agentic-review-benchmarks#11
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.