alberto-art3ch commented on PR #6546:
URL: https://github.com/apache/fineract/pull/6546#issuecomment-6241275633
> @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.
@adamsaghy Thanks for the thorough review — all points confirmed and
addressed:
1. Cumulative EMI with daily interest: with isForPMT, the semi-monthly
nominal period is now daysInYear / 24 (1/24 of the annual rate), like monthly
uses 30 days. New unit test asserts EMI 47.07 for 10/25, 2/17 and 15/31 (it was
44.82 / 42.01 / 46.63 before), and a new E2E checks equal installments with no
oversized last one.
2. Template: the LoanProductData copy constructor now carries both days;
covered by IT and E2E.
3. Re-age: the validator (command and preview) only accepts
BASIC_PERIOD_FREQUENCY_TYPES, so SEMI_MONTHLY gets a 400.
4. Nits: days are cleared when a product or loan moves to another frequency;
comments fixed; changesets renumbered.
While checking parity I also found that the progressive EMI rate factor
lacked the 1/24 "same as repayment period" case its interest twin already had.
The EMI evening-out hid it (same schedule either way), but it's fixed with a
test on the rate factor. I'll update the PR title to "FINERACT-1322: Add
semi-monthly repayment frequency". Docs (semi-monthly-repayment-frequency.adoc)
are updated too.
--
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]
