FINERACT-2259: Manage external IDs-Savings #4623

Lymah123 · 2025-04-25T14:54:51Z

Description

Introduction

Transactions may change due to correction. Correction can be done by reverting a transaction. This can lead to removing and adding transactions back to an account. It results new ID distribution from the transaction sequence. External systems may not want to follow up on these changes but rather access transactions as they are at their end state. In order to be able to direct client immediately to the right record an additional identifier is to be used.

Supporting externalId is a long time wanted functionality with two major benefits:

Not an auto incremented number (privacy, harder to guess)
Not represent any internal state

Requirements

🟩 Supporting external id for identifying and fetching some if the entities in Fineract
🟩 API enhancement to support targeting by external id
✅ External id can be provided in API request
🟩 Extend API response with external id
🟩 Support external id auto generation (configurable)

External identifier

Must be unique
Immutable
Customer (Caller) provided
Ability to generate automatically (configurable)

Database
External identifier field to be stored in the loan table

type: String

indexing: true

Configuration

Enable to auto generate external id (if not provided).

By default it should be disabled.

This configuration already exists!

Goal

This story is focusing to introduce external ids for savings accounts and transactions!

adamsaghy · 2025-04-28T13:07:09Z

@Lymah123 Please run this before any commits:
./gradlew spotlessApply spotbugsMain spotbugsTest checkstyleMain checkstyleTest

Lymah123 · 2025-04-28T15:35:25Z

@Lymah123 Please run this before any commits:
./gradlew spotlessApply spotbugsMain spotbugsTest checkstyleMain checkstyleTest

Ok.

Lymah123 · 2025-05-01T16:26:54Z

Hi @adamsaghy

I've tried building it on my end, but some call sites require externalId implementations, which I'm currently working on. I'll push the updates once I've completed and tested the changes.

I just thought of updating you on the progress.

Thank you.

adamsaghy · 2025-05-12T08:19:14Z

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.
> A failure occurred while executing org.nosphere.apache.rat.RatWork
   > Apache Rat audit failure - 4 unapproved licenses

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
     	See file:///home/runner/work/fineract/fineract/build/reports/rat/index.html

Please make sure all new files has the apache license header! (you can copy from any existing file)

Lymah123 · 2025-05-20T01:17:06Z

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.
> A failure occurred while executing org.nosphere.apache.rat.RatWork
   > Apache Rat audit failure - 4 unapproved licenses

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
     	See file:///home/runner/work/fineract/fineract/build/reports/rat/index.html

Please make sure all new files has the apache license header! (you can copy from any existing file)

@adamsaghy , I have worked on the files.

adamsaghy · 2025-05-20T06:49:28Z

you can see all the checks are red!

home/runner/work/fineract/fineract/fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountWritePlatformServiceJpaRepositoryImpl.java:366: error: method handleFDDeposit in interface DepositAccountDomainService cannot be applied to given types; 2025-05-12T15:46:58.1104587Z final SavingsAccountTransaction deposit = this.depositAccountDomainService.handleFDDeposit(account, fmt, transactionDate, 2025-05-12T15:46:58.1106178Z ^ 2025-05-12T15:46:58.1106991Z > Task :fineract-provider:compileJava 2025-05-12T15:46:58.1108324Z required: FixedDepositAccount,DateTimeFormatter,LocalDate,BigDecimal,PaymentDetail,ExternalId 2025-05-12T15:46:58.1109870Z found: FixedDepositAccount,DateTimeFormatter,LocalDate,BigDecimal,PaymentDetail 2025-05-12T15:46:58.1111070Z reason: actual and formal argument lists differ in length 2025-05-12T15:46:58.1113719Z

Lymah123 · 2025-05-26T14:53:31Z

Hi @adamsaghy, I have made some changes and tried building and testing locally, which was successful.

adamsaghy · 2025-05-26T14:56:06Z

@Lymah123 Please rebase your PR with the latest develop branch!

Also make sure you have pushed all the changes. The above listed errors are compilation errors...

a7med3del1973 · 2025-05-29T20:38:57Z

Please, solve the conflict.

Lymah123 · 2025-05-30T02:36:52Z

I'm experiencing an unusual conflict situation with my PR.

GitHub shows conflicts in three files:

fineract-provider/build.gradle
fineract-provider/src/main/resources/jpa/persistence.xml
fineract-savings/src/main/resources/jpa/savings/persistence.xml

However, my local Git shows no conflicts:

When I run git merge develop → "Already up to date"
When I run git rebase develop → "Current branch is up to date"
I created a fresh branch from develop and cherry-picked my changes without conflicts

What I've Tried

Force pushing with --force-with-lease and --force
Creating new branches from develop
Hard resetting and cherry-picking
Creating a clean branch with conflict-resolution-branch
- This cherry-picked cleanly with all 22 files
- But GitHub still shows "Can't automatically merge"

What approach would you recommend to resolve these "phantom" conflicts?

adamsaghy · 2025-06-02T11:14:04Z

There is no such things as phantom conflicts.. The recommended way is to rebase your feature branch. However what you have tried to do was to rebase the local develop branch, however if you didnt pulled the latest changes of develop branch from the remote, it will not do much things, so let me advise 2 options what you can do:

Option 1:

git checkout develop -> Switch to develop branch
git pull -> Pull latest changes from remote to local develop branch
git checkout external-id-management -> Switch to your feature branch
git rebase develop -> Start rebasing -> Fix conflicts
git rebase --continue -> Finish on Rebasing

Option 2:

git fetch origin -> Fetch latest branches from origin (if origin points to apache/fineract repository)
git rebase origin/develop -> Start rebasing -> Fix conflicts
git rebase --continue -> Finish on Rebasing

Lymah123 · 2025-06-03T03:20:27Z

The issue wasn't actually "phantom" conflicts , thanks for the correction, @adamsaghy
I think it was a remote repository mismatch

Local setup: My origin remote pointed to my fork (Lymah123/fineract)
GitHub PR: The PR was targeting the upstream repository (apache/fineract)
Problem: My local develop branch was synced with my fork, but GitHub was comparing against the upstream repository's develop branch

Why Local Git Showed No Conflicts when I run:

git merge develop     
git rebase develop

These commands were comparing against my fork's develop branch, which was already compatible with my feature branch.

The Real Issue:
GitHub's conflict detection was comparing my PR branch against upstream's develop branch, which had diverged from my fork's develop branch.

Steps I took to resolved it:

Added upstream remote

git remote add upstream https://github.com/apache/fineract.git
git fetch upstream

Rebased against the correct target

git checkout external-id-management
git rebase upstream/develop  # This revealed the real conflicts

Resolved conflicts and updated PR

git add <conflicted-files>
git rebase --continue
git push origin external-id-management --force-with-lease

Key Takeaway
I will always rebase feature branches against the upstream repository's target branch (upstream/develop), not my fork's copy, especially on apache/fineract repository.

My PR is ready for review @adamsaghy. Thanks!

adamsaghy · 2025-06-03T07:47:04Z

/home/runner/work/fineract/fineract/fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountWritePlatformServiceJpaRepositoryImpl.java:366: error: method handleFDDeposit in interface DepositAccountDomainService cannot be applied to given types;
        final SavingsAccountTransaction deposit = this.depositAccountDomainService.handleFDDeposit(account, fmt, transactionDate,
                                                                                  ^
  required: FixedDepositAccount,DateTimeFormatter,LocalDate,BigDecimal,PaymentDetail,ExternalId
  found:    FixedDepositAccount,DateTimeFormatter,LocalDate,BigDecimal,PaymentDetail
  reason: actual and formal argument lists differ in length

adamsaghy · 2025-06-03T07:47:33Z

config/docker/env/aws.env

 AWS_EC2_METADATA_DISABLED=true
-AWS_ENDPOINT_URL=http://localstack:4666
-#FINERACT_AWS_ENDPOINT=http://localstack:4666
+# AWS_ENDPOINT_URL is deprecated, use FINERACT_AWS_ENDPOINT instead


out of scope changes...

This is for the previous PR I opened. I will delete the branch.

Still not reverted...

adamsaghy · 2025-06-03T07:48:41Z

fineract-provider/build.gradle

    }
 }
+
+compileJava.dependsOn(':fineract-core:compileJava')


Alright, I changed it so I can build locally on my end. I will remove it

adamsaghy · 2025-06-03T07:49:53Z

...ct-provider/src/main/java/org/apache/fineract/interoperation/service/InteropServiceImpl.java

                    transferCode, null);
            SavingsAccountTransaction holdTransaction = SavingsAccountTransaction.holdAmount(savingsAccount, savingsAccount.office(),
-                    paymentDetail, transactionDate, Money.of(savingsAccount.getCurrency(), total), false);
+                    paymentDetail, transactionDate, Money.of(savingsAccount.getCurrency(), total), false, ExternalId.empty());


We should check whether enable-auto-generated-external-id is true or false...

if it is true, we need to generate automatically external id.

Ok, I will work on that.

No changes were made...

adamsaghy · 2025-06-03T07:50:05Z

.../org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java

                    isRegularTransaction, fromSavingsAccount.isWithdrawalFeeApplicableForTransfer(), isInterestTransfer, isWithdrawBalance);
            final SavingsAccountTransaction withdrawal = this.savingsAccountDomainService.handleWithdrawal(fromSavingsAccount, fmt,
-                    transactionDate, transactionAmount, paymentDetail, transactionBooleanValues, backdatedTxnsAllowedTill);
+                    transactionDate, transactionAmount, paymentDetail, transactionBooleanValues, backdatedTxnsAllowedTill,


We should check whether enable-auto-generated-external-id is true or false...

if it is true, we need to generate automatically external id.

Example:

if (externalId.isEmpty() && TemporaryConfigurationServiceContainer.isExternalIdAutoGenerationEnabled()) { externalId = ExternalId.generate(); }

org.apache.fineract.portfolio.loanaccount.service.LoanAssemblerImpl#updateFrom, line 537

No changes...

adamsaghy · 2025-06-03T07:52:43Z

...provider/src/main/java/org/apache/fineract/portfolio/savings/domain/FixedDepositAccount.java

        final Money transactionAmountMoney = Money.of(getCurrency(), this.accountTermAndPreClosure.depositAmount());
        final SavingsAccountTransaction transaction = SavingsAccountTransaction.deposit(null, office(), null,
-                this.accountSubmittedOrActivationDate(), transactionAmountMoney, refNo);
+                this.accountSubmittedOrActivationDate(), transactionAmountMoney, refNo, ExternalId.empty());


We should check whether enable-auto-generated-external-id is true or false...

if it is true, we need to generate automatically external id.

adamsaghy · 2025-06-03T07:52:53Z

...ider/src/main/java/org/apache/fineract/portfolio/savings/domain/RecurringDepositAccount.java

                    }
                    final SavingsAccountTransaction transaction = SavingsAccountTransaction.deposit(null, office(), null, dueDate,
-                            installment.getDepositAmountOutstanding(getCurrency()), refNo);
+                            installment.getDepositAmountOutstanding(getCurrency()), refNo, ExternalId.empty());


We should check whether enable-auto-generated-external-id is true or false...

if it is true, we need to generate automatically external id.

adamsaghy · 2025-06-03T07:55:49Z

gradlew.original

@@ -0,0 +1,249 @@
+#!/bin/sh
+
+#


what is this file?

For local build, I will remove it.

adamsaghy

Please see my review and failing tests!

Lymah123 · 2025-06-16T03:51:14Z

Please see my review and failing tests!

I have, and I will work on the requested changes.

github-actions · 2025-07-17T00:25:13Z

This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.

Lymah123 · 2025-07-17T02:46:06Z

I am still working on it. I wasn't available for some times.

adamsaghy · 2025-08-07T08:00:37Z

...ct-provider/src/main/java/org/apache/fineract/interoperation/service/InteropServiceImpl.java

        String transferCode = request.getTransferCode();

+        // Check if auto-generation of external ID is enabled
+        final boolean isAutoGeneratedExternalIdEnabled = this.configurationDomainService.isExternalIdAutoGenerationEnabled();


an IF-ELSE would be nicer here...

adamsaghy · 2025-08-07T08:05:10Z

.../org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java

+     * Returns an appropriate ExternalId based on configuration settings. If auto-generation is enabled and the provided
+     * externalId is empty, generates a new one.
+     */
+    private ExternalId getExternalId(ExternalId externalId) {


We dont need this... a simple IF-ELSE would be enought to either generate a new or return empty...

No need for this, use ExternalIdFactory

adamsaghy · 2025-08-07T08:13:27Z

...provider/src/main/java/org/apache/fineract/portfolio/savings/domain/FixedDepositAccount.java

+     * Returns an appropriate ExternalId based on configuration settings. If auto-generation is enabled and the provided
+     * externalId is empty, generate a new one.
+     */
+    public ExternalId getExternalId(ExternalId externalId) {


No need for this, use ExternalIdFactory

adamsaghy · 2025-08-07T08:13:38Z

...ider/src/main/java/org/apache/fineract/portfolio/savings/domain/RecurringDepositAccount.java

+     * Returns an appropriate ExternalId based on configuration settings. If auto-generation is enabled and provided
+     * externalId is empty, generate a new one.
+     */
+    public ExternalId getExternalId(ExternalId externalId) {


No need for this, use ExternalIdFactory

adamsaghy · 2025-08-07T08:14:48Z

fineract-savings/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccount.java


    public transient ConfigurationDomainService configurationDomainService;

+    public ExternalId getExternalId(ExternalId externalId) {


No need for this, use ExternalIdFactory

adamsaghy · 2025-08-07T08:15:07Z

fineract-savings/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccount.java

            boolean backdatedTxnsAllowedTill) {
        final SavingsAccountTransaction chargeTransaction = SavingsAccountTransaction.waiver(this, office(),
-                DateUtils.getBusinessLocalDate(), transactionAmount);
+                DateUtils.getBusinessLocalDate(), transactionAmount, null, ExternalId.empty());


Rewrite to get the external id as parameter

adamsaghy · 2025-08-07T08:17:37Z

@Lymah123 You need to rewrite the whole PR:

Use ExternalIdFactory in the services to generate an external id. It will check whether new should be generated or not.
Entities and DTOs should get the resolved ExternalId as parameter, no need any further logic in them!
Hardcoding ExternalId.empty() is usually considered incorrect! Use ExternalIdFactory instead to get the appropriate ExternalId object and value!

Lymah123 · 2025-08-13T19:04:43Z

@Lymah123 You need to rewrite the whole PR:

Use ExternalIdFactory in the services to generate an external id. It will check whether new should be generated or not.

Entities and DTOs should get the resolved ExternalId as parameter, no need any further logic in them!

Hardcoding ExternalId.empty() is usually considered incorrect! Use ExternalIdFactory instead to get the appropriate ExternalId object and value!

alright @adamsaghy

Lymah123 · 2025-08-13T19:06:11Z

Hi @adamsaghy, I have gone through all the feedback. I will work on the requested changes. Thanks.

adamsaghy · 2025-08-28T08:13:40Z

Additional issues:

Liquibase scripts are missing (to add the new column to the savings account transaction database table)
Testing is missing
Adding the new field to the supported parameter list is missing
Validation whether the provided external id length is valid is missing

Lymah123 · 2025-08-28T15:36:00Z

Additional issues:

Liquibase scripts are missing (to add the new column to the savings account transaction database table)

Testing is missing

Adding the new field to the supported parameter list is missing

Validation whether the provided external id length is valid is missing

@adamsaghy are these additional issues to work on regards this PR?

github-actions · 2025-09-28T00:24:12Z

This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.

IOhacker · 2025-10-06T02:45:02Z

@Lymah123 are you still working on this PR?

Lymah123 · 2025-10-06T03:30:46Z

@Lymah123 are you still working on this PR?

Yes @IOhacker,

Lymah123 · 2025-10-06T03:32:24Z

Additional issues:

Liquibase scripts are missing (to add the new column to the savings account transaction database table)

Testing is missing

Adding the new field to the supported parameter list is missing

Validation whether the provided external id length is valid is missing

@adamsaghy are these additional issues to work on regards this PR?

@adamsaghy, I need a response to this, please. Thanks!

github-actions · 2025-11-06T00:22:43Z

This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.

vidakovic self-requested a review April 25, 2025 15:43

Lymah123 force-pushed the external-id-management branch from 7b48696 to 88db06f Compare May 5, 2025 10:40

Lymah123 force-pushed the external-id-management branch from 88db06f to fd5bdcc Compare May 12, 2025 15:09

Lymah123 force-pushed the external-id-management branch 2 times, most recently from 4f2ca61 to 459b798 Compare May 26, 2025 12:26

Lymah123 force-pushed the external-id-management branch 2 times, most recently from e27a48a to cca884d Compare May 30, 2025 01:22

Lymah123 closed this May 30, 2025

Lymah123 reopened this May 30, 2025

Lymah123 force-pushed the external-id-management branch 2 times, most recently from ce44487 to 1223eae Compare June 3, 2025 03:09

adamsaghy reviewed Jun 3, 2025

View reviewed changes

adamsaghy requested changes Jun 3, 2025

View reviewed changes

github-actions bot added the stale label Jul 17, 2025

github-actions bot removed the stale label Jul 18, 2025

FINERACT-2259: Manage external IDs-Savings

c565dd9

Lymah123 force-pushed the external-id-management branch from 1223eae to c565dd9 Compare July 31, 2025 22:11

Lymah123 requested a review from adamsaghy July 31, 2025 22:12

adamsaghy reviewed Aug 7, 2025

View reviewed changes

adamsaghy marked this pull request as draft August 28, 2025 08:11

github-actions bot added the stale label Sep 28, 2025

github-actions bot removed the stale label Oct 7, 2025

github-actions bot added the stale label Nov 6, 2025


		public transient ConfigurationDomainService configurationDomainService;

		public ExternalId getExternalId(ExternalId externalId) {

FINERACT-2259: Manage external IDs-Savings #4623

Are you sure you want to change the base?

FINERACT-2259: Manage external IDs-Savings #4623

Conversation

Lymah123 commented Apr 25, 2025

Description

Introduction

Requirements

Configuration

Goal

Uh oh!

adamsaghy commented Apr 28, 2025

Uh oh!

Lymah123 commented Apr 28, 2025

Uh oh!

Lymah123 commented May 1, 2025

Uh oh!

adamsaghy commented May 12, 2025

Uh oh!

Lymah123 commented May 20, 2025

Uh oh!

adamsaghy commented May 20, 2025

Uh oh!

Lymah123 commented May 26, 2025

Uh oh!

adamsaghy commented May 26, 2025

Uh oh!

a7med3del1973 commented May 29, 2025

Uh oh!

Lymah123 commented May 30, 2025

Uh oh!

adamsaghy commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lymah123 commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamsaghy commented Jun 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lymah123 Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Lymah123 commented Jun 16, 2025

Uh oh!

github-actions bot commented Jul 17, 2025

Uh oh!

Lymah123 commented Jul 17, 2025

Uh oh!

adamsaghy commented Jun 2, 2025 •

edited

Loading

Lymah123 commented Jun 3, 2025 •

edited

Loading

adamsaghy Jun 3, 2025 •

edited

Loading

Lymah123 Jun 16, 2025 •

edited

Loading

adamsaghy Aug 7, 2025 •

edited

Loading

adamsaghy Aug 7, 2025 •

edited

Loading

adamsaghy Aug 7, 2025 •

edited

Loading

adamsaghy Aug 7, 2025 •

edited

Loading