You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "mariowise (via GitHub)" <gi...@apache.org> on 2026/08/28 20:01:25 UTC

[PR] FINERACT-2789: Balloon loan not working as expected [fineract]

mariowise opened a new pull request, #6352:
URL: https://github.com/apache/fineract/pull/6352

   # Summary
   
   When creating a loan product that uses progressive schedule, declining balance interest method and has a grace period on principal. The expected behavior is that during the grace period, no principal is applied to the grace period tranches. This currently works in the system for grace periods within the range `{1..N-2}`. Where N is the number of repayments.
   
   When you setup `N-1` as the grace period, the system behaves with the bug, and all tranches have principal on them. The "Bullet/Balloon" loan concept (where the entire principal is repaid fully at the last repayment) is not possible to achieve.
   
   # Root cause analysis
   
   The code lives in `ProgressiveEMICalculator.java`. There's a method called `applyPrincipalMoratoriumIfRequired(...)` that does two things:
   
   1. Marks the first N grace installments as interest-only (correct so far). 
   2. Then it re-runs the amortization math on only the leftover installments — the ones that still owe principal. It does this by slicing the list: `subList(gracePeriods, size)`.
   
   ## Where it breaks
   
   After the moratorium correctly parks the full principal onto that single last installment, a later normalization step kicks in (getEmiAdjustment, checkAndAdjustEmiIfNeededOnRelatedRepaymentPeriods, calculateLastUnpaidRepaymentPeriodEMI). This step's job is normally to fine-tune the last installment so the numbers reconcile perfectly (rounding, etc.).
   
   But when there's only one non-grace period, this step re-does the amortization from scratch and undoes the moratorium's work. It ends up spreading the principal back across installments.
   
   ## Why N-2 works but N-1 doesn't?
   
   When grace is N - 2, the leftover slice has two installments to work with. With two or more periods, the equal-installment solver has "room" to land on a valid split, and the moratorium survives the cleanup step. It's only the single-remaining-period edge case (N - 1) that trips the bug — which is precisely why the reproduction shows "N-2 works, N-1 breaks." The defect lives entirely at that one-period boundary.
   
   # Steps to reproduce
   
   Please see the jira issue at [FINERACT-2789](https://issues.apache.org/jira/browse/FINERACT-2789).
   
   # Proposed solution
   
   Adding a new `principalPaymentGrace` flag that sits next to the existing `interestPaymentGrace` to mark the entries as part of principal grace/moratorium. And avoiding the normalizations steps from breaking the 0 principal rule setup on the grace period setting.
   
   # How is this change tested?
   
   3 new unit tests have been added: a control one to verify N-2 works as it is today (grace period on principal), and 2 more to verify N-1 with two loan lengths. Also, the following command is passing running the whole suite of unit tests.
   
   ```bash
   ./gradlew test \
     -x :twofactor-tests:test \
     -x :oauth2-tests:test \
     -x :integration-tests:test \
     -x :fineract-client:test \
     -x :fineract-client-feign:test \
     -x :fineract-e2e-tests-core:test \
     -x :fineract-e2e-tests-runner:test \
     -x buildJavaSdk
   ```
   
   It has also been manually tested seeing the same correct behavior between N-2 and N-1 grace on principal.
   
   # Recommended reviewers
   
   - adamsaghy@gmail.com
   - airajena0@gmail.com
   - janez@outlook.hu
   - riaskay3@gmail.com
   - soma.soros@dpc.hu
   


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
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. Thanks.
   
   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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "mariowise (via GitHub)" <gi...@apache.org>.
mariowise commented on PR #6352:
URL: https://github.com/apache/fineract/pull/6352#issuecomment-5897417802

   Hi @adamsaghy, I've done the following to address your comments:
   
   1. `principalPaymentGrace` is set only when the period is rewritten to interest-only inside the slice being recalculated, or when a period before that slice already has zero principal. 
   2. `shouldBeAdjusted()` now subtracts principal-grace periods only. Overpaid installments remain in `uncountablePeriods`, so they still stay out of the split, but they no longer lower the threshold or close the gate. Grace still lowers it.
   3. Re-amortization and re-aging now record `PRINCIPAL_GRACE_LIFTED` on the schedule model, and that bit is copied with the model. The next recalculation does not rebuild grace from `graceOnPrincipalPayment`.
   
   Also rebased onto current `develop`. 
   
   


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6352:
URL: https://github.com/apache/fineract/pull/6352#issuecomment-5623079175

   @mariowise Kindly review my comments.


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "jdailey (via GitHub)" <gi...@apache.org>.
jdailey commented on code in PR #6352:
URL: https://github.com/apache/fineract/pull/6352#discussion_r3953352250


##########
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:
   Agreed. E2E test is 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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "mariowise (via GitHub)" <gi...@apache.org>.
mariowise commented on code in PR #6352:
URL: https://github.com/apache/fineract/pull/6352#discussion_r4030461973


##########
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:
   Agreed on the gate, `shouldBeAdjusted()` now uses `numberOfRelatedPeriods() - uncountablePeriods`. For N-1 that is floor(1 / 2) = 0, so a lone balloon period is not equalized. I kept the skip though, without it there is still could be a case where a pair (amortizing, grace) is evaluated once later installments are already paid. The skip makes sure that pair is ineligible.



-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "mariowise (via GitHub)" <gi...@apache.org>.
mariowise commented on code in PR #6352:
URL: https://github.com/apache/fineract/pull/6352#discussion_r4030474773


##########
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:
   Implemented as suggested: grace is marked on the loan window, not the releted suffix.



-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6352:
URL: https://github.com/apache/fineract/pull/6352#issuecomment-6015290171

   @mariowise Please run:
   `./gradlew spotlessApply spotbugsMain spotbugsTest checkstyleMain checkstyleTest`
   `./gradlew --no-daemon build -x test -x cucumber -x doc`


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #6352:
URL: https://github.com/apache/fineract/pull/6352#discussion_r3934024580


##########
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:
   @mariowise Lets lift the flag if period is overridden by reamortization / reage



-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6352:
URL: https://github.com/apache/fineract/pull/6352#issuecomment-5992725122

   @mariowise Thank you for fixing most of the issues. Would you mind reviewing the last 3+1 things I found?
   
   **0. Please squash your commits**
   Since all the commits are for the very same functionality, we should have only 1 commit with all the changes.
   
   **1. Grace checks spread across five places**
   
   Where
   
   [ProgressiveEMICalculator.java:1390](https://github.com/apache/fineract/blob/5d97f29a028d16f5f2bab1ff17a94531da65e0c0/fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/ProgressiveEMICalculator.java#L1390), [:1404](https://github.com/apache/fineract/blob/5d97f29a028d16f5f2bab1ff17a94531da65e0c0/fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/ProgressiveEMICalculator.java#L1404), [:1408](https://github.com/apache/fineract/blob/5d97f29a028d16f5f2bab1ff17a94531da65e0c0/fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/ProgressiveEMICalculator.java#L1408): in checkAndAdjustEmiIfNeededOnRelatedRepaymentPeriods
   [:1912](https://github.com/apache/fineract/blob/5d97f29a028d16f5f2bab1ff17a94531da65e0c0/fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/ProgressiveEMICalculator.java#L1912): getEmiAdjustment
   getUncountablePeriods in the same file
   [EmiAdjustment.java:33](https://github.com/apache/fineract/blob/5d97f29a028d16f5f2bab1ff17a94531da65e0c0/fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/data/EmiAdjustment.java#L33): shouldBeAdjusted
   
   **Problem**
   
   The root cause is that getEmiAdjustment compares the last amortizing EMI with an interest-only grace EMI. The PR fixes it by adding an isPrincipalPaymentGrace() check at every step of the shared EMI-normalization code. Each check is reasonable, but together they make the code fragile: the next change to that code has to remember all of them.
   
   **Simpler alternative:** in calculateEMIValueAndRateFactorsForDecliningBalanceInterestMethod, pass only the non-grace periods to checkAndAdjustEmiIfNeededOnRelatedRepaymentPeriods:
   
   final List<RepaymentPeriod> amortizingRelatedPeriods = relatedRepaymentPeriods.stream()
           .filter(rp -> !rp.isPrincipalPaymentGrace()).toList();
   checkAndAdjustEmiIfNeededOnRelatedRepaymentPeriods(scheduleModel, amortizingRelatedPeriods);
   
   That makes the getUncountablePeriods change, the threshold change and the extra skip in the pairing loop unnecessary. Only the full-model getEmiAdjustment(newScheduleModel.repaymentPeriods()) call would still need to skip grace pairs. Lines 1390 and 1404 would also still need the check, because they loop over all of newScheduleModel and filter only by date.
   
   **2. clearPrincipalGraceLift() is never called**
   
   Where: [ProgressiveLoanInterestScheduleModel.java:156](https://github.com/apache/fineract/blob/5d97f29a028d16f5f2bab1ff17a94531da65e0c0/fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/data/ProgressiveLoanInterestScheduleModel.java#L156)
   
   /**
    * ... {@link #clearPrincipalGraceLift()} is the hook for a later mid-term grace.
    */
   public void clearPrincipalGraceLift() {
       modifiers.put(PRINCIPAL_GRACE_LIFTED, false);
   }
   
   **Problem:** nothing in the codebase calls it, including tests. The Javadoc suggests grace can be restored after a lift, but no code path does that (see #1). It is an API with no user, and it misleads readers.
   
   **Suggested resolution:** remove it, or connect it to the operation that is meant to restore grace and add a test.
   
   **3. shouldBeAdjusted recounts grace periods on every call, and its comment is misleading**
   
   Where: [EmiAdjustment.java:32-33](https://github.com/apache/fineract/blob/5d97f29a028d16f5f2bab1ff17a94531da65e0c0/fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/data/EmiAdjustment.java#L32)
   
   // Overpaid installments stay in uncountablePeriods for adjustment(). They must not move this gate.
   long gracePeriods = relatedRepaymentPeriods.stream().filter(RepaymentPeriod::isPrincipalPaymentGrace).count();
   
   **Problems**
   
   **Repeated work:** the adjustment loop in checkAndAdjust... runs up to 3 times, and each pass counts the grace periods again. getEmiAdjustment already walks the same list in getUncountablePeriods.
   Misleading comment: it talks about overpaid installments, but the next line subtracts grace periods. It explains why overpaid periods are left out, not what the line does, so a reader has to work out the intent.
   
   **Suggested resolution:** add a gracePeriods field to the EmiAdjustment record, set it in getEmiAdjustment, and reword the comment, for example: "Grace periods are excluded from the threshold; overpaid periods are intentionally not (they only affect the divisor in adjustment())."


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
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 &gt;= 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&lt;RepaymentPeriod&gt; repaymentPeriods,
           ProgressiveLoanInterestScheduleModel scheduleModel) {
       if (repaymentPeriods.isEmpty()) {
           return;
       }
       final List&lt;RepaymentPeriod&gt; allPeriods = scheduleModel.repaymentPeriods();
       allPeriods.forEach(rp -&gt; rp.setPrincipalPaymentGrace(false));
       final Integer graceOnPrincipalPayment = scheduleModel.loanProductRelatedDetail().getGraceOnPrincipalPayment();
       if (graceOnPrincipalPayment == null || graceOnPrincipalPayment &lt;= 0) {
           return;
       }
       final int gracePeriods = Math.min(graceOnPrincipalPayment, allPeriods.size());
       final LocalDate sliceFirstDueDate = repaymentPeriods.getFirst().getDueDate();
       allPeriods.subList(0, gracePeriods).forEach(period -&gt; {
           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&lt;RepaymentPeriod&gt; amortizingPeriods = repaymentPeriods.stream()
               .filter(rp -&gt; !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 &gt;= 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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "bharathcgowda (via GitHub)" <gi...@apache.org>.
bharathcgowda commented on code in PR #6352:
URL: https://github.com/apache/fineract/pull/6352#discussion_r3933948776


##########
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:
   @adamsaghy Re-aging and Re-amortization are typically applied to overdue loan accounts when the customer is unable to make the scheduled payments on time. Therefore, I believe that Re-aging/Re-amortization should take precedence over any existing grace period and result in a restructured repayment schedule based on the selected re-aging or re-amortization approach.
   
   The grace period option is not usually provided to the customer after Re-aging/Re-amortization process. However, the user should still be able to apply the **Mid-Term Grace** feature separately, if required, after the loan has been re-aged or re-amortized.
   



-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] FINERACT-2789: Balloon loan not working as expected [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #6352:
URL: https://github.com/apache/fineract/pull/6352


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org