fixes: backend/room-booking#250
Conversation
|
@harshitap1305 is attempting to deploy a commit to the openlake's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe PR updates Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Controller as RoomBooking<br/>Controller
participant DB as MongoDB<br/>Session
participant Booking as Booking<br/>Collection
participant Room as Room<br/>Collection
participant Event as Event<br/>Collection
Client->>Controller: POST /bookRoom (request data)
Controller->>Controller: Validate fields, parse times, check startTime < endTime
Controller->>Room: Verify room exists & active
Controller->>Controller: Authorize role
Controller->>DB: startSession()
DB->>DB: withTransaction() start
DB->>Booking: Find overlapping<br/>pending/approved bookings
Booking-->>DB: Conflicting booking?
alt Conflict Found
DB->>DB: Rollback
DB-->>Controller: Throw conflict error
Controller-->>Client: 409 + conflictingBooking
else No Conflict
DB->>Event: Find overlapping events<br/>(via findOverlappingEvent)
Event-->>DB: Conflicting event?
alt Event Conflict Found
DB->>DB: Rollback
DB-->>Controller: Throw conflict error
Controller-->>Client: 409 + conflictingEvent
else No Conflict
DB->>Booking: Save new booking
Booking-->>DB: Created
DB->>DB: Commit transaction
DB-->>Controller: Success
Controller-->>Client: 201 + booking data
end
end
sequenceDiagram
actor Client
participant Controller as RoomBooking<br/>Controller
participant DB as MongoDB<br/>Session
participant Booking as Booking<br/>Collection
participant Room as Room<br/>Collection
participant Event as Event<br/>Collection
Client->>Controller: PATCH /booking/:id (status)
Controller->>Controller: Validate booking ID & target status
Controller->>Booking: Fetch booking
Booking-->>Controller: Booking data
alt Status not Pending
Controller-->>Client: 409 + error
else Status is Pending
Controller->>DB: startSession()
DB->>DB: withTransaction() start
DB->>Booking: Find other approved<br/>bookings in time range
Booking-->>DB: Conflict?
alt Conflict Found
DB->>DB: Rollback
DB-->>Controller: Throw error
Controller-->>Client: 409 + conflictingBooking
else No Conflict
DB->>Event: Find overlapping events
Event-->>DB: Conflict?
alt Event Conflict
DB->>DB: Rollback
DB-->>Controller: Throw error
Controller-->>Client: 409 + conflictingEvent
else No Conflict
DB->>Booking: Update status, reviewedBy, timestamp
Booking-->>DB: Updated
DB->>Room: Update room timestamp
Room-->>DB: Updated
DB->>DB: Commit transaction
DB-->>Controller: Success
Controller-->>Client: 200 + booking data
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the room booking controller to make booking creation and booking review (approve/reject) more robust by reintroducing transaction/session usage and improving how overlapping events are queried.
Changes:
- Made
findOverlappingEvent()apply a Mongoose session only when one is provided. - Refactored
bookRoomto perform clash checks + booking creation inside a transaction and return richer 409 conflict responses. - Refactored
updateBookingStatusto perform approval validation and status updates inside a transaction (including overlap checks).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const bounds = getDayBounds(referenceDate); | ||
| if (!bounds) return false; | ||
| // Check if the time falls exactly within that local day's 24-hour IST window | ||
| return testDate >= bounds.dayStart && testDate <= bounds.dayEnd; |
There was a problem hiding this comment.
isSameCalendarDay treats dayEnd as inclusive (<=). Since getDayBounds() defines dayEnd as exactly dayStart + 24h, this will classify timestamps exactly at the next day’s 00:00 IST boundary as being on the previous day. Consider making dayEnd exclusive (testDate < bounds.dayEnd) or setting dayEnd to the last millisecond of the day if you want an inclusive comparison.
| return testDate >= bounds.dayStart && testDate <= bounds.dayEnd; | |
| return testDate >= bounds.dayStart && testDate < bounds.dayEnd; |
| const roomForBooking = await Room.findById(booking.room) | ||
| .session(session) | ||
| .select("name room_id location") | ||
| .lean(); | ||
|
|
||
| if (!roomForBooking) { | ||
| const missingRoomError = new Error("Room not found or inactive."); | ||
| missingRoomError.statusCode = 404; | ||
| throw missingRoomError; | ||
| } |
There was a problem hiding this comment.
When approving a booking, the code treats a missing room as "not found or inactive", but Room.findById(...).select("name room_id location") does not check is_active. This can allow approvals for rooms that have been deactivated. Filter by is_active: true (or select is_active and validate it) to match the error message and booking creation behavior.
| await session.withTransaction(async () => { | ||
| const clash = await RoomBooking.findOne({ | ||
| room: roomObjectId, | ||
| status: { $in: ["Pending", "Approved"] }, | ||
| date: { $gte: dayBounds.dayStart, $lt: dayBounds.dayEnd }, | ||
| startTime: { $lt: parsedEndTime }, | ||
| endTime: { $gt: parsedStartTime }, | ||
| }) | ||
| .session(session) | ||
| .select("_id room event startTime endTime status") | ||
| .lean(); | ||
|
|
||
| if (clash) { | ||
| const conflictError = new Error("Room clash detected"); | ||
| conflictError.statusCode = 409; | ||
| conflictError.conflictingBooking = clash; | ||
| throw conflictError; | ||
| } | ||
|
|
||
| const overlappingEvent = await findOverlappingEvent({ | ||
| session, | ||
| venueCandidates, | ||
| start: parsedStartTime, | ||
| end: parsedEndTime, | ||
| excludeEventId: eventId, | ||
| }); | ||
|
|
||
| if (overlappingEvent) { | ||
| const eventConflictError = new Error("Room clash detected"); | ||
| eventConflictError.statusCode = 409; | ||
| eventConflictError.conflictingEvent = overlappingEvent; | ||
| throw eventConflictError; | ||
| } | ||
|
|
||
| const newBooking = new RoomBooking({ | ||
| room: roomObjectId, | ||
| event: eventId || undefined, | ||
| date: dayBounds.dayStart, | ||
| startTime: parsedStartTime, | ||
| endTime: parsedEndTime, | ||
| purpose, | ||
| bookedBy: req.user._id, | ||
| }); | ||
|
|
||
| await newBooking.save({ session }); | ||
| createdBookingId = newBooking._id; | ||
|
|
There was a problem hiding this comment.
The transaction here does not actually prevent two concurrent requests from both passing the overlap checks and inserting overlapping bookings, because each transaction only inserts a new RoomBooking document (no shared document is written that would cause a write conflict). If double-booking under concurrency must be prevented, introduce a per-(room, day) lock document (or another mechanism) that both transactions update within the transaction before re-checking overlaps, so MongoDB can detect a write conflict and force a retry.
| const dayBounds = getDayBounds(booking.date); | ||
| const overlappingApprovedBooking = await RoomBooking.findOne({ | ||
| _id: { $ne: booking._id }, | ||
| room: booking.room, | ||
| status: "Approved", | ||
| date: { $gte: dayBounds.dayStart, $lt: dayBounds.dayEnd }, | ||
| startTime: { $lt: booking.endTime }, | ||
| endTime: { $gt: booking.startTime }, | ||
| }) | ||
| .session(session) | ||
| .select("_id room event startTime endTime") | ||
| .lean(); | ||
|
|
||
| if (overlappingApprovedBooking) { | ||
| const conflictError = new Error( | ||
| "Cannot approve booking due to overlap with another approved booking.", | ||
| ); | ||
| conflictError.statusCode = 409; | ||
| conflictError.conflictingBooking = overlappingApprovedBooking; | ||
| throw conflictError; | ||
| } | ||
|
|
||
| const overlappingEvent = await findOverlappingEvent({ | ||
| session, | ||
| venueCandidates, | ||
| start: booking.startTime, | ||
| end: booking.endTime, | ||
| excludeEventId: booking.event, | ||
| }); | ||
|
|
||
| if (overlappingEvent) { | ||
| const eventConflictError = new Error( | ||
| "Cannot approve booking due to overlap with another event.", | ||
| ); | ||
| eventConflictError.statusCode = 409; | ||
| eventConflictError.conflictingEvent = overlappingEvent; | ||
| throw eventConflictError; | ||
| } | ||
| } | ||
|
|
||
| booking.status = status; | ||
| booking.reviewedBy = req.user._id; | ||
| booking.updated_at = new Date(); | ||
| await booking.save({ session }); | ||
| reviewedBookingId = booking._id; |
There was a problem hiding this comment.
Same concurrency issue on approval: two reviewers (or retries) can concurrently approve overlapping pending bookings since the transaction only updates the specific booking document and reads approved bookings; there’s no shared lock/write that would create a conflict. If exclusive approval is required, use a per-(room, day) lock/update inside the transaction (or another conflict-enforcing strategy) before checking overlaps and setting status to Approved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/roomBookingController.js`:
- Around line 568-577: The Room lookup currently ignores the room's is_active
flag so disabled rooms can still be approved; update the query that populates
roomForBooking to only return active rooms (e.g., replace
Room.findById(booking.room) with a query that includes { _id: booking.room,
is_active: true } or add .where({ is_active: true })), keep the same
.session(session).select("name room_id location").lean() chain, and leave the
existing not-found Error throw intact so inactive rooms trigger the 404 path.
- Around line 363-369: The current same-day check allows an end time exactly at
dayBounds.dayEnd (next day's 00:00); update the validation in the block using
parsedStartTime, parsedEndTime, dayBounds.dayStart and dayBounds.dayEnd so that
bookings ending exactly at dayBounds.dayEnd are rejected. Concretely, keep the
existing isSameCalendarDay checks but add a guard that also fails when
parsedEndTime.getTime() === dayBounds.dayEnd.getTime() (or otherwise ensure
parsedEndTime is strictly before dayBounds.dayEnd) so next-day midnight end
times are treated as cross-day and return the 400 error.
- Around line 559-589: The approval path only checks startTime < endTime but
doesn't prevent approving bookings that span midnight; before approving (in the
block using getDayBounds and RoomBooking.findOne) reapply the same-day
invariant: use getDayBounds(booking.date) to compute the day's bounds and ensure
the booking's start and end datetimes fall inside that single day (e.g., combine
booking.date with booking.startTime/booking.endTime and assert start < end and
end <= dayBounds.dayEnd); if the check fails throw a 400 error (same pattern as
the existing invalidTimeError). This prevents approving cross-day pending rows
that would evade the overlap query and keeps RoomBooking.findOne safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d032e943-b716-4975-8809-de1268ec05bf
📒 Files selected for processing (1)
backend/controllers/roomBookingController.js
| if ( | ||
| !isSameCalendarDay(parsedStartTime, dayBounds.dayStart) || | ||
| !isSameCalendarDay(parsedEndTime, dayBounds.dayStart) | ||
| ) { | ||
| return res | ||
| .status(400) | ||
| .json({ message: "Booking must start and end on the same date." }); |
There was a problem hiding this comment.
Reject next-day midnight end times here.
This check still lets a booking ending exactly at the next day's 00:00 through, because isSameCalendarDay() treats dayEnd as inclusive. That creates a cross-day booking while date is still bucketed under the previous day, so the spillover can be missed by next-day availability/conflict queries.
Proposed fix
- if (
- !isSameCalendarDay(parsedStartTime, dayBounds.dayStart) ||
- !isSameCalendarDay(parsedEndTime, dayBounds.dayStart)
- ) {
+ if (
+ parsedStartTime < dayBounds.dayStart ||
+ parsedStartTime >= dayBounds.dayEnd ||
+ parsedEndTime <= dayBounds.dayStart ||
+ parsedEndTime >= dayBounds.dayEnd
+ ) {
return res
.status(400)
.json({ message: "Booking must start and end on the same date." });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/roomBookingController.js` around lines 363 - 369, The
current same-day check allows an end time exactly at dayBounds.dayEnd (next
day's 00:00); update the validation in the block using parsedStartTime,
parsedEndTime, dayBounds.dayStart and dayBounds.dayEnd so that bookings ending
exactly at dayBounds.dayEnd are rejected. Concretely, keep the existing
isSameCalendarDay checks but add a guard that also fails when
parsedEndTime.getTime() === dayBounds.dayEnd.getTime() (or otherwise ensure
parsedEndTime is strictly before dayBounds.dayEnd) so next-day midnight end
times are treated as cross-day and return the 400 error.
| if (status === "Approved") { | ||
| if (!(booking.startTime < booking.endTime)) { | ||
| const invalidTimeError = new Error( | ||
| "Invalid booking time range for approval.", | ||
| ); | ||
| invalidTimeError.statusCode = 400; | ||
| throw invalidTimeError; | ||
| } | ||
|
|
||
| const roomForBooking = await Room.findById(booking.room) | ||
| .session(session) | ||
| .select("name room_id location") | ||
| .lean(); | ||
|
|
||
| if (!roomForBooking) { | ||
| const missingRoomError = new Error("Room not found or inactive."); | ||
| missingRoomError.statusCode = 404; | ||
| throw missingRoomError; | ||
| } | ||
|
|
||
| const venueCandidates = getRoomVenueCandidates(roomForBooking); | ||
|
|
||
| const dayBounds = getDayBounds(booking.date); | ||
| const overlappingApprovedBooking = await RoomBooking.findOne({ | ||
| _id: { $ne: booking._id }, | ||
| room: booking.room, | ||
| status: "Approved", | ||
| date: { $gte: dayBounds.dayStart, $lt: dayBounds.dayEnd }, | ||
| startTime: { $lt: booking.endTime }, | ||
| endTime: { $gt: booking.startTime }, | ||
| }) |
There was a problem hiding this comment.
Reapply the same-day invariant during approval.
bookRoom now blocks cross-day ranges, but approval only checks startTime < endTime. Any preexisting pending row that spans midnight can still be approved, and this overlap query only scans booking.date's day bucket, so conflicts on the following day are missed.
Proposed fix
if (status === "Approved") {
if (!(booking.startTime < booking.endTime)) {
const invalidTimeError = new Error(
"Invalid booking time range for approval.",
);
invalidTimeError.statusCode = 400;
throw invalidTimeError;
}
+
+ const approvalDayBounds = getDayBounds(booking.date);
+ if (
+ !approvalDayBounds ||
+ booking.startTime < approvalDayBounds.dayStart ||
+ booking.startTime >= approvalDayBounds.dayEnd ||
+ booking.endTime <= approvalDayBounds.dayStart ||
+ booking.endTime >= approvalDayBounds.dayEnd
+ ) {
+ const invalidDayError = new Error(
+ "Booking must start and end on the same date.",
+ );
+ invalidDayError.statusCode = 400;
+ throw invalidDayError;
+ }
const roomForBooking = await Room.findById(booking.room)
.session(session)
.select("name room_id location")
.lean();
@@
- const dayBounds = getDayBounds(booking.date);
+ const dayBounds = approvalDayBounds;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/roomBookingController.js` around lines 559 - 589, The
approval path only checks startTime < endTime but doesn't prevent approving
bookings that span midnight; before approving (in the block using getDayBounds
and RoomBooking.findOne) reapply the same-day invariant: use
getDayBounds(booking.date) to compute the day's bounds and ensure the booking's
start and end datetimes fall inside that single day (e.g., combine booking.date
with booking.startTime/booking.endTime and assert start < end and end <=
dayBounds.dayEnd); if the check fails throw a 400 error (same pattern as the
existing invalidTimeError). This prevents approving cross-day pending rows that
would evade the overlap query and keeps RoomBooking.findOne safe.
| const roomForBooking = await Room.findById(booking.room) | ||
| .session(session) | ||
| .select("name room_id location") | ||
| .lean(); | ||
|
|
||
| if (!roomForBooking) { | ||
| const missingRoomError = new Error("Room not found or inactive."); | ||
| missingRoomError.statusCode = 404; | ||
| throw missingRoomError; | ||
| } |
There was a problem hiding this comment.
Inactive rooms can still be approved.
This lookup never reads is_active, so a room disabled after the booking was created is still treated as approvable. The current error message already implies inactive rooms should be rejected.
Proposed fix
const roomForBooking = await Room.findById(booking.room)
.session(session)
- .select("name room_id location")
+ .select("name room_id location is_active")
.lean();
- if (!roomForBooking) {
+ if (!roomForBooking || !roomForBooking.is_active) {
const missingRoomError = new Error("Room not found or inactive.");
missingRoomError.statusCode = 404;
throw missingRoomError;
}📝 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.
| const roomForBooking = await Room.findById(booking.room) | |
| .session(session) | |
| .select("name room_id location") | |
| .lean(); | |
| if (!roomForBooking) { | |
| const missingRoomError = new Error("Room not found or inactive."); | |
| missingRoomError.statusCode = 404; | |
| throw missingRoomError; | |
| } | |
| const roomForBooking = await Room.findById(booking.room) | |
| .session(session) | |
| .select("name room_id location is_active") | |
| .lean(); | |
| if (!roomForBooking || !roomForBooking.is_active) { | |
| const missingRoomError = new Error("Room not found or inactive."); | |
| missingRoomError.statusCode = 404; | |
| throw missingRoomError; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/roomBookingController.js` around lines 568 - 577, The
Room lookup currently ignores the room's is_active flag so disabled rooms can
still be approved; update the query that populates roomForBooking to only return
active rooms (e.g., replace Room.findById(booking.room) with a query that
includes { _id: booking.room, is_active: true } or add .where({ is_active: true
})), keep the same .session(session).select("name room_id location").lean()
chain, and leave the existing not-found Error throw intact so inactive rooms
trigger the 404 path.
Pull request overview
This PR updates the room booking controller to make booking creation and booking review (approve/reject) more robust by reintroducing transaction/session usage and improving how overlapping events are queried.
Changes:
findOverlappingEvent()apply a Mongoose session only when one is provided.bookRoomto perform clash checks + booking creation inside a transaction and return richer 409 conflict responses.updateBookingStatusto perform approval validation and status updates inside a transaction (including overlap checks).