From e74e02b7654dadd3090aa868a95692a3b3cca09f Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Mon, 6 Mar 2023 13:12:34 +0530 Subject: [PATCH 1/7] fix: consider leaves taken while calculating expired carry-forwarded leaves --- erpnext/hr/doctype/leave_application/leave_application.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/erpnext/hr/doctype/leave_application/leave_application.py b/erpnext/hr/doctype/leave_application/leave_application.py index fff0967f25a..903a413d6bb 100755 --- a/erpnext/hr/doctype/leave_application/leave_application.py +++ b/erpnext/hr/doctype/leave_application/leave_application.py @@ -941,7 +941,7 @@ def get_remaining_leaves( if cf_expiry and allocation.unused_leaves: if getdate(date) > getdate(cf_expiry): # carry forwarded leave expiry date passed - cf_leaves = remaining_cf_leaves = 0 + cf_leaves = remaining_cf_leaves = flt(leaves_taken) else: cf_leaves = flt(allocation.unused_leaves) + flt(leaves_taken) remaining_cf_leaves = _get_remaining_leaves(cf_leaves, cf_expiry) From 52108d52e24b097bf0dc8c87acb23d0e58f7e467 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Mon, 6 Mar 2023 14:27:34 +0530 Subject: [PATCH 2/7] fix: consider leaves taken within carry-forwarded period separately while calculating balance --- .../hr/doctype/leave_application/leave_application.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/erpnext/hr/doctype/leave_application/leave_application.py b/erpnext/hr/doctype/leave_application/leave_application.py index 903a413d6bb..dedfcdbe21c 100755 --- a/erpnext/hr/doctype/leave_application/leave_application.py +++ b/erpnext/hr/doctype/leave_application/leave_application.py @@ -856,6 +856,7 @@ def get_leave_allocation_records(employee, date, leave_type=None): Min(Ledger.from_date).as_("from_date"), Max(Ledger.to_date).as_("to_date"), Ledger.leave_type, + Ledger.employee, ) .where( (Ledger.from_date <= date) @@ -895,6 +896,7 @@ def get_leave_allocation_records(employee, date, leave_type=None): "unused_leaves": d.cf_leaves, "new_leaves_allocated": d.new_leaves, "leave_type": d.leave_type, + "employee": d.employee, } ), ) @@ -939,11 +941,15 @@ def get_remaining_leaves( # balance for carry forwarded leaves if cf_expiry and allocation.unused_leaves: + cf_leaves_taken = get_leaves_for_period( + allocation.employee, allocation.leave_type, allocation.from_date, cf_expiry + ) + if getdate(date) > getdate(cf_expiry): # carry forwarded leave expiry date passed - cf_leaves = remaining_cf_leaves = flt(leaves_taken) + cf_leaves = remaining_cf_leaves = flt(cf_leaves_taken) else: - cf_leaves = flt(allocation.unused_leaves) + flt(leaves_taken) + cf_leaves = flt(allocation.unused_leaves) + flt(cf_leaves_taken) remaining_cf_leaves = _get_remaining_leaves(cf_leaves, cf_expiry) leave_balance = flt(allocation.new_leaves_allocated) + flt(cf_leaves) From c01bed98621caf7ef6f087661365640c0bead379 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Mon, 6 Mar 2023 15:35:49 +0530 Subject: [PATCH 3/7] fix(test): `get_leave_allocation_records` --- erpnext/hr/doctype/leave_application/test_leave_application.py | 1 + 1 file changed, 1 insertion(+) diff --git a/erpnext/hr/doctype/leave_application/test_leave_application.py b/erpnext/hr/doctype/leave_application/test_leave_application.py index 45e9a87428e..11b9a0abd32 100644 --- a/erpnext/hr/doctype/leave_application/test_leave_application.py +++ b/erpnext/hr/doctype/leave_application/test_leave_application.py @@ -1043,6 +1043,7 @@ class TestLeaveApplication(unittest.TestCase): "unused_leaves": 15.0, "new_leaves_allocated": 15.0, "leave_type": leave_type.name, + "employee": employee.name, } self.assertEqual(details.get(leave_type.name), expected_data) From fd5d2ed87f079a270894de6821e406fdc3d78d65 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Thu, 9 Mar 2023 12:44:56 +0530 Subject: [PATCH 4/7] refactor: consider cases for partially consumed cf and new leaves - the above two cases weren't considering the split between cf leaves taken and new leaves taken and substracting all consumed leaves from cf leaves --- .../leave_application/leave_application.py | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/erpnext/hr/doctype/leave_application/leave_application.py b/erpnext/hr/doctype/leave_application/leave_application.py index dedfcdbe21c..de88b3d807c 100755 --- a/erpnext/hr/doctype/leave_application/leave_application.py +++ b/erpnext/hr/doctype/leave_application/leave_application.py @@ -935,25 +935,33 @@ def get_remaining_leaves( return remaining_leaves - leave_balance = leave_balance_for_consumption = flt(allocation.total_leaves_allocated) + flt( - leaves_taken - ) - - # balance for carry forwarded leaves if cf_expiry and allocation.unused_leaves: + # allocation contains both carry forwarded and new leaves cf_leaves_taken = get_leaves_for_period( allocation.employee, allocation.leave_type, allocation.from_date, cf_expiry ) + new_leaves_taken = get_leaves_for_period( + allocation.employee, allocation.leave_type, add_days(cf_expiry, 1), allocation.to_date + ) if getdate(date) > getdate(cf_expiry): - # carry forwarded leave expiry date passed - cf_leaves = remaining_cf_leaves = flt(cf_leaves_taken) + # carry forwarded leaves have expired + cf_leaves = remaining_cf_leaves = 0 else: cf_leaves = flt(allocation.unused_leaves) + flt(cf_leaves_taken) remaining_cf_leaves = _get_remaining_leaves(cf_leaves, cf_expiry) - leave_balance = flt(allocation.new_leaves_allocated) + flt(cf_leaves) - leave_balance_for_consumption = flt(allocation.new_leaves_allocated) + flt(remaining_cf_leaves) + # new leaves allocated - new leaves taken + cf leave balance + # Note: `new_leaves_taken` is added here because its already a -ve number in the ledger + leave_balance = (flt(allocation.new_leaves_allocated) + flt(new_leaves_taken)) + flt(cf_leaves) + leave_balance_for_consumption = ( + flt(allocation.new_leaves_allocated) + flt(new_leaves_taken) + ) + flt(remaining_cf_leaves) + else: + # allocation only contains newly allocated leaves + leave_balance = leave_balance_for_consumption = flt(allocation.total_leaves_allocated) + flt( + leaves_taken + ) remaining_leaves = _get_remaining_leaves(leave_balance_for_consumption, allocation.to_date) return frappe._dict(leave_balance=leave_balance, leave_balance_for_consumption=remaining_leaves) From 7717a8a5e3018180498c69061b59551118f6c55b Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Thu, 9 Mar 2023 12:45:12 +0530 Subject: [PATCH 5/7] test: leave details with application across and after cf leave expiry --- .../test_leave_application.py | 86 ++++++++++++++++++- .../doctype/salary_slip/test_salary_slip.py | 13 +-- 2 files changed, 91 insertions(+), 8 deletions(-) diff --git a/erpnext/hr/doctype/leave_application/test_leave_application.py b/erpnext/hr/doctype/leave_application/test_leave_application.py index 11b9a0abd32..f41fd93e564 100644 --- a/erpnext/hr/doctype/leave_application/test_leave_application.py +++ b/erpnext/hr/doctype/leave_application/test_leave_application.py @@ -96,6 +96,9 @@ class TestLeaveApplication(unittest.TestCase): from_date = get_year_start(getdate()) to_date = get_year_ending(getdate()) self.holiday_list = make_holiday_list(from_date=from_date, to_date=to_date) + list_without_weekly_offs = make_holiday_list( + "Holiday List w/o Weekly Offs", from_date=from_date, to_date=to_date, add_weekly_offs=False + ) if not frappe.db.exists("Leave Type", "_Test Leave Type"): frappe.get_doc( @@ -990,8 +993,12 @@ class TestLeaveApplication(unittest.TestCase): } self.assertEqual(leave_allocation, expected) - @set_holiday_list("Salary Slip Test Holiday List", "_Test Company") + @set_holiday_list("Holiday List w/o Weekly Offs", "_Test Company") def test_leave_details_with_expired_cf_leaves(self): + """Tests leave details: + Case 1: All leaves available before cf leave expiry + Case 2: Remaining Leaves after cf leave expiry + """ employee = get_employee() leave_type = create_leave_type( leave_type_name="_Test_CF_leave_expiry", @@ -1004,11 +1011,11 @@ class TestLeaveApplication(unittest.TestCase): "Leave Ledger Entry", {"transaction_name": leave_alloc.name, "is_carry_forward": 1}, "to_date" ) - # all leaves available before cf leave expiry + # case 1: all leaves available before cf leave expiry leave_details = get_leave_details(employee.name, add_days(cf_expiry, -1)) self.assertEqual(leave_details["leave_allocation"][leave_type.name]["remaining_leaves"], 30.0) - # cf leaves expired + # case 2: cf leaves expired leave_details = get_leave_details(employee.name, add_days(cf_expiry, 1)) expected_data = { "total_leaves": 30.0, @@ -1017,6 +1024,79 @@ class TestLeaveApplication(unittest.TestCase): "leaves_pending_approval": 0.0, "remaining_leaves": 15.0, } + + self.assertEqual(leave_details["leave_allocation"][leave_type.name], expected_data) + + @set_holiday_list("Holiday List w/o Weekly Offs", "_Test Company") + def test_leave_details_with_application_across_cf_expiry(self): + """Tests leave details with leave application across cf expiry, such that: + cf leaves are partially expired and partially consumed + """ + employee = get_employee() + leave_type = create_leave_type( + leave_type_name="_Test_CF_leave_expiry", + is_carry_forward=1, + expire_carry_forwarded_leaves_after_days=90, + ).insert() + + leave_alloc = create_carry_forwarded_allocation(employee, leave_type) + cf_expiry = frappe.db.get_value( + "Leave Ledger Entry", {"transaction_name": leave_alloc.name, "is_carry_forward": 1}, "to_date" + ) + + # leave application across cf expiry + application = make_leave_application( + employee.name, + cf_expiry, + add_days(cf_expiry, 3), + leave_type.name, + ) + + leave_details = get_leave_details(employee.name, add_days(cf_expiry, 4)) + expected_data = { + "total_leaves": 30.0, + "expired_leaves": 14.0, + "leaves_taken": 4.0, + "leaves_pending_approval": 0.0, + "remaining_leaves": 12.0, + } + + self.assertEqual(leave_details["leave_allocation"][leave_type.name], expected_data) + + @set_holiday_list("Holiday List w/o Weekly Offs", "_Test Company") + def test_leave_details_with_application_after_cf_expiry(self): + """Tests leave details with leave application after cf expiry, such that: + cf leaves are completely expired and only newly allocated leaves are consumed + """ + employee = get_employee() + leave_type = create_leave_type( + leave_type_name="_Test_CF_leave_expiry", + is_carry_forward=1, + expire_carry_forwarded_leaves_after_days=90, + ).insert() + + leave_alloc = create_carry_forwarded_allocation(employee, leave_type) + cf_expiry = frappe.db.get_value( + "Leave Ledger Entry", {"transaction_name": leave_alloc.name, "is_carry_forward": 1}, "to_date" + ) + + # leave application after cf expiry + application = make_leave_application( + employee.name, + add_days(cf_expiry, 1), + add_days(cf_expiry, 4), + leave_type.name, + ) + + leave_details = get_leave_details(employee.name, add_days(cf_expiry, 4)) + expected_data = { + "total_leaves": 30.0, + "expired_leaves": 15.0, + "leaves_taken": 4.0, + "leaves_pending_approval": 0.0, + "remaining_leaves": 11.0, + } + self.assertEqual(leave_details["leave_allocation"][leave_type.name], expected_data) @set_holiday_list("Salary Slip Test Holiday List", "_Test Company") diff --git a/erpnext/payroll/doctype/salary_slip/test_salary_slip.py b/erpnext/payroll/doctype/salary_slip/test_salary_slip.py index 32d0c7ed08f..ffe38331ee4 100644 --- a/erpnext/payroll/doctype/salary_slip/test_salary_slip.py +++ b/erpnext/payroll/doctype/salary_slip/test_salary_slip.py @@ -1587,9 +1587,9 @@ def setup_test(): frappe.db.set_value("HR Settings", None, "leave_approval_notification_template", None) -def make_holiday_list(list_name=None, from_date=None, to_date=None): - if not (from_date and to_date): - fiscal_year = get_fiscal_year(nowdate(), company=erpnext.get_default_company()) + +def make_holiday_list(list_name=None, from_date=None, to_date=None, add_weekly_offs=True): + fiscal_year = get_fiscal_year(nowdate(), company=erpnext.get_default_company()) name = list_name or "Salary Slip Test Holiday List" frappe.delete_doc_if_exists("Holiday List", name, force=True) @@ -1600,10 +1600,13 @@ def make_holiday_list(list_name=None, from_date=None, to_date=None): "holiday_list_name": name, "from_date": from_date or fiscal_year[1], "to_date": to_date or fiscal_year[2], - "weekly_off": "Sunday", } ).insert() - holiday_list.get_weekly_off_dates() + + if add_weekly_offs: + holiday_list.weekly_off = "Sunday" + holiday_list.get_weekly_off_dates() + holiday_list.save() holiday_list = holiday_list.name From bc12269ef46940473dc07bdb4ddf5f84557ef919 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Thu, 9 Mar 2023 15:10:28 +0530 Subject: [PATCH 6/7] fix: adjust excess cf leaves in new leaves taken if number exceeds cf leaves allocation --- .../leave_application/leave_application.py | 25 ++++++++--- .../test_leave_application.py | 41 +++++++++++++++++++ 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/erpnext/hr/doctype/leave_application/leave_application.py b/erpnext/hr/doctype/leave_application/leave_application.py index de88b3d807c..08bc93760a3 100755 --- a/erpnext/hr/doctype/leave_application/leave_application.py +++ b/erpnext/hr/doctype/leave_application/leave_application.py @@ -937,12 +937,7 @@ def get_remaining_leaves( if cf_expiry and allocation.unused_leaves: # allocation contains both carry forwarded and new leaves - cf_leaves_taken = get_leaves_for_period( - allocation.employee, allocation.leave_type, allocation.from_date, cf_expiry - ) - new_leaves_taken = get_leaves_for_period( - allocation.employee, allocation.leave_type, add_days(cf_expiry, 1), allocation.to_date - ) + new_leaves_taken, cf_leaves_taken = get_new_and_cf_leaves_taken(allocation, cf_expiry) if getdate(date) > getdate(cf_expiry): # carry forwarded leaves have expired @@ -967,6 +962,24 @@ def get_remaining_leaves( return frappe._dict(leave_balance=leave_balance, leave_balance_for_consumption=remaining_leaves) +def get_new_and_cf_leaves_taken(allocation: Dict, cf_expiry: str) -> Tuple[float, float]: + """returns new leaves taken and carry forwarded leaves taken within an allocation period based on cf leave expiry""" + cf_leaves_taken = get_leaves_for_period( + allocation.employee, allocation.leave_type, allocation.from_date, cf_expiry + ) + new_leaves_taken = get_leaves_for_period( + allocation.employee, allocation.leave_type, add_days(cf_expiry, 1), allocation.to_date + ) + + # using abs because leaves taken is a -ve number in the ledger + if abs(cf_leaves_taken) > allocation.unused_leaves: + # adjust the excess leaves in new_leaves_taken + new_leaves_taken += -(abs(cf_leaves_taken) - allocation.unused_leaves) + cf_leaves_taken = -allocation.unused_leaves + + return new_leaves_taken, cf_leaves_taken + + def get_leaves_for_period( employee: str, leave_type: str, from_date: str, to_date: str, skip_expired_leaves: bool = True ) -> float: diff --git a/erpnext/hr/doctype/leave_application/test_leave_application.py b/erpnext/hr/doctype/leave_application/test_leave_application.py index f41fd93e564..231b14f6163 100644 --- a/erpnext/hr/doctype/leave_application/test_leave_application.py +++ b/erpnext/hr/doctype/leave_application/test_leave_application.py @@ -28,6 +28,7 @@ from erpnext.hr.doctype.leave_application.leave_application import ( get_leave_allocation_records, get_leave_balance_on, get_leave_details, + get_new_and_cf_leaves_taken, ) from erpnext.hr.doctype.leave_policy_assignment.leave_policy_assignment import ( create_assignment_for_multiple_employees, @@ -1063,6 +1064,46 @@ class TestLeaveApplication(unittest.TestCase): self.assertEqual(leave_details["leave_allocation"][leave_type.name], expected_data) + @set_holiday_list("Holiday List w/o Weekly Offs", "_Test Company") + def test_leave_details_with_application_across_cf_expiry_2(self): + """Tests the same case as above but with leave days greater than cf leaves allocated""" + employee = get_employee() + leave_type = create_leave_type( + leave_type_name="_Test_CF_leave_expiry", + is_carry_forward=1, + expire_carry_forwarded_leaves_after_days=90, + ).insert() + + leave_alloc = create_carry_forwarded_allocation(employee, leave_type) + cf_expiry = frappe.db.get_value( + "Leave Ledger Entry", {"transaction_name": leave_alloc.name, "is_carry_forward": 1}, "to_date" + ) + + # leave application across cf expiry, 20 days leave + application = make_leave_application( + employee.name, + add_days(cf_expiry, -16), + add_days(cf_expiry, 3), + leave_type.name, + ) + + # 15 cf leaves and 5 new leaves should be consumed + # after adjustment of the actual days breakup (17 and 3) because only 15 cf leaves have been allocated + new_leaves_taken, cf_leaves_taken = get_new_and_cf_leaves_taken(leave_alloc, cf_expiry) + self.assertEqual(new_leaves_taken, -5.0) + self.assertEqual(cf_leaves_taken, -15.0) + + leave_details = get_leave_details(employee.name, add_days(cf_expiry, 4)) + expected_data = { + "total_leaves": 30.0, + "expired_leaves": 0, + "leaves_taken": 20.0, + "leaves_pending_approval": 0.0, + "remaining_leaves": 10.0, + } + + self.assertEqual(leave_details["leave_allocation"][leave_type.name], expected_data) + @set_holiday_list("Holiday List w/o Weekly Offs", "_Test Company") def test_leave_details_with_application_after_cf_expiry(self): """Tests leave details with leave application after cf expiry, such that: From 072c7e913d247bde90c1ba2c6d71894b0fb3a66e Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 10 Mar 2023 11:44:33 +0530 Subject: [PATCH 7/7] chore: fix linter --- erpnext/payroll/doctype/salary_slip/test_salary_slip.py | 1 - 1 file changed, 1 deletion(-) diff --git a/erpnext/payroll/doctype/salary_slip/test_salary_slip.py b/erpnext/payroll/doctype/salary_slip/test_salary_slip.py index ffe38331ee4..993e62bc691 100644 --- a/erpnext/payroll/doctype/salary_slip/test_salary_slip.py +++ b/erpnext/payroll/doctype/salary_slip/test_salary_slip.py @@ -1587,7 +1587,6 @@ def setup_test(): frappe.db.set_value("HR Settings", None, "leave_approval_notification_template", None) - def make_holiday_list(list_name=None, from_date=None, to_date=None, add_weekly_offs=True): fiscal_year = get_fiscal_year(nowdate(), company=erpnext.get_default_company()) name = list_name or "Salary Slip Test Holiday List"