mariiaKraievska commented on PR #6635:
URL: https://github.com/apache/fineract/pull/6635#issuecomment-6195802800
> @mariiaKraievska Please review below concerns:
>
> The new seed date is still not always the one the loan's schedule was
generated from, though. I traced the date logic by hand and did not run
anything.
>
> ### 1. Seed ignores `repaymentsStartingFromDate`: new bug for the 28th,
29th–31st still not fixed
> `ProgressiveEMICalculator.java:224`
>
> `getStartDate()` returns the first period's `fromDate`, which is the
submitted-on or disbursement date. When `repaymentsStartingFromDate` is set,
`LoanApplicationTerms` seeds the loan's own schedule from that date instead
(`LoanApplicationTerms.java:840-845`).
>
> * **Newly broken:** submitted 31 Jan 2026 with first repayment date 28 Feb
2026. The real seed day is 28, so due dates fall on 28 Mar, 28 Apr, and so on.
A tranche on 10 Apr falls in the period 28 Mar → 28 Apr. The new seed is 31
Jan, so `adjustDate` moves 28 Apr to 30 Apr. The (fromDate, dueDate) match
fails and a full duplicate schedule is appended. Before this PR the seed was 28
Mar, which gives 28 Apr and merges correctly.
> * **Still broken:** submitted on the 15th with first repayment date on the
31st. The seed day is 15, so no month-end adjustment happens. The tranche gets
30 Oct instead of 31 Oct and the schedule is duplicated again.
>
> ### 2. Seed ignores the re-aged anchor date
> `ProgressiveEMICalculator.java:224`
>
> `addRepaymentPeriods` (line 363) uses the re-age `startDate` as its seed
for this reason, but the full-term tranche path has no such handling.
>
> Example: the loan was originally anchored on the 31st and re-aged so
installments fall on the 28th. A tranche in a re-aged period now seeds from the
31st, so 28 Apr becomes 30 Apr and the schedule is duplicated. Before this PR
the seed was the period's `fromDate` (the 28th), which merged correctly. The
reverse case, a 15th anchor re-aged to the 31st, never gets the month-end
adjustment.
>
> ### 3. Underlying cause: the merge appends a whole schedule on any date
mismatch
> `ProgressiveEMICalculator.java:280`
>
> `mergeNewScheduleModelWithExistingOne` matches periods only by exact
`(fromDate, dueDate)` and silently appends every period that doesn't match. Any
difference in date generation therefore produces 2×N installments, with
`fromDate > dueDate` on the first appended one. Causes include the seed issues
above, loan term variations, and rescheduled due dates.
>
> **Suggestion:** reuse the existing periods' due dates for the overlapping
range, and generate only the extra installments at the end with
`resolveRepaymentPeriodLengthGeneratorFunction` /
`generateAdditionalRepaymentPeriodDueDates`, as `addRepaymentPeriods` already
does. Matching by position from `firstDisbursedPeriod` would also work. Either
way, the result stops depending on regenerating identical dates from a seed.
>
> ### 4. No unit test coverage
> `ProgressiveEMICalculator.java:233`
>
> `ProgressiveEMICalculatorTest` already has full-term tranche tests but
gets no month-end case. Please add unit tests for:
>
> * the 31st anchor, and the 29th/30th anchors after Feb 28
> * the `repaymentsStartingFromDate` and re-age variants above
Thanks for the review — addressed as follows:
3 (root cause) — fixed
We no longer regenerate overlapping dates or match by (fromDate, dueDate).
Overlap reuses existing period dates and merges by position from
firstOverlapIndex. Only periods beyond maturity are appended. This removes the
duplicate / inverted-date schedule when seeds differ.
1 & 2
Overlap no longer regenerates dates from a seed — we reuse the existing
periods and merge by index, so RSFD/re-age no longer cause duplicate schedules
there.
Extras (new periods after maturity) are built the same way as the schedule
generator: plusMonths, then CalendarUtils.adjustDate with the seed from
calculateSeedDate. So after 28 Feb on a 31st pattern we get 31 Mar, not 28 Mar,
for RSFD/re-age we keep their due-day.
4 — added in ProgressiveEMICalculatorTest:
- 31st / 29th / 30th anchors after Feb 28 (no duplicate; 31st restores Mar
31)
- repaymentsStartingFromDate on the 28th pattern
- submitted 15th / first repayment 31st
- re-age 31→28 and 15→31
Please take another look
--
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]
