adamsaghy commented on PR #6546:
URL: https://github.com/apache/fineract/pull/6546#issuecomment-6238486120
@alberto-art3ch Please kindly review my concerns below:
### 1. Cumulative loans with daily interest get the wrong EMI (high)
The PR's docs list cumulative loans with daily interest as supported, but
the EMI for them is wrong.
- `LoanApplicationTerms.pmtForInstallment` computes the EMI over a "nominal
period". That period starts on the 1st of the disbursement month and ends at
`getPeriodEndDate`, which for semi-monthly is
`SemiMonthlyScheduleDates.next(1st)` (`LoanApplicationTerms.java:1063`).
- With daily interest, `periodicInterestRate` then charges the actual days
of that period (the `SEMI_MONTHLY` case at `:1474`). Monthly loans avoid this
because they are forced to 30 days through the `DAYS_30` argument; semi-monthly
has no such override.
- So the period is 9 days for the 10/25 pair, 14 days for 1/15 or 15/31, and
**1 day** for 2/17.
- **Example:** 1000 at 24% over a 365-day year, 24 repayments, days 10/25.
The EMI comes out at about 44.8 instead of about 47.1. Each period's real
interest is charged on its actual ~15 days, so the loan under-amortises. The
shortfall piles up in the last installment, which comes out roughly twice the
size of the others.
- The test
`anOnTimeRepaymentKeepsTheScheduleOfALoanWithInterestRecalculation` uses 15/31
(14 days instead of ~15.2) and only compares one period's interest, so it
doesn't catch this.
- **Fix:** use a fixed nominal length for semi-monthly when `isForPMT` is
set: 15 days, or days-in-year / 24.
### 2. `GET /loanproducts/{id}?template=true` loses the due days (medium)
- `handleTemplate` rebuilds the product data with the copy constructor
(`LoanProductData.java:~1136`), and that constructor doesn't copy
`firstRepaymentDayOfMonth` or `secondRepaymentDayOfMonth`.
- The product edit screen therefore shows the days as empty.
- If a client then PUTs the form back with `repaymentFrequencyType` but no
days, `validateSemiMonthlyRepayment` rejects it with a "not null" error.
### 3. Re-aging with `frequencyType=SEMI_MONTHLY` fails with a 500
(low–medium)
- `LoanReAgingValidator.java:114` only checks the frequency type is present,
and the service parses it with `enumValueOfParameterNamed`. The value
`SEMI_MONTHLY` is now accepted.
- It then fails in `getRepaymentPeriodDate` (which the PR made throw
`UnsupportedOperationException`) or in `calculateFirstReAgedPeriodStartDate`
(`IllegalStateException`). The preview endpoint fails the same way.
- Re-aging a semi-monthly loan with its own frequency is an obvious thing
for a user to try. `WHOLE_TERM` and `INVALID` already have this gap, but this
PR makes a new value reachable.
- **Fix:** restrict the validator to `BASIC_PERIOD_FREQUENCY_TYPES`, so the
user gets a 400.
### Lower priority / nits
- **Leftover days on non-semi-monthly products and loans:** if a product is
switched from semi-monthly to monthly without sending the days, it keeps them.
`LoanScheduleAssembler` then copies them onto every new monthly loan regardless
of frequency (`:596`). Nothing breaks, but GET responses show due days on
monthly loans. Consider clearing the days when the frequency isn't semi-monthly.
- **Title:** the PR is titled "Bi Monthly Loan Product", which usually means
every two months, while the feature is semi-monthly. Worth aligning the title
and the JIRA wording.
- **Broken comment:** the comment at `LoanScheduleAssembler.java:210–211`
was split by reformatting ("// The" / "// validator …").
- **Stale comment:** the field comment in `LoanProductRelatedDetail` says
the frequency "derives its second due day from it", which no longer matches the
code.
- **Changeset order:** in `0260_add_semi_monthly_repayment_frequency.xml`
the changeset ids run 1, 4, 2, 5, 3. That's harmless, but tidy it up before
merge.
--
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]
