Repository navigation
Conversation
707da4f to
f3234c3
Compare
galovics
left a comment
There was a problem hiding this comment.
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
falseinstead ofnull, a later catch-up can cleartrue), 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.
WorkingCapitalLoanNearBreachChangeBusinessEventnow 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
rederiveNearBreachlook self-contained, can we pull them into a dedicated component the schedule service calls with the baseline? ThenisStale/hasChangedcan get focused unit tests. Thoughts?
Smaller ones:
applyRepayment/applyRepaymentUndoalready checkisBreachEvaluationDisabled, thenreevaluateNearBreach->resolveParameterschecks 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?evaluateNearBreachandreevaluateNearBreachin the evaluation service are near-duplicates, the first could find the period and delegate withList.of(period). The three similarly named*NearBreachmethods 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-deriveundoTransactionnow does.
Recommendation: REQUEST_CHANGES
f3234c3 to
5d68bd1
Compare
|
Thanks @galovics , all addressed. The seven The snapshot/baseline logic moved out of the schedule service into 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 |
@budaidev Can you place review the below? The overstated claim (queries). The parameters are not passed in. Reopen coverage. A loan can only become ACTIVE again through
|
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.
Behaviour changes
nearBreach used to be immutable once evaluated. It no longer is:
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.