FINERACT-2463: Allow undo of Fixed Deposit transactions

[AshharAhmadKhan]

Description

Fixed Deposit accounts had an undo transaction endpoint that was silently broken in two ways. The API resource was calling undoSavingsAccountTransaction on the command builder, which set entityName = "SAVINGSACCOUNT" — meaning every undo request was routed to the regular savings handler, completely bypassing the Fixed Deposit command handler that already existed and was correctly wired. On top of that, undoFDTransaction in the service layer was an unconditional stub that always threw DepositAccountTransactionNotAllowedException, so even if routing had been correct, nothing would have worked.

This PR fixes both issues and makes undo transactions fully functional for Fixed Deposit accounts.

CommandWrapperBuilder.java — Added the missing undoFixedDepositAccountTransaction() builder method, correctly setting entityName = "FIXEDDEPOSITACCOUNT" and actionName = "UNDOTRANSACTION".

FixedDepositAccountTransactionsApiResource.java — Fixed the routing bug by calling the new FD-specific builder method instead of the savings one.

DepositAccountWritePlatformServiceJpaRepositoryImpl.java — Replaced the throw stub with a full implementation adapted from undoRDTransaction, with FD-specific adjustments: FixedDepositAccount cast, isTransactionsAllowed() status check for ACTIVE and MATURED states, 7-param postInterest signature, and updateMaturityDateAndAmount instead of RD-specific schedule calls.

FixedDepositAccountHelper.java — Added getFixedDepositTransactions() and undoFixedDepositTransaction() helper methods following the existing deprecated helper pattern.

FixedDepositTest.java — Added testFixedDepositAccountUndoTransaction covering the full flow: create client, open and activate FD account, post interest, undo the interest transaction, assert totalInterestPosted returns to zero.

Checklist

• Write the commit message as per our guidelines
• Create/update unit or integration tests for verifying the changes made
• Follow our coding conventions

[IOhacker]

LGTM

[adamsaghy]

@AshharAhmadKhan Please review the failing test cases!

[AshharAhmadKhan]

@adamsaghy @IOhacker

The failing test-core-5 checks have been fixed.

Root cause: After undoing an interest transaction, the DB column total_interest_posted_derived becomes 0. The DepositAccountReadPlatformServiceImpl intentionally uses getBigDecimalDefaultToNullIfZero for all summary fields, which converts 0 to null — this is consistent behaviour across the entire deposit reader. The test was calling .floatValue() directly on a potentially null Float, causing an NPE.

Fix: Added a null check in the test assertion so that a null totalInterestPosted (which semantically means zero) is treated as 0f, which is the correct expected value after an undo.

The service implementation, routing fix, and journal entry logic are all unchanged.

[AshharAhmadKhan]

@adamsaghy went through the changes again to see if I missed anything and I came out with notihng. Please let me know if there is anything else needed.

[adamsaghy]

Please see my concerns around testing.

[AshharAhmadKhan]

@adamsaghy thanks for the review!

For the test I switched it to CASH_BASED accounting and added all three checks you mentioned — transaction marked as reversed, totalInterestPosted going back to zero, and the reversal journal entries (opposite signs on the same date).

For the feign point I had a look at the generated FixedDepositAccountApi and it only exposes handleCommands4 which hits the account level endpoint and takes a single accountId. The undo transaction endpoint needs both accountId and transactionId so there is no matching method in the generated client. I also noticed RecurringDepositAccountHelper.undoTransactionForRecurringDeposit follows the same raw HTTP pattern for the same reason, so this is consistent with the existing deposit helper approach. I have added a comment in the helper explaining the gap. Happy to extend the generated client if you would prefer that approach though.

[adamsaghy]

    Test testFixedDepositAccountUndoTransaction() FAILED
  
    org.opentest4j.AssertionFailedError: Journal Entry not found ==> expected: <true> but was: <false>
        at app//org.apache.fineract.integrationtests.FixedDepositTest.testFixedDepositAccountUndoTransaction(FixedDepositTest.java:3016)
  

[AshharAhmadKhan]

@adamsaghy the failure on 7bad2f6 was a date bug in the test, not in the service. When I computed INTEREST_POSTED_DATE I was using today's date, but Fineract actually books the interest posting at the last day of the posting period which is the end of the month containing the activation date. All the other FD tests in this file use that same end-of-activation-month pattern and I missed it. The fix in 259cc66 resets todaysDate to one month back and advances it to the end of that month before formatting the date, which is exactly what the rest of the test suite does. The journal entry assertions, the balance check and the reversed transaction check are all still there unchanged.

[AshharAhmadKhan]

@adamsaghy @IOhacker

Would either of you mind triggering the runs when you get a chance?
Happy to address anything that comes up. Thanks!

[adamsaghy]

Please use fineract-client-feign only in the integration tests.
Let me know if you need any help with that.

[AshharAhmadKhan]

@adamsaghy thanks for pushing on this, you were right.

I was looking at FixedDepositAccountApi and missed the separate FixedDepositAccountTransactionsApi which has adjustTransaction(accountId, transactionId, command, body) mapping directly to the undo endpoint. This commit wires it into FineractClient as fixedDepositAccountTransactions mirroring how recurringDepositAccountTransactions is already wired, and rewrites undoFixedDepositTransaction to use Calls.ok(getFineractClient().fixedDepositAccountTransactions.adjustTransaction(...)).

getFixedDepositTransactions stays as raw HTTP because GetFixedDepositAccountsAccountIdResponse has no transactions field in the generated model, so the typed client cannot be used there. Happy to hear if you see a better approach.

[adamsaghy]

@adamsaghy thanks for pushing on this, you were right.

I was looking at FixedDepositAccountApi and missed the separate FixedDepositAccountTransactionsApi which has adjustTransaction(accountId, transactionId, command, body) mapping directly to the undo endpoint. This commit wires it into FineractClient as fixedDepositAccountTransactions mirroring how recurringDepositAccountTransactions is already wired, and rewrites undoFixedDepositTransaction to use Calls.ok(getFineractClient().fixedDepositAccountTransactions.adjustTransaction(...)).

getFixedDepositTransactions stays as raw HTTP because GetFixedDepositAccountsAccountIdResponse has no transactions field in the generated model, so the typed client cannot be used there. Happy to hear if you see a better approach.

If a field is missing, we should add it to the swagger definition and it will be generated in the fineract-client automatically.

[AshharAhmadKhan]

@adamsaghy done — added transactions to the swagger definition for GetFixedDepositAccountsAccountIdResponse, added the associations query param to the retrieveOne endpoint so the generated client exposes it, regenerated the client, and rewrote getFixedDepositTransactions to use retrieveOne20(..., "transactions").getTransactions() via the typed client. No more raw HTTP anywhere in the test helpers for this flow.

[AshharAhmadKhan]

@adamsaghy just pushed a fix for the API backward compatibility failure.

The new GetFixedDepositAccountsTransactions inner class I added had accountNo typed as
String — swagger-brake caught this as a breaking change on
GET /v1/fixeddepositaccounts/{accountId} since the outer response class has it as Long.
Changed it to Long to match the existing pattern across all FD and RD swagger classes.

[AshharAhmadKhan]

I’ve gone through the changes again end-to-end, especially around the earlier feedback on test coverage, client usage, and API alignment, and made sure everything is consistent.

@adamsaghy could you take a look when you get a chance and let me know if anything still needs adjustment? Happy to iterate on your feedback.

[AshharAhmadKhan]

@adamsaghy All issues addressed. Please trigger checks when you get a chance.

[AshharAhmadKhan]

@adamsaghy hey, repushed with all five comments addressed. the CI failures on the last run were the org.eclipse.swt CycloneDX infra issue hitting develop too, not from our changes. compilation, spotless, spotbugs and checkstyle all clean locally. could you trigger the checks when you get a chance? thanks!

[AshharAhmadKhan]

Hey @adamsaghy, I realised I hadn't addressed your last comment correctly.

You asked to drop the manual JSON parsing and return the response object directly, but I was still doing new JsonParser().parse(response).getAsJsonObject().get("resourceId").getAsInt() which wasn't the right approach.

To fix this properly, I added a swagger class FixedDepositAccountTransactionsApiResourceSwagger with the request and response schema classes, and wired @RequestBody and @ApiResponses annotations onto the adjustTransaction endpoint. This causes the generator to produce a typed PostFixedDepositAccountsFixedDepositAccountIdTransactionsTransactionIdResponse instead of Call<String>, which lets undoFixedDepositTransaction simply call .getResourceId() and return it as Long.

Could you take a look when you get a chance? Thanks!

[AshharAhmadKhan]

Hey @adamsaghy, wanted to give you a quick update on what changed in this latest push.

The previous approach for getFixedDepositTransactions was using retrieveOne20 on FixedDepositAccountApi to fetch transactions via the associations param. This turned out to be unstable because the numbered suffix is assigned globally across all API files during code generation, so it shifted on CI and caused a compile failure (cannot find symbol: method retrieveOne20).

The fix was to add a proper dedicated GET /v1/fixeddepositaccounts/{accountId}/transactions endpoint to FixedDepositAccountTransactionsApiResource with a stable operationId = "retrieveAllFixedDepositAccountTransactions". This generates a consistently named method on the typed client regardless of environment, and getFixedDepositTransactions now calls it directly via fixedDepositAccountTransactions.retrieveAllFixedDepositAccountTransactions(...).

The swagger response class for this endpoint lives in FixedDepositAccountTransactionsApiResourceSwagger alongside the existing undo/adjust classes, which felt like the right home for it. All six of your earlier comments on the helper are addressed in the same commit.

Could you take a look and trigger the checks when you get a chance? Thanks!

[AshharAhmadKhan]

Hey @adamsaghy, one additional fix in this push (2c12358).

The API compat check was failing with R012 — adjustTransaction had no swagger annotations before this PR so the generator emitted an implicit default response. Adding @ApiResponses with only a 200 dropped it. Fixed by adding @ApiResponse(responseCode = "default", description = "error occurred") alongside the 200.

The E2E shard failures (3 and 9) were both Java heap space OOMs in :fineract-client:buildJavaSdk — both retried and passed on the same run, 13/15 shards green on the same commit. Unrelated to this PR.

[AshharAhmadKhan]

Hey @adamsaghy, one more fix in this push (58f70c6).

Made a small fix, also

The E2E shard failures (8 and 11) are the same infra OOM pattern as before — across two runs now, four different shards have failed (3, 9, 8, 11) with zero overlap, all running loan/EMI feature files unrelated to this PR. Consistent with random runner heap exhaustion on the first SDK generation attempt.

Could you trigger the checks when you get a chance? Thanks!

[AshharAhmadKhan]

Hey @adamsaghy, the cancelled test-core-2 MySQL job looks like a CI timeout rather than an actual failure. Tests were still passing right up to the 60 minute limit and the same shard passed fine on MariaDB. Could you take a look and let me know if anything needs to change on my end?