adamsaghy commented on code in PR #6352:
URL: https://github.com/apache/fineract/pull/6352#discussion_r3981968020
##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/ProgressiveEMICalculator.java:
##########
@@ -1835,6 +1785,9 @@ public EmiAdjustment getEmiAdjustment(final
List<RepaymentPeriod> repaymentPerio
for (int idx = repaymentPeriods.size() - 1; idx > 0; --idx) {
RepaymentPeriod lastPeriod = repaymentPeriods.get(idx);
RepaymentPeriod penultimatePeriod = repaymentPeriods.get(idx - 1);
+ if (lastPeriod.isPrincipalPaymentGrace() ||
penultimatePeriod.isPrincipalPaymentGrace()) {
Review Comment:
Half of this is fixed — getUncountablePeriods now counts principal-grace
periods, so adjustment() divides by the amortizing count.
The other half is still open: EmiAdjustment.shouldBeAdjusted() continues to
size its trigger threshold from numberOfRelatedPeriods(), grace periods
included, so the two halves of the same record now disagree about how many
periods are adjustable.
// EmiAdjustment.java:32
double lowerHalfOfRelatedPeriods = Math.floor(numberOfRelatedPeriods() /
2.0);
For N=8 with graceOnPrincipalPayment=6 only 2 periods amortize, but the gate
is sized on floor(8/2) = 4 instead of floor(2/2) = 1 — a 4x inflated bar. An
EMI gap that adjustment() was just taught to split correctly can still be left
unadjusted because shouldBeAdjusted() never fires. Most visible with
installmentAmountInMultiplesOf set, where rounding is what creates the gap in
the first place.
Suggested fix:
public boolean shouldBeAdjusted() {
double lowerHalfOfRelatedPeriods = Math.floor((numberOfRelatedPeriods()
- uncountablePeriods) / 2.0);
return lowerHalfOfRelatedPeriods > 0.0 && !emiDifference.isZero() &&
emiDifference.abs() //
.multipliedBy(100) //
.isGreaterThan(originalEmi.copy(lowerHalfOfRelatedPeriods)); //
}
Note the side effect, which I think is the behaviour we want: with a single
adjustable period floor(1/2) = 0 closes the gate entirely. A lone period has
nothing to be equalized against — which is exactly the N-1 bullet case this PR
is fixing. If you agree, that also makes the new skip in getEmiAdjustment
largely redundant, so it is worth checking whether both are still needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
