adamsaghy commented on code in PR #6352:
URL: https://github.com/apache/fineract/pull/6352#discussion_r3981987959
##########
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:
<head></head><p dir="ltr" style="caret-color: rgb(0, 0, 0); color: rgb(0, 0,
0); font-style: normal; font-variant-caps: normal; font-weight: 400;
letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: 2; word-spacing: 0px;
-webkit-text-stroke-width: 0px; text-decoration-line: none;
text-decoration-thickness: auto; text-decoration-style: solid;"><em>(anchor
on<span class="Apple-converted-space">
</span><code>period.setPrincipalPaymentGrace(true);</code><span
class="Apple-converted-space"> </span>inside<span
class="Apple-converted-space">
</span><code>applyPrincipalMoratoriumIfRequired</code>)</em></p><p dir="ltr"
style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style: normal;
font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans:
2; text-align: start; text-indent: 0px; text-transform: none; white-space:
normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-
decoration-line: none; text-decoration-thickness: auto; text-decoration-style:
solid;"><strong>Confirmed regression: the suffix-scoped grace marking collapses
the tail of the schedule into an unintended balloon.</strong></p><p dir="ltr"
style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style: normal;
font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans:
2; text-align: start; text-indent: 0px; text-transform: none; white-space:
normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;
text-decoration-line: none; text-decoration-thickness: auto;
text-decoration-style: solid;">The reset was widened to the full model, but
the<span class="Apple-converted-space"> </span><em>marking</em><span
class="Apple-converted-space"> </span>still counts against<span
class="Apple-converted-space"> </span><code>relatedRepaymentPeriods</code><span
class="Apple-converted-space"> </span>— the suffix from<span
class="Apple-converted-space"> </span><code>
calculateFromRepaymentPeriodDueDate</code>, not the loan:</p><div
style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style: normal;
font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans:
2; text-align: start; text-indent: 0px; text-transform: none; white-space:
normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;
text-decoration-line: none; text-decoration-thickness: auto;
text-decoration-style: solid;"><div><div><div><pre>int gracePeriods =
Math.min(graceOnPrincipalPayment,
repaymentPeriods.size());</pre></div><div><div></div></div></div></div></div><p
dir="ltr" style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style:
normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal;
orphans: 2; text-align: start; text-indent: 0px; text-transform: none;
white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width:
0px; text-decoration-line: none; text-decoration-thickness: auto;
text-decoration-sty
le: solid;">When the suffix is no longer than<span
class="Apple-converted-space"> </span><code>graceOnPrincipalPayment</code>,
every remaining period is flagged and set interest-only, and the method returns
at<span class="Apple-converted-space"> </span><code>if (gracePeriods ==
repaymentPeriods.size())</code><span class="Apple-converted-space">
</span>before re-amortizing. The new skip in<span
class="Apple-converted-space"> </span><code>getEmiAdjustment</code><span
class="Apple-converted-space"> </span>then makes every pair skippable, so it
returns the zero-difference fallback,<span class="Apple-converted-space">
</span><code>shouldBeAdjusted()</code><span class="Apple-converted-space">
</span>is false, and<span class="Apple-converted-space">
</span><code>checkAndAdjustEmiIfNeededOnRelatedRepaymentPeriods</code><span
class="Apple-converted-space"> </span>breaks on its first iteration. On develop
that step repaired exactly this state.</p><h4 dir="ltr" style="caret-color: rgb
(0, 0, 0); color: rgb(0, 0, 0); font-style: normal; font-variant-caps: normal;
letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: 2; word-spacing: 0px;
-webkit-text-stroke-width: 0px; text-decoration-line: none;
text-decoration-thickness: auto; text-decoration-style: solid;">Example</h4><p
dir="ltr" style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style:
normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal;
orphans: 2; text-align: start; text-indent: 0px; text-transform: none;
white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width:
0px; text-decoration-line: none; text-decoration-thickness: auto;
text-decoration-style: solid;">Progressive DECLINING_BALANCE, principal 100,
30% p.a., 360/30, N=8 monthly,<span class="Apple-converted-space">
</span><code>graceOnPrincipalPayment=3</code><span
class="Apple-converted-space"> </span>— a valid configuration, si
nce<span class="Apple-converted-space">
</span><code>validateRepaymentPeriodWithGraceSettings</code><span
class="Apple-converted-space"> </span>only rejects<span
class="Apple-converted-space"> </span><code>grace >= N</code>.</p><ol
dir="ltr" start="1" style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);
font-style: normal; font-variant-caps: normal; font-weight: 400;
letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: 2; word-spacing: 0px;
-webkit-text-stroke-width: 0px; text-decoration-line: none;
text-decoration-thickness: auto; text-decoration-style:
solid;"><li><code>addDisbursement(model, 1 January 2024,
100)</code></li><li><code>changeInterestRate(model, 1 July 2024,
24)</code><span class="Apple-converted-space"> </span>— effective date is<span
class="Apple-converted-space">
</span><code>submittedOnDate.minusDays(1)</code><span
class="Apple-converted-space"> </span>= 30 June 2024, so the suffi
x is p6/p7/p8 and<span class="Apple-converted-space"> </span><code>min(3, 3)
== size</code>.</li></ol><div dir="ltr" style="caret-color: rgb(0, 0, 0);
color: rgb(0, 0, 0); font-style: normal; font-variant-caps: normal;
font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start;
text-indent: 0px; text-transform: none; white-space: normal; widows: 2;
word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration-line: none;
text-decoration-thickness: auto; text-decoration-style: solid;"><div>
period | develop | this PR
-- | -- | --
p6 | emi 21.42, principal 19.89 | emi 1.53, principal 0.00
p7 | emi 21.42, principal 20.59 | emi 0.83, principal 0.00
p8 | emi 21.42, principal 21.00 | emi 61.90, principal 60.27
</div></div><p dir="ltr" style="caret-color: rgb(0, 0, 0); color: rgb(0, 0,
0); font-style: normal; font-variant-caps: normal; font-weight: 400;
letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: 2; word-spacing: 0px;
-webkit-text-stroke-width: 0px; text-decoration-line: none;
text-decoration-thickness: auto; text-decoration-style: solid;">Total principal
still reconciles to 100.00, but the customer's last two installments drop to
interest-only and a 61.90 balloon appears out of nowhere. Reachable in
production from<span class="Apple-converted-space">
</span><code>ProgressiveLoanScheduleGenerator:273</code><span
class="Apple-converted-space"> </span>(interest-rate term variation during
schedule regeneration) and<span class="Apple-converted-space">
</span><code>AdvancedPaymentScheduleTransactionProcessor:531</code>.</p><p
dir="ltr" style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style:
normal; f
ont-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans:
2; text-align: start; text-indent: 0px; text-transform: none; white-space:
normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;
text-decoration-line: none; text-decoration-thickness: auto;
text-decoration-style: solid;">This is the case my earlier second-tranche probe
did not reach — there I concluded "not a regression, but the new flag makes it
sticky". That still holds for a suffix longer than the grace count; it does not
hold here.</p><h4 dir="ltr" style="caret-color: rgb(0, 0, 0); color: rgb(0, 0,
0); font-style: normal; font-variant-caps: normal; letter-spacing: normal;
orphans: 2; text-align: start; text-indent: 0px; text-transform: none;
white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width:
0px; text-decoration-line: none; text-decoration-thickness: auto;
text-decoration-style: solid;">Probable fix</h4><p dir="ltr"
style="caret-color: rgb(0, 0, 0); color: rg
b(0, 0, 0); font-style: normal; font-variant-caps: normal; font-weight: 400;
letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: 2; word-spacing: 0px;
-webkit-text-stroke-width: 0px; text-decoration-line: none;
text-decoration-thickness: auto; text-decoration-style: solid;">Anchor the
marking to the loan, the same way the reset now is, and derive the amortizing
set from the flag instead of from a<span class="Apple-converted-space">
</span><code>subList</code><span class="Apple-converted-space">
</span>offset:</p><div style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);
font-style: normal; font-variant-caps: normal; font-weight: 400;
letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: 2; word-spacing: 0px;
-webkit-text-stroke-width: 0px; text-decoration-line: none;
text-decoration-thickness: auto; text-decoration-style: solid;"><div><div>
<div><pre>private void
applyPrincipalMoratoriumIfRequired(List<RepaymentPeriod> repaymentPeriods,
ProgressiveLoanInterestScheduleModel scheduleModel) {
if (repaymentPeriods.isEmpty()) {
return;
}
final List<RepaymentPeriod> allPeriods =
scheduleModel.repaymentPeriods();
allPeriods.forEach(rp -> rp.setPrincipalPaymentGrace(false));
final Integer graceOnPrincipalPayment =
scheduleModel.loanProductRelatedDetail().getGraceOnPrincipalPayment();
if (graceOnPrincipalPayment == null || graceOnPrincipalPayment <= 0) {
return;
}
final int gracePeriods = Math.min(graceOnPrincipalPayment,
allPeriods.size());
final LocalDate sliceFirstDueDate =
repaymentPeriods.getFirst().getDueDate();
allPeriods.subList(0, gracePeriods).forEach(period -> {
period.setPrincipalPaymentGrace(true);
// only rewrite EMI for periods inside the slice being recalculated
if (!period.getDueDate().isBefore(sliceFirstDueDate)) {
final Money interestOnlyEmi = period.getDueInterest();
period.setEmi(interestOnlyEmi);
period.setOriginalEmi(interestOnlyEmi);
}
});
final List<RepaymentPeriod> amortizingPeriods =
repaymentPeriods.stream()
.filter(rp -> !rp.isPrincipalPaymentGrace()).toList();
if (amortizingPeriods.isEmpty()) {
return;
}
calculateOutstandingBalance(scheduleModel);
calculateEMIOnActualModel(amortizingPeriods, scheduleModel);
}</pre></div><div><div></div></div></div></div></div><p dir="ltr"
style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style: normal;
font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans:
2; text-align: start; text-indent: 0px; text-transform: none; white-space:
normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;
text-decoration-line: none; text-decoration-thickness: auto;
text-decoration-style: solid;">Two things this buys us:</p><ul dir="ltr"
style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style: normal;
font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans:
2; text-align: start; text-indent: 0px; text-transform: none; white-space:
normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;
text-decoration-line: none; text-decoration-thickness: auto;
text-decoration-style: solid;"><li>Grace lands on the loan's actual grace
window on every recalculation, which also clears up the pre-exis
ting mis-marking from the second-tranche probe.</li><li>The "whole suffix is
grace" state becomes structurally unreachable. The suffix always contains the
last repayment period, and<span class="Apple-converted-space">
</span><code>grace >= N</code><span class="Apple-converted-space"> </span>is
rejected at both product and loan level, so<span class="Apple-converted-space">
</span><code>amortizingPeriods</code><span class="Apple-converted-space">
</span>is never empty in practice.</li></ul><p dir="ltr" style="caret-color:
rgb(0, 0, 0); color: rgb(0, 0, 0); font-style: normal; font-variant-caps:
normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align:
start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2;
word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration-line: none;
text-decoration-thickness: auto; text-decoration-style: solid;">Note the date
comparison rather than<span class="Apple-converted-space"> </span><code>repayme
ntPeriods.contains(period)</code>:<span class="Apple-converted-space">
</span><code>RepaymentPeriod</code><span class="Apple-converted-space">
</span>carries<span class="Apple-converted-space">
</span><code>@EqualsAndHashCode</code>, so<span class="Apple-converted-space">
</span><code>contains</code><span class="Apple-converted-space"> </span>is a
deep value comparison, not identity — it could match the wrong period and is
O(n²) over the interest-period graph. Filtering on<span
class="Apple-converted-space"> </span><code>dueDate</code><span
class="Apple-converted-space"> </span>mirrors how<span
class="Apple-converted-space">
</span><code>getRelatedRepaymentPeriods</code><span
class="Apple-converted-space"> </span>builds the slice in the first
place.</p><h4 dir="ltr" style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);
font-style: normal; font-variant-caps: normal; letter-spacing: normal; orphans:
2; text-align: start; text-indent: 0px; text-transform: none; white-space
: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;
text-decoration-line: none; text-decoration-thickness: auto;
text-decoration-style: solid;">Test worth adding</h4><p dir="ltr"
style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style: normal;
font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans:
2; text-align: start; text-indent: 0px; text-transform: none; white-space:
normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;
text-decoration-line: none; text-decoration-thickness: auto;
text-decoration-style: solid;">A unit test for a recalculation whose related
suffix is entirely inside the grace count — the N-2 and N-1 tests both start
from a full-schedule recalculation, so neither reaches this path. Asserting
equal installments across p6/p7/p8 after the rate change above would pin it.</p>
--
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]
