fix: reject future dates in attendance regularization (#1340)#1408
fix: reject future dates in attendance regularization (#1340)#1408Xenon010101 wants to merge 3 commits into
Conversation
…ya360#1340) - Add Zod refinements to regularizeSchema: date, checkIn, checkOut must not be in the future - Add runtime guards in regularize() service method - Prevents regularization requests for dates/times that haven't occurred yet
github-actions
Bot
added
quality:clean
|
Hi @Xenon010101, thanks for contributing to InternHack! 🎉 I have automatically:
Our workflows will now analyze your changes to classify:
Tip Ensure your PR description references the issue it resolves (e.g. Happy coding! 🚀 |
📝 WalkthroughWalkthroughThis PR prevents regularizing future attendance data: Zod schema refinements disallow future ChangesAttendance Regularization Future Date Check
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Possibly related issues
Suggested labels
Suggested reviewers
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. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/src/module/attendance/attendance.service.ts (1)
134-137: Note: Service validation errors are caught as HTTP 500 by the controller.The runtime validation guards correctly throw errors when future dates are detected. However, per the context snippet in
attendance.controller.ts:85-95, all service errors are caught and returned as HTTP 500 Internal Server Error. Validation failures like "Cannot regularize future dates" would be more semantically correct as HTTP 400 Bad Request.This is a pre-existing controller behavior, not introduced by this PR. The service layer is correctly implementing business validation. Consider updating the controller in a future PR to distinguish validation errors from actual server errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/module/attendance/attendance.service.ts` around lines 134 - 137, The controller currently turns all thrown service errors into HTTP 500; update attendance.controller.ts to return HTTP 400 for validation failures raised by the service (e.g., the "Cannot regularize future dates" / "Check-in time cannot be in the future" / "Check-out time cannot be in the future" errors thrown from attendance.service.ts). Implement one of two approaches: (A) introduce a specific ValidationError (or reuse Nest's BadRequestException) in attendance.service.ts and throw that instead of generic Error, then in the controller catch ValidationError/BadRequestException and respond with status 400, or (B) keep service throws but detect validation messages/types in attendance.controller.ts and map them to a 400 response while other errors remain 500; ensure you reference the controller method that calls the service (the handler around lines 85–95) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/src/module/attendance/attendance.service.ts`:
- Around line 134-137: The controller currently turns all thrown service errors
into HTTP 500; update attendance.controller.ts to return HTTP 400 for validation
failures raised by the service (e.g., the "Cannot regularize future dates" /
"Check-in time cannot be in the future" / "Check-out time cannot be in the
future" errors thrown from attendance.service.ts). Implement one of two
approaches: (A) introduce a specific ValidationError (or reuse Nest's
BadRequestException) in attendance.service.ts and throw that instead of generic
Error, then in the controller catch ValidationError/BadRequestException and
respond with status 400, or (B) keep service throws but detect validation
messages/types in attendance.controller.ts and map them to a 400 response while
other errors remain 500; ensure you reference the controller method that calls
the service (the handler around lines 85–95) when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dedd1bfd-c081-418a-bfa5-c916930c8b2a
📒 Files selected for processing (2)
server/src/module/attendance/attendance.service.tsserver/src/module/attendance/attendance.validation.ts
- Map service validation errors (future dates, checkIn/checkOut) to HTTP 400 Bad Request instead of 500 Internal Server Error
Code Review — PR #1408: Reject future dates in attendance regularizationHi @Xenon010101, adding future-date validation is the right fix. Found two issues with the implementation pattern. 🔴 Fragile error-message string matching in the controller// attendance.controller.ts
if (msg.includes("must be after") || msg.includes("must not exceed") || msg.includes("Cannot regularize") || msg.includes("cannot be in the future"))
return res.status(400).json({ message: msg });This pattern is dangerous. The controller forwards a 400 only if the error message contains one of these hardcoded substrings. Problems:
Fix: Use a typed error class instead: // attendance.errors.ts
export class AttendanceValidationError extends Error {
constructor(message: string) {
super(message);
this.name = "AttendanceValidationError";
}
}
// attendance.service.ts
if (checkIn > now) throw new AttendanceValidationError("Check-in time cannot be in the future");
// attendance.controller.ts
} catch (error) {
if (error instanceof AttendanceValidationError) {
return res.status(400).json({ message: error.message });
}
console.error(error);
return res.status(500).json({ message: "Internal Server Error" });
}🟡 Future-date validation is duplicated between Zod schema and service layerThe Zod The service-level checks are not wrong to have as a defense-in-depth guard, but they should match the Zod messages exactly (they don't — "Cannot regularize future dates" vs "Cannot regularize future dates" is fine, but "Check-in time cannot be in the future" vs the Zod message needs to be verified). Pick one canonical location and either remove the duplicates or keep them and accept they'll never fire in normal flow. |
Sachinchaurasiya360
left a comment
Sachinchaurasiya360
left a comment
There was a problem hiding this comment.
The Zod validation layer is a good addition and the intent is correct. Three issues need to be fixed before merge.
Bug 1 (Critical) - New service-layer guard is dead code with a divergent error message:
The PR adds an if (date > now) check in attendance.service.ts around line 135, but the pre-existing midnight-comparison guard at line 132 (if (date > today)) always fires first for any future date. The new guard is unreachable. Worse, its error message "Cannot regularize future dates" differs from the existing one "Cannot regularize attendance for future dates", so if the guard order ever changes, clients get inconsistent messages. Remove the new redundant guard from the service - Zod already handles this at the boundary, and the service already had its own guard.
Bug 2 (Critical) - False rejections for IST users:
The Zod refine in attendance.validation.ts compares checkIn and checkOut against new Date() (server UTC wall clock). For users in IST (UTC+5:30), a legitimate checkOut of 11:59 PM IST = 18:29 UTC. If they submit at 11:58 PM IST = 18:28 UTC, the server rejects with "Check-out time cannot be in the future" even though the time is in the past from the user perspective. The validation should compare within the context of the submitted date, not the raw UTC wall clock.
Bug 3 (Medium) - Phantom substring in controller catch block:
attendance.controller.ts has msg.includes("must not exceed") in the new catch logic, but this exact substring does not appear in any service throw in the codebase. It is a phantom check that never matches. This means if that error path is ever hit it silently becomes a 500. Use exact string equality checks (===) like the pattern already used for the checkOut handler in the same file.
Note: This PR and PR #1407 both modify the same regularize catch block in attendance.controller.ts. They will conflict when merged - please coordinate with the #1407 author or rebase onto whichever merges first.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/module/attendance/attendance.controller.ts`:
- Around line 93-97: Define a typed error class AttendanceValidationError in the
attendance service (e.g., export class AttendanceValidationError extends Error)
and update all service validation code (the places currently throwing new Error
for future dates, invalid times, regularize checks, etc.—e.g., the validation
branches used by checkIn/checkOut and other attendance service methods) to throw
AttendanceValidationError instead of plain Error; then change the controller's
error handling in attendance.controller.ts to check error instanceof
AttendanceValidationError and return res.status(400).json({ message:
error.message }) instead of relying on fragile substring matching.
🪄 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: 7496215c-f1cd-4ca7-acff-63f95b2485d0
📒 Files selected for processing (2)
server/src/module/attendance/attendance.controller.tsserver/src/module/attendance/attendance.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/module/attendance/attendance.service.ts
| if (error instanceof Error) { | ||
| const msg = error.message; | ||
| if (msg.includes("must be after") || msg.includes("must not exceed") || msg.includes("Cannot regularize") || msg.includes("cannot be in the future")) | ||
| return res.status(400).json({ message: msg }); | ||
| } |
There was a problem hiding this comment.
Replace fragile string matching with typed error classes.
The reviewer (Sachinchaurasiya360) correctly identified that substring-based error routing is brittle and hard to maintain. If service error messages change, this controller logic may silently break, returning 500 instead of 400 for validation errors.
Additionally, this approach is inconsistent with other methods in this controller (checkIn and checkOut use exact string matching), making the error-handling strategy harder to understand and maintain.
Introduce a typed validation error class (e.g., AttendanceValidationError) in the service layer. Throw it for all validation failures (future dates, invalid times, etc.), then catch instanceof AttendanceValidationError here and return 400 with the error message. This eliminates string coupling and aligns with the reviewer's suggestion.
🏗️ Proposed refactor to use typed error class
First, define a validation error class in the service file:
// server/src/module/attendance/attendance.service.ts
export class AttendanceValidationError extends Error {
constructor(message: string) {
super(message);
this.name = 'AttendanceValidationError';
}
}Then, in the service, replace throw new Error(...) with throw new AttendanceValidationError(...) for all validation failures (future dates, checkIn/checkOut constraints, etc.).
Finally, update the controller to catch the typed error:
} catch (error) {
- if (error instanceof Error) {
- const msg = error.message;
- if (msg.includes("must be after") || msg.includes("must not exceed") || msg.includes("Cannot regularize") || msg.includes("cannot be in the future"))
- return res.status(400).json({ message: msg });
- }
+ if (error instanceof AttendanceValidationError) {
+ return res.status(400).json({ message: error.message });
+ }
console.error(error);
return res.status(500).json({ message: "Internal Server Error" });
}As per coding guidelines, controller layer must handle request/response, call service, and format errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/module/attendance/attendance.controller.ts` around lines 93 - 97,
Define a typed error class AttendanceValidationError in the attendance service
(e.g., export class AttendanceValidationError extends Error) and update all
service validation code (the places currently throwing new Error for future
dates, invalid times, regularize checks, etc.—e.g., the validation branches used
by checkIn/checkOut and other attendance service methods) to throw
AttendanceValidationError instead of plain Error; then change the controller's
error handling in attendance.controller.ts to check error instanceof
AttendanceValidationError and return res.status(400).json({ message:
error.message }) instead of relying on fragile substring matching.
2 participants
fix: reject future dates in attendance regularization (#1340)
Description
Rejects attendance regularization requests where date, checkIn, or checkOut are in the future.
Changes
.refine()toregularizeSchema: ensuresdate,checkIn, andcheckOutare not in the futureregularize()service methodTesting
Closes #1340
Summary by CodeRabbit