Skip to content

FINERACT-2259: Manage external IDs: Savings (transactions)#5801

Open
San-43 wants to merge 2 commits intoapache:developfrom
San-43:FINERACT-2259-Manage-external-IDs-Savings
Open

FINERACT-2259: Manage external IDs: Savings (transactions)#5801
San-43 wants to merge 2 commits intoapache:developfrom
San-43:FINERACT-2259-Manage-external-IDs-Savings

Conversation

@San-43
Copy link
Copy Markdown
Contributor

@San-43 San-43 commented Apr 24, 2026

This PR completes savings transaction externalId support:

  • add savings transaction routes by savings account externalId
  • support transaction operations by transaction externalId
  • add externalId support for holdAmount, releaseAmount and postInterestAsOn
  • align savings transaction validation and OpenAPI metadata
  • add focused tests for savings transaction externalId flows

Checklist

  • 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.

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

San-43 added 2 commits April 23, 2026 20:49
- add savings transaction routes by savings account externalId
  - support transaction operations by transaction externalId
  - add externalId support for holdAmount, releaseAmount and postInterestAsOn
  - align savings transaction validation and OpenAPI metadata
  - add focused tests for savings transaction externalId flows
@San-43
Copy link
Copy Markdown
Contributor Author

San-43 commented Apr 24, 2026

integration test failures were caused by three regressions introduced by these changes:

  • postInterestAsOn validation became backward-incompatible because existing tests/helpers still sent the legacy postInterestManualOrAutomatic flag, while the new validation only accepted isPostInterestAsOn.
  • releaseAmount validation became backward-incompatible because existing callers still sent legacy payloads, including generic transaction fields and isBulk in batch flows.
  • Several interest-posting integration tests were not null-safe after externalId was added to transaction responses, because they called toString() on nullable map values.

There were also two ClientSavingsIntegrationTest cases that started returning 403 once the legacy manual-interest flag was interpreted correctly. In that case the backend behavior was valid: the tests were posting manual interest with a date earlier than already-posted transactions, which is rejected by the existing business rule.

PS: e2e test failures are unrelated Java heap space

I believe I fixed these regressions and the affected tests in my last commit. @adamsaghy please take a look when u have a time, i may need some guidance here.

@San-43
Copy link
Copy Markdown
Contributor Author

San-43 commented Apr 24, 2026

After the latest changes, the remaining failing tests don’t appear to be caused by this PR they’re failing with Java heap space

@San-43
Copy link
Copy Markdown
Contributor Author

San-43 commented Apr 24, 2026

Java heap space for the smoke test with kafka.

Copy link
Copy Markdown
Contributor

@IOhacker IOhacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@San-43 LGTM @Aman-Mittal still 1 commit per PR applies?

@San-43
Copy link
Copy Markdown
Contributor Author

San-43 commented Apr 24, 2026

Hi @IOhacker, thank you for your LGTM and comment.

About the two commits: I read FSIP-7: Traditional Merge Strategy the other day and wanted to keep this PR as two commits because I think it better reflects the fixes made along the way. However, i'm happy to squash into one commit if needed.

@Aman-Mittal
Copy link
Copy Markdown
Contributor

@San-43 LGTM @Aman-Mittal still 1 commit per PR applies?

While FSIP-7 proposal intention was to preserve sign commits for merge instead of "Squash and Commit" it should be merged normally. While it is also mentioned.

Encourage developers to use as few or as many commits as they choose in their PRs, with the intent to most clearly communicate the reviewable progress of their work during PR review and for posterity. This means rebasing locally, adding/removing commits, and force-pushing are all still allowed, although these should be used only as necessary/helpful (e.g. when a new commit on develop assists the PR or to correct noise/mistakes). If force-push is used, communicate it thoughtfully.

Its up to you (reviewer) if you want him rebase or not is multiple commits justify for the work he has done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants