alberto-art3ch commented on PR #6476: URL: https://github.com/apache/fineract/pull/6476#issuecomment-5807233839
> The change is additive and safe - no migration (the value is computed at read time), the Avro field is `["null","string"]` with `default: null`, and `delinquencyStartDate` itself doesn't change behaviour (the code already returned the raw `fromDate`; the PR fixes the Swagger/adoc text that wrongly said grace was added). The date math checks out in the normal case: `generateInitialPeriod` adds the loan-level grace days to period 1's `toDate` and later periods chain from `toDate + 1`, so `fromDate + grace` is right, and the three integration test cases match. > > The concern is that the field assumes period 1 always still carries the grace days, and that isn't true after a supported action: > > * **A frequency reschedule while period 1 is still open** re-dates the period through `calculateRescheduledToDate` (`fromDate + frequency`, extended by pauses), which never adds the grace days back. The schedule then has no grace, but `resolveDelinquencyEffectiveStartDate` still reports `fromDate + graceDays` - and if the new frequency is shorter than the grace, that lands after the new `toDate`. `recalculatePeriodsForPauses` and the breach side's `naturalToDate` both put the grace back, so the paths already disagree. Either re-apply grace for period 1 in the reschedule path or derive the value from the persisted bounds; either way please add an integration test for reschedule-in-period-1. > * **A pause overlapping the grace window** stretches period 1's `toDate` but not the reported start. Is the grace supposed to be "paused" too? Needs a decision and a test (the same question applies to #6482). > * The grace days are read from the loan at read time rather than from what the schedule used - fine as long as they can't change after disbursement, but a short comment would state that assumption. > > Docs and tests: the Javadoc on `resolveScheduleAnchorDate` in the range schedule service still says grace is "added when the derived delinquencyStartDate is computed at read time", and the comment in `WorkingCapitalBreachSchedule.feature` ~L471 still says `delinquencyStartDate = fromDate + delinquencyGraceDays`, contradicting the scenario's own expected value. `WorkingCapitalLoanStartDatesTest` doesn't assert the effective date under the `LOAN_CREATION` anchor, and the mapper test/`EventCheckHelper` don't check the new Avro field. > > Conflict with #6482: 3 files conflict (the adoc, the read service, `WorkingCapitalLoanStartDatesTest`), and `resolveDelinquencyEffectiveStartDate` is a copy of `resolveBreachEffectiveStartDate` with the types swapped. I'd merge the two together or agree an order and share one helper. > > Recommendation: COMMENT Good catch on the reschedule — period 1 was silently losing its grace days there (pre-existing, the new field only exposed it): generation, reschedule and the validator that guards it now share a `calculateNaturalToDate` that applies the grace to period 1 only, covered by a unit test and an integration test for reschedule-in-period-1. Also fixed the stale javadoc and the feature-file comment, added the missing `LOAN_CREATION` and Avro assertions, and documented why the read-time grace lookup can't diverge: `validateForUpdate` rejects any change once the loan leaves submitted-and-pending-approval, and the schedule is only generated at disbursement. On pauses, the current behaviour treats the cool off as calendar days from the anchor and does not pause it — happy to flip that if you think it should be paused; and once #6482 lands I'll extract the shared helper for both effective-start resolvers. -- 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]
