FINERACT-2455: payment rate change EIR history

[budaidev]

Description

Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[galovics]

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.

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:

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

[adamsaghy]

Kindly review my concerns

[adamsaghy]

@budaidev Please rebase

[adamsaghy]

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.

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:

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?

[budaidev]

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.

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:

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

[adamsaghy]

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.

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:

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.

[adamsaghy]

@budaidev Please pull latest changes for your branch then rebase with develop branch.

[adamsaghy]

@budaidev Please review the failing checks.

[adamsaghy]

@budaidev Please review the failing test:

org.apache.fineract.integrationtests.client.feign.tests.FeignWorkingCapitalLoanRateChangeEirHistoryTest
  
    Test A future-dated rate change snapshots its calculated values already at booking time PASSED
    Test A backdated rate change leaves the earlier-booked change's snapshot as-booked (audit trail, no restatement) FAILED
  
    org.opentest4j.AssertionFailedError: 20% change (2019-02-01) booked after the 17% one: 'eir' expected 0.001169946129 but was 0.001185871519 ==> expected: <0> but was: <-1>
        at app//org.apache.fineract.integrationtests.client.feign.tests.FeignWorkingCapitalLoanRateChangeEirHistoryTest.lambda$backdatedRateChange_keepsEarlierBookedSnapshotAsBooked$0(FeignWorkingCapitalLoanRateChangeEirHistoryTest.java:329)

[adamsaghy]

Kindly review my concerns

[adamsaghy]

@budaidev Please rebase

[galovics]

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)

[budaidev]

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.

[adamsaghy]

LGTM

All concerns were addressed