FINERACT-2455: WC - Allow transaction adjustment

[mariiaKraievska]

Description

Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
• I followed the AI Policy.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[galovics]

Reusing the undo core and handing the replacement to processRepaymentLikeTransaction is the right direction, and the plumbing (permission, handler, CommandWrapperBuilder, Swagger, Liquibase) follows the WC conventions. I know it's a draft, so read this as early feedback - but the way the adjust is composed causes a few problems I'd want settled first.

1. The loan history is replayed twice. adjustRepaymentLikeTransaction runs the full undo (reverseRepaymentLikeTransactionCore: reprocess, breach/delinquency undo, reversal journal entries, determineAndTransition, recalculateOverpaidOnDate) and then the whole repayment flow again for the replacement, including a second reprocess when it's backdated with charges, overpayment or charge-off. Every later transaction whose allocation changes gets restated twice (including the charge-off restatement and restateFinalDiscountFeeAmortization), leaving reverse/re-post pairs in the ledger even when the net change is zero. Each pass also publishes its own bulk adjust event, so consumers are told about an intermediate "loan without X" state that never existed. A loan closed or overpaid by X goes closed -> ACTIVE -> closed inside one command, running triggerInlineAmortizationIfLoanClosed/accrueOnClosure twice. That's exactly the shape that burned us in #6281 and #6399. Term loans do it once: mark X reversed with its reversal entries, create X', reprocess once from min(X.date, X'.date), transition status once, one adjust event.

2. The risky paths have no tests. All six e2e scenarios use one accounting product with discount 0, keep the original date, on an ACTIVE loan with no charge-off. Missing: a changed transactionDate (either direction), adjusting the payment that closed or overpaid the loan (status round trip, overpaidOnDate, closure amortization and accrual), a charged-off loan, a product with a discount, a loan with a CBR, and negative cases (already reversed, unsupported type, charge adjustment above the available amount, bad status). No unit tests for adjustRepaymentLikeTransaction / validateAdjustTransaction / validateChargeAdjustmentAmountForAdjust either.

3. External id contract differs from term loans. The original external id is moved from the reversed transaction to the replacement and the request has no externalId parameter. Term loans take the new transaction's externalId from the command and leave the reversed one alone. The reversed transaction in the adjust event now has no external id, so consumers can't match it to what they hold, and .../external-id/{origId} resolves to a different row. Either accept externalId and keep the original on the reversed one, or document the move.

4. Charge adjustment skips checks. validateChargeAdjustmentAmountForAdjust re-implements chargeAdjustmentEntranceValidation/calculateAvailableAmountForChargeAdjustment from the charge service but leaves out wcCharge.isActive() and checkClientActive, and the replacement takes any past date down to disbursement (a normal charge adjustment is always the business date), so it can end up dated before the charge existed. Let's reuse the existing validation (expose it with an exclude-transaction parameter) and decide the date rule.

5. The replacement loses the original's payment details when paymentDetails is omitted (top-level paymentTypeId isn't supported), and gets a new id/createdDate, so it sorts after other same-date transactions entered later, which can change allocation with active charges. Copy the original's PaymentDetail when none is given, and document the ordering.

Minor: the shared request schema now lists adjust-only fields for undo, transactionDate is required even for a zero-amount adjust, and the step def stores its result under LOAN_REPAYMENT_UNDO_RESPONSE. The CI failures look like infra (docker registry download, an untouched avro checkstyle), but need a re-run.

Recommendation: CHANGES_REQUESTED

[mariiaKraievska]

Reusing the undo core and handing the replacement to processRepaymentLikeTransaction is the right direction, and the plumbing (permission, handler, CommandWrapperBuilder, Swagger, Liquibase) follows the WC conventions. I know it's a draft, so read this as early feedback - but the way the adjust is composed causes a few problems I'd want settled first.

1. The loan history is replayed twice. adjustRepaymentLikeTransaction runs the full undo (reverseRepaymentLikeTransactionCore: reprocess, breach/delinquency undo, reversal journal entries, determineAndTransition, recalculateOverpaidOnDate) and then the whole repayment flow again for the replacement, including a second reprocess when it's backdated with charges, overpayment or charge-off. Every later transaction whose allocation changes gets restated twice (including the charge-off restatement and restateFinalDiscountFeeAmortization), leaving reverse/re-post pairs in the ledger even when the net change is zero. Each pass also publishes its own bulk adjust event, so consumers are told about an intermediate "loan without X" state that never existed. A loan closed or overpaid by X goes closed -> ACTIVE -> closed inside one command, running triggerInlineAmortizationIfLoanClosed/accrueOnClosure twice. That's exactly the shape that burned us in #6281 and #6399. Term loans do it once: mark X reversed with its reversal entries, create X', reprocess once from min(X.date, X'.date), transition status once, one adjust event.

2. The risky paths have no tests. All six e2e scenarios use one accounting product with discount 0, keep the original date, on an ACTIVE loan with no charge-off. Missing: a changed transactionDate (either direction), adjusting the payment that closed or overpaid the loan (status round trip, overpaidOnDate, closure amortization and accrual), a charged-off loan, a product with a discount, a loan with a CBR, and negative cases (already reversed, unsupported type, charge adjustment above the available amount, bad status). No unit tests for adjustRepaymentLikeTransaction / validateAdjustTransaction / validateChargeAdjustmentAmountForAdjust either.

3. External id contract differs from term loans. The original external id is moved from the reversed transaction to the replacement and the request has no externalId parameter. Term loans take the new transaction's externalId from the command and leave the reversed one alone. The reversed transaction in the adjust event now has no external id, so consumers can't match it to what they hold, and .../external-id/{origId} resolves to a different row. Either accept externalId and keep the original on the reversed one, or document the move.

4. Charge adjustment skips checks. validateChargeAdjustmentAmountForAdjust re-implements chargeAdjustmentEntranceValidation/calculateAvailableAmountForChargeAdjustment from the charge service but leaves out wcCharge.isActive() and checkClientActive, and the replacement takes any past date down to disbursement (a normal charge adjustment is always the business date), so it can end up dated before the charge existed. Let's reuse the existing validation (expose it with an exclude-transaction parameter) and decide the date rule.

5. The replacement loses the original's payment details when paymentDetails is omitted (top-level paymentTypeId isn't supported), and gets a new id/createdDate, so it sorts after other same-date transactions entered later, which can change allocation with active charges. Copy the original's PaymentDetail when none is given, and document the ordering.

Minor: the shared request schema now lists adjust-only fields for undo, transactionDate is required even for a zero-amount adjust, and the step def stores its result under LOAN_REPAYMENT_UNDO_RESPONSE. The CI failures look like infra (docker registry download, an untouched avro checkstyle), but need a re-run.

Recommendation: CHANGES_REQUESTED

Thanks for the feedback, addressed as follows:

1. Double replay — Fixed: mark reverse + create replacement → single reprocess → one status transition / one adjust event.

2. Risky-path tests — Deferring to QA.

3. External id — Aligned with term loans.

4. Charge adjustment — Fixed: reuse shared entrance validation + replacement date must not be before charge due date.

5. Payment details — Fixed: omit → copy into a new PaymentDetail row.

Minor — transactionDate optional for amount 0, schema/step-def cleaned up.

[galovics]

Thanks @mariiaKraievska, the rework looks much better. The adjust is now a single pass (mark reversed, create the replacement, one reprocess from min(original date, new date), one status transition, one adjust event with both sides), the reversed transaction keeps its external id and the replacement takes externalId from the command like term loans, the charge adjustment goes through the shared entrance validation with the replaced transaction excluded plus a due-date floor, and the original payment details are copied when none are given. The minors are sorted too and CI is green.

The one thing I can't accept is "deferring to QA" on the tests. The new flow isn't reusing the undo core anymore, it's a hand-assembled sequence (reversal JE, replacement relation, reprocess routing, breach undo/apply, determineAndTransition, recalculateOverpaidOnDate, closure amortization and accrual), and the feature file still has the exact same six happy-path scenarios as before: same-date, ACTIVE loan, no discount, no charge-off. None of them touches a changed transactionDate, the adjustment of the payment that closed or overpaid the loan, a charged-off loan or a discount product, and those are exactly the paths this rewrite changed. The same goes for the negative cases (already reversed, unsupported type, charge adjustment above the available amount or dated before the charge's due date, wrong loan status). Plus there's still no unit test for adjustRepaymentLikeTransaction / validateAdjustTransaction.

This is money-moving logic that rewrites the ledger. I don't want the first time we see a backdated adjust on an overpaid loan to be in QA, or worse in prod. One more thing, this one is a real bug on exactly that untested path: recalculateOverpaidOnDate(loan, newTransaction) (~L1338) uses the replacement's date for its "may be stale" guard, which skips when that date is after the stored overpaidOnDate. Say X at D1 set overpaidOnDate = D1 and a later payment Y at D3 also overpays. Adjust X to a date after D1 with an amount that no longer overpays: the guard skips and D1 stays, even though the earliest overpaying transaction is now Y at D3. Undo passes the reversed transaction so it doesn't have this. Pass whichever of the original and the replacement has the earlier date, or call it for both.

Please add at least the changed-date (both directions), closed/overpaid and charge-off e2e scenarios plus the negative cases before we take it out of draft.

Recommendation: REQUEST_CHANGES

[somasorosdpc]

I think the related entity here is the working capital loan transaction.

[mariiaKraievska]

Fixed. Thanks

[galovics]

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:

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

[mariiaKraievska]

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:

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 - #6542.