fix: Make BOOKING_CANCELLED` webhook payload consistent for regular cancellation and request-reschedule flow#22
fix: Make BOOKING_CANCELLED` webhook payload consistent for regular cancellation and request-reschedule flow#22
Conversation
WalkthroughThis PR adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
🤖 Fix all issues with AI agents
In
`@packages/features/webhooks/lib/factory/versioned/v2021-10-20/BookingPayloadBuilder.ts`:
- Around line 45-50: The extra.requestReschedule field in
BookingPayloadBuilder.ts is defaulting to true (requestReschedule:
dto.requestReschedule ?? true) causing cancellations to be treated as reschedule
requests when unset; change the default to false by using dto.requestReschedule
?? false so that only explicit true values indicate a reschedule; update the
occurrence in BookingPayloadBuilder (the extra: { cancelledBy,
cancellationReason, requestReschedule } block) to use the corrected default.
🧹 Nitpick comments (1)
packages/lib/server/service/__tests__/BookingWebhookFactory.test.ts (1)
88-116: Consider adding explicit assertions for the new fields' default values.The test correctly expects the new keys to exist but doesn't verify their values. Since the factory defaults these fields when not provided (
eventTypeId: null,length: null,iCalSequence: null,eventTitle: null,requestReschedule: false), consider adding explicit assertions to catch regressions:💡 Suggested improvement
expect(payload.cancelledBy).toBe("test@example.com"); + + // Verify new fields default to expected values when not provided + expect(payload.eventTypeId).toBeNull(); + expect(payload.length).toBeNull(); + expect(payload.iCalSequence).toBeNull(); + expect(payload.eventTitle).toBeNull(); + expect(payload.requestReschedule).toBe(false); + expect(payload.status).toBe("CANCELLED"); // Verify no extra properties exist
📜 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 (2)
packages/features/webhooks/lib/factory/base/BaseBookingPayloadBuilder.ts (1)
packages/platform/libraries/index.ts (1)
WebhookTriggerEvents(36-36)
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 (19)
packages/features/bookings/lib/handleSeats/cancel/cancelAttendeeSeat.ts (1)
162-168: LGTM -requestReschedule: truecorrectly identifies seat cancellations during reschedule flow.The hardcoded
truevalue is appropriate here sincecancelAttendeeSeatis invoked as part of the reschedule workflow, allowing downstream webhook consumers to distinguish this from a regular cancellation.docs/developing/guides/automation/webhooks.mdx (1)
314-316: Documentation correctly showsrequestReschedule: falsefor standard cancellation.The example payload accurately reflects that regular cancellations will have
requestReschedule: false, helping API consumers understand how to distinguish between standard cancellations and reschedule-triggered cancellations.packages/features/webhooks/lib/sendPayload.ts (1)
82-100: LGTM - Type extension forrequestRescheduleis correctly defined.The optional boolean field is appropriately added to
EventPayloadType, maintaining backward compatibility while enabling the new cancellation distinction feature.packages/features/webhooks/lib/service/BookingWebhookService.ts (1)
117-120: LGTM - Properly passesrequestReschedulefrom params to DTO.The service layer correctly passes through the
requestReschedulevalue without transformation, consistent with how other cancellation-related fields are handled.packages/features/webhooks/lib/factory/base/BaseBookingPayloadBuilder.ts (1)
14-18: LGTM - Type definition correctly extended withrequestReschedule.The
BookingExtraDataMaptype forBOOKING_CANCELLEDis properly extended with the new optional boolean field, providing type safety for the payload builders.packages/features/webhooks/lib/types/params.ts (1)
40-66: LGTM!The type additions are well-placed and consistent with the PR objectives.
iCalSequencecorrectly belongs in the booking object as booking-specific data, whilerequestRescheduleas a top-level boolean appropriately indicates the flow type.packages/features/bookings/lib/handleCancelBooking.ts (2)
413-431: Feature flag gating andrequestReschedule: falselook correct.The implementation correctly:
- Gates webhook dispatch behind
ENABLE_WEBHOOKSenvironment variable (enabled by default)- Sets
requestReschedule: falsefor regular cancellation flow, distinguishing it from the reschedule flowHowever, consider extracting the feature flag check to a shared utility for consistency across the codebase, especially since other webhook dispatch locations may need the same check.
110-121: LGTM!The refactored function calls using object parameters improve readability and make the intent clearer (e.g.,
actorType: "guest"vs positional arguments).packages/lib/server/service/BookingWebhookFactory.ts (2)
49-57: LGTM!The interface extension is well-designed with appropriate optional types that align with the PR objectives for making the cancellation payload consistent.
126-137: LGTM!Good use of nullish coalescing with sensible defaults:
nullfor optional data fields andfalsefor therequestRescheduleboolean. The"CANCELLED" as constprovides proper type narrowing for consumers.packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts (2)
274-279: LGTM!The new payload fields are correctly populated:
requestReschedule: trueproperly distinguishes this flow from regular cancellationiCalSequenceis incremented consistently with the cancellation flow (using?? 0as a safe default before incrementing)- Optional fields use appropriate
?? nullfallbacks
106-106: LGTM!The nullish coalescing assignment
bookingToReschedule.eventType ?? {}is cleaner and functionally equivalent to the previous guarded initialization.packages/features/webhooks/lib/dto/types.ts (2)
42-58: LGTM!The DTO type extensions are consistent with the rest of the type system.
iCalSequencein the booking object andrequestRescheduleat the root level align perfectly withBookingCancelledParamsinparams.ts.
571-588: LGTM!Adding
requestReschedule?: booleantoEventPayloadTypeensures the field can flow through the entire webhook pipeline consistently.packages/features/bookings/repositories/BookingRepository.ts (5)
140-179: LGTM — RR filters look consistent with no‑show and routing constraints.The added
noShowHost, timestamp‑basis, and routing‑form filters align with the intended RR selection behavior.
328-328: LGTM — constructor formatting change only.
659-672: LGTM — PENDING confirmation query is clear and scoped.
1658-1688: LGTM — event type and iCal fields are now included in the select.This matches the expanded webhook payload shape for cancellations/reschedules.
1919-1925: LGTM — signature formatting only.
✏️ 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.
The default ?? true means that if a caller forgets to set requestReschedule, the webhook will incorrectly indicate a reschedule request. Based on the PR's semantic intent and the documentation example (which shows false), regular cancellations should default to false, with only explicit reschedule flows setting 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, The extra.requestReschedule field in
BookingPayloadBuilder.ts is defaulting to true (requestReschedule:
dto.requestReschedule ?? true) causing cancellations to be treated as reschedule
requests when unset; change the default to false by using dto.requestReschedule
?? false so that only explicit true values indicate a reschedule; update the
occurrence in BookingPayloadBuilder (the extra: { cancelledBy,
cancellationReason, requestReschedule } block) to use the corrected default.
2 participants
Benchmark PR from qodo-benchmark#694
Summary by CodeRabbit
New Features
requestRescheduleflag indicating if a reschedule was requested.iCalSequencefield to webhook payloads for better calendar synchronization.Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.