adamsaghy commented on PR #6264:
URL: https://github.com/apache/fineract/pull/6264#issuecomment-5452030740
> > > The design is well thought through overall - the as-booked snapshot
semantics, the additive/nullable migration, and the test coverage (7
integration tests plus solid e2e assertions) are all good. Two things I'd like
fixed before this merges, plus a couple of questions.
> > > **1. `rateSegmentAt` can silently resolve to the wrong rate change's
segment.**
> > > ```java
> > > public RateSegment rateSegmentAt(final LocalDate date) {
> > > return segmentForDay(splitDayIndexFor(date));
> > > }
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > `splitDayIndexFor` clamps to `scheduleTerm()`, and `segmentForDay`
returns the _last_ segment whose `startDayIndex() <= dayIndex`. A rate increase
shortens the schedule term, so a later-effective change that's already been
clamped can resolve to the same split index as the change being booked right
now. `applyRateChange` then removes segments at-or-after that index and drops
the wrong one, and `recordCalculatedValues` persists another change's
EIR/balance/term into this row - with no error, no log, nothing. Since
`applyRateChange` always adds its own segment at exactly `splitDayIndex`, an
exact-match guard closes this off cheaply:
> > > ```java
> > > final int split = splitDayIndexFor(date);
> > > final RateSegment seg = segmentForDay(split);
> > > return seg != null && seg.startDayIndex() == split ? seg : null;
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > The existing `segment == null` fallback in `recordCalculatedValues`
already handles the null case gracefully (leaves the snapshot unset rather than
wrong).
> > > **2. EIR is rounded with the tenant's money rounding mode, but a rate
isn't money.** `MoneyHelper.getRoundingMode()` is tenant-configurable
(UP/DOWN/HALF_*), so two tenants with identical inputs will store different
EIRs for the same rate change. The javadoc right above this code says scales
are fixed specifically so "API responses and event payloads carry the same
value whichever database the tenant runs on" - the rounding mode undermines
that same stated goal. A fixed `RoundingMode.HALF_UP` (which the test helper
itself already uses) would match the comment's intent.
> > > Smaller things: the snapshot in `recordCalculatedValues` relies on
dirty checking rather than an explicit save, unlike every other write in that
method - would be good for consistency and so a future refactor (e.g. splitting
the regenerate call into its own transaction) can't silently drop it. Also, the
row mixes a _restated_ `previousRate` (rewritten by `restatePreviousRates` when
a backdated change slots in) with a _never-restated_ `eir` snapshot computed
against whatever the predecessor was at booking time - your own feature file
shows a row where `previousRate: 19.0` but the EIR is the one computed against
a `previousRate` of `11.0`. Either both should be as-booked or both restated,
otherwise I don't think a reader of the history can trust the row.
> > > Recommendation: CHANGES_REQUESTED
> >
> >
> > @budaidev Have you had the chance to review these concerns?
>
> 1. `rateSegmentAt` — implemented as you suggested
> 2. Fixed.
> +1 . Question — restated `previousRate` vs as-booked `eir`
> Of the two options you offered, I don't think either is quite right on
its own:
>
> * _Both as-booked_ (stop restating `previousRate`) is the cleanest audit
model, but restatement exists
> deliberately so the history reads as one chain in effective-date order,
and dropping it changes the meaning
> of an existing REST/Avro field.
> * _Both restated_ isn't reliably possible — the EIR snapshot can't be
recomputed once the schedule model has
> been rewritten, which is the whole reason it's stored rather than
derived.
> What I'd propose instead is to keep both facts and stop them being
confusable: add an immutable
> `asBookedPreviousRate` next to the restated `previousRate`, giving two
explicit tuples:
> * as-booked audit: `asBookedPreviousRate` + `newRate` + EIR and the
derived snapshot
> * current effective chain: `previousRate` + `newRate` + `effectiveDate`
Sounds good to me to store these values as part of the previous rate to
avoid confusion and unnecessary recalculation.
--
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]
