oleksii-novikov-onix commented on PR #6399: URL: https://github.com/apache/fineract/pull/6399#issuecomment-5591649520
> The feature itself is well-structured and follows the module's conventions closely - Liquibase is clean (insert-only, never deletes `m_external_event_configuration`, permissions mirror the existing charge-adjustment pair exactly), and the e2e coverage of waiver/undo/re-waive/charged-off combinations is genuinely thorough. > > One real correctness issue I'd want fixed before this leaves draft: a waiver permanently mutates the balance (`feeWaived`/`penaltyWaived`), and `WorkingCapitalLoanTransactionReprocessingServiceImpl.replayChargeOff` recomputes the charge-off snapshot from that live balance state on every reprocess - but the waiver transaction itself is excluded from the replay and its own journal entries are never restated. Concretely: fee accrued -> charge-off (credits Fees Receivable) -> waiver -> a later backdated repayment triggers a reprocess -> the charge-off gets restated with the fee portion now zero, reversing its Fees Receivable credit, while the waiver's credit to `INCOME_FROM_CHARGE_OFF_FEES` stays untouched. Net: Fees Receivable is stranded and Fee Charge Off carries a one-sided credit. In the extreme case where the waiver zeroes total outstanding, the reprocess would silently lift the charge-off. Either the waived buckets need to participate in the chronological replay, or `replayCh argeOff` needs to exclude waivers dated after the charge-off. > > Related: the accounting routing for the waiver uses the raw `loan.isChargedOff()` flag rather than the date-ordering-aware `transactionFinder.isAfterActiveChargeOffForAccountingRouting` that every other posting site in the module uses. For a charge accrued before the charge-off this gives the right answer (and I see the deliberate comment explaining that choice), but `chargeAccrualPostings` doesn't gate on the charged-off flag at all - so a charge added and accrued _after_ a charge-off still gets a live Fees Receivable debit, and waiving it would credit the charge-off income account while leaving that receivable on the books forever. Since the validator explicitly permits charges on charged-off loans, this looks reachable and worth an e2e case either way. > > One more worth a look: `undoChargeWaiver` reverses the waiver's own journal entries and reopens the loan, but if the waiver had triggered `triggerInlineAmortizationIfLoanClosed` at closure time, the undo doesn't unwind that final lump-sum amortization the way `undoDiscountFeeAdjustment`/`replayChargeOff` do for their equivalent side effects. > > Recommendation: CHANGES_REQUESTED 1. Charge-off snapshot vs the waived buckets Fixed the second way you suggested. replayChargeOff adds back the waivers that sort after the charge-off, so the snapshot is the outstanding at the charge-off's own position in the replay. Ones sorting before it stay subtracted, as they already had when it was booked. The waived buckets still survive the reset, as LoanCharge.amountWaived does on the term loan side. Resetting them would let a payment replayed before the waiver consume what was forgiven. With date-aware routing a waiver dated to a past due date can land before an active charge-off, so booking one now passes the same reprocessing gate a backdated repayment does. That covers the silent lift: a waiver sorting after the charge-off always puts back at least its own amount, so it cannot zero the snapshot. One sorting before it still can, but only when nothing was owed on that date, and now at booking with the charge-off-lifted event rather than inside a later reprocess. 2. Routing on isChargedOff instead of the finder Fixed, isAfterActiveChargeOffForAccountingRouting now. It was written the other way because the finder alone credits Fees Receivable for a waiver dated before a charge-off that had already relieved it, driving the account negative. It is safe only with the snapshot fix and the gate, so all three landed together. The charged-off accounting scenario changed with it: the waiver there is dated on the due date, so it credits Interest/Fee Receivable, and the charge-off no longer carries the fee. 3. chargeAccrualPostings not gated on charged-off The route as described is unreachable, createLoanCharge rejects new charges on a charged-off loan. Reachable is a charge added before the charge-off and accrued after, and there the two net out: the charge amount enters the balance when the charge is added, so the charge-off credits the receivable whether or not it was accrued, and the accrual debits it back. New e2e asserts it. Which is why the gate stays off: had the accrual debited the charge-off income account instead, the charge-off's credit of 100 would have nothing to match and Fees Receivable would sit at minus 100 permanently. One variant is genuinely broken: waive before the accrual, charge off, undo. The charge-off saw a zero fee outstanding and never relieved the receivable, and the accrual after the undo debits it anyway. Fixed by reprocessing on undo, as undoDiscountFeeAdjustment does. The "Undoing a waiver on a charged-off loan" scenario covers it. 4. Inline amortization on undo Left as is. The generic undoTransaction does not unwind it either, and both paths you compared reach their amortization through a transaction relation, while the closure one is linked to nothing. It self-corrects: processDiscountFeeAmortization targets schedule minus already posted, so the first COB after the loan reopens adjusts the excess. The charged-off variant has nothing to correct, despite the COB step skipping charged-off loans: charge-off requires an active loan and recognizes the whole remaining discount then, so a later closure finds a zero remainder. That leaves the window until that COB. Closing it in the waiver undo alone would still leave it for an undone repayment that closed the loan, so it belongs on the generic undo. What changed The new aggregate query, the snapshot in replayChargeOff, the finder for routing, and the reprocess gate on booking a backdated waiver and on undoing one. Two new e2e scenarios, one updated. -- 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]
