adamsaghy commented on PR #6339: URL: https://github.com/apache/fineract/pull/6339#issuecomment-5478072631
> The rewrite of `computeActualAmortizations` looks like the actual root cause rather than a patch - pricing each period at what that period actually bills, instead of dividing the whole payment by the arrival day's flat instalment, is what makes the old `settlePositionIfFullyPaid`/`totalBilledIfAnyMoneyPaid`/`consumeExpectedAmortization` hack unnecessary. Regression coverage is solid too (`testOverpaidLoanProjectsNothingRatherThanUnEarningFee` reproduces the reported case, plus undo/replay symmetry tests). > > Two correctness gaps found on closer inspection though, both reachable, plus one unexplained regression in the e2e expectations: > > **1. `findClosingPeriod` breaks when rate segments make the balance sequence non-monotonic.** It does a forward scan for the first period whose balance is `<= 0`, but `balances` isn't guaranteed to be non-increasing - a rate-segment boundary resets `prevBalance` to a value frozen when the segment was created. A backdated large repayment that zeroes the balance early, followed by a later segment boundary that "resurrects" a stale higher balance, makes this method return the wrong index and dump the entire rounding residual on the wrong period while excluding everything from the real closing period onward from the settle. This is the same class of bug found on #6281 - the fix should scan backwards like the existing `lastBillingPeriod(tail)` does in the same file, for symmetry. > > **2. `computeActualAmortizations` prices periods at the uncapped `expectedPaymentForDay`, not at what the plan actually billed.** The plan walk deliberately caps billed payments at `.min(grown)` when a segment or repayment moves the balance out from under a solved instalment - but the consumption walk reads the uncapped value. Wherever that cap binds, a payment covering everything the plan actually bills still won't fully consume the plan, reintroducing the exact under-earning bug this PR exists to fix, just now scoped to capped periods. The plan walk already computes `billedPayments` - it should be threaded through to `computeActualAmortizations` instead of recomputing from the uncapped value. > > Also, in `WorkingCapitalPeriodPaymentRate.feature`, two scenarios that previously closed cleanly on their final period now gain an extra one-line "stub" period billing 5-6 cents (39→40 periods, 40→41 periods). That's a user-visible degradation on a customer-facing schedule and it's not explained anywhere in the PR, unlike the other feature-file comments which are unusually thorough for this PR. Could you walk through what's driving that, and whether it's related to finding #1? > > Recommendation: CHANGES_REQUESTED @mariiaKraievska Would you mind to review Arnold's concerns, please? -- 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]
