adamsaghy commented on PR #6405:
URL: https://github.com/apache/fineract/pull/6405#issuecomment-5889140784
@Eyad-Sharkawy Can you please review the below concerns or mine?
**Gap?**
The penalty keeps being subtracted from the stored rate
To handle the override, the PR moved the premature-closure penalty outside
the chart != null block. It still ends with this.nominalAnnualInterestRate =
applicableInterestRate; (FixedDepositAccount.java:193,
RecurringDepositAccount.java:245).
- Before this PR: the rate was re-read from the chart on every call, so
writing the penalised rate back did no harm.
- With the override on: the stored rate is the only source of the rate, so
each call lowers it by the penalty again.
This happens in a real API flow:
1. POST /fixeddepositaccounts/{id}?command=calculatePrematureAmount (the
preview) runs
DepositAccountPreMatureCalculationPlatformServiceImpl.calculatePreMatureAmount.
That method has a plain read-write @Transactional.
2. The call goes through calculatePreMatureInterest →
calculateInterestPayable → getEffectiveInterestRateAsFraction(…, true). That
changes the rate on a live database entity, and the change is saved when the
transaction commits.
3. Each preview lowers the stored rate by the penalty: with a 7.5% custom
rate and a 1% penalty, it goes 7.5 → 6.5 → 5.5 and so on, until it reaches 0.
4. The real prematureClose then subtracts the penalty once more from
whatever is left, so the interest posted is lower than it should be.
The RD endpoint works the same way. I traced this through the code but
didn't run it. It's easy to demonstrate: calling the method from
testFixedDepositEffectiveInterestRateWithOverriddenRateAndPrematureClosurePenalty
a second time returns 0.055 instead of 0.065.
**Suggested fix?**
Don't write the penalised rate back to nominalAnnualInterestRate when the
override is on. Alternatively, calculate the penalised rate in a local variable
and leave the stored custom rate alone. -> Is this correct?
**Change not mentioned in the description**
The same move means FD accounts with no chart at all now get the
premature-closure penalty. That happens when the product had no applicable
chart (DepositAccountAssembler.java:308-313); before this PR, those accounts
had no penalty. These accounts also get the drift bug above. This may be
intended, but the description only talks about overridden accounts, so the
author should either confirm it or keep the old behaviour.
--
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]
