adamsaghy commented on PR #6352: URL: https://github.com/apache/fineract/pull/6352#issuecomment-5712612022
@mariowise Thank you for the rework. Could you please review the below findings / concerns? 1. **A period can now be flagged principalPaymentGrace while carrying principal**. The flag is stamped on the loan's whole grace window, but the EMI rewrite is guarded by !period.getDueDate().isBefore(sliceFirstDueDate) — so grace periods before the slice get the flag without the interest-only EMI. Probed: re-amortize on 15 Feb, then rate change on 1 Apr → p2 ends up principalPaymentGrace=true with principal 11.13. The invariant "flagged ⇒ zero principal" no longer holds. Currently latent — the stale flags sit outside the slice passed to checkAndAdjustEmiIfNeededOnRelatedRepaymentPeriods, so they only reach the full-model getEmiAdjustment call at :1292, which just feeds a hasLessEmiDifference comparison. Worth either scoping the stamp to the slice as well, or documenting that the flag means "in the loan's grace window", not "interest-only". 2. **The shouldBeAdjusted() change reaches beyond grace loans.** uncountablePeriods also counts overpaid periods, so the threshold now drops for any progressive loan with an installment paid above its EMI. It only ever lowers the bar (more equalization), except when n - uncountable <= 1 closes the gate entirely. I probed one case — no-grace loan, p1 overpaid 42.50, mid-loan rate change — and the schedule is byte-identical before and after. That's one data point, not coverage. CI is still running on this head (build-core pending, E2E shards not started). I'd wait for the 20 E2E shards before merging, since that is the only real guard on this particular change. 3. **liftPrincipalPaymentGrace doesn't durably deliver bharathcgowda's T6 decision.** The flag is cleared during re-amortization, but the next recalculation re-stamps it from getGraceOnPrincipalPayment(), which never changes. Probed re-amort → later rate change: p3 comes back interest-only. While it is not a regression — develop does the same thing, and this commit substantially improves it (develop: p3/p4/p5 deferred, tail 30.82 / 30.82 / 31.69; now: only p3 deferred, tail equalized at 18.86). I wonder whether we could find a way to consistently handle this. -- 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]
