Backend work for Smart Room Booking & Clash Detection#233
Open
gmarav05 wants to merge 8 commits intoOpenLake:mainfrom
Open
Backend work for Smart Room Booking & Clash Detection#233gmarav05 wants to merge 8 commits intoOpenLake:mainfrom
gmarav05 wants to merge 8 commits intoOpenLake:mainfrom
Conversation
|
@gmarav05 is attempting to deploy a commit to the openlake's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds Room and RoomBooking Mongoose models, a new roomBookingController with full room and booking REST handlers, an Express router mounted at Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Routes as /api/rooms Router
participant Controller as roomBookingController
participant Models as Room & RoomBooking Models
participant DB as Database
Client->>Routes: POST /api/rooms/book (payload)
Routes->>Controller: bookRoom(req)
Controller->>Models: resolve room id (Room.findOne / findById)
Models->>DB: query Room
DB-->>Models: room doc
Models-->>Controller: room
Controller->>Models: query existing bookings (day bounds, statuses)
Models->>DB: query RoomBooking (overlap check)
DB-->>Models: matching bookings[]
Models-->>Controller: bookings (maybe conflict)
alt Conflict detected
Controller-->>Routes: 409 Conflict (conflicting booking)
Routes-->>Client: 409
else No conflict
Controller->>DB: begin transaction
Controller->>Models: update Room.updated_at
Controller->>Models: create RoomBooking
Models->>DB: insert booking
DB-->>Models: created booking
Controller->>DB: commit
Controller-->>Routes: 201 Created (booking)
Routes-->>Client: 201
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 65-66: The populated bookedBy user documents are exposing full
user data; update both populate calls (the one using
RoomBooking.find(...).populate('room event bookedBy') and the similar populate
at lines ~121-122) to restrict fields by using the object form: populate({ path:
'bookedBy', select: '_id name email' }) (or whichever minimal safe fields your
User model exposes), e.g., replace the string 'bookedBy' with the object
selector so only safe user fields are returned while keeping room and event
populations unchanged.
- Around line 19-23: getAllRooms currently returns only static Room docs from
Room.find and must be extended to include live status fields (status,
current_event, occupied_until). After fetching rooms in getAllRooms, retrieve
live occupancy data (from your RoomStatus/Occupancy store or service) for each
room (e.g., via RoomStatus.findOne({ roomId }) or
RoomStatusService.getStatus(room.id)), merge those fields into each room object,
and then send the augmented array with res.json; perform the per-room lookups
concurrently with Promise.all to avoid serial waits and handle missing status
records by supplying sensible defaults.
- Around line 31-39: The clash logic in the RoomBooking.findOne query ignores
the booking date and doesn’t validate time ranges; update the controller to
first validate that startTime < endTime (reject requests where startTime >=
endTime) and then include the booking date in the query (e.g., filter by date or
by combining date+time) so only bookings on the same date are checked; keep the
existing room: roomId and status: { $in: ['Pending','Approved'] } filters and
use the same overlap condition ({ startTime: { $lt: endTime }, endTime: { $gt:
startTime } }) but applied together with date to prevent cross-day matches.
- Around line 77-84: Before updating to 'Approved', load the target booking (use
RoomBooking.findById(id)) to get its room and time window, then if status ===
'Approved' run a query for any other booking with status 'Approved' for the same
room that overlaps the target booking's start/end (e.g. findOne({ _id: { $ne: id
}, room: booking.room, status: 'Approved', $or: [{ start: { $lt: booking.end,
$gte: booking.start } }, { end: { $gt: booking.start, $lte: booking.end } }, {
start: { $lte: booking.start }, end: { $gte: booking.end } }] }) ) and if one
exists return a 409/conflict; only then perform the update
(RoomBooking.findByIdAndUpdate or findOneAndUpdate). Ensure you reference
RoomBooking.findById and RoomBooking.findByIdAndUpdate in the change.
In `@backend/models/schema.js`:
- Around line 648-666: The roomSchema is missing the API-required fields room_id
and allowed_roles; update the mongoose roomSchema to add a room_id field
(String, required, unique — generated if absent via a default function or a
pre('save') hook using UUID logic) and an allowed_roles field (Array of String,
required or with a sensible default like []), and ensure any creation/update
code that constructs Room documents (e.g., create handlers or Model.create
usages) no longer omit room_id so responses match the API contract; keep the
symbol name roomSchema and adjust indexes/validation as needed.
In `@backend/routes/roomBooking.js`:
- Around line 11-13: The route paths are incorrect: change router.get('/rooms',
isAuthenticated, roomBookingController.getAllRooms) to router.get('/',
isAuthenticated, roomBookingController.getAllRooms) so the mounted /api/rooms
yields GET /api/rooms; add a new parameterized route router.get('/:room_id',
isAuthenticated, roomBookingController.getRoomById) (or the appropriate
controller method that returns room details/status) to expose GET
/api/rooms/:room_id; also update the other similar occurrence (the one around
line 35) to use '/' or '/:room_id' instead of duplicating "rooms". Ensure the
referenced controller method name matches an exported function in
roomBookingController.
In `@backend/seed.js`:
- Around line 26-31: The seedRooms function is defined but never invoked, so
sampleRooms are not inserted; call seedRooms from the main seeding flow (for
example inside the top-level async seed/run function or after other seed calls)
or add it to the Promise.all/await chain that runs other seed helpers so it
executes during seeding; locate the seedRooms function and ensure the script
invokes seedRooms() (or exports and calls it where other seeds like
seedUsers/seedBookings are executed) so rooms get inserted on a fresh seed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/controllers/roomBookingController.jsbackend/index.jsbackend/models/schema.jsbackend/routes/roomBooking.jsbackend/seed.js
Member
|
@gmarav05 fix the clash detection algorithm and other bugs in the PR |
Author
|
Yeah sure i am currently working on that only. i will send it by eod. |
Author
|
@harshitap1305 Can you check if any changes are required also can you please check again with the Coderabbit? Just In case i am missing something. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
backend/seed.js (1)
1147-1147: Remove the secondseedRooms()pass.Line 1147 deletes and recreates every
Roomafter the rest of the seed flow has finished. That regenerates room_ids and will orphan any room-linked seed data as soon as this script starts creatingRoomBookingrows or other room references.♻️ Proposed fix
await seedEvents(); await seedAchievements(); await seedFeedbacks(); - await seedRooms(); console.log("\n✅ Seeding completed successfully!");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/seed.js` at line 1147, The script calls seedRooms() twice causing rooms to be recreated and orphaning room-linked data; remove the second invocation (the await seedRooms(); near the end of the file) so rooms are only seeded once, and verify no other duplicate calls to seedRooms remain (ensure RoomBooking or other room-referencing seeding runs after the single seedRooms() call).
🤖 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 190-198: The handler currently returns the flat room document
enriched with occupancy fields (via Room.findById, SAFE_ROOM_SELECT,
buildRoomStatusMap, enrichRoom) but must return the room-detail shape required
by GET /room/:room_id: wrap the response into the detail payload by constructing
schedule: { start, end } from the occupancy/statusMap data and metadata: {
capacity, features } from the room document (instead of the flat keys), then
return res.json(detailPayload); update the code around Room.findById/enrichRoom
to build and return this detailPayload object (preserving existing values but
reshaping to the detail contract).
- Around line 62-89: buildRoomStatusMap currently only queries RoomBooking and
ignores active Event-only occupancies; update it to also query the Event model
for events whose startTime <= now < endTime for the given roomIds (use the same
SAFE_EVENT_SELECT projection) and merge those results into the returned
statusMap so rooms with active events (even without a RoomBooking) are marked
status: "occupied" with current_event and occupied_until populated; ensure you
reference the existing variables/function name buildRoomStatusMap,
SAFE_EVENT_SELECT, and RoomBooking when implementing the additional Event query
and merge logic.
- Around line 91-97: The code returns the undocumented status string "available"
for empty rooms; update enrichRoom (function enrichRoom) to return "vacant"
instead of "available" in its fallback status and similarly change the
create-room response logic (the create-room response branch that currently emits
"available") to emit "vacant" so the API matches the documented enum ("vacant" |
"occupied"); ensure the three fallback fields (status, current_event,
occupied_until) keep the same values but with status set to "vacant".
- Around line 276-303: The current read-then-write flow (RoomBooking.findOne ...
then new RoomBooking ... booking.save) is TOCTOU-prone; wrap the check+insert in
a MongoDB transaction (start a session, run RoomBooking.findOne with { session
}, abort if clash, then insert/save the new RoomBooking with the same session
and commit) so the check and write are atomic and serialized; apply the same
transactional pattern to the other create/approve flow that also uses
RoomBooking.findOne and booking.save (the second block referenced in the
comment) so concurrent requests cannot both pass the check and double-book a
room.
- Around line 371-375: After retrieving the booking with
RoomBooking.findById(id) in the roomBookingController handler, restrict further
review/approval to only bookings with status "Pending": check booking.status (or
the enum used) and if it's not "Pending" return an error response (e.g., 400/409
with a clear message like "Only pending bookings can be reviewed") so
Cancelled/Rejected/Completed bookings cannot be resurrected; update any related
logic that assumes reviewable bookings and add/adjust tests to cover non-pending
status rejection.
In `@backend/routes/roomBooking.js`:
- Around line 22-27: The POST /bookings route is incorrectly blocked by the
authorizeRole(ROLE_GROUPS.ADMIN) middleware so non-admins cannot submit booking
requests; remove the authorizeRole(ROLE_GROUPS.ADMIN) middleware from the
router.post that registers roomBookingController.bookRoom (leave isAuthenticated
in place) so the controller's own room.allowed_roles and bookedBy logic can
enforce permissions and create pending requests; apply the same removal for the
other booking-creation route that uses authorizeRole(ROLE_GROUPS.ADMIN) (the
second occurrence mentioned) while keeping admin-only middleware on routes that
truly require admin privileges.
---
Nitpick comments:
In `@backend/seed.js`:
- Line 1147: The script calls seedRooms() twice causing rooms to be recreated
and orphaning room-linked data; remove the second invocation (the await
seedRooms(); near the end of the file) so rooms are only seeded once, and verify
no other duplicate calls to seedRooms remain (ensure RoomBooking or other
room-referencing seeding runs after the single seedRooms() call).
🪄 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: eecdffb9-7715-4cdc-a2c5-4911886ea9c0
📒 Files selected for processing (4)
backend/controllers/roomBookingController.jsbackend/models/schema.jsbackend/routes/roomBooking.jsbackend/seed.js
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/models/schema.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/models/passportConfig.js (1)
68-75: Add explicit null check in deserializeUser for better clarity and robustness.While the
isAuthenticatedmiddleware prevents requests with null users from reaching route handlers, explicitly handling the case where a user is deleted from the database but their session persists makes the code more defensive and intent-clear. This prevents any potential edge cases in session management.Suggested improvement
passport.deserializeUser(async (id, done) => { try { let user = await User.findById(id); + if (!user) { + return done(null, false); + } done(null, user); } catch (err) { done(err, null); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/models/passportConfig.js` around lines 68 - 75, In passport.deserializeUser's async callback (the function passed to passport.deserializeUser) explicitly handle the case where User.findById(id) returns null: after awaiting User.findById(id) check if user is null and call done(null, false) (and optionally log a warning) instead of passing null user into done; leave the try/catch for DB errors unchanged so errors still call done(err, null).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/models/passportConfig.js`:
- Around line 68-75: In passport.deserializeUser's async callback (the function
passed to passport.deserializeUser) explicitly handle the case where
User.findById(id) returns null: after awaiting User.findById(id) check if user
is null and call done(null, false) (and optionally log a warning) instead of
passing null user into done; leave the try/catch for DB errors unchanged so
errors still call done(err, null).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e259762-d0e6-48a7-92e7-00306cedd3cb
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.husky/pre-commitbackend/index.jsbackend/models/passportConfig.js
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/index.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/routes/roomBooking.js (1)
51-74: Extract the review-role list from a shared constant.These two arrays duplicate each other and already diverge from the controller's
ADMIN_ROLESby omittingROLES.CLUB_COORDINATOR. Reusing one exported group/constant here would keep room-review permissions from drifting across create/cancel/review paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/roomBooking.js` around lines 51 - 74, The duplicated role array passed into authorizeRole for router.patch and router.put should be replaced with a single exported constant (e.g., REVIEW_ROLES or reuse the controller's ADMIN_ROLES) to prevent drift; define and export that shared constant from a central module (or the controller that already defines ADMIN_ROLES), import it where the routes are declared, and pass it into authorizeRole in both router.patch and router.put instead of the inline array (affecting router.patch, router.put, authorizeRole, and roomBookingController.updateBookingStatus references).
🤖 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 357-363: Clash detection queries currently only check RoomBooking
(e.g., the RoomBooking.findOne that sets clash) but buildRoomStatusMap treats
Event as occupancy too, so modify the overlap checks (both the initial
create/check block around the clash variable and the approval flow after loading
the booking's room/location) to also query the Event collection for overlapping
events in the same room/time window and treat them as conflicts; when checking
during approval, exclude the Event linked to the current RoomBooking (if any) to
avoid self-conflict. Ensure you mirror the same time/room filtering and the
exclusion of a linked event in both places so bookings cannot be
created/approved over event-only usages.
- Around line 94-105: The venue->room mapping only indexes room.name and
room.room_id but must also index room.location (use the room.location field) so
lookups using Event.schedule.venue will resolve correctly; also when computing
live occupancy exclude cancelled events by adding a filter on the Event query
(or schedule entry) to skip events with a cancelled status/flag (e.g.,
Event.status === 'cancelled' or schedule.cancelled) before counting overlaps.
Update the block that builds venueToRoomId (referencing Room.find and
venueToRoomId) to set venueToRoomId.set(room.location, String(room._id)) and
update the occupancy query code that counts overlapping schedules to filter out
cancelled events (the same change should be applied to the similar mapping/usage
later in the file).
---
Nitpick comments:
In `@backend/routes/roomBooking.js`:
- Around line 51-74: The duplicated role array passed into authorizeRole for
router.patch and router.put should be replaced with a single exported constant
(e.g., REVIEW_ROLES or reuse the controller's ADMIN_ROLES) to prevent drift;
define and export that shared constant from a central module (or the controller
that already defines ADMIN_ROLES), import it where the routes are declared, and
pass it into authorizeRole in both router.patch and router.put instead of the
inline array (affecting router.patch, router.put, authorizeRole, and
roomBookingController.updateBookingStatus references).
🪄 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: b5ec991a-b6f9-46f5-9075-39c182ca36dc
📒 Files selected for processing (4)
backend/controllers/roomBookingController.jsbackend/models/passportConfig.jsbackend/routes/roomBooking.jsbackend/seed.js
✅ Files skipped from review due to trivial changes (1)
- backend/seed.js
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/models/passportConfig.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/controllers/roomBookingController.js (2)
651-686: Consider preventing re-cancellation of already cancelled or rejected bookings.The handler allows setting status to
Cancelledregardless of current status. This could allow re-cancelling an alreadyCancelledorRejectedbooking. Consider adding a check similar toupdateBookingStatus.♻️ Optional fix
if (!booking) { return res.status(404).json({ message: "Booking not found" }); } + + if (["Cancelled", "Rejected"].includes(booking.status)) { + return res.status(409).json({ message: "Booking is already cancelled or rejected." }); + } if (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/roomBookingController.js` around lines 651 - 686, The cancelBooking handler currently sets booking.status = "Cancelled" unconditionally; add a guard like in updateBookingStatus to reject attempts to cancel when booking.status is already "Cancelled" or "Rejected" (check booking.status before mutating), returning a 400/appropriate response (e.g., res.status(400).json({ message: "Booking already cancelled or rejected" })); keep existing authorization logic (ADMIN_ROLES, req.user._id) and only update updated_at and save when the status transition is allowed, then continue to populate and return responseBooking as before.
5-12: Consider usingROLE_GROUPS.ADMINfromroles.jsinstead of duplicating.This
ADMIN_ROLESarray duplicatesROLE_GROUPS.ADMINdefined inbackend/utils/roles.js. Consider importing and reusing it to avoid maintenance drift.♻️ Proposed refactor
-const { ROLES } = require("../utils/roles"); +const { ROLES, ROLE_GROUPS } = require("../utils/roles"); -const ADMIN_ROLES = [ - ROLES.PRESIDENT, - ROLES.GENSEC_SCITECH, - ROLES.GENSEC_ACADEMIC, - ROLES.GENSEC_CULTURAL, - ROLES.GENSEC_SPORTS, - ROLES.CLUB_COORDINATOR, -]; +const ADMIN_ROLES = ROLE_GROUPS.ADMIN;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/roomBookingController.js` around lines 5 - 12, The ADMIN_ROLES array in roomBookingController.js duplicates ROLE_GROUPS.ADMIN from roles.js; import ROLE_GROUPS (or ROLE_GROUPS.ADMIN) from roles.js and replace the local hard-coded ADMIN_ROLES definition by referencing ROLE_GROUPS.ADMIN (or remove ADMIN_ROLES and use ROLE_GROUPS.ADMIN in its call sites), ensuring any existing uses (e.g., permission checks in roomBookingController) still refer to the imported group to avoid duplication and maintenance drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/controllers/roomBookingController.js`:
- Around line 651-686: The cancelBooking handler currently sets booking.status =
"Cancelled" unconditionally; add a guard like in updateBookingStatus to reject
attempts to cancel when booking.status is already "Cancelled" or "Rejected"
(check booking.status before mutating), returning a 400/appropriate response
(e.g., res.status(400).json({ message: "Booking already cancelled or rejected"
})); keep existing authorization logic (ADMIN_ROLES, req.user._id) and only
update updated_at and save when the status transition is allowed, then continue
to populate and return responseBooking as before.
- Around line 5-12: The ADMIN_ROLES array in roomBookingController.js duplicates
ROLE_GROUPS.ADMIN from roles.js; import ROLE_GROUPS (or ROLE_GROUPS.ADMIN) from
roles.js and replace the local hard-coded ADMIN_ROLES definition by referencing
ROLE_GROUPS.ADMIN (or remove ADMIN_ROLES and use ROLE_GROUPS.ADMIN in its call
sites), ensuring any existing uses (e.g., permission checks in
roomBookingController) still refer to the imported group to avoid duplication
and maintenance drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bda0aa32-b4dd-446c-a118-68e0184effe8
📒 Files selected for processing (5)
backend/controllers/roomBookingController.jsbackend/models/passportConfig.jsbackend/routes/roomBooking.jsbackend/seed.jsbackend/utils/roles.js
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/models/passportConfig.js
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.
name: " Pull Request"
about: Propose and submit changes to the project for review
title: "PR: [Brief Description of Changes]"
labels: ""
assignees: harshitap1305, sakshi1755
Related Issue
Changes Introduced
Why This Change?
Screenshots / Video (if applicable)
Testing
npm testin the relevant directory).Documentation Updates
README.mdwith new instructions.Checklist
git checkout -b feature/my-amazing-feature).Deployment Notes
💬 Additional Notes
Summary by CodeRabbit
New Features
Chores