alberto-art3ch commented on PR #6072: URL: https://github.com/apache/fineract/pull/6072#issuecomment-5028945029
> I see some discrepancies between this logic and breach disable / enable: > > **1. Enable persistence model is different.** > > Breach persists a separate ENABLE row (the validator news up an ENABLE-typed action, it gets saveAndFlush'd as saved) and separately closes the DISABLE row. The delinquency PR persists no ENABLE row: the validator returns the existing active DISABLE (findActiveDisable), and the write service just stamps its endDate and re-saves it. So DelinquencyAction.ENABLE is added to the enum but never stored; the entityId returned on enable is the DISABLE's id. Weaker audit trail than breach, and an inconsistency with the precedent. > > **2. Enable end-date is off by one vs breach.** > > Breach closes the window at enableStart.minusDays(1): disable is in effect [start, enable-1], which is exactly why its query uses the inclusive endDate >= :date. The PR sets endDate = businessDate (the enable day itself) and queries endDate is null. So the two features disagree on whether the enable day is still "disabled," and that's coupled to the query difference from before: they're one design decision, made two different ways. > > **3. Disable/enable side-effects are genuinely new, not inherited from breach.** > > On DISABLE the PR actively calls liftDelinquencyClassification(loan, startDate); breach has no DISABLE branch at all in its write service (it just saves the row and relies on the eval guard). On ENABLE the PR treats the disabled window like a pause: extendPeriodsForPause + evaluateExpiredPeriods + classifyDelinquency: whereas breach only calls reprocessBreachSchedule and does not extend/pause periods. So the "disabled window behaves like a pause" behavior (which the PR's own comments claim mirrors breach) is not actually what breach does. > > **4. Bundled unrelated change (scope creep).** InternalWorkingCapitalLoanApiResource.activateLoan now seeds WorkingCapitalLoanBalance (principal + discount, zero overpayment) to avoid the loan flipping to OVERPAID on repayment. Nothing to do with disable/enable: it's a fix to the internal activation helper riding along in this PR. > > **5. Likely docs build break, and diverges from the breach doc wiring.** > > The PR adds include::chapters/working-capital-disable/index.adoc[leveloffset=+1] to index.adoc, but that file doesn't exist in the tree and isn't added by the diff. Every other working-capital chapter: including breach (working-capital-breach-management.adoc): lives under chapters/features/ and is pulled in through features/index.adoc, not as a new top-level include. So this is both inconsistent with the breach approach and a dangling include that should fail the asciidoctor build.For balance, the thing I'd earlier have expected to differ but doesn't: the "block pause/resume/reschedule while disabled" guard is mirrored faithfully (breach.is.disabled → delinquency.is.disabled). Thanks for the review, @adamsaghy — all valid. I've pushed a follow-up: 1 & 2. Enable persistence + end-date. Aligned with breach: enable now persists its own dedicated ENABLE row (so entityId is the ENABLE's id), and closes the disable at enableDate - 1. The "active disable" query now matches breach too (end_date IS NULL OR end_date >= :date). 3. Side-effects. The disable/enable effects are intentionally delinquency-specific (lift/reclassify vs. breach's schedule reprocess) — breach's schedule mechanics don't carry over. I fixed the misleading comments that claimed this "mirrors breach". 4. Docs. Moved the chapter under chapters/features/ and wired it through features/index.adoc; removed the dangling top-level include. asciidoctor builds clean now. (Root cause: the docs/ gitignore rule silently dropped the new .adoc/.puml files — force-added.) 5. Balance seed. This one is actually a prerequisite for this PR's own ITs (they repay activated loans via the internal helper, which flip to OVERPAID without the seed). Kept it here with a rationale comment, but happy to split it into its own PR if you'd prefer. Anything else please let me know. Thanks! -- 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]
