FINERACT-2324: Remove "loan.getLoanTransactions()" from reprocessing (reverse-replay) related actions #5041

mariiaKraievska · 2025-09-17T12:15:38Z

Description

Describe the changes made and why they were made.

Ignore if these details are present on the associated Apache Fineract JIRA ticket.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Write the commit message as per https://github.com/apache/fineract/#pull-requests
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 coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/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
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

...rc/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanTransactionRepository.java

...loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanBalanceService.java

adamsaghy · 2025-09-19T12:52:02Z

...loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanBalanceService.java

+        updateLoanSummaryDerivedFieldsInternal(loan, false);
+    }
+
+    public void updateLoanSummaryDerivedFieldsViaPersistence(final Loan loan) {


Same as above...

adamsaghy · 2025-09-19T12:52:40Z

...loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanBalanceService.java

+    }
+
+    private void refreshSummaryAndBalancesForDisbursedLoanInternal(final Loan loan, final boolean viaPersistence) {
+        final Money overpaidBy = viaPersistence ? calculateTotalOverpaymentViaPersistence(loan) : calculateTotalOverpayment(loan);


I dont think we should maintain two versions... use only 1 which fetches the required transactions.

adamsaghy · 2025-09-19T12:52:51Z

...loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanBalanceService.java

+        updateLoanOutstandingBalancesInternal(loan, loanTransactions);
+    }
+
+    private void updateLoanOutstandingBalancesViaPersistence(final Loan loan) {


Same as above

adamsaghy · 2025-09-19T12:54:05Z

...ava/org/apache/fineract/portfolio/loanaccount/service/LoanDownPaymentHandlerServiceImpl.java

                && loanTransactionProcessingService.canProcessLatestTransactionOnly(loan, loanTransaction, currentInstallment); //
        if (processLatest) {
            loanTransactionProcessingService.processLatestTransaction(loan.getTransactionProcessingStrategyCode(), loanTransaction,
                    new TransactionCtx(loan.getCurrency(), loan.getRepaymentScheduleInstallments(), loan.getActiveCharges(),


You might wanna rework the TransactionCtx a little bit and use Builder instead of constructor calls. With Builder pattern we can avoid set values to fields that are irrelevant to us: null and Collections.emptyList

adamsaghy · 2025-09-19T12:55:02Z

...oanaccount/domain/transactionprocessor/impl/AdvancedPaymentScheduleTransactionProcessor.java

        LocalDate currentDate = DateUtils.getBusinessLocalDate();
        Pair<ChangedTransactionDetail, ProgressiveLoanInterestScheduleModel> result = reprocessProgressiveLoanTransactions(disbursementDate,
-                currentDate, loanTransactions, currency, installments, charges);
+                currentDate, loanTransactionsToReprocess, currency, installments, charges, loanTransactions);


what is the difference between loanTransactionsToReprocess and loanTransactions list?

We have a reprocessParticularTransactions method, which in turn uses reprocessLoanTransactions, it passes certain specific transactions as the loanTransactionsToReprocess parameter. However, during reprocessing, we also need all loan transactions for certain calculations, so I introduced a new loanTransactions parameter that contains all loan transactions.

adamsaghy · 2025-09-19T12:55:36Z

...pache/fineract/portfolio/loanaccount/service/InterestScheduleModelRepositoryWrapperImpl.java

            if (savedModel.isPresent() && progressiveLoanModel.get().getBusinessDate().isBefore(businessDate)) {
                ProgressiveTransactionCtx ctx = new ProgressiveTransactionCtx(loan.getCurrency(), loan.getRepaymentScheduleInstallments(),
-                        Set.of(), new MoneyHolder(loan.getTotalOverpaidAsMoney()), new ChangedTransactionDetail(), savedModel.get());
+                        Set.of(), new MoneyHolder(loan.getTotalOverpaidAsMoney()), new ChangedTransactionDetail(), savedModel.get(),


We can use Builder here and we dont need to create "dummy" objects.

I dont see any changes.

adamsaghy · 2025-09-19T12:57:32Z

.../org/apache/fineract/portfolio/loanaccount/service/ReprocessLoanTransactionsServiceImpl.java

+        List<LoanTransaction> transactions = Collections.emptyList();
+        if (loanTransaction.isInterestRefund() || loanTransaction.isContractTermination() || loanTransaction.isChargeback()
+                || loanTransaction.isChargeOff()) {
+            transactions = loan.getLoanTransactions();


why? we should stop using loan.getLoanTransactions() not introducing new occurrences... Please fetch from database the list of transactions that are required.

But you dont need the accrual transactions for example, right? So immediately you can fetch only the absolutely necessary transactions only. Am I missing something here?

adamsaghy · 2025-09-19T12:57:44Z

.../org/apache/fineract/portfolio/loanaccount/service/ReprocessLoanTransactionsServiceImpl.java

        final ChangedTransactionDetail changedTransactionDetail = loanTransactionProcessingService.reprocessLoanTransactions(
                loan.getTransactionProcessingStrategyCode(), loan.getDisbursementDate(), loanTransactions, loan.getCurrency(),
-                loan.getRepaymentScheduleInstallments(), loan.getActiveCharges());
+                loan.getRepaymentScheduleInstallments(), loan.getActiveCharges(), loan.getLoanTransactions());


why? we should stop using loan.getLoanTransactions() not introducing new occurrences... Please fetch from database the list of transactions that are required.

I tried to do so. But here is the same problem as above, connected to calling the loan.addLoanTransaction() method in services.

The challenge:
There are many places where we:

First add new transaction to loan.loanTransactions

Call reprocessing methods

Only after that persist this new transaction

Whenever we creates a new loan transactions, provide that or those transactions and as parameter can be added to the reprocessing.

adamsaghy

Please kindly see my review!

adamsaghy · 2025-09-24T12:48:29Z

.../org/apache/fineract/portfolio/loanaccount/service/LoanTransactionProcessingServiceImpl.java

        } else {
-            return loanRepaymentScheduleTransactionProcessor.reprocessLoanTransactions(disbursementDate, loanTransactions, currency,
-                    installments, charges);
+            return loanRepaymentScheduleTransactionProcessor.reprocessLoanTransactions(disbursementDate, loanTransactionsToReprocess,


what is the difference between loanTransactionsToReprocess and loanTransactions and why to provide both?

…(reverse-replay) related actions

…Transaction, builder for ProgressiveTransactionCtx

…s()" from reprocessTransactionsAndFetchChangedTransactions

mariiaKraievska force-pushed the FINERACT-2324/remove-getLoanTransactions-from-reprocessing-reverse-replay-related-actions branch 4 times, most recently from c014381 to 9238eb9 Compare September 19, 2025 06:08

mariiaKraievska marked this pull request as ready for review September 19, 2025 08:12

adamsaghy reviewed Sep 19, 2025

View reviewed changes

...rc/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanTransactionRepository.java Show resolved Hide resolved

adamsaghy reviewed Sep 19, 2025

View reviewed changes

...loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanBalanceService.java Show resolved Hide resolved

adamsaghy reviewed Sep 19, 2025

View reviewed changes

adamsaghy requested changes Sep 19, 2025

View reviewed changes

mariiaKraievska force-pushed the FINERACT-2324/remove-getLoanTransactions-from-reprocessing-reverse-replay-related-actions branch from 9238eb9 to d33e148 Compare September 19, 2025 14:55

adamsaghy reviewed Sep 24, 2025

View reviewed changes

FINERACT-2324: Remove "loan.getLoanTransactions()" from reprocessing …

dcf9824

…(reverse-replay) related actions

mariiaKraievska force-pushed the FINERACT-2324/remove-getLoanTransactions-from-reprocessing-reverse-replay-related-actions branch from d33e148 to fb91081 Compare September 25, 2025 07:10

FINERACT-2324: Remove "loan.getLoanTransactions()" from processLatest…

9fe158f

…Transaction, builder for ProgressiveTransactionCtx

mariiaKraievska force-pushed the FINERACT-2324/remove-getLoanTransactions-from-reprocessing-reverse-replay-related-actions branch from fb91081 to 9fe158f Compare September 25, 2025 08:09

FINERACT-2324: Intermediate stage of removal "loan.getLoanTransaction…

c04c730

…s()" from reprocessTransactionsAndFetchChangedTransactions

adamsaghy marked this pull request as draft October 15, 2025 08:07

FINERACT-2324: Remove "loan.getLoanTransactions()" from reprocessing (reverse-replay) related actions #5041

Are you sure you want to change the base?

FINERACT-2324: Remove "loan.getLoanTransactions()" from reprocessing (reverse-replay) related actions #5041

Conversation

mariiaKraievska commented Sep 17, 2025

Description

Checklist

Uh oh!

Uh oh!

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants