Re: [PR] FINERACT-1322: Add Support for a Bi Monthly Loan Product [fineract]

["adamsaghy (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org