alberto-art3ch commented on PR #6508:
URL: https://github.com/apache/fineract/pull/6508#issuecomment-5896690216
> > > @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
>
> We should not post 0 amount transaction anywhere. i am happy to fix this
issue since it relates to overdraft handling as part of this PR.
>
> It think it is still open:
>
> * The sign-flip test (confirmed): it never reaches the job's correction
code. Its backdated withdrawal makes the withdrawal handling re-post interest
itself, before the job runs. So the test would also pass under the rejected
version it was written to guard against.
> * The zero-amount note (plausible): you said receivable path still posts a
zero-amount transaction, but the job never inserts a zero-amount row
(SavingsSchedularInterestPoster.java:211). The test's "non-zero only" filter
works around something that can't reach the database. It should assert that no
zero-amount posting exists.
@adamsaghy Both points fixed, and I extended the zero-amount fix to the
domain path as you suggested
--
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]
