mariiaKraievska commented on PR #6454:
URL: https://github.com/apache/fineract/pull/6454#issuecomment-6059793034
> Thanks @mariiaKraievska and @MarianaDmytrivBinariks, the new scenarios are
exactly what I was missing. Changed date in both directions, closed and
overpaid loans, charge-off, written-off, already undone, future date, before
disbursement, unsupported types and the charge adjustment limit are all there
now, with journal entries checked too. That covers my test concern, thanks.
>
> Two things in the code are still blocking though.
>
> The `overpaidOnDate` issue from my last review is still there.
`adjustRepaymentLikeTransaction` still calls
`transactionProcessor.recalculateOverpaidOnDate(loan, newTransaction)`, so the
guard looks at the replacement's date. If you move an overpaying transaction to
a later date and it doesn't overpay anymore, the guard skips and the old date
stays. Pass whichever of the original and the replacement is earlier (or call
it for both).
>
> The second one came in with the rebase. `develop` now has
`recalculateSettlementDates(loan)` and `closureIncomeDate(...)`, and the
Javadoc says it "must be called wherever a transaction can settle the loan".
The repayment path, undo and discount fee adjustment all call it, but adjust
doesn't. Adjust calls `determineAndTransition(loan, transactionDate)`, then
`triggerInlineAmortizationIfLoanClosed(loan, transactionDate)`, then
`accrueOnClosure(loan, transactionDate)`. So when an adjust closes or overpays
the loan, `maturedOnDate`/`closedOnDate` keep the raw stamp, and
`settlementDate`/`closureIncomeDate` read that stale value. That's exactly the
backdated case the new helpers were added for. I think it should follow the
same sequence as the discount fee adjustment path:
>
> ```java
> stateMachine.determineAndTransition(loan, transactionDate);
> transactionProcessor.recalculateOverpaidOnDate(loan, <earlier of
original/replacement>);
> transactionProcessor.recalculateSettlementDates(loan);
> transactionProcessor.triggerInlineAmortizationIfLoanClosed(loan,
transactionDate);
> chargeAccrualService.accrueOnClosure(loan,
transactionProcessor.closureIncomeDate(loan, transactionDate));
> ```
>
> Please also add a scenario for each. A backdated adjust that closes the
loan should assert the closed/matured date. And the case from my previous
review, where X overpays at D1, Y overpays at D3, then X gets adjusted to a
later date with a non-overpaying amount, should assert `overpaidOnDate` = D3.
>
> CI is red too. The new "Payout Refund transaction Adjustment on overpaid
loan - UC6" fails in E2E shard 18: after the inline COB on 13 January there's
an extra transaction (8 rows vs 7 expected). Please check whether that's the
scenario expectation or the settlement date issue above showing up. I suspect
it's the latter, and I'd fix that first.
>
> Minor: the Swagger description for `externalId` got scrambled in the
rebase ("Optional external for the created transaction. If command=adjust id
for the replacement transaction..."), can you fix the wording?
>
> Recommendation: REQUEST_CHANGES
Thanks for the review — addressed all blocking points:
- **overpaidOnDate**: now probes with the earlier of original vs replacement
date
- **settlement / closure**: adjust follows the same sequence
(`recalculateSettlementDates` + `closureIncomeDate` for accrual); added UC21
(backdated adjust closes → closed/matured dates) and UC22 (overpaidOnDate stays
on the later overpayment)
- **UC6**: fixed
- **Swagger** `externalId` wording fixed
Also reworked the core adjust path to undo + create repayment-like
(`processUndo` then `processRepaymentLike`), so it reuses the same processing
as standalone undo/repayment and stays consistent with the parallel delta-based
adjust PR - https://github.com/apache/fineract/pull/6542.
--
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]
