mariiaKraievska commented on PR #6398: URL: https://github.com/apache/fineract/pull/6398#issuecomment-5588205194
> This directly overlaps with #6264 and #6343 on the same JIRA ticket - 6 of the same files touched, including a three-way constructor collision on `ProjectedAmortizationScheduleModel` where this PR adds a field called `annualEir` (the operator-supplied target) right as #6343 adds `annualEffectiveInterestRate` (the solved output) - near-identical names, opposite meanings, on the same class. This needs to be settled with the other two PR authors before any of the three goes further. > > Two reachable NPEs, both blocking: > > * `withDiscount()` unconditionally dereferences `totalPaymentVolume.getAmount()`, but `generateFromAnnualEir` passes `null` for that field. `regenerate()` correctly branches on whether `annualEir != null`, but `withDiscount()` doesn't - and it's reached from a routine unearned-fee query (`totalActualAmortizationWithDiscount`) on any Annual EIR loan. > * `resolveAnnualEir` in `WorkingCapitalLoanAmortizationScheduleWriteServiceImpl` is missing the same `getLoanProduct() != null && getRelatedDetail() != null` guard that its sibling method `resolvePaymentAmountCalculationStrategy` has right above it. > > Also blocking: the new Liquibase part (`0077_wc_payment_amount_calculation_strategy.xml`) duplicates a changeset number already taken by `0077_wc_loan_recovery_payment.xml` on develop. Needs a rebase and renumber. > > On the math itself - `dailyRateFromAnnualEir` computes the deannualized rate via `Math.pow(...).doubleValue()` and discards the passed-in `MathContext` on the result, rather than reusing #6343's `TvmFunctions.deannualize` (which has an overflow-safe Newton-Raphson implementation of the identical formula in the same file). And the stored `effectiveInterestRate` for this strategy is the target rate, not the realized IRR of the actual (whole-cent-rounded) schedule - which will disagree with what a client recomputes from the emitted payment rows. > > A few more worth addressing: validation is asymmetric between create and update (a TPV loan can have `annualEir` set via update with no strategy check, and vice versa for `periodPaymentRate`), and the loan-level `annualEir` override has no min/max bounds unlike its `periodPaymentRate` counterpart - a real pricing-control gap, not just incompleteness. > > I know it's a draft, but given the collision with two other open PRs on the same fields, I'd rather this get coordinated before more work goes into any of the three. > > Recommendation: CHANGES_REQUESTED @galovics Thanks for the review. **Naming #6343:** Coordinated with the @oleksii-novikov-onix . `annualEir` here stays the **operator target input** for the ANNUAL_EIR strategy. #6343 will rename the solved output to `calculatedAnnualEir`, so the two won’t collide in meaning. File overlap with those PRs we’ll handle on rebase/merge order as usual. **Blocking / other notes:** Addressed on this branch — `withDiscount()` Annual EIR branch, `resolveAnnualEir` null-guards, Liquibase renumber, `TvmFunctions.deannualize` for daily rate, `effectiveInterestRate` = realized IRR, create/update strategy gates, and loan-level `annualEir` min/max. -- 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]
