adamsaghy commented on PR #6588:
URL: https://github.com/apache/fineract/pull/6588#issuecomment-6148497073
@Cocoa-Puffs Please review the below:
## Review: FINERACT-2816 – monthEndDueDateStrategy
### 1. Disbursement / interest-charged-from date on the 1st is unrolled as
if it had rolled forward
`fineract-loan/.../loanschedule/domain/LoanApplicationTerms.java:1216`
`calculatePeriodsBetweenDates` passes every start date through
`unrollChainedDueDate`, so any start date that falls on the 1st is unrolled.
That includes the disbursement date and the interest-charged-from date, which
are never rolled.
**Example:** `FIRST_DAY_OF_NEXT_MONTH`, `repaymentsStartingFromDate` =
2025-03-31 (seed day 31), disbursement on 2025-03-01, cumulative loan with
`SAME_AS_REPAYMENT_PERIOD` and `allowPartialPeriodInterestCalculation`.
- `calculatePeriodsInLoanTerm` calls
`calculatePeriodsBetweenDates(2025-03-01, 2025-08-31)`.
- `unroll()` turns 03-01 into 02-28.
- The result is 6.0 periods instead of about 5.97 (5 + 30/31).
The same misreading in `calculateLoanTermFrequency` inflates first-period
interest, and total flat interest is overstated. No due date was actually
rolled.
### 2. Whether a date was rolled is guessed from it being a 1st
`fineract-loan/.../loanschedule/domain/DefaultScheduledDateGenerator.java:133`
The cumulative path keeps chaining from the previous due date. It infers
that a 1st was rolled when the seed day is greater than 28 and the previous
month is too short. Only due-date variations are treated as exceptions, so any
genuine 1st that enters the chain gets misread.
**Example:** `ApplyHolidaysToLoansTasklet:180` calls
`generateNextRepaymentDate(adjustedRescheduleToDate, …)`. With a holiday
reschedule-to date of 1 March and seed day 31, the chain unrolls it to 28 Feb.
The next due date becomes 31 Mar instead of 1 Apr.
Issue #1 has the same root cause. Computing each due date as `seed + n ×
repayEvery` via `MonthlyDueDateResolver.plusMonths`, as the progressive paths
already do, would remove the guessing.
### 3. Fixed-length max date rolls forward on calendar-linked loans
`fineract-loan/.../loanschedule/domain/LoanApplicationTerms.java:2137`
`calculateMaxDateForFixedLength` applies the strategy unconditionally.
`generateNextRepaymentDate` sets the strategy to `null` for calendar-linked
loans, so the two disagree.
**Example:** calendar-linked monthly loan with `fixedLength` = 1 month,
starting 31 Jan, with `FIRST_DAY_OF_NEXT_MONTH`.
- The generated last due date is 28 Feb.
- `calculateMaxDateForFixedLength` returns 1 Mar.
- `generateRepaymentPeriods` / `getLastRepaymentDate` overwrite the last
installment with 1 Mar, against the meeting calendar.
### 4. Calendar loans accept a strategy that is then ignored
`fineract-provider/.../loanschedule/service/LoanScheduleAssembler.java:558`
A loan-level `FIRST_DAY_OF_NEXT_MONTH` on a calendar-synced loan is only
logged at debug level. The value is persisted on the loan and returned by `GET
/loans/{id}`, but due-date generation ignores it. Only the fixed-length path
from #3 uses it.
Rejecting it in validation, or not storing it, would avoid misleading API
clients. The `@Slf4j` and the debug log, added only for this, could then go.
### 5. Changing the product on a loan application keeps the old product's
strategy
`fineract-provider/.../loanaccount/service/LoanAssemblerImpl.java:576`
When a loan application is modified to a different `productId`,
`loanRepaymentScheduleDetail` keeps the previous product's
`monthEndDueDateStrategy` unless the request also sends
`monthEndDueDateStrategy`.
**Example:** a loan is created under product A (`FIRST_DAY_OF_NEXT_MONTH`)
and modified to product B (`LAST_DAY_OF_MONTH`) without the field. The schedule
is regenerated from the loan's stored detail and still rolls forward.
### 6. The due-date variation scan runs before the strategy short-circuit
(efficiency)
`fineract-loan/.../loanschedule/domain/LoanApplicationTerms.java:1946`
`unrollChainedDueDate` streams all due-date variations before
`MonthlyDueDateResolver.unroll` checks `rollsForward`. Every existing loan
(null or `LAST_DAY_OF_MONTH`) pays an O(N) scan on each
`generateNextRepaymentDate` / `calculatePeriodsBetweenDates` call, and then the
result is discarded.
Check `MonthEndDueDateStrategy.rollsForward(...)` and `dayOfMonth == 1`
first.
### 7. `plusPeriods` duplicates `getRepaymentPeriodDate(…, strategy)` (reuse)
`fineract-progressive-loan/.../AdvancedPaymentScheduleTransactionProcessor.java:469`
The new private `plusPeriods` helpers in
`AdvancedPaymentScheduleTransactionProcessor` and `ProgressiveEMICalculator`
re-implement the new 4-arg `ScheduledDateGenerator.getRepaymentPeriodDate`: use
the resolver for `MONTHS`, a plain `plus` otherwise. The processor already has
`scheduledDateGenerator` in scope.
With three copies of the rule, a later change has to be made in all three,
or schedules diverge between re-age, due-date change and generation.
### 8. `validateMonthEndDueDateStrategy` is duplicated (reuse)
`fineract-provider/.../loanaccount/serialization/LoanApplicationValidator.java:823`
`LoanApplicationValidator` and `LoanProductDataValidator` each add an
identical private `validateMonthEndDueDateStrategy` (`parameterExists`, then
`extractStringNamed`, then `isOneOfEnumValues`). Both copies have to be kept in
sync, for example if the strategy gets restricted to monthly frequency or
rejected for calendar loans.
--
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]
