[Bhavya-Sonigra]

Description

Overview

• Scope: Part of FINERACT-3147 / FINERACT-3149 ("Split large service classes by functionality"), addressing the decomposition of the LoanWritePlatformServiceJpaRepositoryImpl monolith (~3,700 lines).
• Design & Compatibility: Pure architectural refactoring with zero behavioral or calculation changes. All 13 extracted transaction methods continue to be exposed through LoanWritePlatformServiceJpaRepositoryImpl via direct delegation, guaranteeing complete backward compatibility for all existing callers.
• Verification: Validated with dedicated unit tests for delegation and transaction execution, Spotless, Checkstyle, RAT checks, and verified against all loan transaction integration test suites.

---

Summary of Changes

Area / Component	Change Type	Details
fineract-loan / LoanTransactionWritePlatformService.java	New Interface	Declares the contract for the 13 loan transaction methods (makeLoanRepayment, adjustLoanTransaction, chargebackLoanTransaction, etc.).
fineract-provider / LoanTransactionWritePlatformServiceImpl.java	New Service	Implements the transaction write logic and internal helper routines.
fineract-provider / LoanWritePlatformServiceJpaRepositoryImpl.java	Refactor	Injects LoanTransactionWritePlatformService, delegates transaction calls, removes 6 obsolete dependencies, and reduces class size by >800 lines.
fineract-provider / LoanAccountConfiguration.java	Configuration	Registers the LoanTransactionWritePlatformService Spring bean definition.
fineract-provider / *Test.java	Testing	Added unit tests in LoanTransactionWritePlatformServiceImplTest and 13 delegation tests in LoanWritePlatformServiceJpaRepositoryImplTest.

Extracted Transaction Methods (13)

1. makeGLIMLoanRepayment
2. makeLoanRepayment
3. makeLoanRepaymentWithChargeRefundChargeType
4. makeInterestPaymentWaiver
5. adjustLoanTransaction
6. chargebackLoanTransaction
7. waiveInterestOnLoan
8. recoverFromGuarantor
9. creditBalanceRefund
10. makeLoanRefund
11. makeRefund
12. makeManualInterestRefund
13. handleChargebackTransaction

(Associated internal helper routines migrated: recalculateLoanWithInterestPaymentWaiverTxn, reprocessChangedLoanTransactions, saveNote, validateLoanTransactionAmountChargeBack, waiveInterest, checkIfLoanIsPaidInAdvance, saveAndFlushLoanWithDataIntegrityViolationChecks, checkClientOrGroupActive, createNote).

Checklist

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

• 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.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
• I followed the AI Policy.

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

[galovics]

Hi @Bhavya-Sonigra, thanks for the PR, splitting this class has been overdue for a long time.

I diffed the moved methods against develop and they are a faithful move (only blank lines and one dead commented-out line dropped), and the @Transactional annotations came along too. Nice. A couple of things though.

The bigger question for me is the delegation layer. Right now LoanWritePlatformService still declares all 13 methods and LoanWritePlatformServiceJpaRepositoryImpl just forwards them, so the interface stays as big as before. Is the plan to point the command handlers at LoanTransactionWritePlatformService directly in a follow-up and then remove these from LoanWritePlatformService? I'm fine with doing it in 2 steps, but if that's the plan, please make it part of the ticket so the forwarding methods don't stay forever. Thoughts?

CI never ran on this PR, so I can't approve yet. Once the workflows are approved and green I'm happy to go ahead.

fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanTransactionWritePlatformServiceImpl.java:966

+    private void checkClientOrGroupActive(final Loan loan) {
+        final Client client = loan.client();
+        if (client != null && client.isNotActive()) {
+            throw new ClientNotActiveException(client.getId());
+        }

This is a 1:1 copy of LoanUtilService.checkClientOrGroupActive, and you already inject loanUtilService here. Can we just call that one instead of adding yet another copy?

Same goes for createNote and saveAndFlushLoanWithDataIntegrityViolationChecks, they now exist in both classes byte-for-byte. I'd rather not double the copy paste while we're splitting the class to reduce maintenance cost. What do you think?

fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java:2385

+    public void handleChargebackTransaction(final Loan loan, LoanTransaction chargebackTransaction) {
+        this.loanTransactionWritePlatformService.handleChargebackTransaction(loan, chargebackTransaction);
+    }

This one isn't on LoanWritePlatformService and after the move nobody calls it anymore (the only caller was chargebackLoanTransaction, which moved too). Let's get rid of it, together with its delegation test.

Thanks.

[Bhavya-Sonigra]

Hi @galovics,

Thank you for the thorough review and constructive feedback! All points have been addressed in the latest update:

1. Delegation Layer & 2-Step Approach:

   • A 2-step approach makes complete sense. Step 1 (this PR) focuses on a clean, faithful service extraction with zero risk of breaking callers via forwarding.
   • As suggested, I will create a dedicated follow-up subtask under FINERACT-3147 to rewire the command handlers directly to LoanTransactionWritePlatformService and drop the deprecated forwarders from LoanWritePlatformService.

2. Deduplication of Helper Methods:

   • checkClientOrGroupActive: Removed the duplicate private implementations in both LoanTransactionWritePlatformServiceImpl and LoanWritePlatformServiceJpaRepositoryImpl; both now call this.loanUtilService.checkClientOrGroupActive(loan).
   • createNote: Centralized note creation in LoanUtilService (implementing ILoanUtilService), reusing its existing NoteRepository.
   • saveAndFlushLoanWithDataIntegrityViolationChecks: Enhanced LoanAccountServiceImpl to persist loan.getRepaymentScheduleInstallments() before flushing (preserving the EclipseLink workaround), and updated both services to delegate to loanAccountService.saveAndFlushLoanWithDataIntegrityViolationChecks(loan).
   • Removed unused fields (loanRepaymentScheduleInstallmentRepository, loanRepositoryWrapper, noteRepository) from LoanTransactionWritePlatformServiceImpl.

3. handleChargebackTransaction:

   • Removed from LoanWritePlatformServiceJpaRepositoryImpl along with its delegation test.
   • Removed from LoanTransactionWritePlatformService interface and made package-private in LoanTransactionWritePlatformServiceImpl.

Could you please approve and trigger the GitHub Actions workflow when you have a moment? Thanks!

[galovics]

Thanks for the quick turnaround @Bhavya-Sonigra. CI is green now, the duplicated checkClientOrGroupActive / createNote copies are gone and handleChargebackTransaction is cleaned up, nice. The 2-step plan works for me, please link the follow-up ticket here once it exists so we don't forget about the forwarding methods.

One thing I'm not comfortable with though, see below. The rest is fine.

fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanAccountServiceImpl.java:56

     public Loan saveAndFlushLoanWithDataIntegrityViolationChecks(final Loan loan) {
         try {
+            loanRepaymentScheduleInstallmentRepository.saveAll(loan.getRepaymentScheduleInstallments());
             return this.loanRepositoryWrapper.saveAndFlush(loan);

Hmm, this isn't a refactor anymore. LoanAccountService.saveAndFlushLoanWithDataIntegrityViolationChecks is shared, it's called from LoanAccountDomainServiceJpa (so every repayment), LoanChargeWritePlatformServiceImpl, LoanRescheduleRequestWritePlatformServiceImpl and LoanAdjustmentServiceImpl too. With this line all of those now do an extra saveAll on every installment of the loan, which they didn't do before. Plus the bean is @ConditionalOnMissingBean, so anybody providing their own LoanAccountService silently loses the EclipseLink workaround that the 2 write services relied on. And the comment explaining why the workaround is there got dropped on the way.

I'd rather keep the other callers untouched in this PR. Either keep a single private copy of the old method in one place (e.g. next to the 2 write services) or add a separate, explicitly named method on LoanAccountService for the "save installments first" variant, and carry the EclipseLink comment over. If you really want to unify the behaviour for all callers, that's fine, but let's do it in a separate PR with its own ticket. Thoughts?

Thanks.

[Bhavya-Sonigra]

Hi @galovics,

Thanks for catching that! Modifying the existing shared method indeed had unintended side-effects on all other callers across the codebase.

I have updated the PR following your suggestion:

1. Restored Original Method: LoanAccountService.saveAndFlushLoanWithDataIntegrityViolationChecks has been restored to its original implementation without the installment saveAll, ensuring all existing callers (LoanAccountDomainServiceJpa, LoanChargeWritePlatformServiceImpl, LoanRescheduleRequestWritePlatformServiceImpl, LoanAdjustmentServiceImpl) remain completely untouched.
2. Dedicated Workaround Method: Added a separate, explicitly named method saveAndFlushLoanAndInstallmentsWithDataIntegrityViolationChecks on LoanAccountService and LoanAccountServiceImpl carrying over the full EclipseLink explanation comment.
3. Write Services: Pointed LoanWritePlatformServiceJpaRepositoryImpl and LoanTransactionWritePlatformServiceImpl exclusively to this new method.
4. Follow-up Ticket: Created and linked the follow-up subtask Fineract-3221 to rewire the command handlers and remove the forwarding methods.

All unit tests, Spotless, RAT, and Checkstyle are passing locally. Could you please take another look and trigger the workflows when you have a moment? Thanks!