adamsaghy commented on code in PR #6352:
URL: https://github.com/apache/fineract/pull/6352#discussion_r3933123607
##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/ProgressiveEMICalculator.java:
##########
@@ -1743,6 +1691,7 @@ private void
applyPrincipalMoratoriumIfRequired(List<RepaymentPeriod> repaymentP
if (repaymentPeriods.isEmpty()) {
return;
}
+ repaymentPeriods.forEach(rp -> rp.setPrincipalPaymentGrace(false));
Review Comment:
This is not really needed.
##########
fineract-progressive-loan/src/test/java/org/apache/fineract/portfolio/loanproduct/calc/ProgressiveEMICalculatorTest.java:
##########
@@ -5316,6 +5313,153 @@ public void test_principalGraceForProgressiveSchedule()
{
checkPeriod(interestSchedule, 5, 25.35, 0.15, 25.20, 0.0, false);
}
+ /**
Review Comment:
Stale test javadoc. test_principalGrace_nMinus1_shouldProduceBulletLoan says
"This assertion therefore FAILS on 1.15.0 and documents the defect." It reads
like a deliberately-failing test; it passes with the fix. Same for the "Actual
on the current engine…" paragraph. Worth rewording to describe the expected
post-fix behaviour.
##########
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:
EmiAdjustment denominator is now inconsistent. adjustment() divides by
numberOfRelatedPeriods() - uncountablePeriods, and shouldBeAdjusted() uses half
of numberOfRelatedPeriods() — both count grace periods that the PR now excludes
from the update loop. getUncountablePeriods (:2036) only filters on paid
amount. Net effect is systematic under-correction per iteration, with the loop
capped at 3. Counting principal-grace periods as uncountable would keep it
consistent.
##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/ProgressiveEMICalculator.java:
##########
@@ -1753,6 +1702,7 @@ private void
applyPrincipalMoratoriumIfRequired(List<RepaymentPeriod> repaymentP
Money interestOnlyEmi = period.getDueInterest();
period.setEmi(interestOnlyEmi);
period.setOriginalEmi(interestOnlyEmi);
+ period.setPrincipalPaymentGrace(true);
Review Comment:
Related: the reset has narrower scope than the marking
repaymentPeriods.forEach(rp -> rp.setPrincipalPaymentGrace(false));
ProgressiveEMICalculator.java:1694 operates on relatedRepaymentPeriods,
which is the suffix from calculateFromRepaymentPeriodDueDate — not the whole
model. Compare applyInterestMoratoriumIfRequired, which resets across
scheduleModel.repaymentPeriods(). Two consequences:
A flag set during initial generation on periods 0..k can never be cleared by
a later mid-loan recalculation.
A mid-loan recalculation marks the first N periods of the suffix as grace. I
confirmed with a second-tranche probe (grace=3, disburse 100 then 50 on
2024-04-15) that periods 3–5 end up with zero principal — but that also happens
on develop, so the mis-marking is pre-existing and the PR actually cleans up
one artifact of it (period 5 no longer lands at EMI 0). Not a regression, but
the new flag makes it sticky, so it's worth resetting over the full model here.
##########
fineract-progressive-loan/src/test/java/org/apache/fineract/portfolio/loanproduct/calc/ProgressiveEMICalculatorTest.java:
##########
@@ -5316,6 +5313,153 @@ public void test_principalGraceForProgressiveSchedule()
{
checkPeriod(interestSchedule, 5, 25.35, 0.15, 25.20, 0.0, false);
}
+ /**
+ * Control case for the bullet-loan boundary defect.
+ *
+ * With N=8 repayments and graceOnPrincipalPayment = N-2 = 6, the schedule
behaves correctly: installments 1..6 are
+ * interest-only and the principal is amortized across the final two
installments (7 and 8). This test passes on the
+ * current engine and is here to demonstrate that the defect exercised by
+ * {@link #test_principalGrace_nMinus1_shouldProduceBulletLoan()} is
specific to the grace = N-1 boundary.
+ */
+ @Test
+ public void test_principalGrace_nMinus2_deferralWorks() {
Review Comment:
Coverage is unit-only. The scenario is a user-visible loan-schedule outcome;
an integration/e2e case for the bullet product would guard the full
disbursement→schedule path. Optional, but the re-amortization regression above
is exactly the kind of thing unit tests at this level missed.
Can we have an E2E test case for this?
##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/ProgressiveEMICalculator.java:
##########
@@ -1743,6 +1691,7 @@ private void
applyPrincipalMoratoriumIfRequired(List<RepaymentPeriod> repaymentP
if (repaymentPeriods.isEmpty()) {
return;
}
+ repaymentPeriods.forEach(rp -> rp.setPrincipalPaymentGrace(false));
Review Comment:
the flag goes stale after re-amortization
principalPaymentGrace is cleared in exactly one place —
applyPrincipalMoratoriumIfRequired — which is only reached from
calculateEMIValueAndRateFactors*. The re-amortization and re-age paths never
call it, but they do overwrite the grace periods' EMI via
calculateEMIOnActualModel, and then call
checkAndAdjustEmiIfNeededOnRelatedRepaymentPeriods
(ProgressiveEMICalculator.java:880, :938, :1092). Those periods now carry
principal but are still flagged, so they're skipped by the equalization.
Probe: N=8, 30% p.a., grace on principal = 3, disburse 100 on 2024-01-01,
re-amortize on 2024-02-15.
idx | baseline EMI | with PR
-- | -- | --
1 | 16.13 | 15.75
2 | 16.13 | 15.75
3–6 | 16.13 | 16.29
7 | 16.15 | 16.34
Total principal still reconciles to 100.00, so no money is lost — but a
re-amortized schedule that used to be a clean equal-installment schedule now
has two odd installments. The same code path applies to
updateModelRepaymentPeriodsDuringReAmortizationWithEqualInterestSplit and
updateModelRepaymentPeriodsDuringReAge; I only measured the plain
re-amortization case.
We should decide whether reamortization or reacting overrides grace periods,
or if grace periods take precedence and no principal is set for them in any
situation. Please share your thoughts. @bharathgowda @bharathgowda Please share
your thoughts.
--
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]
