budaidev commented on PR #6264:
URL: https://github.com/apache/fineract/pull/6264#issuecomment-5440439099
> > 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`
--
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]
