alberto-art3ch commented on PR #6508:
URL: https://github.com/apache/fineract/pull/6508#issuecomment-5882863396
> @alberto-art3ch Please review the below findings / concerns
>
> ## Summary
> The PR fixes the reported bug. On overdraft products without an
interest-receivable GL mapping, the job now finds the overdraft interest it
already posted, so it stops creating a new one on every run.
>
> The fix, though, changes how the lookup type is chosen for **every**
overdraft product, including those with a receivable mapping. That opens up two
likely regressions. A narrower fix at the lookup call site would solve the
reported bug without them.
>
> > Findings marked **Plausible** come from reading the code. They have not
been reproduced by running the code.
>
> ## Findings
> ### 1. Receivable-GL path: a zero-interest overdraft period can reverse
the real interest posting — _Correctness, Plausible_
> `SavingsAccountInterestPostingServiceImpl.java:89`
>
> On products with `glAccountIdForInterestReceivable` set,
`calculateInterestUsing` builds **two** `PostingPeriod`s per interval, and both
use the same posting date:
>
> * a primary period
> * a secondary period
>
> Before this PR, each period's type was set explicitly with
`setOverdraftInterest(true/false)`. It is now derived from
`interestEarned.isLessThanZero()`. So an overdraft-flagged period whose
interest is exactly **0** now looks up a regular `INTEREST_POSTING`.
>
> **Scenario:**
>
> 1. Take an accrual product with overdraft allowed and an
interest-receivable mapping. Its overdraft rate is 0, or the overdraft stays
below `minOverdraftForInterestCalculation`.
> 2. In one month the running balance is positive for some days and negative
for others.
> 3. The positive period creates interest posting **X**.
> 4. The overdraft period (interest 0) searches `INTEREST_POSTING` on the
same date and finds **X**. Because `hasNotAmount(0)` is true, it **reverses X**
and posts a 0 interest posting.
> 5. Every later run repeats the same reversal.
>
> Before this PR, step 4 searched `OVERDRAFT_INTEREST` and left X alone.
>
> ### 2. When the interest's sign flips, the stale posting of the other type
stays active — _Correctness, Plausible_
> `SavingsAccountInterestPostingServiceImpl.java:89`
>
> The typed lookup only finds a posting whose type matches the sign in the
current run. Suppose the interest for a date that was already posted changes
sign. The existing posting of the other type is then never found or reversed,
and a second posting is created next to it.
>
> **Scenario (overdraft product without a receivable mapping):**
>
> 1. The job posts `INTEREST_POSTING +2.74` on date D.
> 2. The balance for that period later turns negative. This can happen
through a backdated withdrawal, or a pivot-date recalculation that skips the
domain-side repost.
> 3. The next job run gets negative interest. It searches only
`OVERDRAFT_INTEREST` on D, finds nothing, and creates overdraft interest.
**+2.74 stays active.**
>
> Before this PR, the flag was always `false` for these products. The lookup
found +2.74, `correctionRequired` was true, and the posting was reversed and
replaced correctly.
>
> ### 3. The fix is at the wrong level: choose the lookup by path, not by
sign — _Altitude_
> `SavingsAccountInterestPostingServiceImpl.java:93`
>
> A lookup filtered by type is only needed on the receivable-GL path,
because that is the only place two periods share a posting date. Everywhere
else, `findInterestPostingTransactionFor` matches **either** type:
>
> * the non-overdraft branch of this service
> * the domain-side `SavingsAccountPostInterestServiceImpl`
>
> Deriving the type from the sign adds a special case that changes both
paths, and it causes findings 1 and 2.
>
> ### 4. The new tests don't cover the receivable-GL path or sign changes —
_Test coverage_
> `SavingsInterestPostingJobIntegrationTest.java:174`
>
> Both new tests use products with accounting `NONE`, so the receivable-GL
path, whose lookup key also changed, is untested. Findings 1 and 2 would pass
CI as the tests stand.
>
> ### 5. The stored `isOverdraft` flag on the transaction changes from false
to true — _Correctness, Plausible_
> `SavingsAccountInterestPostingServiceImpl.java:110, 146`
>
> The same `isOverdraft` local is passed to
`SavingsAccountTransactionData.overdraftInterest(..., isOverdraft)` and stored
on the transaction's `isOverdraft` field. Lombok and Jackson expose that field.
For products without a receivable mapping it used to be `false` and is now
`true`. I found no internal code that reads it, but the PR description doesn't
mention this side effect.
>
> ### 6. `isOverdraft` is always `true` where it is passed to
`overdraftInterest` — _Simplification_
> `SavingsAccountInterestPostingServiceImpl.java:144`
>
> Both `overdraftInterest(...)` calls sit in the `else` of
`isGreaterThanOrEqualTo(zero)`, where `isLessThanZero()` is always true. The
argument is therefore the constant `true`, which is misleading to a reader. The
local is also a boxed `Boolean` built from a primitive, while
`findInterestPostingTransactionForInterest` takes a primitive `boolean`.
>
> ### 7. The daily-posting test only checks one of the posted dates — _Test
coverage_
> `SavingsInterestPostingJobIntegrationTest.java:183`
>
> `testRunningPostInterestJobTwiceDoesNotCreateDuplicateOverdraftInterest`
runs at 2022-04-13 on a daily-posting product, so postings land on April 11, 12
and 13. The test only counts April 11, so a duplicate on the 12th or 13th would
go unnoticed.
>
> ## Recommendations
> ### Main: choose the lookup by path and keep the per-period flag where it
matters
> Revert the flag change and pick the lookup at the call site instead:
>
> ```java
> final Boolean isOverdraft = interestPostingPeriod.isOverdraftInterest();
> ...
> if (this.depositAccountType(savingsAccountData) ==
DepositAccountType.SAVINGS_DEPOSIT
> && savingsAccountData.isAllowOverdraft()
> &&
!MathUtil.isZero(savingsAccountData.getGlAccountIdForInterestReceivable())) {
> // two posting periods share the posting date only on this path
> postingTransaction =
findInterestPostingTransactionForInterest(interestPostingTransactionDate,
savingsAccountData,
> isOverdraft);
> } else {
> postingTransaction =
findInterestPostingTransactionFor(interestPostingTransactionDate,
savingsAccountData);
> }
> ```
>
> This:
>
> * fixes the reported duplicate on products without a receivable mapping,
because the lookup matches either type
> * keeps the correction when the sign flips (the existing
`correctionRequired` branch already handles a found posting of either type)
> * leaves the receivable-GL path unchanged
>
> **Alternative:** in the `else` branch of `calculateInterestUsing` (around
line 339), set the `PostingPeriod` flag from the sign instead of hard-coding
`false`. The flag would then be meaningful on every path. Note that this still
loses the correction when the sign flips (finding 2).
>
> ### Tests to add
> 1. **Receivable-GL path:** an accrual overdraft product with an
interest-receivable mapping, both positive and negative balances in one month,
and the job run twice. Assert one active posting of each type on the posting
date and no reversed interest posting.
> 2. **Sign flip:** interest posted as positive, then the balance for that
period made negative. Assert that the positive posting is reversed and exactly
one active overdraft interest posting remains.
> 3. **Zero overdraft interest on the receivable path:** overdraft rate 0,
or an overdraft below `minOverdraftForInterestCalculation`. Assert that the
regular interest posting is not reversed.
> 4. **Daily test (finding 7):** assert the per-date count for every posted
date (April 11–13), or the total count as the monthly test does.
>
> ### Minor
> * If the sign-based approach is kept, make `isOverdraft` a primitive
`boolean` and pass `true` directly to `overdraftInterest(...)` (finding 6).
> * Mention the change to the stored `isOverdraft` transaction flag in the
PR description, or avoid it (finding 5).
@adamsaghy
Reverted the sign-based flag and moved the decision to the call site: the
type-filtered lookup now runs only on the interest-receivable path (where two
periods share a posting date), everywhere else the lookup matches either type.
This fixes the reported duplicate, keeps the correction when the sign flips
(findings 1–3), and makes findings 5 and 6 moot since the stored flag reverts
to its previous value.
Added tests for the receivable path (both postings survive two runs), zero
overdraft interest on that path (the regular posting is not reversed), the sign
flip, and per-date counts on the daily test (finding 7).
Note on finding 1: on the receivable path a zero-earning overdraft period
still posts a redundant zero-amount transaction on the same date. That predates
this PR and is not addressed here — Could we handle in this ticket or other one?
@bharathcgowda FYI
--
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]
