Merge pull request #34388 from ruchamahabal/fix-balance-edge-cases-v13

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,
 				}
 			),
 		)
@@ -933,26 +935,51 @@ 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
+		new_leaves_taken, cf_leaves_taken = get_new_and_cf_leaves_taken(allocation, cf_expiry)
+
 		if getdate(date) > getdate(cf_expiry):
-			# carry forwarded leave expiry date passed
+			# carry forwarded leaves have expired
 			cf_leaves = remaining_cf_leaves = 0
 		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)
-		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)
 
 
+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:

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,
@@ -96,6 +97,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 +994,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 +1012,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 +1025,119 @@ 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_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:
+		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")
@@ -1043,6 +1164,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)
 

erpnext/payroll/doctype/salary_slip/test_salary_slip.py

@@ -1587,9 +1587,8 @@ 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 +1599,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