API created for Leave#11
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a leave management system with new Changes
Sequence DiagramsequenceDiagram
actor Client
participant LeaveController
participant Employee Model
participant LeaveApplication Model
participant Database
Client->>LeaveController: POST /leaves (createLeave)
LeaveController->>Employee Model: Resolve employee from session
Employee Model->>Database: Query employee by ID
Database-->>Employee Model: Return employee record
LeaveController->>LeaveApplication Model: Validate dates & create document
LeaveApplication Model->>Database: Insert new leave with PENDING status
Database-->>LeaveApplication Model: Return created leave
LeaveApplication Model-->>Client: Return leave record
Client->>LeaveController: GET /leaves (getLeaves)
alt Admin Role
LeaveController->>LeaveApplication Model: Query with optional status filter
else Employee Role
LeaveController->>LeaveApplication Model: Query only own leaves
end
LeaveApplication Model->>Database: Fetch leave records & populate employee
Database-->>LeaveApplication Model: Return populated records
LeaveApplication Model-->>Client: Return normalized leaves
Client->>LeaveController: PATCH /leaves/:id (updateLeaveStatus)
LeaveController->>LeaveApplication Model: Update status by ID
LeaveApplication Model->>Database: Modify leave document
Database-->>LeaveApplication Model: Return updated leave
LeaveApplication Model-->>Client: Return updated record
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/controllers/employeeController.js (1)
91-110:⚠️ Potential issue | 🔴 CriticalAdd missing
joinDateto destructuring inupdateEmployee.Line 91 is missing
joinDatefrom the destructuring statement, but line 110 references it, causing a runtimeReferenceErrorwhen the update endpoint is called.Proposed fix
- const { firstName, lastName, email, phone, position, department, basicSalary, allowances, deductions, password, role, bio, employmentStatus } = req.body; + const { firstName, lastName, email, phone, position, department, basicSalary, allowances, deductions, joinDate, password, role, bio, employmentStatus } = req.body;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/controllers/employeeController.js` around lines 91 - 110, The updateEmployee handler destructures req.body but omits joinDate while later using joinDate in Employee.findByIdAndUpdate; add joinDate to the destructuring list in updateEmployee (alongside firstName, lastName, email, etc.) so joinDate is defined, and ensure the existing call to new Date(joinDate) remains valid when passed to Employee.findByIdAndUpdate.
🧹 Nitpick comments (1)
package.json (1)
1-5: 💤 Low valueConsider adding package metadata and scripts.
Standard package.json files typically include:
name,version,descriptionfor package identificationauthorandlicensefor attribution and compliancescriptssection for common tasks (start, dev, test, etc.)enginesto specify Node.js version requirementsWhile not blocking, these fields improve maintainability and documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 1 - 5, package.json currently only lists dependencies; add standard package metadata fields (name, version, description), attribution fields (author, license), a scripts object with at least start, dev, and test entries, and an engines field specifying supported Node.js versions; update the existing package.json by inserting these keys at the top level so tools and contributors can identify and run the package (refer to the package.json top-level object and the "dependencies" key when locating where to add the new fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/controllers/leaveController.js`:
- Around line 113-114: The validation array for the status check in
leaveController.js contains a typo ("AP[ROVED") causing valid "APPROVED"
statuses to be rejected; update the string list used in the includes check (the
array literal used where status is tested) to ["APPROVED", "REJECTED",
"PENDING"] so that the variable status is correctly validated against the proper
values (adjust the includes(...) call in the status validation block).
- Around line 117-122: After calling
LeaveApplication.findByIdAndUpdate(req.params.id, { status }, { returnDocument:
'after' }) in the leaveController, add a null check for the returned leave and
if it's null send a 404 response (e.g., res.status(404).json({ success: false,
error: 'Leave application not found' })) instead of returning success: true with
data: null; keep the existing successful res.json({ success: true, data: leave
}) path for non-null results.
- Around line 26-30: The date validation contains a duplicated condition and
doesn't check that endDate is after startDate; update the second check in
leaveController.js to compare parsed Dates (new Date(endDate) < new
Date(startDate)) and return the "end date must be greater than start date"
response, leaving the first check that ensures neither startDate nor endDate is
in the past (new Date(startDate) < today || new Date(endDate) < today) intact;
use the startDate and endDate symbols to locate and replace the duplicate
condition with the proper comparison.
In `@server/models/LeaveApplication.js`:
- Around line 16-23: The model allows endDate earlier than startDate; add a
validation on the LeaveApplication schema to reject such records: either add a
custom validator for the endDate field that checks (this.startDate && value >=
this.startDate) and returns a validation error message, or add a
schema.pre('validate', ...) hook on the LeaveApplication schema that
throws/invalidates when endDate < startDate; ensure the validation runs on
updates (use runValidators for update operations) so invalid ranges cannot be
persisted.
- Around line 6-8: The schema field employeeId in LeaveApplication.js uses the
misspelled mongoose.Schhema.Types.ObjectId which breaks model initialization;
change the reference to mongoose.Schema.Types.ObjectId (update the employeeId
definition) so the schema uses the correct ObjectId type.
---
Outside diff comments:
In `@server/controllers/employeeController.js`:
- Around line 91-110: The updateEmployee handler destructures req.body but omits
joinDate while later using joinDate in Employee.findByIdAndUpdate; add joinDate
to the destructuring list in updateEmployee (alongside firstName, lastName,
email, etc.) so joinDate is defined, and ensure the existing call to new
Date(joinDate) remains valid when passed to Employee.findByIdAndUpdate.
---
Nitpick comments:
In `@package.json`:
- Around line 1-5: package.json currently only lists dependencies; add standard
package metadata fields (name, version, description), attribution fields
(author, license), a scripts object with at least start, dev, and test entries,
and an engines field specifying supported Node.js versions; update the existing
package.json by inserting these keys at the top level so tools and contributors
can identify and run the package (refer to the package.json top-level object and
the "dependencies" key when locating where to add the new fields).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 086cd9dd-f744-46de-bf52-a9ffb7b9db2c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonserver/controllers/attendanceController.jsserver/controllers/authController.jsserver/controllers/employeeController.jsserver/controllers/leaveController.jsserver/middleware/auth.jsserver/models/Employee.jsserver/models/LeaveApplication.jsserver/models/User.jsserver/routes/AttendanceRoutes.js
Comment on lines
+26
to
+30
| if (new Date(startDate) < today || new Date(endDate) < today) | ||
| return res.status(400).json({ message: "Please provide valid dates" }); | ||
|
|
||
| if (new Date(endDate) < today || new Date(startDate) < today) | ||
| return res.status(400).json({ message: "end date must be greater than start date" }); |
There was a problem hiding this comment.
Critical bug: Date validation logic is incorrect and duplicated.
Lines 29-30 have the same condition as lines 26-27, making the second check unreachable and failing to validate that endDate >= startDate. Users can submit a leave where endDate is before startDate.
🐛 Proposed fix
if (new Date(startDate) < today || new Date(endDate) < today)
return res.status(400).json({ message: "Please provide valid dates" });
- if (new Date(endDate) < today || new Date(startDate) < today)
+ if (new Date(endDate) < new Date(startDate))
return res.status(400).json({ message: "end date must be greater than start date" });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/controllers/leaveController.js` around lines 26 - 30, The date
validation contains a duplicated condition and doesn't check that endDate is
after startDate; update the second check in leaveController.js to compare parsed
Dates (new Date(endDate) < new Date(startDate)) and return the "end date must be
greater than start date" response, leaving the first check that ensures neither
startDate nor endDate is in the past (new Date(startDate) < today || new
Date(endDate) < today) intact; use the startDate and endDate symbols to locate
and replace the duplicate condition with the proper comparison.
Comment on lines
+113
to
+114
| if (!["AP[ROVED", "REJECTED", "PENDING"].includes(status)) | ||
| return res.status(400).json({ error: "invalid status" }); |
There was a problem hiding this comment.
Critical typo: "AP[ROVED" should be "APPROVED".
This typo causes all legitimate "APPROVED" status updates to be rejected with "invalid status" error.
🐛 Proposed fix
- if (!["AP[ROVED", "REJECTED", "PENDING"].includes(status))
+ if (!["APPROVED", "REJECTED", "PENDING"].includes(status))
return res.status(400).json({ error: "invalid status" });📝 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.
Suggested change
| if (!["AP[ROVED", "REJECTED", "PENDING"].includes(status)) | |
| return res.status(400).json({ error: "invalid status" }); | |
| if (!["APPROVED", "REJECTED", "PENDING"].includes(status)) | |
| return res.status(400).json({ error: "invalid status" }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/controllers/leaveController.js` around lines 113 - 114, The validation
array for the status check in leaveController.js contains a typo ("AP[ROVED")
causing valid "APPROVED" statuses to be rejected; update the string list used in
the includes check (the array literal used where status is tested) to
["APPROVED", "REJECTED", "PENDING"] so that the variable status is correctly
validated against the proper values (adjust the includes(...) call in the status
validation block).
Comment on lines
+117
to
+122
| const leave = await LeaveApplication.findByIdAndUpdate(req.params.id, { status }, { returnDocument: 'after' }); | ||
|
|
||
| return res.json({ | ||
| success: true, | ||
| data: leave | ||
| }) |
There was a problem hiding this comment.
Missing null check when leave is not found.
If req.params.id doesn't match any document, findByIdAndUpdate returns null and the response will indicate success with data: null. This should return a 404 error instead.
🐛 Proposed fix
const leave = await LeaveApplication.findByIdAndUpdate(req.params.id, { status }, { returnDocument: 'after' });
+ if (!leave)
+ return res.status(404).json({ error: "Leave application not found" });
+
return res.json({
success: true,
data: leave
})📝 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.
Suggested change
| const leave = await LeaveApplication.findByIdAndUpdate(req.params.id, { status }, { returnDocument: 'after' }); | |
| return res.json({ | |
| success: true, | |
| data: leave | |
| }) | |
| const leave = await LeaveApplication.findByIdAndUpdate(req.params.id, { status }, { returnDocument: 'after' }); | |
| if (!leave) | |
| return res.status(404).json({ error: "Leave application not found" }); | |
| return res.json({ | |
| success: true, | |
| data: leave | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/controllers/leaveController.js` around lines 117 - 122, After calling
LeaveApplication.findByIdAndUpdate(req.params.id, { status }, { returnDocument:
'after' }) in the leaveController, add a null check for the returned leave and
if it's null send a 404 response (e.g., res.status(404).json({ success: false,
error: 'Leave application not found' })) instead of returning success: true with
data: null; keep the existing successful res.json({ success: true, data: leave
}) path for non-null results.
Comment on lines
+6
to
+8
| employeeId: { | ||
| type: mongoose.Schhema.Types.ObjectId, | ||
| ref: "Employee", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the typo and expected Schema API usage
rg -n 'Schhema|Schema\.Types\.ObjectId' server/models/LeaveApplication.jsRepository: harshGupta090722/EMS
Length of output: 113
Fix typo in ObjectId type definition—Schhema is invalid and will break schema initialization at runtime.
Line 7 uses mongoose.Schhema.Types.ObjectId where Schhema is misspelled. The correct property is mongoose.Schema.Types.ObjectId. This typo will cause the model to fail when loaded.
Proposed fix
- type: mongoose.Schhema.Types.ObjectId,
+ type: mongoose.Schema.Types.ObjectId,📝 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.
Suggested change
| employeeId: { | |
| type: mongoose.Schhema.Types.ObjectId, | |
| ref: "Employee", | |
| employeeId: { | |
| type: mongoose.Schema.Types.ObjectId, | |
| ref: "Employee", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/models/LeaveApplication.js` around lines 6 - 8, The schema field
employeeId in LeaveApplication.js uses the misspelled
mongoose.Schhema.Types.ObjectId which breaks model initialization; change the
reference to mongoose.Schema.Types.ObjectId (update the employeeId definition)
so the schema uses the correct ObjectId type.
Comment on lines
+16
to
+23
| startDate: { | ||
| type: Date, | ||
| required: true | ||
| }, | ||
| endDate: { | ||
| type: Date, | ||
| required: true | ||
| }, |
There was a problem hiding this comment.
Add date-order validation for leave periods.
endDate can currently be earlier than startDate, which allows invalid leave applications to be persisted.
Proposed fix
endDate: {
type: Date,
- required: true
+ required: true,
+ validate: {
+ validator: function (value) {
+ return !this.startDate || value >= this.startDate;
+ },
+ message: "endDate must be greater than or equal to startDate"
+ }
},📝 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.
Suggested change
| startDate: { | |
| type: Date, | |
| required: true | |
| }, | |
| endDate: { | |
| type: Date, | |
| required: true | |
| }, | |
| startDate: { | |
| type: Date, | |
| required: true | |
| }, | |
| endDate: { | |
| type: Date, | |
| required: true, | |
| validate: { | |
| validator: function (value) { | |
| return !this.startDate || value >= this.startDate; | |
| }, | |
| message: "endDate must be greater than or equal to startDate" | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/models/LeaveApplication.js` around lines 16 - 23, The model allows
endDate earlier than startDate; add a validation on the LeaveApplication schema
to reject such records: either add a custom validator for the endDate field that
checks (this.startDate && value >= this.startDate) and returns a validation
error message, or add a schema.pre('validate', ...) hook on the LeaveApplication
schema that throws/invalidates when endDate < startDate; ensure the validation
runs on updates (use runValidators for update operations) so invalid ranges
cannot be persisted.
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
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores