Conversation
| { | ||
| oldStatus: BookingStatus.ACCEPTED, | ||
| uid: booking.uid, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Bug: In handleConfirmation, the oldStatus for a non-recurring booking is hardcoded to BookingStatus.ACCEPTED instead of using the booking's actual status before the update.
Severity: MEDIUM
Suggested Fix
Replace the hardcoded BookingStatus.ACCEPTED with booking.status to correctly capture the booking's status before it was updated. This will ensure the audit trail reflects the correct state transition, for example, from PENDING to ACCEPTED.
acceptedBookings = [
{
oldStatus: booking.status, // Use the original status
uid: booking.uid,
},
];Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/features/bookings/lib/handleConfirmation.ts#L412-L416
Potential issue: When confirming a non-recurring booking in `handleConfirmation`, the
`oldStatus` is hardcoded to `BookingStatus.ACCEPTED`. However, the booking's status is
updated to `ACCEPTED` earlier in the function. The original `booking` object, which
holds the pre-update status (e.g., `PENDING`), is available. This hardcoded value causes
event handlers like `fireBookingAcceptedEvent` to receive incorrect data, logging a
status transition from `ACCEPTED` to `ACCEPTED` instead of the correct `PENDING` to
`ACCEPTED`. This is inconsistent with the logic for recurring bookings, which correctly
uses the pre-update status.
Did we get this right? 👍 / 👎 to inform future reviews.
| rejectedBookings = updatedRecurringBookings.map((recurringBooking) => ({ | ||
| uid: recurringBooking.uid, | ||
| oldStatus: recurringBooking.status, | ||
| })); |
There was a problem hiding this comment.
Bug: When rejecting recurring bookings, the oldStatus is sourced from bookings that have already been updated to REJECTED, leading to incorrect audit data.
Severity: MEDIUM
Suggested Fix
To get the correct pre-update status, map over unconfirmedRecurringBookings instead of updatedRecurringBookings when constructing the rejectedBookings array. This will provide the correct oldStatus (e.g., PENDING) for the audit event.
rejectedBookings = unconfirmedRecurringBookings.map((recurringBooking) => ({
uid: recurringBooking.uid,
oldStatus: recurringBooking.status,
}));Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/trpc/server/routers/viewer/bookings/confirm.handler.ts#L444-L447
Potential issue: When rejecting recurring bookings in the `confirm.handler`, the code
determines the `oldStatus` from `updatedRecurringBookings`. This variable holds the
bookings *after* their status has been updated to `REJECTED`. As a result, the
`oldStatus` is incorrectly reported as `REJECTED` instead of its original state (e.g.,
`PENDING`). This will cause incorrect audit data to be recorded, showing a `REJECTED` to
`REJECTED` transition instead of the actual `PENDING` to `REJECTED` transition. The
correct pre-update booking data is available in the `unconfirmedRecurringBookings`
variable.
Did we get this right? 👍 / 👎 to inform future reviews.
Benchmark PR from qodo-benchmark#714