utafrali commented on code in PR #6782:
URL: https://github.com/apache/fineract/pull/6782#discussion_r4410795141
##########
fineract-core/src/main/java/org/apache/fineract/portfolio/savings/domain/interest/PostingPeriod.java:
##########
@@ -291,32 +291,72 @@ public BigDecimal calculateInterest(final
CompoundInterestValues compoundInteres
// for each compounding period accumulate the amount of interest
// to be applied to the balanced for interest calculation
for (final CompoundingPeriod compoundingPeriod :
this.compoundingPeriods) {
+ final BigDecimal interestUnrounded = interestOn(compoundingPeriod,
compoundInterestValues.getcompoundedInterest());
+ compound(compoundingPeriod, interestUnrounded,
compoundInterestValues);
+ interestEarned = interestEarned.add(interestUnrounded);
+ }
- final BigDecimal interestUnrounded =
compoundingPeriod.calculateInterest(this.interestCompoundingType,
- this.interestCalculationType,
compoundInterestValues.getcompoundedInterest(), this.interestRateAsFraction,
- this.daysInYear,
this.minBalanceForInterestCalculation.getAmount(),
this.overdraftInterestRateAsFraction,
- this.minOverdraftForInterestCalculation.getAmount());
- BigDecimal unCompoundedInterest =
compoundInterestValues.getuncompoundedInterest().add(interestUnrounded);
-
compoundInterestValues.setuncompoundedInterest(unCompoundedInterest);
- LocalDate compoundingPeriodEndDate =
compoundingPeriod.getPeriodInterval().endDate();
- if
(!SavingsCompoundingInterestPeriodType.DAILY.equals(this.interestCompoundingType))
{
- compoundingPeriodEndDate =
determineInterestPeriodEndDateFrom(compoundingPeriod.getPeriodInterval().startDate(),
- this.interestCompoundingType,
compoundingPeriod.getPeriodInterval().endDate(),
- this.getFinancialYearBeginningMonth());
- }
+ updateInterestEarned(interestEarned);
+ return interestEarned;
+ }
- if
(compoundingPeriodEndDate.equals(compoundingPeriod.getPeriodInterval().endDate()))
{
- BigDecimal interestCompounded =
compoundInterestValues.getcompoundedInterest().add(unCompoundedInterest);
-
compoundInterestValues.setCompoundedInterest(interestCompounded);
- compoundInterestValues.setZeroForInterestToBeUncompounded();
- }
- interestEarned = interestEarned.add(interestUnrounded);
+ /**
+ * Calculates the interest of the two periods that the overdraft split
builds for one posting interval, one for the
+ * days in credit and one for the overdrawn days. They cover different
days of the same compounding periods, so they
+ * are calculated one compounding period at a time and the interest of
both is compounded at its end, as it is when
+ * the interval is not split. Calculated one after the other, the second
period would get the interest of the first
+ * compounded into all its days, before the end of the compounding period
it was earned in.
+ */
+ public static BigDecimal calculateInterest(final PostingPeriod first,
final PostingPeriod second,
+ final CompoundInterestValues compoundInterestValues) {
Review Comment:
Having a `public static BigDecimal calculateInterest(PostingPeriod,
PostingPeriod, CompoundInterestValues)` alongside an instance method `public
BigDecimal calculateInterest(CompoundInterestValues)` with the same name is
legal Java but will surprise readers: calling `period.calculateInterest(...)`
when you meant the static overload is an easy mistake. A name like
`calculateInterestForSplitInterval` would make the call sites in
`CompoundInterestHelper` self-documenting and remove any ambiguity.
##########
fineract-savings/src/test/java/org/apache/fineract/portfolio/savings/service/SavingsAccountOverdraftInterestSplitTest.java:
##########
@@ -118,14 +121,58 @@ void
overdraftSplitMonthlyPostingCountsOverdraftCarriedIntoNextMonthOnce() {
.isCloseTo(interestFor(withoutSplit, businessDate,
februaryStart, februaryEnd), within(new BigDecimal("0.01")));
}
+ @Test
+ void
overdraftSplitMonthlyCompoundingChargesOverdraftInterestForDaysOverdrawnInTheSameMonth()
{
+ final LocalDate businessDate = LocalDate.of(2025, 2, 2);
+ final LocalDate januaryEnd = LocalDate.of(2025, 1, 31);
+
+ // in credit from Jan 1 to Jan 7, then overdrawn by 10 until the month
end: the interest of the days in
+ // credit is only compounded at the end of January, so it must not
count for the overdrawn days
+ final SavingsAccountData withoutSplit =
accountOverdrawnWithinFirstMonth(false,
SavingsPostingInterestPeriodType.MONTHLY, "21");
+ final SavingsAccountData withSplit =
accountOverdrawnWithinFirstMonth(true,
SavingsPostingInterestPeriodType.MONTHLY, "21");
+
+ // 10,000 for 7 days at 10% and -10 for 24 days at 21%, both over 365
days
+ assertThat(interestFor(withSplit, businessDate, ACTIVATION_DATE,
januaryEnd, period -> !period.isOverdraftInterest()))
+ .as("interest for the days in
credit").isEqualByComparingTo("19.18");
+ assertThat(interestFor(withSplit, businessDate, ACTIVATION_DATE,
januaryEnd, PostingPeriod::isOverdraftInterest))
+ .as("overdraft interest for the overdrawn
days").isEqualByComparingTo("-0.14");
+ assertThat(interestFor(withSplit, businessDate, ACTIVATION_DATE,
januaryEnd)).as("January interest with the overdraft split")
+ .isCloseTo(interestFor(withoutSplit, businessDate,
ACTIVATION_DATE, januaryEnd), within(new BigDecimal("0.01")));
+ }
+
+ @Test
+ void
overdraftSplitMonthlyCompoundingPaysNoInterestForOverdrawnDaysWithoutOverdraftRate()
{
+ final SavingsAccountData account =
accountOverdrawnWithinFirstMonth(true,
SavingsPostingInterestPeriodType.MONTHLY, "0");
+
+ assertThat(interestFor(account, LocalDate.of(2025, 2, 2),
ACTIVATION_DATE, LocalDate.of(2025, 1, 31),
+ PostingPeriod::isOverdraftInterest)).as("interest for the
overdrawn days").isEqualByComparingTo(BigDecimal.ZERO);
+ }
+
+ @Test
+ void overdraftSplitQuarterlyPostingCompoundsBothPartsAtEachMonthEnd() {
+ final LocalDate businessDate = LocalDate.of(2025, 4, 2);
+ final LocalDate quarterEnd = LocalDate.of(2025, 3, 31);
+
+ final SavingsAccountData withoutSplit =
accountOverdrawnWithinFirstMonth(false,
SavingsPostingInterestPeriodType.QUATERLY, "21");
+ final SavingsAccountData withSplit =
accountOverdrawnWithinFirstMonth(true,
SavingsPostingInterestPeriodType.QUATERLY, "21");
+
+ assertThat(interestFor(withSplit, businessDate, ACTIVATION_DATE,
quarterEnd)).as("Q1 interest with the overdraft split")
+ .isCloseTo(interestFor(withoutSplit, businessDate,
ACTIVATION_DATE, quarterEnd), within(new BigDecimal("0.01")));
+ }
+
private BigDecimal interestFor(final SavingsAccountData account, final
LocalDate businessDate, final LocalDate periodStart,
final LocalDate periodEnd) {
+ return interestFor(account, businessDate, periodStart, periodEnd,
period -> true);
+ }
+
+ private BigDecimal interestFor(final SavingsAccountData account, final
LocalDate businessDate, final LocalDate periodStart,
+ final LocalDate periodEnd, final Predicate<PostingPeriod> part) {
final List<PostingPeriod> postingPeriods =
service.calculateInterestUsing(MATH_CONTEXT, businessDate, false, false, 1,
null, false,
account);
return postingPeriods.stream()
.filter(period ->
period.getPeriodInterval().startDate().equals(periodStart)
&&
period.getPeriodInterval().endDate().equals(periodEnd))
-
.map(PostingPeriod::getInterestEarned).reduce(Money.zero(CURRENCY),
Money::plus).getAmount();
+
.filter(part).map(PostingPeriod::getInterestEarned).reduce(Money.zero(CURRENCY),
Money::plus).getAmount();
}
private SavingsAccountData accountWithTwoDeposits(final boolean
allowOverdraft, final boolean interestReceivableMapped) {
Review Comment:
The PR description explicitly calls out daily compounding as a special case
— the first period's daily interest feeds into the second period's balance each
day — and the static method has a dedicated branch for it
(`SavingsCompoundingInterestPeriodType.DAILY.equals(first.interestCompoundingType)`).
There is no test exercising that path. For a financial calculation fix this is
a meaningful gap: please add a test analogous to
`overdraftSplitQuarterlyPostingCompoundsBothPartsAtEachMonthEnd` but with
`SavingsCompoundingInterestPeriodType.DAILY`, asserting that the split result
is within tolerance of the non-split result.
##########
fineract-core/src/main/java/org/apache/fineract/portfolio/savings/domain/interest/CompoundInterestHelper.java:
##########
@@ -48,23 +48,42 @@ public Money calculateInterestForAllPostingPeriods(final
MonetaryCurrency curren
BigDecimal compoundedInterest = BigDecimal.ZERO;
BigDecimal unCompoundedInterest = BigDecimal.ZERO;
final CompoundInterestValues compoundInterestValues = new
CompoundInterestValues(compoundedInterest, unCompoundedInterest);
- for (final PostingPeriod postingPeriod : allPeriods) {
-
- final BigDecimal interestEarnedThisPeriod =
postingPeriod.calculateInterest(compoundInterestValues);
+ int index = 0;
+ while (index < allPeriods.size()) {
+ final PostingPeriod postingPeriod = allPeriods.get(index);
+ // the overdraft split builds two periods for one posting
interval, which are calculated together
+ final PostingPeriod otherPeriodOfInterval = index + 1 <
allPeriods.size()
+ && hasSameInterval(postingPeriod, allPeriods.get(index +
1)) ? allPeriods.get(index + 1) : null;
+ final List<PostingPeriod> periodsOfInterval;
+ if (otherPeriodOfInterval == null) {
+ postingPeriod.calculateInterest(compoundInterestValues);
+ periodsOfInterval = List.of(postingPeriod);
+ } else {
+ PostingPeriod.calculateInterest(postingPeriod,
otherPeriodOfInterval, compoundInterestValues);
+ periodsOfInterval = List.of(postingPeriod,
otherPeriodOfInterval);
+ }
- final Money moneyToBePostedForPeriod = Money.of(currency,
interestEarnedThisPeriod);
+ for (final PostingPeriod period : periodsOfInterval) {
+ final Money moneyToBePostedForPeriod = Money.of(currency,
period.getInterestEarnedUnrounded());
- interestEarned = interestEarned.plus(moneyToBePostedForPeriod);
- // these checks are for fixed deposit account for not include
- // interest for accounts which has post interest to linked savings
- // account and if already transfered then it includes in interest
- // calculation.
- if (!(postingPeriod.isInterestTransfered() ||
!interestTransferEnabled
- || (lockUntil != null &&
!DateUtils.isAfter(postingPeriod.dateOfPostingTransaction(), lockUntil)))) {
- compoundInterestValues.setCompoundedInterest(BigDecimal.ZERO);
+ interestEarned = interestEarned.plus(moneyToBePostedForPeriod);
+ // these checks are for fixed deposit account for not include
+ // interest for accounts which has post interest to linked
savings
+ // account and if already transfered then it includes in
interest
+ // calculation.
+ if (!(period.isInterestTransfered() || !interestTransferEnabled
+ || (lockUntil != null &&
!DateUtils.isAfter(period.dateOfPostingTransaction(), lockUntil)))) {
+
compoundInterestValues.setCompoundedInterest(BigDecimal.ZERO);
+ }
}
+ index += periodsOfInterval.size();
}
return interestEarned;
}
+
+ private static boolean hasSameInterval(final PostingPeriod postingPeriod,
final PostingPeriod other) {
+ return
postingPeriod.getPeriodInterval().startDate().equals(other.getPeriodInterval().startDate())
+ &&
postingPeriod.getPeriodInterval().endDate().equals(other.getPeriodInterval().endDate());
+ }
Review Comment:
`hasSameInterval` detects the paired split periods by checking that
consecutive entries share the same start and end date. This relies on the two
halves always being placed adjacently in `allPeriods`. Nothing in this file
validates or documents that ordering guarantee. If the list ever comes in a
different order (e.g., sorted differently elsewhere), the pairing silently
fails and the original bug reappears without any error. A short comment stating
the expected invariant — or an assertion checked before the loop — would make
this assumption explicit.
##########
fineract-core/src/main/java/org/apache/fineract/portfolio/savings/domain/interest/PostingPeriod.java:
##########
@@ -291,32 +291,72 @@ public BigDecimal calculateInterest(final
CompoundInterestValues compoundInteres
// for each compounding period accumulate the amount of interest
// to be applied to the balanced for interest calculation
for (final CompoundingPeriod compoundingPeriod :
this.compoundingPeriods) {
+ final BigDecimal interestUnrounded = interestOn(compoundingPeriod,
compoundInterestValues.getcompoundedInterest());
+ compound(compoundingPeriod, interestUnrounded,
compoundInterestValues);
+ interestEarned = interestEarned.add(interestUnrounded);
+ }
- final BigDecimal interestUnrounded =
compoundingPeriod.calculateInterest(this.interestCompoundingType,
- this.interestCalculationType,
compoundInterestValues.getcompoundedInterest(), this.interestRateAsFraction,
- this.daysInYear,
this.minBalanceForInterestCalculation.getAmount(),
this.overdraftInterestRateAsFraction,
- this.minOverdraftForInterestCalculation.getAmount());
- BigDecimal unCompoundedInterest =
compoundInterestValues.getuncompoundedInterest().add(interestUnrounded);
-
compoundInterestValues.setuncompoundedInterest(unCompoundedInterest);
- LocalDate compoundingPeriodEndDate =
compoundingPeriod.getPeriodInterval().endDate();
- if
(!SavingsCompoundingInterestPeriodType.DAILY.equals(this.interestCompoundingType))
{
- compoundingPeriodEndDate =
determineInterestPeriodEndDateFrom(compoundingPeriod.getPeriodInterval().startDate(),
- this.interestCompoundingType,
compoundingPeriod.getPeriodInterval().endDate(),
- this.getFinancialYearBeginningMonth());
- }
+ updateInterestEarned(interestEarned);
+ return interestEarned;
+ }
- if
(compoundingPeriodEndDate.equals(compoundingPeriod.getPeriodInterval().endDate()))
{
- BigDecimal interestCompounded =
compoundInterestValues.getcompoundedInterest().add(unCompoundedInterest);
-
compoundInterestValues.setCompoundedInterest(interestCompounded);
- compoundInterestValues.setZeroForInterestToBeUncompounded();
- }
- interestEarned = interestEarned.add(interestUnrounded);
+ /**
+ * Calculates the interest of the two periods that the overdraft split
builds for one posting interval, one for the
+ * days in credit and one for the overdrawn days. They cover different
days of the same compounding periods, so they
+ * are calculated one compounding period at a time and the interest of
both is compounded at its end, as it is when
+ * the interval is not split. Calculated one after the other, the second
period would get the interest of the first
+ * compounded into all its days, before the end of the compounding period
it was earned in.
+ */
+ public static BigDecimal calculateInterest(final PostingPeriod first,
final PostingPeriod second,
+ final CompoundInterestValues compoundInterestValues) {
+ BigDecimal firstInterestEarned = BigDecimal.ZERO;
+ BigDecimal secondInterestEarned = BigDecimal.ZERO;
+ for (int i = 0; i < first.compoundingPeriods.size(); i++) {
+ final CompoundingPeriod compoundingPeriod =
first.compoundingPeriods.get(i);
Review Comment:
The loop assumes `second.compoundingPeriods.size() ==
first.compoundingPeriods.size()`, which holds today because both split periods
share the same `periodInterval` and `interestCompoundingType` and compounding
periods are derived from those alone. However, this invariant is not expressed
in code — a future change to how split periods are constructed would produce an
`IndexOutOfBoundsException` with no obvious cause.
Add a guard at the top of the method:
```java
if (first.compoundingPeriods.size() != second.compoundingPeriods.size()) {
throw new IllegalArgumentException(
"Overdraft split periods must have the same number of compounding
periods, got " +
first.compoundingPeriods.size() + " and " +
second.compoundingPeriods.size());
}
```
##########
fineract-core/src/main/java/org/apache/fineract/portfolio/savings/domain/interest/CompoundInterestHelper.java:
##########
@@ -48,23 +48,42 @@ public Money calculateInterestForAllPostingPeriods(final
MonetaryCurrency curren
BigDecimal compoundedInterest = BigDecimal.ZERO;
BigDecimal unCompoundedInterest = BigDecimal.ZERO;
final CompoundInterestValues compoundInterestValues = new
CompoundInterestValues(compoundedInterest, unCompoundedInterest);
- for (final PostingPeriod postingPeriod : allPeriods) {
-
- final BigDecimal interestEarnedThisPeriod =
postingPeriod.calculateInterest(compoundInterestValues);
+ int index = 0;
+ while (index < allPeriods.size()) {
+ final PostingPeriod postingPeriod = allPeriods.get(index);
+ // the overdraft split builds two periods for one posting
interval, which are calculated together
+ final PostingPeriod otherPeriodOfInterval = index + 1 <
allPeriods.size()
+ && hasSameInterval(postingPeriod, allPeriods.get(index +
1)) ? allPeriods.get(index + 1) : null;
+ final List<PostingPeriod> periodsOfInterval;
+ if (otherPeriodOfInterval == null) {
+ postingPeriod.calculateInterest(compoundInterestValues);
+ periodsOfInterval = List.of(postingPeriod);
+ } else {
+ PostingPeriod.calculateInterest(postingPeriod,
otherPeriodOfInterval, compoundInterestValues);
+ periodsOfInterval = List.of(postingPeriod,
otherPeriodOfInterval);
Review Comment:
The return value of `PostingPeriod.calculateInterest(postingPeriod,
otherPeriodOfInterval, compoundInterestValues)` is silently discarded; the
caller reads interest amounts via `getInterestEarnedUnrounded()` instead. The
instance `calculateInterest` has the same issue at line 60. If the return value
is no longer meaningful at the call site, changing both signatures to `void`
removes the false impression that callers should use the returned `BigDecimal`.
If it is kept as `BigDecimal` (for use in other contexts), annotate it
`@SuppressWarnings("UnusedReturnValue")` or add a comment, otherwise static
analysis tools will flag it as dead code.
--
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]
