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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
