adamsaghy commented on PR #6144: URL: https://github.com/apache/fineract/pull/6144#issuecomment-5934093555
@AshharAhmadKhan https://github.com/apache/fineract/pull/6136 was merged. However can you please review the below findings / concerns? The central claim does not hold. The PR says reverseTransfer(...) is the correct way to reverse the loan side, and that this side is always "a plain REFUND". Two problems: 1. reverseTransfer only sets the reversed flag. All it does is call loanTransaction.reverse() (plus manuallyAdjustedOrReversed(), added by this PR) and save the transaction. Creating the refund, by contrast, also: recalculates the loan summary (total_overpaid_derived is a stored column), updates the loan status, posts journal entries, raises business events. The undo reverses none of these. If the refund closed an overpaid loan, the loan stays closed after the undo, and the refund's GL entries remain. The existing callers the PR cites as precedent don't support it. They run during undo-disbursal, after undoDisbursal() has already reset the loan state. The new e2e scenario only checks the "Reverted" column in the transactions tab, so it can't catch any of this. 2. The loan side isn't always a refund. Disbursing a loan to savings also creates a Loan→Savings transfer, with a DISBURSEMENT loan transaction (LoanWritePlatformServiceJpaRepositoryImpl:1745–1750). Before this PR, undoing it threw UnsupportedOperationException. Now it removes the money from savings and flags the disbursement as reversed. But the loan stays active, and its schedule and summary are untouched. Not mentioned in the description: The manuallyAdjustedOrReversed() line changes reverseTransfer for every existing caller, not just the new path. The PR contains [apache/fineract#6136 · FINERACT-2690: Fix undo account…](https://github.com/apache/fineract/pull/6136)'s lookup change but not its test fix or permission migration. Merged on its own, AccountTransferOverdraftTest still undoes with the details id. The two PRs will conflict in undo(). The new e2e scenario has the same create-vs-undo id mismatch as [apache/fineract#6136 · FINERACT-2690: Fix undo account…](https://github.com/apache/fineract/pull/6136). **Recommendation** You should use the same methods to "undone" from Loan to Savings account transactions: ``` this.savingsAccountWritePlatformService.undoTransaction(transaction.getToSavingsTransaction().getSavingsAccount().getId(), transaction.getToSavingsTransaction().getId(), true); final ExternalId reversalTxnExternalId = externalIdFactory.create(); LoanAdjustmentParameter parameter = LoanAdjustmentParameter.builder().transactionAmount(BigDecimal.ZERO) .paymentDetail(paymentDetail).transactionDate(transaction.getFromLoanTransaction().getTransactionDate()) .txnExternalId(transaction.getFromLoanTransaction().getExternalId()).reversalTxnExternalId(reversalTxnExternalId) .noteText(null).build(); this.loanAdjustmentService.adjustLoanTransaction(transaction.getFromLoanTransaction().getLoan(), transaction.getFromLoanTransaction(), parameter, null, new HashMap<>()); transaction.reverse(); -- 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]
