budaidev commented on PR #6264: URL: https://github.com/apache/fineract/pull/6264#issuecomment-5599871780
> Both blockers from last round are properly fixed, and the `rateSegmentAt` fix is better than what I originally asked for - it now records exactly what each rate change solved to, keyed by its own effective date, and returns null on a miss rather than a neighbor's numbers, with a test pinning that exact case. The tenant-rounding concern is resolved by removing the contradictory rescale entirely. > > But fixing it surfaced a new problem: nothing explicitly rounds the EIR before persisting anymore, so the value depends on whatever the DB does with a 19-significant-digit `BigDecimal` against a `DECIMAL(19,6)` column. Concretely, the business event is raised in the same transaction as the calculation and reads the still-managed, unrounded entity (then widens to 8dp for Avro), while a later GET reads back the DB-rounded 6dp value - so the event and the API can report different numbers for the same rate change. A single explicit `setScale(6, RoundingMode.HALF_EVEN)` at the point of calculation would close this. > > The `previousRate`/EIR inconsistency I raised originally (a history row can carry a restated `previousRate` next to a never-restated EIR snapshot computed against a different predecessor) is still present - the Swagger/Avro docs now explain the as-booked semantics, which helps a reader, but the underlying row still carries two different definitions of "before." I'd want an explicit decision here (restate both or neither) rather than leaving it documented-but-unresolved. > > And the collision with #6343 is still open, and it's compounded - both PRs are re-deriving the same formula in different classes, and now #6343 is also making `calculatedAnnualEir` a percentage on this same endpoint while this PR keeps it a fraction. Whoever merges second is going to have a bad time, and one of the two fixes to the underlying `pow(365)` bug is going to get lost. > > Two smaller things: there's an unrelated CI workflow change (`SKIP_SDK_GEN`) bundled in that skips building the avro-schemas SDK, which seems risky specifically in a PR that adds a new avro schema - and a test using `ReflectionTestUtils.invokeMethod` on a private method by string name, which will silently stop testing anything on the next rename. > > Recommendation: CHANGES_REQUESTED (down from before - the original two issues are resolved, but the #6343 collision plus the new rounding-divergence bug keep this from being clean) @galovics thanks. The rounding divergence is fixed: annualEirPercentage is now rounded at the point of calculation (setScale(6, HALF_EVEN), constants on ProjectedAmortizationScheduleModel), so the entity the business event serialises already carries the value the column stores, and it is the same convention #6343 uses. On previousRate vs the EIR snapshot, the decision was made but never reached the PR: previousRate stays the restated chain link in effective-date order, while the three snapshot fields (calculatedAnnualEir, dailyPaymentAmount, segmentTerm) are as-booked and never restated, because the schedule model is rewritten by the change and cannot be recomputed later; the Swagger/Avro docs say so. The SKIP_SDK_GEN workflow change is removed from this PR and recordCalculatedValues is package-private now with the test calling it directly, so a rename breaks the compile instead of the test. -- 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]
