[PR] FINERACT-2455: WC - Introduce new field which reflects the current payment rate [fineract]

["mariiaKraievska (via GitHub)" <gi...@apache.org>]

mariiaKraievska opened a new pull request, #6510:
URL: https://github.com/apache/fineract/pull/6510

   ## 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](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
   - [ ] 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](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
   - [ ] Follow our [coding conventions](https://cwiki.apache.org/confluence/display/FINERACT/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"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
   - [ ] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
   - [ ] I followed the [AI Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy).
   
   Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
   


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2455: WC - Introduce new field which reflects the current payment rate [fineract]

["mariiaKraievska (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2455: WC - Introduce new field which reflects the current payment rate [fineract]

["adamsaghy (via GitHub)" <gi...@apache.org>]

adamsaghy merged PR #6510:
URL: https://github.com/apache/fineract/pull/6510


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org