oleksii-novikov-onix commented on PR #6343:
URL: https://github.com/apache/fineract/pull/6343#issuecomment-5490977558
> Replacing the hardcoded `pow(365)` with `npvDayCount` is a genuine
correctness fix, and I like the legacy-model fallback in
`annualEffectiveInterestRate()` for schedules persisted before this change. But
there's a scope-collision problem that needs sorting out before anything else
here.
>
> **This directly overlaps with #6264 ("payment rate change EIR history"),
on the same JIRA ticket.** Both PRs touch 9 of the same files, including
`ProjectedAmortizationScheduleModel.java` and the WC account Avro schema. Both
replace the exact same hardcoded `pow(365)` call in
`WorkingCapitalLoanApplicationReadPlatformServiceImpl`, but with two different
helper methods living in two different classes. And the two PRs make mutually
exclusive contract changes: #6264 keeps `dailyEir` on the response/event and
documents it in a new Avro record; this PR deletes `dailyEir` from the
response, both mappers, and the existing Avro schema. Whichever of these two
merges second is going to break the other. This needs to be resolved between
the two PRs - either merge them, or have one explicitly build on top of the
other, before either goes further.
>
> Two issues on the substance, both of which also apply to #6264 (already
flagged there) but are worse here since this PR feeds the rounded value back
into the schedule math:
>
> **Tenant money rounding mode applied to a rate, and that rate then drives
the schedule.** `normalizedAnnualRate` rounds with `mc.getRoundingMode()`,
which is the tenant-configurable money rounding mode - and the result is fed
straight back through `deannualize` to become `effectiveInterestRate`, the
value the whole amortization schedule discounts on. A tenant configured `FLOOR`
and one configured `CEILING` will get materially different WC schedules for
identical loan inputs. A rate isn't money; this should use a fixed rounding
mode (#6264 has already been corrected to `RoundingMode.HALF_EVEN` for the same
issue - worth reusing that constant if the two PRs get reconciled).
>
> **The rate-segment's annual EIR is computed and then thrown away.**
`computeScheduleParams` returns `annualEir` in `ScheduleParams`, but
`applyRateChange` only pulls `segment.eir()` and drops `segment.annualEir()` on
the floor - so after a payment rate change, the reported annual rate goes stale
while the schedule itself is segment-aware. That directly contradicts the new
Swagger text ("annual effective rate the schedule runs on").
>
> One more concrete bug: `deannualize`'s Newton-Raphson seed
(`Math.pow(...)`) can produce `Infinity` for an extreme daily IRR (reachable on
a short schedule with a large discount fee relative to net disbursement), which
then throws `NumberFormatException` out of `BigDecimal.valueOf`. The one
existing caller of this path swallows it by accident (`NumberFormatException
extends IllegalArgumentException`), but `applyRateChange`'s new call site has
no such guard and would surface this as a 500. Also: zero unit tests for the
new `TvmFunctions` numerical kernel (hand-rolled Newton-Raphson, no test class
at all).
>
> Recommendation: CHANGES_REQUESTED
1. Scope collision with #6264. We touch 10 of the same files, and a test
merge gives 3 conflicts: the shared feature file, the Swagger block around
dailyEir, and enrichWithRateAndTerm, where we both replace the hardcoded 365.
The helpers differ - annualiseEir does not round, normalizedAnnualRate rounds
to six decimals and feeds the result back into the daily rate - and
ProjectedAmortizationScheduleModel merges with no conflict at all, ending up
with both. The rest is not exclusive: we drop dailyEir from the account, #6264
adds a daily rate to each history row.
2. Tenant money rounding mode on a rate. Fixed: normalizedAnnualRate now
rounds with a fixed HALF_EVEN constant, the same value #6264 uses.
3. Segment annual EIR dropped. The annual rate is not dropped -
computeScheduleParams derives the segment's daily rate from it, so every
segment does run on its own normalised rate. The wrong part was the text: the
value on the account is the rate the loan was priced at, matching the period
payment rate next to it, which is also left as booked because what is in force
on a date is read from the rate-change history. Swagger, the Avro doc and the
javadoc now say that; the segment's own annual rate belongs on a history row,
which is #6264's subject.
4. Seed can overflow to Infinity. Fixed.
5. No unit tests. Added TvmFunctionsTest.
--
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]
