fix: Make BOOKING_CANCELLED` webhook payload consistent for regular cancellation and request-reschedule flow#106
Open
Conversation
Code Review by Qodo
1. process.env in handleCancelBooking
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+413
to
+416
| // Only send webhooks if enabled in environment | ||
| const webhooksEnabled = process.env.ENABLE_WEBHOOKS !== "false"; | ||
|
|
||
| const promises = webhooksEnabled ? webhooks.map((webhook) => |
There was a problem hiding this comment.
1. process.env in handlecancelbooking 📘 Rule violation ⛯ Reliability
• The cancellation handler reads process.env.ENABLE_WEBHOOKS directly inside business logic to decide whether to emit webhooks. • This violates the requirement to limit direct environment access to configuration/environment utilities, reducing testability and increasing risk of runtime misconfiguration. • The webhook enablement flag should be injected via a config object or read through a dedicated configuration abstraction.
Agent prompt
## Issue description
`handleCancelBooking` directly reads `process.env.ENABLE_WEBHOOKS` inside business logic, which violates the rule that environment variables must only be accessed in configuration/environment-detection code.
## Issue Context
Webhook emission should be controlled via an injected/configured flag (e.g., `deps.config.webhooksEnabled`) or a dedicated config module that encapsulates `process.env` reads.
## Fix Focus Areas
- packages/features/bookings/lib/handleCancelBooking.ts[413-430]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| extra: { | ||
| cancelledBy: dto.cancelledBy, | ||
| cancellationReason: dto.cancellationReason, | ||
| requestReschedule: dto.requestReschedule ?? true, |
There was a problem hiding this comment.
2. Wrong reschedule default 🐞 Bug ✓ Correctness
• Webhook v2021-10-20 builder defaults requestReschedule to true when omitted, which will mark *all* BOOKING_CANCELLED webhooks as reschedule-related for subscribers on that version. • This contradicts the docs and other cancellation senders in this PR that use/assume false, and can break downstream automations that rely on this flag.
Agent prompt
### Issue description
`packages/features/webhooks` v2021-10-20 payload builder currently defaults `requestReschedule` to `true` for BOOKING_CANCELLED when the DTO doesn't set it, which mislabels normal cancellations as reschedule requests.
### Issue Context
Docs and other cancellation senders in this PR treat `requestReschedule` as `false` for standard cancellations.
### Fix Focus Areas
- packages/features/webhooks/lib/factory/versioned/v2021-10-20/BookingPayloadBuilder.ts[37-50]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+413
to
+430
| // Only send webhooks if enabled in environment | ||
| const webhooksEnabled = process.env.ENABLE_WEBHOOKS !== "false"; | ||
|
|
||
| const promises = webhooksEnabled ? webhooks.map((webhook) => | ||
| sendPayload(webhook.secret, eventTrigger, new Date().toISOString(), webhook, { | ||
| ...evt, | ||
| ...eventTypeInfo, | ||
| status: "CANCELLED", | ||
| smsReminderNumber: bookingToDelete.smsReminderNumber || undefined, | ||
| cancelledBy: cancelledBy, | ||
| requestReschedule: false, | ||
| }).catch((e) => { | ||
| logger.error( | ||
| `Error executing webhook for event: ${eventTrigger}, URL: ${webhook.subscriberUrl}, bookingId: ${evt.bookingId}, bookingUid: ${evt.uid}`, | ||
| safeStringify(e) | ||
| ); | ||
| }) | ||
| ); | ||
| ) : []; |
There was a problem hiding this comment.
3. Webhook gate incomplete 🐞 Bug ⛯ Reliability
• ENABLE_WEBHOOKS gating was added only to the main cancellation handler; other cancellation paths still emit webhooks unconditionally. • This creates unreliable operational behavior: setting ENABLE_WEBHOOKS=false will not actually disable all cancellation-related webhooks, leading to partial/accidental emissions.
Agent prompt
### Issue description
`ENABLE_WEBHOOKS` is only checked in one cancellation code path, so webhooks are still emitted by other cancellation flows even when operators believe webhooks are disabled.
### Issue Context
There is already an env-driven decision point in `sendOrSchedulePayload` (`TASKER_ENABLE_WEBHOOKS`) which is a more central place to enforce webhook-level gating.
### Fix Focus Areas
- packages/features/bookings/lib/handleCancelBooking.ts[413-431]
- packages/features/bookings/lib/handleSeats/cancel/cancelAttendeeSeat.ts[170-184]
- packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts[305-314]
- packages/features/webhooks/lib/sendOrSchedulePayload.ts[6-10]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+274
to
+278
| eventTypeId: bookingToReschedule.eventTypeId, | ||
| length: bookingToReschedule.eventType?.length ?? null, | ||
| iCalSequence: (bookingToReschedule.iCalSequence ?? 0) + 1, | ||
| eventTitle: bookingToReschedule.eventType?.title ?? null, | ||
| requestReschedule: true, |
There was a problem hiding this comment.
4. Icalsequence not persisted 🐞 Bug ⛯ Reliability
• The request-reschedule flow includes an incremented iCalSequence in the webhook payload, but does not persist the increment to the booking record. • This can cause inconsistencies between webhook data and the booking source-of-truth (booking.iCalSequence), and diverges from the standard cancellation flow which persists iCalSequence when cancelling.
Agent prompt
### Issue description
The request-reschedule flow increments `iCalSequence` for the webhook payload but does not update the booking record. This can create inconsistency between stored booking data and emitted webhook data.
### Issue Context
Standard cancellation persists iCalSequence on cancellation; request-reschedule should likely do the same if it emits the incremented value.
### Fix Focus Areas
- packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts[106-113]
- packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts[248-279]
- packages/features/bookings/repositories/BookingRepository.ts[1693-1720]
ⓘ 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
2 participants
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#11