FINERACT-1981: Progressive Loan schedule handling - Installment charges by mariiaKraievska · Pull Request #4800 · apache/fineract

mariiaKraievska · 2025-06-24T14:16:41Z

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.

adamsaghy · 2025-06-24T14:46:47Z

@mariiaKraievska Please run spotlessApply to avoid checkstyle issues!

fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java

adamsaghy · 2025-06-26T08:11:05Z

...ineract/portfolio/loanaccount/domain/SingleLoanChargeRepaymentScheduleProcessingWrapper.java

            if (installment.isDownPayment()) {
                continue;
            }
-            boolean installmentChargeApplicable = !installment.isRecalculatedInterestComponent();


Any reason to remove this flag?

I have reverted this change, since we agreed to disallow installment fee charges that are calculated as a percentage of principal or interest when interest recalculation is enabled.

adamsaghy · 2025-06-26T08:11:27Z

...ineract/portfolio/loanaccount/domain/SingleLoanChargeRepaymentScheduleProcessingWrapper.java

        if (loanCharge.isFeeCharge() && loanCharge.isDueAtDisbursement()) {
            return zero;
        }
-        if (loanCharge.isInstalmentFee() && isInstallmentChargeApplicable) {


Any reason to remove this flag?

I have reverted this change, since we agreed to disallow installment fee charges that are calculated as a percentage of principal or interest when interest recalculation is enabled.

adamsaghy · 2025-06-26T08:11:35Z

...ineract/portfolio/loanaccount/domain/SingleLoanChargeRepaymentScheduleProcessingWrapper.java

        if (!predicate.test(loanCharge)) {
            return zero;
        }
-        if (loanCharge.isInstalmentFee() && isInstallmentChargeApplicable) {


Any reason to remove this flag?

I have reverted this change, since we agreed to disallow installment fee charges that are calculated as a percentage of principal or interest when interest recalculation is enabled.

adamsaghy · 2025-06-26T08:12:15Z

...java/org/apache/fineract/portfolio/loanaccount/serialization/LoanChargeApiJsonValidator.java

        for (LoanCharge loanCharge : charges) {
            String errorcode = null;
            switch (loanCharge.getChargeCalculation()) {
-                case PERCENT_OF_AMOUNT:


Any reason to remove these?

I have reverted this change, since we agreed to disallow installment fee charges that are calculated as a percentage of principal or interest when interest recalculation is enabled.

adamsaghy

Please review my questions

adamsaghy · 2025-07-01T18:15:27Z

@mariiaKraievska please rebase

adamsaghy · 2025-07-07T09:50:51Z

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

+        for (LoanInstallmentCharge oldCharge : loanCharge.getLoanInstallmentCharge()) {
+            oldCharge.getInstallment().getInstallmentCharges().remove(oldCharge);
+        }
+        loanCharge.getLoanInstallmentCharge().clear();


Why do we need to drop all charges? That would recreate charges every time... Was the previous version which only updated the existing once wrong?

Thank you for your question!
After reverting to the previous logic, I encountered a database integrity error during testing (specifically on charge-off with accelerate maturity date behaviour), caused by a foreign key constraint violation between m_loan_installment_charge and m_loan_repayment_schedule.
It seems that the old approach does not always properly remove or update all related installment charges before the repayment schedule is changed, which leads to this issue.
That’s why the logic to clear and recreate all charges was introduced — to ensure data consistency and avoid such integrity problems.

Well, it is suboptimal, as we are recreating charges which are unchanged :/ I wonder whether we should have some logic which can compare them properly...

Please check updated version of this method 🙏

adamsaghy · 2025-07-08T08:52:28Z

@mariiaKraievska Please rebase this PR.

mariiaKraievska · 2025-07-08T09:19:43Z

@mariiaKraievska Please rebase this PR.

Done

…dule handling

adamsaghy

LGTM

mariiaKraievska force-pushed the FINERACT-1981/progressive-loan-schedule-handling-installment-charges branch 3 times, most recently from 52c8414 to 81b573a Compare June 25, 2025 09:38

adamsaghy reviewed Jun 26, 2025

View reviewed changes

fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java Show resolved Hide resolved

adamsaghy reviewed Jun 26, 2025

View reviewed changes

adamsaghy requested changes Jun 26, 2025

View reviewed changes

mariiaKraievska force-pushed the FINERACT-1981/progressive-loan-schedule-handling-installment-charges branch 2 times, most recently from c2123dd to eb81c8d Compare July 7, 2025 06:59

adamsaghy reviewed Jul 7, 2025

View reviewed changes

mariiaKraievska force-pushed the FINERACT-1981/progressive-loan-schedule-handling-installment-charges branch from eb81c8d to d6b4e43 Compare July 7, 2025 12:04

mariiaKraievska force-pushed the FINERACT-1981/progressive-loan-schedule-handling-installment-charges branch from 6ba1912 to 6af18e7 Compare July 8, 2025 09:19

mariiaKraievska and others added 2 commits July 8, 2025 23:26

FINERACT-1981: Progressive Loan schedule handling - Installment charges

533dd4c

FINERACT-1981: added e2e test for validation of progressive Loan sche…

a8b56f3

…dule handling

mariiaKraievska force-pushed the FINERACT-1981/progressive-loan-schedule-handling-installment-charges branch from 6af18e7 to a8b56f3 Compare July 8, 2025 20:28

adamsaghy approved these changes Jul 8, 2025

View reviewed changes

adamsaghy marked this pull request as ready for review July 8, 2025 21:22

adamsaghy merged commit 3f359bb into apache:develop Jul 8, 2025
39 checks passed

adamsaghy deleted the FINERACT-1981/progressive-loan-schedule-handling-installment-charges branch July 8, 2025 21:22

Conversation

mariiaKraievska commented Jun 24, 2025

Description

Checklist

Uh oh!

adamsaghy commented Jun 24, 2025

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!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

adamsaghy commented Jul 1, 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!

adamsaghy commented Jul 8, 2025

Uh oh!

mariiaKraievska commented Jul 8, 2025

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants