alberto-art3ch commented on PR #6482: URL: https://github.com/apache/fineract/pull/6482#issuecomment-5816496327
> The PR is small, additive and mirrors #6476 nicely (no migration, nullable Avro field with `default: null`, additive REST field). But `breachEffectiveStartDate = fromDate + breachGraceDays` assumes period 1 still carries the grace days, and two existing schedule operations break that, so the API can return a wrong date. > > **1. A reset inside the grace window gives an effective start after the period has ended.** `splitPeriodAtReset` cuts period 1 down to `[D, R-1]` but keeps it as period 1, and once that date passes it's flagged as breached. If `R-1 < D+g` the field returns `D+g`, after the breached period's own `toDate` (and possibly after the reset). Example: D=Jan 1, g=5, reset on Jan 3 gives period 1 = [Jan 1, Jan 2] and an effective start of Jan 6. Undo is fine (`restoreSplitPeriod` rebuilds through `naturalToDate`, which adds the grace back). > > **2. A reschedule during period 1 removes the grace but the field still reports it.** `rescheduleMinimumPayment` re-dates the open period via `calculateRescheduledToDate` (new frequency plus pauses only), never adding `breachGraceDays`, while the comment on `naturalToDate` says every path that re-dates a period must agree on the grace rule. `recalculatePeriodsForPauses` and `restoreSplitPeriod` do put it back, so the paths already disagree. After a reschedule the schedule has no grace window but the API still returns `fromDate + g`, which can also land after the new `toDate`. > > 3. A pause overlapping the grace window stretches `toDate` but not the reported start - is the grace meant to be paused too? Needs a product decision and a test (same question as [FINERACT-2455: Working Capital - Delinquency Effective Date #6476](https://github.com/apache/fineract/pull/6476)). > > The grace rule already lives in `WorkingCapitalLoanBreachScheduleServiceImpl.naturalToDate`; computing the effective start there (or at least clamping/returning null when `fromDate + g` is after `period.getToDate()`) is safer than repeating the arithmetic in the read service. Please add integration tests for reset-in-grace-window (and its undo), reschedule-in-period-1, and a pause across the grace window; the mapper test also doesn't assert the new Avro field. > > **Docs.** `working-capital-loan-start-dates.adoc` still says `delinquencyStartDate = fromDate + delinquencyGraceDays` in several places (lines 44, 57, 127, 153, 177 and the "Null Treated as Zero for Grace" section) while the code and the integration test use the raw `fromDate` - the new example JSON now shows a corrected breach date next to a still-wrong delinquency date. The Swagger and test-class Javadoc on the delinquency side are stale too, which is why this conflicts with #6476 in the adoc, the read service and `WorkingCapitalLoanStartDatesTest`. I'd merge #6476 and this one together (or agree an order and share one `effectiveStartDate(periodNumber, fromDate, graceDays)` helper). Also worth deciding whether a null value for every breach after period 1 is the contract you want, or `breachStartDate` as the fallback - and use the same rule in both PRs. > > Recommendation: CHANGES_REQUESTED Confirmed on the reset: `splitPeriodAtReset` cuts period 1 at the reset date but keeps it as period 1, so the effective start could land after the period ended — the resolver now returns `null` when `fromDate + grace` falls past the period's own `toDate` (a cool off that never took place), with integration tests for reset-in-grace-window and its undo. On the reschedule, the breach side is already correct: a RESCHEDULE goes through `replayForBreachAction` → `naturalToDate`, which re-applies the grace to period 1 — the real drift was the validator's guard, which computed without it, so the rule now lives in a shared `calculateNaturalToDate` used by generation, replay and the validator (I also removed the orphaned javadoc on `replayForBreachAction` that still described the pre-replay behaviour, and added a reschedule-in-period-1 test). Docs rewritten: the delinquency sections described behaviour the code never had — including a `[source,java]` block that doesn't exist in the codebase — plus the stale Swagger and test javadoc, and a new scenario for the reset case; the mapper test now asserts the Avro field. Pauses over the grace window are left as the open product decision, and I'd still agree a merge order with #6476 since the adoc overlaps. -- 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]
