adamsaghy commented on PR #6546:
URL: https://github.com/apache/fineract/pull/6546#issuecomment-5908531280
@alberto-art3ch Please review below my concerns:
### 1. The schedule is wrong end to end, and CI shows it
All 8 schedule-generation tests in the PR's own
`LoanSemiMonthlyRepaymentScheduleTest` fail on MariaDB, MySQL and PostgreSQL.
In every one, the first due date is **disbursement + 1 year**. For example, a
loan disbursed on 02 Jan 2023 is expected to fall due on 2023-01-10 but falls
due on 2024-01-02.
- **Cause:** `LoanScheduleAssembler.deriveFirstRepaymentDate` (around line
1195) branches on daily, weekly and monthly, and everything else falls into
`else { // yearly loan } → seedDate.plusYears(repaymentEvery)`. The PR adds no
semi-monthly branch, so the first repayment date is wrong before the new
`SemiMonthlyScheduleDates.next()` is ever called.
- **Why nothing caught it:** `SemiMonthlyScheduleDatesTest` passes, but it
only tests the helper on its own.
- **What this breaks in the spec:** Rule 6 (disbursed 12 Jan should give 15
Jan; 20 Jan should give 1 Feb), Example 1, Example 2, and the acceptance
criterion "two repayment dates per calendar month".
- **When a first repayment date is supplied:** if the caller passes
`repaymentsStartingFromDate`, that date is used as-is and is not moved onto a
configured day.
### 2. The configuration doesn't match the spec
| Spec | PR |
|---|---|
| Two fields: first day 1–28, second day 2–28 or "Last Day" | One field,
`firstRepaymentDayOfMonth`, allowed 1–15. The second day is derived: 1→15,
15→last day, anything else→day+15 |
| Rule 1 (second > first) and Rule 3 (no duplicates) are checked on user
input | Always true by construction, because there is no second field to
validate |
| Rule 2: second day must be 2–28 or Last Day |
`firstRepaymentDayOfMonth=14` gives the **29th** in every month except non-leap
February, which the spec disallows |
The only pairs you can set are 1&15, 2&17 … 13&28, 14&29, and 15&Last. Many
pairs the spec allows can't be configured at all: 1 & Last Day, 1 & 20, 10 &
20, 5 & 25, 20 & Last Day, or any first day above 15. The spec's own examples
(1&15, 15&Last, 10&25) happen to be reachable. This is a design choice the
author needs to justify or change, not a bug.
### 3. What matches the spec
- A new frequency type, `SEMI_MONTHLY(6)`, is added after `INVALID`, so
existing rows are unaffected.
- Last Day resolution is correct in the helper: 28/29 Feb, 30 Apr, 31 Jul
(Rules 4 and 5, unit-tested).
- 24 periods per year is used for the APR and periods-per-year calculations.
- The existing daily, weekly, monthly and yearly paths look untouched. The
switch statements that gained a `SEMI_MONTHLY` case either do nothing or throw
on purpose.
### 4. Smaller gaps against the spec
- **Progressive schedules are blocked, but only at product create.**
`effectiveLoanScheduleType(element)` defaults to `CUMULATIVE` when
`loanScheduleType` isn't in the request. So a `PUT` that sets only
`repaymentFrequencyType=6` on an existing progressive product passes the check.
The loan application validator has no such check at all. I haven't traced what
the progressive engine then does. Note that the spec's "declining balance/EIR"
wording implies progressive should be supported, not rejected.
- **Spec Example 3 can't be entered as written.** `LoanScheduleValidator`
requires `loanTermFrequencyType` to equal `repaymentFrequencyType`, so "Term 6
Months, 12 installments" must be entered as a term of 12 semi-monthly periods.
- **A loan's missing day is silently defaulted.** A loan can switch to
`repaymentFrequencyType=6` without a `firstRepaymentDayOfMonth` (only the
product validator requires it), and the code then quietly uses 15, i.e. the
15th & last day.
### Suggested asks for the author
1. Add semi-monthly handling to `deriveFirstRepaymentDate` and get the
integration tests green.
2. Either implement the spec's two fields (first 1–28, second 2–28 or Last
Day, with validation for order and duplicates), or get the spec changed to the
derived-day model. If the derived model stays, disallow 14, since it produces
the 29th.
3. Make the progressive guard use the product's stored schedule type on
update, and add the same guard to the loan application.
I reviewed the PR against its head commit in a temporary worktree; your
current branch wasn't touched. I can post this as a PR comment if you want.
--
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]
