You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "alberto-art3ch (via GitHub)" <gi...@apache.org> on 2026/09/25 03:19:07 UTC
[PR] FINERACT-2855: Savings duplicated overdraft interest transaction [fineract]
alberto-art3ch opened a new pull request, #6508:
URL: https://github.com/apache/fineract/pull/6508
## Description
The savings interest posting job looked up the existing posting by a PostingPeriod flag that is only set for products mapped to an interest receivable GL account, so overdraft interest on any other product was searched as a regular interest posting, never matched, and re-created on every run.
The lookup now derives the transaction type from the sign of the interest earned — the same criterion already used to pick the transaction factory — and two integration tests cover the duplicate on repeated runs and on successive business dates.
[FINERACT-2855](https://issues.apache.org/jira/browse/FINERACT-2855)
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update [unit or integration tests](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [ ] Follow our [coding conventions](https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions).
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [ ] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
- [ ] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
- [ ] I followed the [AI Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy).
Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2855: Savings duplicated overdraft interest transaction [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6508:
URL: https://github.com/apache/fineract/pull/6508#issuecomment-5831189632
@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).
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2855: Savings duplicated overdraft interest transaction [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #6508:
URL: https://github.com/apache/fineract/pull/6508
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2855: Savings duplicated overdraft interest transaction [fineract]
Posted by "alberto-art3ch (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2855: Savings duplicated overdraft interest transaction [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6508:
URL: https://github.com/apache/fineract/pull/6508#issuecomment-5889941504
> > @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.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2855: Savings duplicated overdraft interest transaction [fineract]
Posted by "bharathcgowda (via GitHub)" <gi...@apache.org>.
bharathcgowda commented on PR #6508:
URL: https://github.com/apache/fineract/pull/6508#issuecomment-5827408992
@adamsaghy could you please review this PR?
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2855: Savings duplicated overdraft interest transaction [fineract]
Posted by "alberto-art3ch (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2855: Savings duplicated overdraft interest transaction [fineract]
Posted by "alberto-art3ch (via GitHub)" <gi...@apache.org>.
alberto-art3ch commented on PR #6508:
URL: https://github.com/apache/fineract/pull/6508#issuecomment-6277110916
> Hi @alberto-art3ch, thanks for the PR. The tests for the successive-business-dates and receivable-product cases are what I'd want, but the code and the description do not match.
>
> * The description says the lookup "derives the transaction type from the sign of the interest earned". The diff adds `!MathUtil.isZero(getGlAccountIdForInterestReceivable())`, which gates on the receivable GL mapping. Which one is it? `MathUtil.isZero(Long)` is false for null, so this only works because the read service turns null into 0 - is that guaranteed on every path that builds `SavingsAccountData`? An explicit mapped/unmapped flag (or an extracted `hasInterestReceivableMapping()`) would be less fragile.
> * `hasInterestToPost` / the zero skip changes behaviour for every savings product: periods that earn zero no longer post a zero-amount transaction. That affects 0% products, zero-balance days, reports that count postings and `recalucateDailyBalanceDetails`. Bugfix? If it is needed, say why in the description and add a test for a plain 0% product, otherwise split it out.
> * Why touch the legacy `SavingsAccountPostInterestServiceImpl` at all? The bug is not fixed there and `addPostingTransaction` / `addTransaction` are single-use helpers. Keep this PR to the lookup fix. `Boolean isOverdraft` should be a primitive.
> * `testBackdatedWithdrawalReplacesTheInterestPostingWithOverdraftInterest` - its javadoc admits the withdrawal does the correction, not the job, so it passes without the fix. Please remove it. The two near-identical overdraft product builders and 5 count helpers could be one parametrised builder and two helpers.
Thanks @galovics, all four points addressed in 5e09f47.
Gate vs. description. The description was stale from an earlier iteration, rewritten now. The typed lookup is gated on the interest receivable mapping on purpose: that is the same condition calculateInterestUsing already uses to split each interval into a positive-balance period and an overdraft period sharing one posting date. Only there does the lookup need to tell the two types apart. Every other product gets a single period per date, and the lookup matches either type. On null: you're right that MathUtil.isZero(Long) is false for null. It only worked because the interest posting row mapper reads the column with rs.getLong (null → 0), which is the only path that populates it. I extracted SavingsAccountData.hasInterestReceivableMapping(), which treats null and 0 alike, and used it for both the split and the lookup so they can't drift.
Zero skip. Now scoped to receivable-mapped products only. There, one of the two periods sharing a posting date regularly earns zero. A zero amount is created as a regular interest posting whatever the period type, so the typed lookup of a zero-earning overdraft period never matches it and would post it again on every run (covered by testZeroOverdraftInterestPostsNothingAndLeavesTheInterestPostingAlone). Every other product, 0% ones included, keeps posting zero amounts exactly as before, and the factory is back to >= 0. The reason is in the description.
Legacy service. Reverted entirely. Its lookup is type-agnostic, so the bug doesn't apply there. isOverdraft is a primitive now.
Tests. Removed testBackdatedWithdrawalReplacesTheInterestPostingWithOverdraftInterest and its builder. The two monthly overdraft builders share one parametrised request, and the count helpers are down to one stream helper plus one counter, driven by type predicates.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org