fix(leave_allocation): handle expired leave allocation#4559
Conversation
…hide new allocation field
|
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:
WalkthroughThis pull request updates the expire_allocation function to include type hints and to mark linked Leave Allocation documents expired while resetting new_leaves_allocated and total_leaves_allocated to 0. The Leave Allocation form refresh handler now hides the new_leaves_allocated field when the document is expired. Tests were added to expire an allocation, assert cleared counters, and verify manual reallocation updates totals while new_leaves_allocated remains 0. 🚥 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hrms/hr/doctype/leave_ledger_entry/leave_ledger_entry.py (1)
229-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply consistent field updates to carry-forward allocations.
The
expire_allocationfunction now updates the Leave Allocation document with zeroed fields (line 222-226), butexpire_carried_forward_allocationonly creates a ledger entry without updating the Leave Allocation document. This creates an inconsistency where carry-forward allocations retain theirnew_leaves_allocatedandtotal_leaves_allocatedvalues after expiry, while regular allocations have them zeroed.🔧 Proposed fix to add field updates for carry-forward expiration
def expire_carried_forward_allocation(allocation): """Expires remaining leaves in the on carried forward allocation""" from hrms.hr.doctype.leave_application.leave_application import get_leaves_for_period leaves_taken = get_leaves_for_period( allocation.employee, allocation.leave_type, allocation.from_date, allocation.to_date, skip_expired_leaves=False, ) leaves = flt(allocation.leaves) + flt(leaves_taken) # allow expired leaves entry to be created if leaves > 0: args = frappe._dict( transaction_name=allocation.name, transaction_type="Leave Allocation", leaves=leaves * -1, is_carry_forward=allocation.is_carry_forward, is_expired=1, from_date=allocation.to_date, to_date=allocation.to_date, ) create_leave_ledger_entry(allocation, args) + + frappe.db.set_value( + "Leave Allocation", + allocation.name, + {"expired": 1, "new_leaves_allocated": 0, "total_leaves_allocated": 0}, + )🤖 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 `@hrms/hr/doctype/leave_ledger_entry/leave_ledger_entry.py` around lines 229 - 253, The carry-forward expiry function expire_carried_forward_allocation currently only creates a ledger entry but does not update the Leave Allocation doc fields like expire_allocation does; after creating the ledger entry with create_leave_ledger_entry(allocation, args) update the allocation record to zero out the same fields expire_allocation resets (e.g. new_leaves_allocated, total_leaves_allocated, leaves or whichever allocation fields are zeroed in expire_allocation) and persist the change (use the same persistence approach used in expire_allocation such as allocation.db_set or allocation.save) so carry-forward allocations are consistently updated when expired.
🤖 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 `@hrms/hr/doctype/leave_allocation/leave_allocation.js`:
- Around line 31-33: The current code hides new_leaves_allocated for any expired
allocation (frm.doc.expired) but the backend zeroes both new_leaves_allocated
and total_leaves_allocated, causing a mismatch; update the client logic to hide
both fields when frm.doc.expired by calling
frm.set_df_property("new_leaves_allocated", "hidden", 1) and
frm.set_df_property("total_leaves_allocated", "hidden", 1) in the same
expired-check block (or, if the intended behavior is only for earned leave,
instead add the earned-leave condition around the existing frm.doc.expired check
to hide only new_leaves_allocated).
In `@hrms/hr/doctype/leave_ledger_entry/leave_ledger_entry.py`:
- Around line 222-226: The cancellation path in Leave Ledger Entry only unsets
the expired flag but does not restore fields that were zeroed
(new_leaves_allocated, total_leaves_allocated); update the on_cancel method on
the Leave Ledger Entry class to restore those original values for the related
Leave Allocation: either read and restore values saved on the expiry ledger
entry (add fields to the ledger like prev_new_leaves_allocated and
prev_total_leaves_allocated when expiring in the code that calls
frappe.db.set_value), or recompute the allocation totals from prior ledger
entries and set them back via frappe.db.set_value on the same allocation record;
ensure you reference the allocation by allocation.name and restore expired,
new_leaves_allocated and total_leaves_allocated together.
---
Outside diff comments:
In `@hrms/hr/doctype/leave_ledger_entry/leave_ledger_entry.py`:
- Around line 229-253: The carry-forward expiry function
expire_carried_forward_allocation currently only creates a ledger entry but does
not update the Leave Allocation doc fields like expire_allocation does; after
creating the ledger entry with create_leave_ledger_entry(allocation, args)
update the allocation record to zero out the same fields expire_allocation
resets (e.g. new_leaves_allocated, total_leaves_allocated, leaves or whichever
allocation fields are zeroed in expire_allocation) and persist the change (use
the same persistence approach used in expire_allocation such as
allocation.db_set or allocation.save) so carry-forward allocations are
consistently updated when expired.
🪄 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: 7679fd3f-8a8c-4b7f-81c2-327fb7a71597
📒 Files selected for processing (2)
hrms/hr/doctype/leave_allocation/leave_allocation.jshrms/hr/doctype/leave_ledger_entry/leave_ledger_entry.py
|
@iamkhanraheel Please add unit test for this scenario |
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 `@hrms/hr/doctype/leave_allocation/test_leave_allocation.py`:
- Around line 636-644: Add assertions to ensure the earned-leave counter doesn't
change during manual allocations: after each call to
leave_allocation.allocate_leaves_manually(...) and leave_allocation.reload(),
assert that leave_allocation.new_leaves_allocated == 0 in addition to the
existing total_leaves_allocated checks so the test verifies only
total_leaves_allocated is updated while new_leaves_allocated remains unchanged.
🪄 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: 2d11aeb5-164c-41c9-a407-49c45d18ea2d
📒 Files selected for processing (1)
hrms/hr/doctype/leave_allocation/test_leave_allocation.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hrms/hr/doctype/leave_allocation/leave_allocation.js (1)
134-134: ⚡ Quick winConsider conditional reload for consistency.
The
reload_doc()call executes unconditionally, while the similar callback forallocate_leaves_manually(lines 101-106) only reloads on success. For consistency with the established pattern in this file and Frappe best practices, consider moving the reload inside the success condition:♻️ Align with the callback pattern used elsewhere in this file
callback: function (r) { if (!r.exc) { frappe.msgprint(__("Allocation Expired!")); + frm.reload_doc(); } - frm.reload_doc(); },🤖 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 `@hrms/hr/doctype/leave_allocation/leave_allocation.js` at line 134, The unconditional frm.reload_doc() should be moved into the success branch of the callback to match the pattern used by allocate_leaves_manually; locate the callback that currently ends with frm.reload_doc() and wrap that call so it only runs when the server response indicates success (e.g., inside the if (!r.exc) / if (r.message) success check), mirroring how allocate_leaves_manually handles reloads.
🤖 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 `@hrms/hr/doctype/leave_allocation/leave_allocation.js`:
- Line 134: The unconditional frm.reload_doc() should be moved into the success
branch of the callback to match the pattern used by allocate_leaves_manually;
locate the callback that currently ends with frm.reload_doc() and wrap that call
so it only runs when the server response indicates success (e.g., inside the if
(!r.exc) / if (r.message) success check), mirroring how allocate_leaves_manually
handles reloads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e1c1d4b-5fa0-43ec-9278-f8b62c2539ea
📒 Files selected for processing (1)
hrms/hr/doctype/leave_allocation/leave_allocation.js
iamkhanraheel
force-pushed
the
fix/leave_allocation_data_after_expire
branch
from
4803da1 to
21dbdb7
Compare
2 participants
Reason
leaveExpiryBug.mov
Changes Done
LeaveExpiryFix.mov
Summary by CodeRabbit
Bug Fixes
Tests