Skip to content

FINERACT-2455: WC near-breach re-evaluation on monetary events and COB - #6601

Draft
budaidev wants to merge 1 commit into
apache:developfrom
openMF:FINERACT-2455/wc-near-breach-reevaluation
Draft

budaidev wants to merge 1 commit into
apache:developfrom
openMF:FINERACT-2455/wc-near-breach-reevaluation

Conversation

@budaidev

@budaidev budaidev commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Description

near_breach on a breach schedule period was set once by the WC_NEAR_BREACH_EVALUATION COB step and never touched again, so payments, undos and reprocessing left it stale. The open period's value is now re-derived from the current paid amount on COB and on every monetary path; closed periods stay as they were.

  • One derivation (WorkingCapitalLoanNearBreachEvaluationServiceImpl.resolveNearBreach) is shared by COB and the monetary paths: near_breach = paid < (evalIndex + 1) × threshold% × min_payment at the latest elapsed checkpoint, null until a checkpoint elapses. With nothing to judge against (no demand, no checkpoint in the period) the value is kept.
  • COB re-derives on every run, so a period cleared by a payment can be raised again at a later missed checkpoint.
  • Repayment and repayment undo re-derive the touched period while it is open; undoing a loan-closing repayment re-evaluates once the loan is active again.
  • Reset, undo reset, pause/reschedule replay and explicit reprocess compare against a snapshot taken before the change, so only periods whose inputs moved are re-derived and an event fires only when a persisted value changes. - Replay and reprocess derive after the last period's minimum payment is capped to the remaining balance.
  • The monetary path resolves as of business date − 1, the date COB evaluated, so a same-day payment cannot settle a checkpoint COB has not closed yet.
  • Breach disable uses the state-based isBreachDisabled check from develop.
  • working-capital-breach-management.adoc updated.

Behaviour changes

nearBreach used to be immutable once evaluated. It no longer is:

  • Breach schedule API: after an elapsed checkpoint is satisfied, nearBreach is false instead of null. A catch-up or backdated repayment into the open period clears a true to false, and a later missed checkpoint can raise it again.
  • WorkingCapitalLoanNearBreachChangeBusinessEvent: external consumers will see it in new situations:
  • null → false at the first satisfied checkpoint (COB);
  • true → false after a repayment, goodwill credit or backdated repayment;
  • on repayment undo, breach reset / undo reset, pause, and breach ENABLE when the persisted value changes. It is still fired only when the persisted value actually changes.
  • E2E: WorkingCapitalNearBreachEvaluation.feature scenarios that asserted the old immutable contract are rewritten to the new one (C76638, C80952, C80954, C80955, C85321, C85322, C85323; titles updated where they said "immutable"). The matching TestRail cases need the same update.

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.
  • I followed the AI Policy.

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

@budaidev
budaidev force-pushed the FINERACT-2455/wc-near-breach-reevaluation branch from 707da4f to f3234c3 Compare October 7, 2026 20:07
@budaidev budaidev changed the title FINERACT-2817: Add loan withdrawal transaction FINERACT-2455: WC near-breach re-evaluation on monetary events and COB Oct 7, 2026

@galovics galovics left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Hi @budaidev, thanks for the PR. The logic looks sound to me and the unit and integration coverage is solid, but CI is red because of this change, so I can't approve as is.

fineract-e2e-tests-runner/src/test/resources/features/WorkingCapitalNearBreachEvaluation.feature:

E2E shard 15 fails 7 of 43 scenarios (lines 98, 592, 654, 688, 955, 996, 1031). They are existing scenarios that still describe the old behaviour: "near breach is immutable - stays true after subsequent payment" (C76638), UC3 "stays immutable when backdated repayment...", UC5, UC6 credit balance refund and RESCHEDULE UC7/UC8/UC9. The PR changes that on purpose (the flag is recalculated, a satisfied checkpoint becomes false instead of null, a later catch-up can clear true), but the feature file wasn't updated. Please rewrite them to the new contract and update the matching TestRail cases. Since some titles say "immutable" outright, this is a visible behaviour change and it should be called out as one, not just re-baselined.

PR description:

It's still the empty template. WorkingCapitalLoanNearBreachChangeBusinessEvent now fires on null to false at the first satisfied checkpoint, on true to false after repayments, and on undo, reset, pause and enable. External consumers will see that, so please say so in the description, link the JIRA and tick the checklist.

WorkingCapitalLoanBreachScheduleServiceImpl (NearBreachSnapshot / NearBreachBaseline / rederiveNearBreach):

This class is already over 900 lines and now it also tracks near-breach snapshots and diffs them. Unless we have a really really good reason, I'd rather not keep growing it. The records and rederiveNearBreach look self-contained, can we pull them into a dedicated component the schedule service calls with the baseline? Then isStale/hasChanged can get focused unit tests. Thoughts?

Smaller ones:

  • applyRepayment/applyRepaymentUndo already check isBreachEvaluationDisabled, then reevaluateNearBreach -> resolveParameters checks it again plus an action lookup. That's 2 extra queries per repayment and undo on the hot path, can we pass the resolved parameters in?
  • evaluateNearBreach and reevaluateNearBreach in the evaluation service are near-duplicates, the first could find the period and delegate with List.of(period). The three similarly named *NearBreach methods are confusing too.
  • Not verified: can the undo paths in WorkingCapitalLoanChargeWritePlatformServiceImpl (e.g. charge adjustment undo) reopen a closed loan? If so they'd miss the extra re-derive undoTransaction now does.

Recommendation: REQUEST_CHANGES

@budaidev
budaidev force-pushed the FINERACT-2455/wc-near-breach-reevaluation branch from f3234c3 to 5d68bd1 Compare October 8, 2026 13:16
@budaidev

budaidev commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

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).

@adamsaghy

Copy link
Copy Markdown
Contributor

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)
  • 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants