adamsaghy commented on PR #6417:
URL: https://github.com/apache/fineract/pull/6417#issuecomment-5622841148
@Cocoa-Puffs Please review below:
Findings
1. The new integration test's assertion is far weaker than its own failure
message — FeignLoanReAgeAccrualReconciliationTest.java:113
assertTrue(notDueGrowthSinceSettled.compareTo(BigDecimal.ZERO) > 0,
"totalUnpaidPayableNotDueInterest should have grown by roughly " +
totalAccruedSinceSettled[0] + ...)
The message claims the bucket should grow by roughly the accrual total, but
the assert only checks it moved off zero. A regression that reinstates partial
freezing — bucket grows 0.01 against 5.77 of posted accruals — passes. Since
the whole point is that the two reconcile, and you measured exact agreement
(9.68 vs 9.68) in the review thread, assert near-equality with a one-cent
tolerance instead.
2. Third implementation of the same proration formula —
ProgressiveEMICalculator.java:1905
RepaymentPeriod.calculateFixedInterestTillDate() already does exactly this
day-proration and is what the accrual-posting path uses. The new private method
computes the same thing from targetDate directly. I understand why it can't
call the existing one as-is (calculateRateFactorForScheduleTillDateInclusive
only truncates the interest period containing the target date, leaving later
ones intact, so the firstIP.fromDate → lastIP.dueDate span is wrong there).
But these two must stay numerically identical forever, or the summary and
the accrual transactions diverge again — the exact bug this PR fixes. And they
already disagree at one boundary: for a zero-length period (fromDate ==
dueDate) the existing method explicitly returns the full fixed interest (if
(length == 0) ... = getFixedInterest()), while the new one hits
!targetDate.isAfter(rp.getFromDate()) first and zeroes it. I couldn't confirm a
zero-length period with non-zero fixed interest is reachable, so I'm not
calling this a live defect — but it's the kind of drift a shared
RepaymentPeriod.calculateFixedInterestTillDate(LocalDate targetDate) used by
both call sites would prevent.
3. No unit coverage for the new overload
ProgressiveEMICalculatorTest has ~15 existing getOutstandingAmountsTillDate
re-age tests, but none passes fixedInterestTillDate = true. The boundaries
carrying the logic are all untested and cheap to cover: targetDate == fromDate
(→ zero), targetDate == dueDate (→ full), targetDate past due date (→ full),
and totalDays == 0. Right now the only verification is e2e/integration, which
won't tell you which branch broke.
4. Minor: test name states the bug, and asserts the opposite
testNotDueInterestStaysFlatDespiteContinuingDailyAccrualAfterReAge reads as
the repro title; the test now asserts the bucket does not stay flat. Something
like notDueInterestTracksDailyAccrualAfterReAge matches what it guards. Same
for the log.info lines and the "doesn't exercise the reported bug" message —
they read as investigation scaffolding rather than a regression test.
5. Minor: latestNotDue[0].subtract(settledNotDue[0]) at line 105 NPEs if
getTotalUnpaidPayableNotDueInterest() comes back null, which would mask the
real failure behind a confusing stack trace.
Nothing here is a blocker on the production change itself — findings 1 and 3
are the ones I'd want addressed before merge, since as written the new test
barely constrains the behaviour it exists to protect. CI was still pending
across all shards when I checked, so the e2e expectation updates are unverified.
--
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]
