[Cocoa-Puffs]

Description

Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
• I followed the AI Policy.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[mariiaKraievska]

Align ticket id in commits (FINERACT-2885 vs branch FINERACT-2816)

[mariiaKraievska]

@Cocoa-Puffs Could you please confirm one AC14 point for progressive loans?

generateRepaymentPeriods still applies adjustRepaymentDate only to the last installment (pre-existing). AC14 says working-day/holiday adjustment then applies “as today” — so intermediate progressive due dates after FIRST_DAY_OF_NEXT_MONTH stay on the strategy date even if that day is non-working, while cumulative would move them.

Example: disbursed 31 Jan 2025, 3 months, FIRST_DAY_OF_NEXT_MONTH, weekdays only / move to next working day. Strategy gives 1 Mar 2025 (Sat); cumulative becomes 3 Mar (testRolledDateIsMovedOffNonWorkingDay), progressive would keep 1 Mar. There is no progressive counterpart of that test in this PR.

Is that the intended behaviour here, or should progressive be aligned with cumulative?

[Cocoa-Puffs]

Good catch. It looks like a genuine error, but since it's pre-existing I would rather have it fixed it in a follow-up PR than with this changeset.

[adamsaghy]

@Cocoa-Puffs Please rebase

[adamsaghy]

@Cocoa-Puffs Can you please review the below concerns?

1. Re-aged installments ignore FIRST_DAY_OF_NEXT_MONTH unless interest recalculation is on

The PR passes the strategy into one re-age path, handleReAgeWithInterestRecalculationEnabled (AdvancedPaymentScheduleTransactionProcessor ~L4276). That path builds a temporary schedule from LoanApplicationTerms, so its dates move forward as expected.

The other re-age paths still compute dates with ScheduledDateGenerator.getRepaymentPeriodDate(...), which is a plain plusMonths and doesn't know about the strategy:

• Default re-age (handleReAgeWithCommonStrategy), AdvancedPaymentScheduleTransactionProcessor ~L4162:
  scheduledDateGenerator.getRepaymentPeriodDate(freqType, freqNumber * i, startDate)
• Equal-amortization re-age, ProgressiveEMICalculator.updateModelForReageEqualAmortization ~L2320, same call.
• Due-date change on re-aged installments, handleDueDateChangeOnRepaymentPeriod (dateShiftOnly branch) and shiftReAgedInstallmentsAfterReschedule: newDueDate.plus(offset * repayEvery, MONTHS).

Example: Loan with FIRST_DAY_OF_NEXT_MONTH, re-aged with start date 31 Jan, monthly, 3 installments.

• Interest recalculation off: 31 Jan, 28 Feb, 31 Mar. February is pulled back to the 28th; the setting is silently ignored.
• Interest recalculation on: 31 Jan, 1 Mar, 31 Mar.

So the same loan setting gives different due dates depending on an unrelated flag. The ticket asks to shift all due dates, so re-aged installments should follow the setting too.

Suggested fix: Use MonthlyDueDateResolver.plusMonths(startDate, n, strategy) for monthly stepping in these places. A strategy-aware overload of getRepaymentPeriodDate would also work. Then add one re-age scenario to MonthEndDueDateStrategyTest; none of the 18 IT methods or 12 E2E scenarios cover re-aging.

• Empty PR description. It's the unfilled template. Please summarise the behaviour, including that meeting-calendar loans ignore the setting, plus the new API field and the DB migration.
• E2E UC9 changes a shared global product and changes it back at the end of the scenario. If an assertion fails in between, the product keeps FIRST_DAY_OF_NEXT_MONTH and later scenarios using it can fail. Safer to create a product just for this scenario.

[Cocoa-Puffs]

Re-aged installments ignoring FIRST_DAY_OF_NEXT_MONTH

I reproduced it before fixing. With a re-age starting 31 Jan, equal-amortization re-age produced 31 Jan, 28 Feb, 31 Mar, while the recalculated-interest path produced 31 Jan, 1 Mar, 31 Mar.

Changes:

Added a strategy-aware overload ScheduledDateGenerator.getRepaymentPeriodDate(frequency, n, startDate, strategy). For monthly periods it uses MonthlyDueDateResolver.plusMonths; for other frequencies it’s unchanged. Both re-age loops now pass the loan’s strategy.
The due-date shifts after a re-age (the dateShiftOnly branch of handleDueDateChangeOnRepaymentPeriod and shiftReAgedInstallmentsAfterReschedule) now step through the same resolver instead of newDueDate.plus(..., MONTHS). They now match the EMI-calculator branch beside them, which already used the strategy.

E2E UC9 changing a shared product

UC9 now uses its own product, LP2_MONTH_END_DUE_DATE_PRODUCT_CHANGE, through a new step, "Loan product {string} dedicated to this scenario exists with monthEndDueDateStrategy {string}". The step creates the product from the LP2 roll template, or resets its strategy if it already exists, so a failed earlier run can’t leave it in the wrong state. The restore step at the end of UC9 is gone, and LP2_MONTHLY_END_DUE_DATE_ROLL is no longer modified by any scenario.

[adamsaghy]

@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.

[Cocoa-Puffs]

I have reviewed / resolved the concerns.

1. Disbursement on the 1st unrolled. I believe the logic should stay

With FIRST_DAY_OF_NEXT_MONTH and anchor day 31, February’s due date is 1 Mar. That makes 1 Mar a regular point on the loan’s monthly grid, so 1 Mar → 31 Mar is one whole period. It doesn’t matter whether a due date was rolled there or the loan was disbursed there. The rest of the code treats it the same way: for a first due date of 31 Mar, idealDisbursementDateForMonthlyRepayments also gives 1 Mar. It’s also the whole-period rule the ticket confirmed for 30/360.

5.97 periods would be correct under LAST_DAY_OF_MONTH, where February’s due date is 28 Feb and 1 Mar → 31 Mar is a day short of a full period.

The comment in calculatePeriodsBetweenDates suggested that only rolled due dates are affected, so I reworded it.

2. Rolled dates inferred from the 1st. It’s chained by design: reschedules restart the chain, and the clamp keeps the original anchor after a reschedule to the 28th or later.

Computing seed + n × repayEvery would put AC 1 (existing schedules unchanged) and AC 10 (variations as today) at risk. The holiday example can’t happen: with “reschedule to next repayment date”, ApplyHolidaysToLoansTasklet always starts from an existing due date of the loan, and in a roll-forward loan a 1st after a short month is a rolled date unless a reschedule put it there, which is already handled. Verified with a new API test, testHolidayRescheduledToNextRepaymentDateKeepsRolledDates. A holiday on 31 Mar gives 1 Mar, 1 May, 31 May, 1 Jul against 28 Feb, 30 Apr, 31 May, 30 Jun when clamped: every installment moves one slot forward on its own grid (AC 9).

3. Fixed-length max date on calendar loans. Fixed. calculateMaxDateForFixedLength ignores the strategy when the loan has a meeting calendar (AC 8 and AC 11). While verifying this I found the strategy also leaked into progressive interest on calendar loans (30/360 whole-period counting), both at schedule generation and when transactions reprocess the loan. Both routes now ignore it for calendar-linked loans:

• generation: LoanApplicationTerms.toLoanConfigurationDetails.
• reprocessing: LoanConfigurationDetailsMapper with a new LoanMeetingCalendarService, which only queries when the strategy rolls forward.
• the processor’s direct reads of the strategy go through the same rule.

testMeetingCalendarDecidesDueDatesOfCalendarLoans now covers cumulative, progressive and progressive fixed-length calendar loans. It checks dates and interest, before and after a repayment.

4. Calendar loans store an ignored strategy. Keeping both the stored value and the debug log, because the ticket asks for them:

AC 8: “log at debug when the attribute is set but superseded by a calendar”;
AC 15: the loan “inherits the product value at submission and reads it back”.

Rejecting the combination was the alternative the ticket considered and decided against. With the fixes in point 3 the value now has no effect anywhere. The Swagger descriptions say it doesn’t apply to loans following a meeting calendar.

5. Product change keeps the old strategy. Fixed. When a modify changes productId and doesn’t send monthEndDueDateStrategy, the loan takes the new product’s value; an explicit value still wins. Covered by testChangingTheProductOfAnApplicationTakesItsStrategy.

6. Variation scan before the strategy check. Fixed. unrollChainedDueDate returns early unless the date is the 1st and the strategy rolls forward.

7. Duplicated plusPeriods. Fixed. One MonthlyDueDateResolver.plusPeriods is used by the processor and the EMI calculator. It isn’t routed through ScheduledDateGenerator, because LoanScheduleGeneratorTest builds the EMI calculator with a mocked generator.

8. Duplicated validator. Fixed. LoanApplicationValidator uses LoanProductDataValidator.validateMonthEndDueDateStrategy.

Also found while verifying. Before disbursement, the first due date of a roll-forward loan was derived with plain plusMonths in LoanScheduleAssembler.deriveFirstRepaymentDate. Submitted loans and the calculateLoanSchedule preview showed 28 Feb instead of 1 Mar, which breaks AC 2/3. Fixed and covered by testScheduleOfSubmittedLoanRollsForward.

[adamsaghy]

Reviewed against the FINERACT-2816 story. The date logic matches the ACs well: one MonthlyDueDateResolver behind both former clamp copies, the anchor is restored through unroll, meeting-calendar loans are left alone with a debug log, and the fixed-length maturity, re-age and EMI-side date paths all use the strategy. The interest behaviour also matches the intended business rule:

• 30/360: a rolled period is still one whole month, so interest per period does not change (testRolledDueDateKeepsInterestPerPeriodUnder30360, e2e UC3/UC4).
• Actual/Actual: a rolled period is a day longer and the next one a day shorter, so interest moves between the two periods (testRolledDueDateMovesInterestBetweenPeriodsUnderActualActual, worked example on both schedule types).

PR hygiene:

• The PR description is still the empty template, and none of the checklist boxes are ticked.
• There are two commits with the same headline. Please squash them into one FINERACT-2816: ... commit before merge.