adamsaghy commented on PR #6601: URL: https://github.com/apache/fineract/pull/6601#issuecomment-6076604218
> Thanks @galovics , all addressed. > > The seven `WorkingCapitalNearBreachEvaluation.feature` scenarios are rewritten to the new contract, the "immutable" titles are renamed, and the PR description now calls out the behaviour change for API and `WorkingCapitalLoanNearBreachChangeBusinessEvent` consumers. > > The snapshot/baseline logic moved out of the schedule service into `WorkingCapitalLoanNearBreachBaseline` and `WorkingCapitalLoanNearBreachRederivation`, with focused unit tests for `isStale`/`hasChanged`. Repayment and undo now check `isBreachDisabled` once and pass the resolved parameters in, the COB path delegates to the same `rederiveNearBreach`, and the `*NearBreach` methods are renamed by intent (`evaluateNearBreachOnCob`, `rederiveNearBreach`, `resolveNearBreachValue`). > > On the undo question, yes: charge waiver undo, discount fee adjustment undo, undo write-off and a charge that reopens a closed loan could all reopen it without a re-derive, so they now go through one `rederiveNearBreachIfReopened` helper (charge adjustment undo was already covered by the generic undo). @budaidev Can you place review the below? **The overstated claim (queries).** The parameters are not passed in. `applyRepayment` and `applyRepaymentUndo` still look them up themselves, through `rederiveOpenPeriod` → `resolveParametersWithBreachEvaluationEnabled`. That method skips the second `isBreachDisabled` check but still runs the RESCHEDULE near-breach action lookup on every repayment and undo. So the hot path went from 2 extra queries to 1, not 0. That one lookup is arguably unavoidable, since a RESCHEDULE action can exist without any product-level near-breach config. It's still worth asking budaidev to correct the reply so galovics doesn't think the lookup is gone. A small side note: the public `resolveParametersWithBreachEvaluationEnabled` is only correct if the caller has already done the disabled check, and only its Javadoc says so. **Reopen coverage.** A loan can only become ACTIVE again through `LOAN_REOPENED` (from overpaid or closed-obligations-met) or `LOAN_WRITTEN_OFF_UNDO`. Every call site that can trigger either one now calls the helper: - generic undo, after the status transition ([WorkingCapitalLoanWritePlatformServiceImpl.java:1282](fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/service/WorkingCapitalLoanWritePlatformServiceImpl.java:1282)) - charge waiver undo (`:852`) - discount fee adjustment undo (`:914`) - undo write-off (`WorkingCapitalLoanWriteOffWriteServiceImpl:209`) - adding a charge that reopens a loan (`WorkingCapitalLoanChargeWritePlatformServiceImpl:324`) `CHARGE_ADJUSTMENT` is routed to the generic `undoTransaction` (`:809`), so the reply is right that it was already covered. The other status changes can only close a loan (the charge waiver itself, credit balance refund), or the undo never changes status (recovery payment, which is only allowed while the loan is written off). -- 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]
