fix: Leave Encashment calculations

[ruchamahabal]

Problem:

The Leave Balance calculation in Leave Encashment is incorrect:

get_leave_allocation doesn't fetch allocation.from_date:

erpnext/erpnext/hr/doctype/leave_encashment/leave_encashment.py

Lines 128 to 136 in 9fb7b49

	def get_leave_allocation(self):
	leave_allocation = frappe.db.sql(
	"""select name, to_date, total_leaves_allocated, carry_forwarded_leaves_count from `tabLeave Allocation` where '{0}'
	between from_date and to_date and docstatus=1 and leave_type='{1}'
	and employee= '{2}'""".format(
	self.encashment_date or getdate(nowdate()), self.leave_type, self.employee
	),
	as_dict=1,
	) # nosec

but from_date is being accessed in get_unused_leaves for getting encashable leave balance which is None:

erpnext/erpnext/hr/doctype/leave_encashment/leave_encashment.py

Lines 107 to 111 in 9fb7b49

	self.leave_balance = (
	allocation.total_leaves_allocated
	- allocation.carry_forwarded_leaves_count
	- get_unused_leaves(self.employee, self.leave_type, allocation.from_date, self.encashment_date)
	)

Also, the above formula is not correct for calculating leave balance. It's subtracting unused leaves from allocated leaves. Instead, it should remove used leaves from leaves allocated to get the encashable balance. So if 10 leaves are allocated 6 leaves are used, balance should be 4 but this does 10 - 4 = 6. 🤷🏻‍♀️

This feature seems to be broken since the time it was introduced. It also has a test but the test was incorrect causing no failures.

Fix:

Fetch from_date in the query

Change the leave balance calculation formula to:

self.leave_balance = (
	allocation.total_leaves_allocated
	- allocation.carry_forwarded_leaves_count
	# adding this because the function returns a -ve number
	+ get_leaves_for_period(
		self.employee, self.leave_type, allocation.from_date, self.encashment_date
	)
)

Subtract the leaves taken from leaves allocated to get the balance leaves that can be encashed.

Closes #28020, Closes #23448, Closes #24706

[ruchamahabal]

Also, I don't know if this is how it should be. The Encashment threshold days setting seems to be adding some functionality that should work exactly opposite to what it's doing right now maybe?

As per docs and functionality:

Which organization has a threshold above which you can encash? Like if you have 15 leaves, you cannot encash 5 but you can encash 10?

Expected (Maybe):

Shouldn't it be like; you can only encash up to 5 leaves because 5 is the threshold. Currently, it's very confusing for the user.
Maybe we should have minimum and maximum thresholds as separate fields. I am not sure about how HRs use this.

cc: @nabinhait

[codecov]

Codecov Report

Merging #31062 (5fe1616) into develop (3019f93) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop   #31062      +/-   ##
===========================================
- Coverage    63.04%   62.94%   -0.10%     
===========================================
  Files          986      986              
  Lines        67343    67345       +2     
===========================================
- Hits         42457    42392      -65     
- Misses       24886    24953      +67     

Impacted Files	Coverage Δ
...xt/hr/doctype/leave_encashment/leave_encashment.py	78.82% <100.00%> (+0.51%)	⬆️
...unts/report/purchase_register/purchase_register.py	34.21% <0.00%> (-44.08%)	⬇️
...ext/accounts/report/balance_sheet/balance_sheet.py	36.36% <0.00%> (-21.82%)	⬇️
...e_purchase_register/item_wise_purchase_register.py	57.42% <0.00%> (-20.80%)	⬇️
...em_wise_sales_register/item_wise_sales_register.py	51.13% <0.00%> (-12.22%)	⬇️
...saction/incorrect_balance_qty_after_transaction.py	88.37% <0.00%> (-9.31%)	⬇️
...t/accounts/report/sales_register/sales_register.py	70.76% <0.00%> (-7.02%)	⬇️
...urity_shortfall/process_loan_security_shortfall.py	93.75% <0.00%> (-6.25%)	⬇️
erpnext/hr/doctype/job_offer/job_offer.py	82.92% <0.00%> (-4.88%)	⬇️
...it_and_loss_statement/profit_and_loss_statement.py	71.42% <0.00%> (-3.90%)	⬇️
... and 29 more

[ruchamahabal]

Expected (Maybe):

Shouldn't it be like; you can only encash up to 5 leaves because 5 is the threshold. Currently, it's very confusing for the user.
Maybe we should have minimum and maximum thresholds as separate fields. I am not sure about how HRs use this.

Will look into this in a separate PR.