galovics commented on code in PR #5825:
URL: https://github.com/apache/fineract/pull/5825#discussion_r3221770353
##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/transactionprocessor/impl/AdvancedPaymentScheduleTransactionProcessor.java:
##########
@@ -650,11 +650,17 @@ private void handleInterestRefund(final LoanTransaction
loanTransaction, final T
final Money interestAfterRefund =
interestRefundService.totalInterestByTransactions(this, loan.getId(),
targetDate,
modifiedTransactions, unmodifiedTransactionIds,
ctx.getActiveLoanTermVariations());
final Money newAmount =
interestBeforeRefund.minus(progCtx.getSumOfInterestRefundAmount()).minus(interestAfterRefund);
- loanTransaction.updateAmount(newAmount.getAmount());
+
loanTransaction.updateAmount(MathUtil.negativeToZero(newAmount).getAmount());
Review Comment:
I'm not convinced this is the right fix. Clamping a negative value to zero
hides the question of why `newAmount` went negative in the first place. Under
what scenario does `interestBeforeRefund - sumOfInterestRefundAmount -
interestAfterRefund` come out negative, and is that a legitimate state or a
symptom of something else being off?
If it's legitimate, the comment below explains the `0` case well - but it
should also explain when/why `newAmount` is negative and why zero is the
correct truncation. If it's not legitimate, we're papering over a real bug.
Could you walk me through it?
##########
fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/stepdef/loan/LoanStepDef.java:
##########
@@ -2169,7 +2169,8 @@ public void loanOverpaid(double totalOverpaidExpected) {
() -> fineractClient.loans().retrieveLoan(loanId,
Map.of("staffInSelectedOfficeOnly", "false")));
testContext().set(TestContextKey.LOAN_RESPONSE, loanDetailsResponse);
- Double totalOverpaidActual =
loanDetailsResponse.getTotalOverpaid().doubleValue();
+ Double totalOverpaidActual = (loanDetailsResponse.getTotalOverpaid()
!= null) ? loanDetailsResponse.getTotalOverpaid().doubleValue()
Review Comment:
Why the null check? This method is `loanOverpaid(...)` - if `totalOverpaid`
is null we should fail loudly, not silently coerce to 0.0 and then assert
against expected. That assertion will pass when the API stops returning the
field at all, which is exactly the regression we don't want to miss.
##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/transactionprocessor/impl/AdvancedPaymentScheduleTransactionProcessor.java:
##########
@@ -650,11 +650,17 @@ private void handleInterestRefund(final LoanTransaction
loanTransaction, final T
final Money interestAfterRefund =
interestRefundService.totalInterestByTransactions(this, loan.getId(),
targetDate,
modifiedTransactions, unmodifiedTransactionIds,
ctx.getActiveLoanTermVariations());
final Money newAmount =
interestBeforeRefund.minus(progCtx.getSumOfInterestRefundAmount()).minus(interestAfterRefund);
- loanTransaction.updateAmount(newAmount.getAmount());
+
loanTransaction.updateAmount(MathUtil.negativeToZero(newAmount).getAmount());
}
progCtx.setSumOfInterestRefundAmount(progCtx.getSumOfInterestRefundAmount().add(loanTransaction.getAmount()));
}
}
+ // A zero-amount interest refund has no effect on balances; processing
it would incorrectly
+ // zero the overpaymentHolder via handleOverpayment, causing
subsequent CBR transactions
+ // to see an empty holder and create phantom outstanding on additional
installments.
+ if (!loanTransaction.getAmount(ctx.getCurrency()).isGreaterThanZero())
{
Review Comment:
I'd really like a unit test pinning this down - the bugfix is subtle and the
Cucumber scenarios don't really show what `handleRepayment` is doing to
`overpaymentHolder` here. Please add a test to cover this so it never reoccurs.
Also: the early return is positioned after the
`interestRefundService.handle(...)` block - is that intentional? If the amount
is zero, do we still want the side-effect of
`progCtx.setSumOfInterestRefundAmount(...)` running above? Walk me through it
please.
##########
fineract-e2e-tests-runner/src/test/java/org/apache/fineract/test/initializer/global/LoanProductGlobalInitializerStep.java:
##########
@@ -4961,6 +4961,47 @@ public void initialize() throws Exception {
responseLoanProductsRequestLP2AdvPaymentIntEmiActualActualIntRefundFullZeroIntChargeOffAccrualActivity);
});
+ tasks.add(() -> {
+ // LP2 + zero-interest chargeOff behaviour + progressive loan
schedule + horizontal + interest recalculation
+ // + accrual activity posting, with LAST_INSTALLMENT
future-installment allocation rule baked in for every
+ // advanced-allocation transaction type — exercises code paths
that the more common NEXT_INSTALLMENT
Review Comment:
Please use a regular dash (`-`) instead of em-dashes here and in the lines
below. Match the rest of the file.
##########
fineract-e2e-tests-runner/src/test/resources/features/LoanCBR.feature:
##########
@@ -1789,3 +1789,202 @@ Feature: Credit Balance Refund
| 01 April 2025 | Disbursement | 243.79 | 0.0 | 0.0
| 0.0 | 0.0 | 243.79 | false | false |
| 01 April 2025 | Down Payment | 61.0 | 61.0 | 0.0
| 0.0 | 0.0 | 182.79 | false | false |
Then Loan status will be "ACTIVE"
+
+ @TestRailId:C78812
+ Scenario: Verify that Loan ends in correct state after CBR and backdated
Goodwill Credit with both totalOutstanding and totalOverpaid > 0
+ When Admin sets the business date to "02 September 2025"
+ And Admin creates a client with random data
+
+ # Migration loan: submitted & disbursed back-dated to 06 April 2025, 6
monthly installments
+ And Admin creates a fully customized loan with the following data:
+ | LoanProduct
| submitted on date | with Principal | ANNUAL
interest rate % | interest type | interest calculation period |
amortization type | loanTermFrequency | loanTermFrequencyType | repaymentEvery
| repaymentFrequencyType | numberOfRepayments | graceOnPrincipalPayment |
graceOnInterestPayment | interest free period | Payment strategy |
+ |
LP2_ADV_PYMNT_INT_DAILY_EMI_ACTUAL_ACTUAL_INT_REFUND_FULL_ZERO_INT_CHARGE_OFF_ACCRUAL_ACTIVITY_LI
| 06 April 2025 | 1316.49 | 12.2062 |
DECLINING_BALANCE | DAILY | EQUAL_INSTALLMENTS | 6
| MONTHS | 1 | MONTHS | 6
| 0 | 0 | 0
| ADVANCED_PAYMENT_ALLOCATION |
+ And Admin successfully approves the loan on "06 April 2025" with "1316.49"
amount and expected disbursement date on "06 April 2025"
+ And Admin successfully disburse the loan on "06 April 2025" with "1316.49"
EUR transaction amount
+
+ # 4 backdated AUTOPAY repayments of 227.31 EUR (still on system date 02
September 2025)
+ And Customer makes "REPAYMENT" transaction with "AUTOPAY" payment type on
"06 May 2025" with 227.31 EUR transaction amount and system-generated
Idempotency key
+ And Customer makes "REPAYMENT" transaction with "AUTOPAY" payment type on
"06 June 2025" with 227.31 EUR transaction amount and system-generated
Idempotency key
+ And Customer makes "REPAYMENT" transaction with "AUTOPAY" payment type on
"06 July 2025" with 227.31 EUR transaction amount and system-generated
Idempotency key
+ And Customer makes "REPAYMENT" transaction with "AUTOPAY" payment type on
"06 August 2025" with 227.31 EUR transaction amount and system-generated
Idempotency key
+
+ # 5th installment via AUTOPAY on 06 September 2025
+ When Admin sets the business date to "06 September 2025"
+ And Admin runs inline COB job for Loan
+ And Customer makes "REPAYMENT" transaction with "AUTOPAY" payment type on
"06 September 2025" with 227.31 EUR transaction amount and system-generated
Idempotency key
+
+ # 6th installment via REAL_TIME on 02 October 2025
+ When Admin sets the business date to "02 October 2025"
+ And Admin runs inline COB job for Loan
+ And Customer makes "REPAYMENT" transaction with "REAL_TIME" payment type
on "02 October 2025" with 227.00 EUR transaction amount and system-generated
Idempotency key
+
+ # 3 MIRs (interestRefundCalculation=false) on 29 October 2025 → loan flips
to OVERPAID
+ When Admin sets the business date to "29 October 2025"
+ And Admin runs inline COB job for Loan
+ And Customer makes "MERCHANT_ISSUED_REFUND" transaction with "AUTOPAY"
payment type on "29 October 2025" with 242.00 EUR transaction amount and
system-generated Idempotency key and interestRefundCalculation false
+ And Customer makes "MERCHANT_ISSUED_REFUND" transaction with "AUTOPAY"
payment type on "29 October 2025" with 242.00 EUR transaction amount and
system-generated Idempotency key and interestRefundCalculation false
+ And Customer makes "MERCHANT_ISSUED_REFUND" transaction with "AUTOPAY"
payment type on "29 October 2025" with 30.49 EUR transaction amount and
system-generated Idempotency key and interestRefundCalculation false
+ Then Loan status will be "OVERPAID"
+
+ # 30 October 2025 → CBR 514.49
+ When Admin sets the business date to "30 October 2025"
+ And Admin runs inline COB job for Loan
+ And Admin makes Credit Balance Refund transaction on "30 October 2025"
with 514.49 EUR transaction amount
+
+ # 11 December 2025 → INTEREST_REFUND on MIR1 + backdated GOODWILL_CREDIT
(txn date 28 October 2025, BEFORE the MIRs)
+ When Admin sets the business date to "11 December 2025"
+ And Admin runs inline COB job for Loan
+ And Admin manually adds Interest Refund for "1"th "MERCHANT_ISSUED_REFUND"
transaction made on "29 October 2025" with 0.01 EUR interest refund amount
+ And Customer makes "GOODWILL_CREDIT" transaction with "AUTOPAY" payment
type on "28 October 2025" with 0.01 EUR transaction amount and system-generated
Idempotency key
+
+ # 12 December 2025 → CBR 27.92
+ When Admin sets the business date to "12 December 2025"
+ And Admin runs inline COB job for Loan
+ And Admin makes Credit Balance Refund transaction on "12 December 2025"
with 27.92 EUR transaction amount
+
+ # 16 December 2025 → INTEREST_REFUND on MIR2 & MIR3 + another backdated
GOODWILL_CREDIT
+ When Admin sets the business date to "16 December 2025"
+ And Admin runs inline COB job for Loan
+ And Admin manually adds Interest Refund for "2"th "MERCHANT_ISSUED_REFUND"
transaction made on "29 October 2025" with 0.01 EUR interest refund amount
+ And Admin manually adds Interest Refund for "3"th "MERCHANT_ISSUED_REFUND"
transaction made on "29 October 2025" with 0.01 EUR interest refund amount
+ And Customer makes "GOODWILL_CREDIT" transaction with "AUTOPAY" payment
type on "28 October 2025" with 0.01 EUR transaction amount and system-generated
Idempotency key
+
+ # 17 December 2025 → final CBR 0.01 — should fully close the loan
+ When Admin sets the business date to "17 December 2025"
+ And Admin runs inline COB job for Loan
+ And Admin makes Credit Balance Refund transaction on "17 December 2025"
with 0.01 EUR transaction amount
+ Then Loan has 0.0 outstanding amount
+ And Loan has 0.0 overpaid amount
+ And Loan status will be "CLOSED_OBLIGATIONS_MET"
+ And Loan Repayment schedule has 6 periods, with the following data for
periods:
+ | Nr | Days | Date | Paid date | Balance of loan |
Principal due | Interest | Fees | Penalties | Due | Paid | In advance |
Late | Outstanding |
+ | | | 06 April 2025 | | 1316.49 |
| | 0.0 | | 0.0 | 0.0 | |
| |
+ | 1 | 30 | 06 May 2025 | 06 May 2025 | 1102.39 |
214.1 | 13.21 | 0.0 | 0.0 | 227.31 | 227.31 | 0.0 |
0.0 | 0.0 |
+ | 2 | 31 | 06 June 2025 | 06 June 2025 | 886.51 |
215.88 | 11.43 | 0.0 | 0.0 | 227.31 | 227.31 | 0.0 |
0.0 | 0.0 |
+ | 3 | 30 | 06 July 2025 | 06 July 2025 | 668.09 |
218.42 | 8.89 | 0.0 | 0.0 | 227.31 | 227.31 | 0.0 |
0.0 | 0.0 |
+ | 4 | 31 | 06 August 2025 | 06 August 2025 | 447.71 |
220.38 | 6.93 | 0.0 | 0.0 | 227.31 | 227.31 | 0.0 |
0.0 | 0.0 |
+ | 5 | 31 | 06 September 2025 | 06 September 2025 | 225.04 |
222.67 | 4.64 | 0.0 | 0.0 | 227.31 | 227.31 | 0.0 |
0.0 | 0.0 |
+ | 6 | 30 | 06 October 2025 | 02 October 2025 | 0.0 |
225.04 | 1.96 | 0.0 | 0.0 | 227.0 | 227.0 | 227.0 |
0.0 | 0.0 |
+ And Loan Repayment schedule has the following data in Total row:
+ | Principal due | Interest | Fees | Penalties | Due | Paid | In
advance | Late | Outstanding |
+ | 1316.49 | 47.06 | 0.0 | 0.0 | 1363.55 | 1363.55 |
227.0 | 0.0 | 0.0 |
+
+ @TestRailId:C78851
+ Scenario: Verify that backdated GoodwillCredit on fully paid loan followed
by CBR closes the loan
+ When Admin sets the business date to "15 December 2025"
+ And Admin creates a client with random data
+ And Admin creates a fully customized loan with the following data:
+ | LoanProduct
| submitted on date | with Principal | ANNUAL
interest rate % | interest type | interest calculation period |
amortization type | loanTermFrequency | loanTermFrequencyType | repaymentEvery
| repaymentFrequencyType | numberOfRepayments | graceOnPrincipalPayment |
graceOnInterestPayment | interest free period | Payment strategy |
+ |
LP2_ADV_PYMNT_INT_DAILY_EMI_ACTUAL_ACTUAL_INT_REFUND_FULL_ZERO_INT_CHARGE_OFF_ACCRUAL_ACTIVITY_LI
| 01 January 2025 | 300 | 0 |
DECLINING_BALANCE | DAILY | EQUAL_INSTALLMENTS | 3
| MONTHS | 1 | MONTHS | 3
| 0 | 0 | 0
| ADVANCED_PAYMENT_ALLOCATION |
+ And Admin successfully approves the loan on "01 January 2025" with "300"
amount and expected disbursement date on "01 January 2025"
+ And Admin successfully disburse the loan on "01 January 2025" with "300"
EUR transaction amount
+ And Customer makes "REPAYMENT" transaction with "AUTOPAY" payment type on
"01 February 2025" with 100.00 EUR transaction amount and system-generated
Idempotency key
+ And Customer makes "REPAYMENT" transaction with "AUTOPAY" payment type on
"01 March 2025" with 100.00 EUR transaction amount and system-generated
Idempotency key
+ And Customer makes "REPAYMENT" transaction with "AUTOPAY" payment type on
"01 April 2025" with 100.00 EUR transaction amount and system-generated
Idempotency key
+ Then Loan status will be "CLOSED_OBLIGATIONS_MET"
+ # Backdated GoodwillCredit dated BEFORE maturity (01 April 2025) on an
already-closed loan
+ When Customer makes "GOODWILL_CREDIT" transaction with "AUTOPAY" payment
type on "15 March 2025" with 0.50 EUR transaction amount and system-generated
Idempotency key
+ Then Loan status will be "OVERPAID"
+ And Loan has 0.0 outstanding amount
+ And Loan has 0.5 overpaid amount
+ # CBR equal to overpayment closes the loan; the schedule's last
installment must remain intact
+ When Admin makes Credit Balance Refund transaction on "15 April 2025" with
0.5 EUR transaction amount
+ Then Loan status will be "CLOSED_OBLIGATIONS_MET"
+ And Loan has 0.0 outstanding amount
+ And Loan has 0.0 overpaid amount
+ And Loan Repayment schedule has 3 periods, with the following data for
periods:
+ | Nr | Days | Date | Paid date | Balance of loan |
Principal due | Interest | Fees | Penalties | Due | Paid | In advance | Late
| Outstanding |
+ | | | 01 January 2025 | | 300.0 |
| | 0.0 | | 0.0 | 0.0 | | |
|
+ | 1 | 31 | 01 February 2025 | 01 February 2025 | 200.0 |
100.0 | 0.0 | 0.0 | 0.0 | 100.0 | 100.0 | 0.0 | 0.0
| 0.0 |
+ | 2 | 28 | 01 March 2025 | 01 March 2025 | 100.0 |
100.0 | 0.0 | 0.0 | 0.0 | 100.0 | 100.0 | 0.0 | 0.0
| 0.0 |
+ | 3 | 31 | 01 April 2025 | 01 April 2025 | 0.0 |
100.0 | 0.0 | 0.0 | 0.0 | 100.0 | 100.0 | 0.5 | 0.0
| 0.0 |
+
+ @TestRailId:C78852
+ Scenario: Verify that Reverse-replay reduces overpayment so an earlier CBR
re-runs with principalPortion > 0
+ When Admin sets the business date to "15 December 2025"
+ And Admin creates a client with random data
+ And Admin creates a fully customized loan with the following data:
+ | LoanProduct
| submitted on date | with Principal | ANNUAL
interest rate % | interest type | interest calculation period |
amortization type | loanTermFrequency | loanTermFrequencyType | repaymentEvery
| repaymentFrequencyType | numberOfRepayments | graceOnPrincipalPayment |
graceOnInterestPayment | interest free period | Payment strategy |
+ |
LP2_ADV_PYMNT_INT_DAILY_EMI_ACTUAL_ACTUAL_INT_REFUND_FULL_ZERO_INT_CHARGE_OFF_ACCRUAL_ACTIVITY_LI
| 01 January 2025 | 300 | 0 |
DECLINING_BALANCE | DAILY | EQUAL_INSTALLMENTS | 3
| MONTHS | 1 | MONTHS | 3
| 0 | 0 | 0
| ADVANCED_PAYMENT_ALLOCATION |
+ And Admin successfully approves the loan on "01 January 2025" with "300"
amount and expected disbursement date on "01 January 2025"
+ And Admin successfully disburse the loan on "01 January 2025" with "300"
EUR transaction amount
+ And Customer makes "REPAYMENT" transaction with "AUTOPAY" payment type on
"01 February 2025" with 100.00 EUR transaction amount and system-generated
Idempotency key
+ And Customer makes "REPAYMENT" transaction with "AUTOPAY" payment type on
"01 March 2025" with 100.00 EUR transaction amount and system-generated
Idempotency key
+ # Final repayment overpays by 50 EUR
+ And Customer makes "REPAYMENT" transaction with "AUTOPAY" payment type on
"01 April 2025" with 150.00 EUR transaction amount and system-generated
Idempotency key
+ Then Loan status will be "OVERPAID"
+ And Loan has 50.0 overpaid amount
+ # CBR equals overpayment, after maturity → loan closes
+ When Admin makes Credit Balance Refund transaction on "15 April 2025" with
50 EUR transaction amount
+ Then Loan status will be "CLOSED_OBLIGATIONS_MET"
+ And Loan has 0.0 outstanding amount
+ # Reverse the SECOND repayment → reverse-replay re-runs the CBR with
smaller overpayment
+ When Customer undo "1"th repayment on "01 March 2025"
+ Then Loan status will be "ACTIVE"
+ And Loan has 100.0 outstanding amount
+ And Loan Repayment schedule has 4 periods, with the following data for
periods:
+ | Nr | Days | Date | Paid date | Balance of loan |
Principal due | Interest | Fees | Penalties | Due | Paid | In advance | Late
| Outstanding |
+ | | | 01 January 2025 | | 300.0 |
| | 0.0 | | 0.0 | 0.0 | | |
|
+ | 1 | 31 | 01 February 2025 | 01 March 2025 | 200.0 | 100.0
| 0.0 | 0.0 | 0.0 | 100.0 | 100.0 | 0.0 | 100.0 |
0.0 |
+ | 2 | 28 | 01 March 2025 | 01 April 2025 | 100.0 | 100.0
| 0.0 | 0.0 | 0.0 | 100.0 | 100.0 | 0.0 | 100.0 |
0.0 |
+ | 3 | 31 | 01 April 2025 | | 0.0 | 100.0
| 0.0 | 0.0 | 0.0 | 100.0 | 50.0 | 0.0 | 0.0 |
50.0 |
+ | 4 | 14 | 15 April 2025 | | 0.0 | 50.0
| 0.0 | 0.0 | 0.0 | 50.0 | 0.0 | 0.0 | 0.0 |
50.0 |
+
+ @TestRailId:C78853
+ Scenario: Verify CBR + backdated GoodwillCredit on NEXT_INSTALLMENT product
closes the loan
Review Comment:
C78812 and C78853 look almost identical step-by-step, the only material
difference being the product (LAST_INSTALLMENT vs NEXT_INSTALLMENT). That's
fine for coverage but the diff is enormous - any chance you could DRY this with
a Scenario Outline / Examples table over the product name? Not a blocker, but
~200 lines of near-duplicate Gherkin is hard to maintain.
##########
fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/data/loanproduct/DefaultLoanProduct.java:
##########
@@ -97,6 +97,7 @@ public enum DefaultLoanProduct implements LoanProduct {
LP2_ADV_PYMNT_INTEREST_DAILY_EMI_ACTUAL_ACTUAL_INTEREST_REFUND_INTEREST_RECALCULATION_MULTIDISB,
//
LP2_ADV_PYMNT_INTEREST_DAILY_INTEREST_RECALCULATION_ZERO_INTEREST_CHARGE_OFF_BEHAVIOUR,
//
LP2_ADV_PYMNT_INTEREST_DAILY_INTEREST_RECALCULATION_ZERO_INTEREST_CHARGE_OFF, //
+
LP2_ADV_PYMNT_INT_DAILY_EMI_ACTUAL_ACTUAL_INT_REFUND_FULL_ZERO_INT_CHARGE_OFF_ACCRUAL_ACTIVITY_LI,
//
Review Comment:
These enum names are getting completely out of hand. I won't block on it
since the rest of the file is just as bad - but for the future: what does `LI`
mean to someone who didn't read this PR? Just `LAST_INSTALLMENT` would be
clearer than another cryptic 2-letter suffix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
