oleksii-novikov-onix commented on PR #6343:
URL: https://github.com/apache/fineract/pull/6343#issuecomment-5602615929
> Four of the five things from last round are genuinely fixed, and well -
the rounding mode is now a documented fixed constant (`RoundingMode.HALF_EVEN`)
rather than the tenant's money mode, the Newton-Raphson overflow is fixed by
splitting the decimal exponent instead of going through `double`, and
`TvmFunctions` now has 16 unit tests covering round-trips and both overflow
directions. The rate-segment staleness is now a documented design decision
rather than an accident, though it does depend on #6264 for a way to see the
current in-force rate.
>
> The collision with #6264 is still completely open though, and it's sharper
than before: both PRs were pushed within the same minute today, both still
touch the same 9 files, and now they define the same field incompatibly - this
PR makes `calculatedAnnualEir` a rounded percentage while #6264 keeps it an
unrounded fraction, on the same endpoint. #6264's new Swagger text ("Note:
periodPaymentRateHistory[].calculatedAnnualEir is a percentage" contrasted with
the fraction at the top level) would be actively wrong the moment this PR
lands. This needs to be resolved between the two PRs - ideally by having this
PR read the annual rate off #6264's `Solved` record instead of maintaining a
second annualization helper.
>
> Beyond the collision, CI is actually red right now, and it looks like the
same root cause across all of the failures: the rate round-trip (solve IRR ->
round to 6dp annual -> spread back to a periodic rate) moves real cents in the
amortization, and the expected values in the test suite were only partially
updated to match - `ProjectedAmortizationScheduleCalculatorTest` and one e2e
scenario both show the sweep stopped partway through. And the `dailyEir`
removal is actually failing the project's own `run-api-backward-compatibility`
gate right now (4 confirmed `R014` errors), not just something that will show
up as a warning.
>
> One more: this modifies the `WorkingCapitalLoanAccountDataV1` avro schema
in place (deleting `dailyEir`, redefining `calculatedAnnualEir`'s meaning)
rather than creating a V2, which the project's own reliable-event-framework doc
says should happen for exactly this kind of change to a published V1 record.
>
> There's also an unrelated commit in here
(`SavingsInterestPostingJobIntegrationTest` business-date pinning) that has
nothing to do with WC or annual EIR - should be its own PR.
>
> To get this to green: resolve the collision with #6264 (one unit, one
helper), fix the CI failures (either the amortization drift is intentional and
every expected value needs updating, or the round-trip shouldn't be perturbing
the schedule at all), restore `dailyEir` or get sign-off on removing it, and
split out the unrelated commit.
>
> Recommendation: CHANGES_REQUESTED
**Collision with #6264.** Nine files overlap, and that line in
[#6264](https://github.com/openMF/fineract/issues/6264)
's Swagger ("unlike the top-level calculatedAnnualEir, which is a fraction")
does become wrong once this lands. But the two PRs define the field the same
way:
[#6264](https://github.com/openMF/fineract/issues/6264)
already has ANNUAL_EIR_SCALE = 6, ANNUAL_EIR_ROUNDING = HALF_EVEN and
annualEirPercentage = annualiseEir(dailyEir, npvDayCount) × 100 rounded to six
decimals. That is exactly what our AmortizationParams.calculatedAnnualEir does
— same base, same scale, same rounding, same unit. The only real difference is
timing: we have already moved the top-level field to a percentage, and #6264
still describes the old fraction. Once both land, both surfaces are percentages
at scale 6 and they agree. The unit is not ours to change either — "store the
annualized calculated eir as percentage, so we got a higher precision" is the
instruction this PR implements.
On reading the value off #6264's Solved, it works the other way round.
Solved already exists in develop, and this PR is the one adding
calculatedAnnualEir to it.
[#6264](https://github.com/openMF/fineract/issues/6264)
adds RateChangeSolve, a map keyed by rate change date, and that map is
empty for a loan that never had a rate change, so the account-level value
cannot come from there.
What we suggest instead:
[#6264](https://github.com/openMF/fineract/issues/6264)
reads Solved.calculatedAnnualEir() from this PR for its history rows. Both
of its helpers then become unused, and the fraction sentence goes away with
them.
**CI.** Fixed, and green on the current head.
**dailyEir and the R014 gate**. Confirmed. The four errors are one field on
four endpoints: the template, the paged list, {loanId} and
external-id/{loanExternalId}. Removing it is what the AC asks for: "let's
return annual effective interest rate (only) on loan details".
**Avro V1 versus V2**. The rule is in the docs, but it has never been used
for WC: there is no *V2.avsc file anywhere in the WC module. This schema was
created by this ticket in July and has already been amended in place once
since, and both changes were merged. I'm fine with creating a V2 if you want
the rule applied to WC. @adamsaghy What do you think?
**The savings commit**. Agreed, and done. It is out of this branch (three
commits now, all WC) and raised separately as #6422.
--
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]
