feat(attendance): auto-populate employee shift#4443
feat(attendance): auto-populate employee shift#4443krishna-254 wants to merge 1 commit intofrappe:developfrom
Conversation
WalkthroughThis pull request introduces automatic shift population functionality across two forms: Attendance and Attendance Request. When an employee is assigned or the attendance/from date is modified, and the shift field remains unset, the forms trigger a server call to fetch the applicable shift type. A new whitelisted backend 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
🧹 Nitpick comments (2)
hrms/hr/doctype/attendance_request/attendance_request.js (1)
27-37: Minor: the two handlers are identical and can just callset_employee_shiftdirectly.
employee(frm)andfrom_date(frm)run the exact same guard, andset_employee_shiftalready re-checksemployee/from_datebefore the server call. You can collapse both tofrm.trigger("set_employee_shift")and centralize the!frm.doc.shiftguard there to avoid drift.♻️ Possible refactor
- employee(frm) { - if (frm.doc.employee && frm.doc.from_date && !frm.doc.shift) { - frm.trigger("set_employee_shift"); - } - }, - - from_date(frm) { - if (frm.doc.employee && frm.doc.from_date && !frm.doc.shift) { - frm.trigger("set_employee_shift"); - } - }, - - set_employee_shift(frm) { - if (!frm.doc.employee || !frm.doc.from_date) return; + employee(frm) { + frm.trigger("set_employee_shift"); + }, + + from_date(frm) { + frm.trigger("set_employee_shift"); + }, + + set_employee_shift(frm) { + if (!frm.doc.employee || !frm.doc.from_date || frm.doc.shift) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hrms/hr/doctype/attendance_request/attendance_request.js` around lines 27 - 37, Both employee(frm) and from_date(frm) duplicate the same guard and can simply trigger set_employee_shift; update the two handlers (employee and from_date) to call frm.trigger("set_employee_shift") directly, then move the !frm.doc.shift check into the start of the set_employee_shift handler so that set_employee_shift re-checks employee, from_date, and !frm.doc.shift before making any server calls.hrms/hr/doctype/attendance/attendance.py (1)
478-478: Nit: redundantemployee andin permission check.The earlier guard on line 475 already returns when
employeeis falsy, so theemployee andconjunct here is dead. Minor readability:🧹 Tiny cleanup
- if employee and not frappe.has_permission("Employee", "read", employee): + if not frappe.has_permission("Employee", "read", employee): return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hrms/hr/doctype/attendance/attendance.py` at line 478, Remove the redundant guard in the permission check by dropping the `employee and` conjunct so the condition becomes just `if not frappe.has_permission("Employee", "read", employee):`; this keeps the earlier falsy-employee early return intact and preserves the intended permission check in the attendance.py code line that currently reads `if employee and not frappe.has_permission("Employee", "read", employee):`.
🤖 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/attendance_request/attendance_request.js`:
- Around line 27-55: The JS shift auto-population currently calls the backend
method "hrms.hr.doctype.attendance.attendance.get_employee_shift" from
set_employee_shift (and triggered by employee and from_date handlers), but that
backend only accepts employee and for_date and drops consider_default_shift;
update the frappe.call in set_employee_shift to call
"hrms.hr.doctype.shift_assignment.shift_assignment.get_employee_shift" instead,
keeping the same args (employee, for_date from frm.doc.from_date,
consider_default_shift: true) and the same callback logic that sets
frm.set_value("shift", r.message) when r.message exists and frm.doc.shift is
empty.
In `@hrms/hr/doctype/attendance/attendance.js`:
- Line 50: The object property for_date currently uses an unreachable fallback
("for_date: frm.doc.attendance_date || frappe.datetime.get_today()"); remove the
dead fallback so it reads "for_date: frm.doc.attendance_date" (reference:
attendance.js, frm.doc.attendance_date, frappe.datetime.get_today(), for_date) —
the backend already defaults to nowdate(), so dropping the "||
frappe.datetime.get_today()" avoids misleading readers.
In `@hrms/hr/doctype/attendance/attendance.py`:
- Around line 489-500: The query fetching Shift Assignment via frappe.get_all
(assigning to shifts) can return expired records because it only filters on
"start_date" <= for_date; update the filters for the "Shift Assignment" query to
also require that "end_date" is either null/empty or >= for_date (i.e., add an
end_date condition using ("is", "NULL") OR ("<=", for_date) appropriately),
referencing the existing variables employee and for_date; alternatively, replace
this local lookup with the existing shift_assignment.get_employee_shift helper
which already enforces end_date correctly.
- Around line 473-505: The current get_employee_shift in attendance.py should be
replaced with a thin wrapper that delegates to the canonical helper
hrms.hr.doctype.shift_assignment.shift_assignment.get_employee_shift; remove the
duplicated filtering/permission logic and instead import that helper and call it
with the appropriate parameters (map for_date to the helper's for_timestamp
argument, and add the consider_default_shift and next_shift_direction parameters
so callers can pass them through), then return the helper's result; ensure the
wrapper's signature includes the extra params (consider_default_shift=False,
next_shift_direction=None) and convert for_date to the expected timestamp type
if needed before calling the helper.
---
Nitpick comments:
In `@hrms/hr/doctype/attendance_request/attendance_request.js`:
- Around line 27-37: Both employee(frm) and from_date(frm) duplicate the same
guard and can simply trigger set_employee_shift; update the two handlers
(employee and from_date) to call frm.trigger("set_employee_shift") directly,
then move the !frm.doc.shift check into the start of the set_employee_shift
handler so that set_employee_shift re-checks employee, from_date, and
!frm.doc.shift before making any server calls.
In `@hrms/hr/doctype/attendance/attendance.py`:
- Line 478: Remove the redundant guard in the permission check by dropping the
`employee and` conjunct so the condition becomes just `if not
frappe.has_permission("Employee", "read", employee):`; this keeps the earlier
falsy-employee early return intact and preserves the intended permission check
in the attendance.py code line that currently reads `if employee and not
frappe.has_permission("Employee", "read", employee):`.
🪄 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: b6d910da-7a78-4bcc-8557-80b5994bf653
📒 Files selected for processing (3)
hrms/hr/doctype/attendance/attendance.jshrms/hr/doctype/attendance/attendance.pyhrms/hr/doctype/attendance_request/attendance_request.js
| employee(frm) { | ||
| if (frm.doc.employee && frm.doc.from_date && !frm.doc.shift) { | ||
| frm.trigger("set_employee_shift"); | ||
| } | ||
| }, | ||
|
|
||
| from_date(frm) { | ||
| if (frm.doc.employee && frm.doc.from_date && !frm.doc.shift) { | ||
| frm.trigger("set_employee_shift"); | ||
| } | ||
| }, | ||
|
|
||
| set_employee_shift(frm) { | ||
| if (!frm.doc.employee || !frm.doc.from_date) return; | ||
|
|
||
| frappe.call({ | ||
| method: "hrms.hr.doctype.attendance.attendance.get_employee_shift", | ||
| args: { | ||
| employee: frm.doc.employee, | ||
| for_date: frm.doc.from_date, | ||
| consider_default_shift: true, | ||
| }, | ||
| callback(r) { | ||
| if (r.message && !frm.doc.shift) { | ||
| frm.set_value("shift", r.message); | ||
| } | ||
| }, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there is no existing whitelisted method on the backend accepting consider_default_shift
rg -nP --type=py -C2 '\bdef\s+get_employee_shift\s*\('Repository: frappe/hrms
Length of output: 792
🏁 Script executed:
#!/bin/bash
# Get full signature and implementation of get_employee_shift in attendance.py
sed -n '474,510p' hrms/hr/doctype/attendance/attendance.pyRepository: frappe/hrms
Length of output: 720
🏁 Script executed:
#!/bin/bash
# Get full signature and implementation of get_employee_shift in shift_assignment.py
sed -n '455,490p' hrms/hr/doctype/shift_assignment/shift_assignment.pyRepository: frappe/hrms
Length of output: 1439
🏁 Script executed:
#!/bin/bash
# Check the actual JavaScript code in attendance_request.js
cat -n hrms/hr/doctype/attendance_request/attendance_request.js | head -70Repository: frappe/hrms
Length of output: 1790
🏁 Script executed:
#!/bin/bash
# Search for any references to consider_default_shift in the codebase
rg "consider_default_shift" --type=py --type=jsRepository: frappe/hrms
Length of output: 2239
🏁 Script executed:
#!/bin/bash
# Search for default_shift handling in attendance-related files
rg "default_shift" hrms/hr/doctype/attendance/ hrms/hr/doctype/attendance_request/ --type=py --type=jsRepository: frappe/hrms
Length of output: 353
Call the correct backend endpoint for shift auto-population with default shift fallback support.
The JavaScript code passes consider_default_shift: true to hrms.hr.doctype.attendance.attendance.get_employee_shift, but the backend method signature only accepts employee and for_date. The consider_default_shift parameter will be silently dropped by Frappe, causing employees with only a default_shift (no Shift Assignment) to not get the shift auto-populated.
The correct implementation exists in hrms.hr.doctype.shift_assignment.shift_assignment.get_employee_shift, which accepts consider_default_shift and properly consults Employee.default_shift as a fallback. Switch to calling that endpoint instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hrms/hr/doctype/attendance_request/attendance_request.js` around lines 27 -
55, The JS shift auto-population currently calls the backend method
"hrms.hr.doctype.attendance.attendance.get_employee_shift" from
set_employee_shift (and triggered by employee and from_date handlers), but that
backend only accepts employee and for_date and drops consider_default_shift;
update the frappe.call in set_employee_shift to call
"hrms.hr.doctype.shift_assignment.shift_assignment.get_employee_shift" instead,
keeping the same args (employee, for_date from frm.doc.from_date,
consider_default_shift: true) and the same callback logic that sets
frm.set_value("shift", r.message) when r.message exists and frm.doc.shift is
empty.
| method: "hrms.hr.doctype.attendance.attendance.get_employee_shift", | ||
| args: { | ||
| employee: frm.doc.employee, | ||
| for_date: frm.doc.attendance_date || frappe.datetime.get_today(), |
There was a problem hiding this comment.
Dead fallback — frappe.datetime.get_today() is unreachable.
Line 44 already returns early when attendance_date is falsy, so || frappe.datetime.get_today() can never trigger here. Drop it to avoid misleading readers; the backend already defaults to nowdate() when for_date is missing.
🧹 Suggested cleanup
- for_date: frm.doc.attendance_date || frappe.datetime.get_today(),
+ for_date: frm.doc.attendance_date,📝 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.
| for_date: frm.doc.attendance_date || frappe.datetime.get_today(), | |
| for_date: frm.doc.attendance_date, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hrms/hr/doctype/attendance/attendance.js` at line 50, The object property
for_date currently uses an unreachable fallback ("for_date:
frm.doc.attendance_date || frappe.datetime.get_today()"); remove the dead
fallback so it reads "for_date: frm.doc.attendance_date" (reference:
attendance.js, frm.doc.attendance_date, frappe.datetime.get_today(), for_date) —
the backend already defaults to nowdate(), so dropping the "||
frappe.datetime.get_today()" avoids misleading readers.
| @frappe.whitelist() | ||
| def get_employee_shift(employee: str, for_date: str | date | None = None) -> str | None: | ||
| if not employee: | ||
| return None | ||
|
|
||
| if employee and not frappe.has_permission("Employee", "read", employee): | ||
| return None | ||
|
|
||
| if not for_date: | ||
| for_date = nowdate() | ||
|
|
||
| for_date = getdate(for_date) | ||
|
|
||
| if not frappe.has_permission("Shift Assignment", "read"): | ||
| return None | ||
|
|
||
| shifts = frappe.get_all( | ||
| "Shift Assignment", | ||
| filters={ | ||
| "employee": employee, | ||
| "docstatus": 1, | ||
| "status": "Active", | ||
| "start_date": ("<=", for_date), | ||
| }, | ||
| fields=["shift_type", "start_date"], | ||
| order_by="start_date desc", | ||
| limit=1, | ||
| ) | ||
|
|
||
| if shifts: | ||
| return shifts[0].shift_type | ||
|
|
||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the existing helper's signature and behavior (default_shift + end_date handling)
ast-grep --pattern $'def get_employee_shift($$$):
$$$'Repository: frappe/hrms
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Find the shift_assignment.py file
fd -type f -name "shift_assignment.py" | head -20Repository: frappe/hrms
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Find shift_assignment.py file
find . -name "shift_assignment.py" -type fRepository: frappe/hrms
Length of output: 110
🏁 Script executed:
#!/bin/bash
# First, check file size
wc -l ./hrms/hr/doctype/shift_assignment/shift_assignment.pyRepository: frappe/hrms
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Search for the get_employee_shift function definition
rg -n "def get_employee_shift" ./hrms/hr/doctype/shift_assignment/shift_assignment.pyRepository: frappe/hrms
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Read the get_employee_shift function (lines 455-540)
sed -n '455,540p' ./hrms/hr/doctype/shift_assignment/shift_assignment.pyRepository: frappe/hrms
Length of output: 3072
🏁 Script executed:
#!/bin/bash
# Find get_shift_for_timestamp and get_shift_details functions
rg -n "def get_shift_for_timestamp|def get_shift_details" ./hrms/hr/doctype/shift_assignment/shift_assignment.pyRepository: frappe/hrms
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Read get_shift_for_timestamp (lines 448-454)
sed -n '448,454p' ./hrms/hr/doctype/shift_assignment/shift_assignment.pyRepository: frappe/hrms
Length of output: 263
🏁 Script executed:
#!/bin/bash
# Find get_shifts_for_date function
rg -n "def get_shifts_for_date" ./hrms/hr/doctype/shift_assignment/shift_assignment.pyRepository: frappe/hrms
Length of output: 148
🏁 Script executed:
#!/bin/bash
# Read get_shifts_for_date (lines 406-447)
sed -n '406,447p' ./hrms/hr/doctype/shift_assignment/shift_assignment.pyRepository: frappe/hrms
Length of output: 1427
🏁 Script executed:
#!/bin/bash
# Find get_shift_for_time function
rg -n "def get_shift_for_time" ./hrms/hr/doctype/shift_assignment/shift_assignment.pyRepository: frappe/hrms
Length of output: 217
🏁 Script executed:
#!/bin/bash
# Read get_shift_for_time (lines 301-405)
sed -n '301,405p' ./hrms/hr/doctype/shift_assignment/shift_assignment.pyRepository: frappe/hrms
Length of output: 3890
🏁 Script executed:
#!/bin/bash
# Read get_shift_details (lines 624-700)
sed -n '624,700p' ./hrms/hr/doctype/shift_assignment/shift_assignment.pyRepository: frappe/hrms
Length of output: 2760
🏁 Script executed:
#!/bin/bash
# Search for usages of get_employee_shift from shift_assignment
rg "get_employee_shift" --type py -B 2 -A 2 | head -80Repository: frappe/hrms
Length of output: 5565
🏁 Script executed:
#!/bin/bash
# Check how the attendance.py function is called to understand parameter expectations
rg "get_employee_shift" ./hrms/hr/doctype/attendance/ -B 3 -A 3Repository: frappe/hrms
Length of output: 1066
🏁 Script executed:
#!/bin/bash
# Read the full JS call to see if consider_default_shift is being passed
sed -n '1,600p' ./hrms/hr/doctype/attendance/attendance.js | grep -A 10 "get_employee_shift"Repository: frappe/hrms
Length of output: 387
🏁 Script executed:
#!/bin/bash
# Check imports in attendance.py
head -50 ./hrms/hr/doctype/attendance/attendance.py | grep -E "^from|^import"Repository: frappe/hrms
Length of output: 481
Replace this partial reimplementation with a thin wrapper around the existing shift_assignment.get_employee_shift() helper.
The existing helper at hrms.hr.doctype.shift_assignment.shift_assignment.get_employee_shift(employee, for_timestamp=None, consider_default_shift=False, next_shift_direction=None) is the canonical implementation. The current attendance.py version duplicates partial logic and critically:
- Silently drops
consider_default_shift, which the JS caller passes astrue, breaking expected behavior - Omits
end_datefiltering, returning expired shift assignments - Lacks
next_shift_directionsupport
Replace with a wrapper that properly delegates to the existing helper:
♻️ Suggested wrapper
+from datetime import datetime
+from hrms.hr.doctype.shift_assignment.shift_assignment import (
+ get_employee_shift as _get_employee_shift,
+)
+
`@frappe.whitelist`()
-def get_employee_shift(employee: str, for_date: str | date | None = None) -> str | None:
- if not employee:
- return None
-
- if employee and not frappe.has_permission("Employee", "read", employee):
- return None
-
- if not for_date:
- for_date = nowdate()
-
- for_date = getdate(for_date)
-
- if not frappe.has_permission("Shift Assignment", "read"):
- return None
-
- shifts = frappe.get_all(
- "Shift Assignment",
- filters={
- "employee": employee,
- "docstatus": 1,
- "status": "Active",
- "start_date": ("<=", for_date),
- },
- fields=["shift_type", "start_date"],
- order_by="start_date desc",
- limit=1,
- )
-
- if shifts:
- return shifts[0].shift_type
-
- return None
+def get_employee_shift(
+ employee: str,
+ for_date: str | date | None = None,
+ consider_default_shift: bool = False,
+) -> str | None:
+ if not employee or not frappe.has_permission("Employee", "read", employee):
+ return None
+ if not frappe.has_permission("Shift Assignment", "read"):
+ return None
+
+ for_timestamp = datetime.combine(
+ getdate(for_date) if for_date else getdate(nowdate()),
+ datetime.min.time()
+ )
+ shift_details = _get_employee_shift(
+ employee,
+ for_timestamp,
+ consider_default_shift=cint(consider_default_shift),
+ )
+ return shift_details.shift_type.name if shift_details else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hrms/hr/doctype/attendance/attendance.py` around lines 473 - 505, The current
get_employee_shift in attendance.py should be replaced with a thin wrapper that
delegates to the canonical helper
hrms.hr.doctype.shift_assignment.shift_assignment.get_employee_shift; remove the
duplicated filtering/permission logic and instead import that helper and call it
with the appropriate parameters (map for_date to the helper's for_timestamp
argument, and add the consider_default_shift and next_shift_direction parameters
so callers can pass them through), then return the helper's result; ensure the
wrapper's signature includes the extra params (consider_default_shift=False,
next_shift_direction=None) and convert for_date to the expected timestamp type
if needed before calling the helper.
| shifts = frappe.get_all( | ||
| "Shift Assignment", | ||
| filters={ | ||
| "employee": employee, | ||
| "docstatus": 1, | ||
| "status": "Active", | ||
| "start_date": ("<=", for_date), | ||
| }, | ||
| fields=["shift_type", "start_date"], | ||
| order_by="start_date desc", | ||
| limit=1, | ||
| ) |
There was a problem hiding this comment.
Missing end_date filter — may return an expired Shift Assignment.
Shift Assignment has an optional end_date (per the doctype schema) and a status of Active is kept even after the assignment ends (the status field is allow_on_submit and isn't automatically flipped when end_date passes). With only start_date <= for_date, this query can pick an assignment whose end_date is before for_date, populating an expired shift on the form. Add an end-date condition:
🛡️ Proposed fix (if keeping the local implementation)
shifts = frappe.get_all(
"Shift Assignment",
filters={
"employee": employee,
"docstatus": 1,
"status": "Active",
"start_date": ("<=", for_date),
},
+ or_filters=[
+ ["end_date", "is", "not set"],
+ ["end_date", ">=", for_date],
+ ],
fields=["shift_type", "start_date"],
order_by="start_date desc",
limit=1,
)Note: this becomes moot if you adopt the refactor in the sibling comment (the existing shift_assignment.get_employee_shift already handles end_date correctly).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hrms/hr/doctype/attendance/attendance.py` around lines 489 - 500, The query
fetching Shift Assignment via frappe.get_all (assigning to shifts) can return
expired records because it only filters on "start_date" <= for_date; update the
filters for the "Shift Assignment" query to also require that "end_date" is
either null/empty or >= for_date (i.e., add an end_date condition using ("is",
"NULL") OR ("<=", for_date) appropriately), referencing the existing variables
employee and for_date; alternatively, replace this local lookup with the
existing shift_assignment.get_employee_shift helper which already enforces
end_date correctly.
1 participant
Closes #4413
Changes:
Summary by CodeRabbit
Release Notes