Skip to content

chore: Integrate confirmation booking audit#58

Open
tomerqodo wants to merge 2 commits intosentry_combined_20260121_augment_sentry_coderabbit_1_base_chore_integrate_confirmation_booking_audit_pr714from
sentry_combined_20260121_augment_sentry_coderabbit_1_head_chore_integrate_confirmation_booking_audit_pr714
Open

chore: Integrate confirmation booking audit#58
tomerqodo wants to merge 2 commits intosentry_combined_20260121_augment_sentry_coderabbit_1_base_chore_integrate_confirmation_booking_audit_pr714from
sentry_combined_20260121_augment_sentry_coderabbit_1_head_chore_integrate_confirmation_booking_audit_pr714

Conversation

@tomerqodo
Copy link

Benchmark PR from qodo-benchmark#714

Comment on lines +412 to +416
{
oldStatus: BookingStatus.ACCEPTED,
uid: booking.uid,
},
];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +444 to +447
rejectedBookings = updatedRecurringBookings.map((recurringBooking) => ({
uid: recurringBooking.uid,
oldStatus: recurringBooking.status,
}));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments