mariiaKraievska commented on PR #6510: URL: https://github.com/apache/fineract/pull/6510#issuecomment-6035425518
> ## Review > Nice and small, and reusing `rateInEffectAt` means the read side can't drift from what the validator and write service apply. The Avro field is nullable with a null default, so that's compatible. A few things before it leaves draft: > > * `WorkingCapitalLoanPeriodPaymentRateChangeReadServiceImpl` (`strategy != null && !strategy.isTpv()`): a null strategy is treated as TPV and gets a rate, but the Swagger, Avro doc and javadoc all say "null when the loan uses a non-TPV strategy". If null-as-TPV is deliberate for old rows, write it as `strategy == null || strategy.isTpv()` and say so, otherwise drop the null checks. The `details == null` guard also looks unreachable since `retrieveOne` loads with full details. > * `WorkingCapitalLoanApplicationReadPlatformServiceImpl` (~L193): the rate-change table is read twice per GET and per WC business event, `retrieveRateChangeHistory(loan)` and then `findByWorkingCapitalLoanIdAndReversedFalse`. Could the effective rate come from the rows you already have? > * Tests: no unit test for the new service method, and the e2e doesn't cover a change dated after the business date. That's the case where "effective" differs from "latest", so the fallback to `paymentRate` is never shown. A reversed and a backdated change would be nice too. Also `WorkingCapitalEffectivePaymentRate.feature` only checks that the event schema has the field. Can we assert the value the event carries after the rate change, like the Annual EIR step def does? > * `checkWorkingCapitalPeriodPaymentRateInEffect` javadoc says the loan resource doesn't move when a rate change is booked, which is only half true now. > * Fill in the PR description when it leaves draft. > > Recommendation: COMMENT Thanks — addressed: 1. strategy == null || strategy.isTpv() (null = TPV, as elsewhere in WC). dropped details == null. 2. effectivePaymentRate now comes from the already-loaded history (one rate-change read). 3. Tests — unit for null-strategy, future/backdated/reversed + event value covered in e2e. 4. checkWorkingCapitalPeriodPaymentRateInEffect javadoc — clarified paymentRate vs effectivePaymentRate. -- 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]
