mariiaKraievska commented on PR #6454:
URL: https://github.com/apache/fineract/pull/6454#issuecomment-5816248468
> 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 l
oans 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.
--
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]
