fix(leave-application): prevent half-day selection on holidays#4141
fix(leave-application): prevent half-day selection on holidays#4141deepeshgarg007 merged 13 commits intofrappe:developfrom
Conversation
krishna-254
requested review from
asmitahase and
ruchamahabal
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA frontend method 🚥 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. 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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
hrms/hr/doctype/leave_application/test_leave_application.py (3)
1347-1369:⚠️ Potential issue | 🔴 CriticalTest may break when
nowdate()is a holiday — attendance and leave dates can diverge.The attendance is marked for
nowdate()(line 1358), buthalf_day_dateis computed viaget_non_holiday_date(employee.name, nowdate())which may return an earlier date if today is a holiday. In that case, the leave application (lines 1363-1364) covers onlyhalf_day_date(notnowdate()), so it won't update the attendance created on line 1358. The assertions at lines 1376-1379 would then fail.The attendance date should also be set to
half_day_date:🐛 Proposed fix
+ half_day_date = self.get_non_holiday_date(employee.name, nowdate()) attendance_name = mark_attendance( - employee=employee.name, attendance_date=nowdate(), status="Half Day", half_day_status="Absent" + employee=employee.name, attendance_date=half_day_date, status="Half Day", half_day_status="Absent" ) - half_day_date = self.get_non_holiday_date(employee.name, nowdate())
1381-1413:⚠️ Potential issue | 🔴 CriticalSame attendance/leave date mismatch issue as above.
Attendance is marked for
nowdate()(line 1391) but the leave application useshalf_day_datewhich might differ. The assertions at lines 1409-1413 check the attendance atattendance_name(fornowdate()), which the leave application may not have updated ifhalf_day_date != nowdate().🐛 Proposed fix
+ half_day_date = self.get_non_holiday_date(employee.name, nowdate()) attendance_name = mark_attendance( - employee=employee.name, attendance_date=nowdate(), status="Absent" + employee=employee.name, attendance_date=half_day_date, status="Absent" ) - half_day_date = self.get_non_holiday_date(employee.name, nowdate())
1415-1456:⚠️ Potential issue | 🔴 CriticalSame pattern: attendance query uses
nowdate()but leave may be on a different date.Line 1436 and 1452 filter attendance by
attendance_date: nowdate(), but the leave applications usehalf_day_datewhich can differ fromnowdate(). Usehalf_day_dateconsistently:🐛 Proposed fix
half_day_status_after_first_application = frappe.get_value( "Attendance", - filters={"attendance_date": nowdate(), "leave_application": first_leave_application.name}, + filters={"attendance_date": half_day_date, "leave_application": first_leave_application.name}, fieldname="half_day_status", )And similarly for the second query at line 1450-1453.
🤖 Fix all issues with AI agents
In `@hrms/hr/doctype/leave_application/leave_application.js`:
- Around line 170-173: The half_day_date handler currently calls
frm.trigger("calculate_total_days") immediately, causing a race with the async
validate_half_day_date call; remove the immediate
frm.trigger("calculate_total_days") from the half_day_date function so only
validate_half_day_date (which performs the async server call) runs and then
calls calculate_total_days in its callback (see validate_half_day_date and
calculate_total_days) to avoid stale or duplicate calculations.
- Around line 233-255: In validate_half_day_date, guard against missing
prerequisites (frm.doc.employee, frm.doc.from_date, frm.doc.to_date) before
making the frappe.call to get_half_day_validation: if any are falsy, clear the
half_day_date (frm.set_value("half_day_date", "")) and show a short message
prompting the user to set Employee/From Date/To Date, then return early;
otherwise proceed with the existing frappe.call and keep the current callback
that triggers calculate_total_days on success.
In `@hrms/hr/doctype/leave_application/leave_application.py`:
- Around line 936-943: The long conditional assigning number_of_days should be
split for formatting while preserving the logic: keep the existing checks using
half_day_is_holiday (from is_holiday(employee, date=half_day_date)),
getdate(from_date), getdate(to_date), half_day_date and date_diff(to_date,
from_date). Refactor the elif into shorter statements by extracting booleans
like half_day_in_range = half_day_date and getdate(from_date) <=
getdate(half_day_date) <= getdate(to_date) and not half_day_is_holiday, then set
number_of_days = date_diff(to_date, from_date) + 0.5 when half_day_in_range,
otherwise keep the single-day fallback number_of_days = date_diff(to_date,
from_date) + 1; ensure no change to the holiday-handling logic.
- Around line 211-212: The long conditional line in validate() using
self.half_day, self.half_day_date and is_holiday should be wrapped to satisfy
formatter limits: split the condition across multiple lines (or assign the
boolean to a clearly named variable like is_half_day_holiday) and then call
frappe.throw(_("Half Day Date cannot be a holiday")) when true; keep the same
symbols (self.half_day, self.half_day_date, is_holiday, frappe.throw) and
identical error text but format the expression over multiple lines or via a
temporary variable to pass ruff-format.
🧹 Nitpick comments (3)
hrms/hr/doctype/leave_application/leave_application.py (2)
912-920: Missing input validation on whitelisted endpoint.
get_half_day_validationis@frappe.whitelist()and accepts anemployeeparameter directly from client calls. While this is a common pattern in this codebase, there's no guard against missing/empty arguments. Ifemployeeorhalf_day_dateis empty/None,is_holidaymay raise an unexpected error instead of a clear validation message.Consider adding a basic guard:
🛡️ Suggested improvement
`@frappe.whitelist`() def get_half_day_validation(employee, from_date, to_date, half_day_date): + if not (employee and from_date and to_date and half_day_date): + return False + if is_holiday(employee=employee, date=half_day_date): return False
25-25: Consider merging imports from the same module.Line 24 already imports
get_holiday_list_for_employeefromerpnext.setup.doctype.employee.employee. This new import ofis_holidayfrom the same module could be combined into a single import statement.♻️ Suggested merge
-from erpnext.setup.doctype.employee.employee import get_holiday_list_for_employee -from erpnext.setup.doctype.employee.employee import is_holiday +from erpnext.setup.doctype.employee.employee import get_holiday_list_for_employee, is_holidayhrms/hr/doctype/leave_application/test_leave_application.py (1)
914-916: Redundant no-op:add_days(half_day_date, 0).
from_date = add_days(half_day_date, 0)is equivalent to justfrom_date = half_day_date. If the intent is to create a copy,getdate(half_day_date)would be clearer.♻️ Suggested simplification
half_day_date = self.get_non_holiday_date(employee.name, add_days(nowdate(), -3)) - from_date = add_days(half_day_date, 0) + from_date = half_day_date to_date = add_days(half_day_date, 10)
| validate_half_day_date: function (frm) { | ||
| if (frm.doc.half_day_date) { | ||
| return frappe.call({ | ||
| method: "hrms.hr.doctype.leave_application.leave_application.get_half_day_validation", | ||
| args: { | ||
| employee: frm.doc.employee, | ||
| from_date: frm.doc.from_date, | ||
| to_date: frm.doc.to_date, | ||
| half_day_date: frm.doc.half_day_date, | ||
| }, | ||
| callback: function (r) { | ||
| if (r && r.message) { | ||
| frm.trigger("calculate_total_days"); | ||
| } else { | ||
| frm.set_value("half_day_date", ""); | ||
| frappe.msgprint( | ||
| __("Half Day Date must be between From Date and To Date and should not be a holiday."), | ||
| ); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Validation should also guard against missing prerequisite fields.
If employee, from_date, or to_date are not yet set when the user picks a half-day date, the server call will be made with undefined values. Add a guard similar to get_leave_balance (lines 207-213):
🛡️ Suggested improvement
validate_half_day_date: function (frm) {
- if (frm.doc.half_day_date) {
+ if (frm.doc.half_day_date && frm.doc.employee && frm.doc.from_date && frm.doc.to_date) {
return frappe.call({📝 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.
| validate_half_day_date: function (frm) { | |
| if (frm.doc.half_day_date) { | |
| return frappe.call({ | |
| method: "hrms.hr.doctype.leave_application.leave_application.get_half_day_validation", | |
| args: { | |
| employee: frm.doc.employee, | |
| from_date: frm.doc.from_date, | |
| to_date: frm.doc.to_date, | |
| half_day_date: frm.doc.half_day_date, | |
| }, | |
| callback: function (r) { | |
| if (r && r.message) { | |
| frm.trigger("calculate_total_days"); | |
| } else { | |
| frm.set_value("half_day_date", ""); | |
| frappe.msgprint( | |
| __("Half Day Date must be between From Date and To Date and should not be a holiday."), | |
| ); | |
| } | |
| } | |
| }); | |
| } | |
| }, | |
| validate_half_day_date: function (frm) { | |
| if (frm.doc.half_day_date && frm.doc.employee && frm.doc.from_date && frm.doc.to_date) { | |
| return frappe.call({ | |
| method: "hrms.hr.doctype.leave_application.leave_application.get_half_day_validation", | |
| args: { | |
| employee: frm.doc.employee, | |
| from_date: frm.doc.from_date, | |
| to_date: frm.doc.to_date, | |
| half_day_date: frm.doc.half_day_date, | |
| }, | |
| callback: function (r) { | |
| if (r && r.message) { | |
| frm.trigger("calculate_total_days"); | |
| } else { | |
| frm.set_value("half_day_date", ""); | |
| frappe.msgprint( | |
| __("Half Day Date must be between From Date and To Date and should not be a holiday."), | |
| ); | |
| } | |
| } | |
| }); | |
| } | |
| }, |
🤖 Prompt for AI Agents
In `@hrms/hr/doctype/leave_application/leave_application.js` around lines 233 -
255, In validate_half_day_date, guard against missing prerequisites
(frm.doc.employee, frm.doc.from_date, frm.doc.to_date) before making the
frappe.call to get_half_day_validation: if any are falsy, clear the
half_day_date (frm.set_value("half_day_date", "")) and show a short message
prompting the user to set Employee/From Date/To Date, then return early;
otherwise proceed with the existing frappe.call and keep the current callback
that triggers calculate_total_days on success.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@hrms/hr/doctype/leave_application/leave_application.py`:
- Around line 915-923: The whitelisted function get_half_day_validation lacks
type annotations causing linter failures; add type hints for all arguments (e.g.
employee: str, from_date: str, to_date: str, half_day_date: str or appropriate
date type) and update the function signature accordingly to satisfy the linter;
optionally rename the parameters (for example from_date -> from_dt and to_date
-> to_dt) to avoid shadowing the imported getdate if you prefer, and adjust all
references inside get_half_day_validation to use the new names.
🧹 Nitpick comments (1)
hrms/hr/doctype/leave_application/leave_application.py (1)
940-948: Single-day half-day case (line 942-943) doesn't checkhalf_day_is_holiday.When
from_date == to_date, the code unconditionally setsnumber_of_days = 0.5without checking whether that day is a holiday. The newvalidate_datesguard prevents this for new applications, butget_number_of_leave_daysis a public whitelisted function also called fromget_leaves_for_periodfor historical data. For consistency and defensive correctness, consider adding the holiday check to this branch as well.♻️ Suggested change
if cint(half_day) == 1: half_day_is_holiday = is_holiday(employee=employee, date=half_day_date) if getdate(from_date) == getdate(to_date): - number_of_days = 0.5 + number_of_days = 1 if half_day_is_holiday else 0.5 elif ( half_day_date and getdate(from_date) <= getdate(half_day_date) <= getdate(to_date) and not half_day_is_holiday ):
|
This pull request is being marked as inactive because of no recent activity. It will be closed in 3 days if no further activity occurs. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hrms/hr/doctype/leave_application/test_leave_application.py`:
- Around line 914-917: The test derives half_day_date via get_non_holiday_date
which makes expected ledger split assertions (-9.5, -1.0) flaky when
holidays/weekends shift; fix by either pinning the holiday/calendar context for
the test (mock or set the holiday list/calendar so get_non_holiday_date returns
a deterministic date) or compute expected ledger splits dynamically from the
computed from_date/to_date/half_day_date window instead of hardcoding -9.5 and
-1.0; update assertions that reference those hardcoded values (also at the
occurrences around lines 942-943) to use the derived values or the pinned
holiday setup so the test is stable.
- Around line 1360-1369: Tests compute half_day_date via get_non_holiday_date
but still use nowdate() when creating/reading Attendance and asserting half-day
status; update the test flows around make_leave_application, Attendance
creation/lookup and assertions (e.g., where Attendance.get, frappe.get_doc or
any attendance fixtures are created/read) to use the computed half_day_date
variable instead of nowdate(), ensuring every attendance creation and
half_day_status/"Half Day" assertion uses half_day_date consistently (this
applies to the blocks around the make_leave_application call and the blocks at
the other ranges noted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35a65ee1-9b5c-4b76-a5a1-19927b076125
📒 Files selected for processing (2)
hrms/hr/doctype/leave_application/leave_application.pyhrms/hr/doctype/leave_application/test_leave_application.py
| half_day_date = self.get_non_holiday_date(employee.name, nowdate()) | ||
| leave_application = make_leave_application( | ||
| employee.name, | ||
| nowdate(), | ||
| nowdate(), | ||
| half_day_date, | ||
| half_day_date, | ||
| leave_type.name, | ||
| submit=True, | ||
| half_day=1, | ||
| half_day_date=nowdate(), | ||
| half_day_date=half_day_date, | ||
| ) |
There was a problem hiding this comment.
Half-day tests still mix half_day_date with nowdate()-based attendance checks.
You now compute half_day_date as the effective working date, but these flows still create/read attendance anchored to nowdate() in downstream lines. If half_day_date != nowdate(), assertions for "Half Day"/half_day_status can fail incorrectly. Use half_day_date consistently for attendance creation and lookup in these test cases.
Also applies to: 1393-1402, 1424-1449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hrms/hr/doctype/leave_application/test_leave_application.py` around lines
1360 - 1369, Tests compute half_day_date via get_non_holiday_date but still use
nowdate() when creating/reading Attendance and asserting half-day status; update
the test flows around make_leave_application, Attendance creation/lookup and
assertions (e.g., where Attendance.get, frappe.get_doc or any attendance
fixtures are created/read) to use the computed half_day_date variable instead of
nowdate(), ensuring every attendance creation and half_day_status/"Half Day"
assertion uses half_day_date consistently (this applies to the blocks around the
make_leave_application call and the blocks at the other ranges noted).
…tions and refactor get_half_day_validation function
krishna-254
force-pushed
the
fix/holiday-check-for-half-day
branch
from
e91f1cd to
72beff9
Compare
|
This pull request is being marked as inactive because of no recent activity. It will be closed in 3 days if no further activity occurs. |
|
@Mergifyio rebase |
| if is_holiday(employee=self.employee, date=self.half_day_date): | ||
| return False | ||
|
|
||
| if not (getdate(self.from_date) <= getdate(self.half_day_date) <= getdate(self.to_date)): |
There was a problem hiding this comment.
This should also be validated on server side. De-duplicate this with line 210-215 with a single function
validate_half_day_date and call it from client-side?
…factor related functions
|
This pull request is being marked as inactive because of no recent activity. It will be closed in 3 days if no further activity occurs. |
4 participants
Add server-side validation and complementary client-side
checks to prevent selecting holidays as half_day_date.
Closes #3331
Summary by CodeRabbit
Bug Fixes
New Features
Tests