fix: reject negative/zero work hours in attendance regularization (#1337)#1407
fix: reject negative/zero work hours in attendance regularization (#1337)#1407Xenon010101 wants to merge 2 commits into
Conversation
…chinchaurasiya360#1337) - Add Zod refinements to regularizeSchema: checkOut > checkIn, workHours <= 24 - Add runtime guards in regularize() service method - Clamp HALF_DAY/PRESENT logic to positive work hours only
github-actions
Bot
added
quality:clean
|
Warning Review limit reached
More reviews will be available in 5 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughWork hours validation constraints are enforced at both schema and service layers in the attendance regularization flow. The Zod schema now validates ChangesWork Hours Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
github-actions
Bot
added
gssoc:approved
|
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! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/src/module/attendance/attendance.validation.ts (1)
21-27: ⚡ Quick winConsolidate duplicate
workHourscalculations into one validation pass.The two
.refine()blocks duplicate date parsing and duration math. Use a single.superRefine()so the rule logic stays in sync and easier to maintain.Proposed refactor
-export const regularizeSchema = z.object({ +export const regularizeSchema = z.object({ employeeId: z.number().int().positive(), date: z.string().datetime(), checkIn: z.string().datetime(), checkOut: z.string().datetime(), notes: z.string().min(1, "Reason for regularization is required").max(500), -}).refine((data) => { - const workHours = (new Date(data.checkOut).getTime() - new Date(data.checkIn).getTime()) / 3600000; - return workHours > 0; -}, { message: "checkOut must be after checkIn" }).refine((data) => { - const workHours = (new Date(data.checkOut).getTime() - new Date(data.checkIn).getTime()) / 3600000; - return workHours <= MAX_WORK_HOURS; -}, { message: `Work hours must not exceed ${MAX_WORK_HOURS}` }); +}).superRefine((data, ctx) => { + const workHours = (new Date(data.checkOut).getTime() - new Date(data.checkIn).getTime()) / 3600000; + + if (workHours <= 0) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: "checkOut must be after checkIn", + path: ["checkOut"], + }); + } + + if (workHours > MAX_WORK_HOURS) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `Work hours must not exceed ${MAX_WORK_HOURS}`, + path: ["checkOut"], + }); + } +});As per coding guidelines: "Apply DRY principle: no duplicate helpers, shared animation variants per file".
🤖 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.validation.ts` around lines 21 - 27, Duplicate workHours calculation in the two .refine() calls should be consolidated into a single .superRefine() on the same Zod schema: compute workHours once from data.checkIn and data.checkOut, then add two checks inside the superRefine callback—(1) if workHours <= 0 addIssue({ path: ["checkOut"], message: "checkOut must be after checkIn" }) and (2) if workHours > MAX_WORK_HOURS addIssue({ path: ["checkOut"], message: `Work hours must not exceed ${MAX_WORK_HOURS}` }); update the schema where those .refine() calls exist to use .superRefine() so the parsing and error reporting for checkIn/checkOut are centralized and DRY.server/src/module/attendance/attendance.service.ts (1)
136-137: ⚡ Quick winAvoid cross-layer drift by sharing the max-hours constant.
24is enforced in both schema and service with separate literals/constants. Move it to a shared attendance constant and import in both places to keep the contract synchronized.🤖 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 136 - 137, The service currently hardcodes 24 in the work-hours validation (see the workHours checks in attendance.service.ts: "if (workHours > 24) ..."), which duplicates the schema's limit; extract a single shared constant (e.g. ATTENDANCE_MAX_HOURS or MAX_WORK_HOURS) into the attendance constants module, import that constant into both the schema and the attendance service, and replace the literal 24 in the service (and schema) with the imported constant so the max-hours constraint is defined in one place and reused.
🤖 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 136-137: The service currently hardcodes 24 in the work-hours
validation (see the workHours checks in attendance.service.ts: "if (workHours >
24) ..."), which duplicates the schema's limit; extract a single shared constant
(e.g. ATTENDANCE_MAX_HOURS or MAX_WORK_HOURS) into the attendance constants
module, import that constant into both the schema and the attendance service,
and replace the literal 24 in the service (and schema) with the imported
constant so the max-hours constraint is defined in one place and reused.
In `@server/src/module/attendance/attendance.validation.ts`:
- Around line 21-27: Duplicate workHours calculation in the two .refine() calls
should be consolidated into a single .superRefine() on the same Zod schema:
compute workHours once from data.checkIn and data.checkOut, then add two checks
inside the superRefine callback—(1) if workHours <= 0 addIssue({ path:
["checkOut"], message: "checkOut must be after checkIn" }) and (2) if workHours
> MAX_WORK_HOURS addIssue({ path: ["checkOut"], message: `Work hours must not
exceed ${MAX_WORK_HOURS}` }); update the schema where those .refine() calls
exist to use .superRefine() so the parsing and error reporting for
checkIn/checkOut are centralized and DRY.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75667374-c57d-41f8-a59f-60f16249f4ed
📒 Files selected for processing (2)
server/src/module/attendance/attendance.service.tsserver/src/module/attendance/attendance.validation.ts
- Replace duplicate .refine() with single .superRefine() - Extract MAX_WORK_HOURS to shared attendance.constants.ts - Import constant in both validation.ts and service.ts - Return HTTP 400 for service validation errors in controller
github-actions
Bot
added
level:intermediate
Code Review — PR #1407: Reject negative/zero work hours in attendance regularizationHi @Xenon010101, the 🔴 Fragile error-message string matching in the controller (same pattern as #1408)// attendance.controller.ts
if (msg.includes("must be after") || msg.includes("must not exceed") || ...)
return res.status(400).json({ message: msg });This PR adds Beyond the conflict, the substring matching is fragile — if either error message is ever changed, the controller silently returns 500 instead of 400. Use a typed error class instead (same fix as described in #1408). 🟡 Validation logic duplicated between Zod schema and service layerBoth
Since Zod runs first at the request boundary, the service-level checks are dead code in practice. This creates maintenance overhead — both places need to stay in sync when the rules change. Recommendation: Keep validation in Zod (single source of truth at the boundary), remove the duplicate guards from the service layer, and rely on the typed error class approach for any service-layer business logic errors. 🟡 Merge conflict with PR #1408Both PRs modify |
Sachinchaurasiya360
left a comment
Sachinchaurasiya360
left a comment
There was a problem hiding this comment.
The Zod superRefine is correct and well-placed. Two issues before merge.
Issue 1 (Medium) - Fragile substring matching in controller catch block:
attendance.controller.ts lines 92-95 use msg.includes("must be after"), msg.includes("must not exceed") etc. This couples service error strings to the controller with no type safety - any future typo fix in the error message silently turns a 400 into a 500. The same file already uses exact string equality (===) for the checkOut handler - use that pattern here too.
Issue 2 (Low) - Duplicate validation logic in service layer:
The workHours guard (if (workHours <= 0) and if (workHours > MAX_WORK_HOURS)) exists in both attendance.validation.ts (via superRefine, which runs first) and in attendance.service.ts. Since Zod always runs at the controller boundary before the service is called, the service-layer guards are dead code in normal execution. Remove them from the service. The existing date > today guard in the service is a legitimate business rule and should stay - this note only applies to the workHours arithmetic checks.
Note: This PR and PR #1408 both modify the regularize catch block in attendance.controller.ts. They will conflict - please rebase onto whichever merges first.
The Zod superRefine logic itself, the MAX_WORK_HOURS constant extraction, and the 24-hour upper bound are all correct. Fix these two issues and it is ready to merge.
2 participants
fix: reject negative/zero work hours in attendance regularization (#1337)
Description
Rejects negative and zero work hours in attendance regularization requests.
Changes
.refine()toregularizeSchema: ensures checkOut > checkIn and work hours ≤ 24regularize()service method: throws if workHours ≤ 0 or > 24Testing
Closes #1337
Summary by CodeRabbit
Release Notes