FINERACT-2455: WC - Support dynamic repayment frequency type and repa…

[budaidev]

…yment interval

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.

[galovics]

Review summary

Nice work overall, the schedule engine change is clean and well tested. @budaidev, the test coverage is solid. My main concern is the upgrade path for existing products, the migration only covers half of it. Plus the changelog number is going to collide. I know it's still a draft, but I can't approve the upgrade path as is.

Recommendation: REQUEST_CHANGES

Inline comments

fineract-working-capital-loan/src/main/resources/db/changelog/tenant/module/workingcapitalloan/parts/0087_wc_scheduled_loan_daily_repayment_frequency.xml:30-41

Also on this changeset: every m_wc_loan with an amortization model is forced to repayment_every=1, DAYS. A loan that was DAYS/30 will report repaymentEvery: 1 from GET after the upgrade, nothing records the original value and there's no rollback. The schedule itself is already protected (a legacy model with no frequency is read as daily), so can we prefer the persisted model's frequency when regenerating a loan that already has a schedule instead of rewriting the loan?

same file, the comment

+    <!-- A persisted schedule was built daily whatever frequency the loan or its product stored, and is rebuilt from the
+         loan's frequency; pinning those loans to DAYS/1 keeps the rebuild on the dates already agreed. Loans without a
+         schedule and products keep what was configured. -->

I'm not convinced "products keep what was configured" is safe. Until now repaymentEvery/repaymentFrequencyType had no effect on the schedule, so I'd assume most tenants left whatever value was there. Our own defaults were 30 DAYS, and you had to change WorkingCapitalLoanProductTestBuilder, WorkingCapitalRequestFactory and the feature file from 30 to 1 to keep the tests green. A real tenant with a 30 DAYS product will get the same change silently on upgrade: every new loan (and every loan that doesn't have an amortization model yet) gets 30-day periods. On PAYMENT_AMOUNT products it's even worse, because paymentAmount/min/maxPaymentAmount were daily amounts and now they're applied per period, so the configured value will be 30x off.

I think we either need to normalize existing products (and loans without a schedule) to DAYS/1 too, or at least make it very visible in the release notes that this has to be reviewed before upgrading. Personally I'd go with the migration, it's less surprising. Thoughts?

fineract-working-capital-loan/src/main/resources/db/changelog/tenant/module/workingcapitalloan/module-changelog-master.xml:111

   <include relativeToChangelogFile="true" file="parts/0086_wc_payment_amount_strategy.xml"/>
+  <include relativeToChangelogFile="true" file="parts/0087_wc_scheduled_loan_daily_repayment_frequency.xml"/>

Just a heads up, #6542 and #6454 both add a 0087_... part in the same module too. The changeset ids are different so nothing breaks, but whoever merges second will have to renumber and rebase. Let's coordinate this.

ProjectedAmortizationScheduleModel.java:774 (currentFirstPeriodDayOffset) / :787 (calculateAllocationDate)

A payment on the disbursement date used to shift a daily schedule by one day. For monthly it now pulls every due date and the maturity a full month earlier, and monthly_repaymentOnTheDisbursementDateMovesEveryDueDateOnePeriodEarlier locks that in. Is that intended, or an artifact of the daily design? Needs a product decision.

Smaller

• A partial override mixes the loan's unit with the product's interval (override only the type to MONTHS on a DAYS/30 product and you get a 30-month period). Require both together or validate the combination.
• The daily-only public overloads in ProjectedAmortizationScheduleModel (generate, generateFromAnnualEir, ...) are only used by tests now, and any future production caller silently gets a daily schedule. Remove them.
• generateAndSaveAmortizationSchedule (internal API) skips requireSchedulableFrequency, so a stored YEARS gives a 500.
• invalid.period.frequency.type is hard-coded in two more places instead of reusing INVALID_PERIOD_FREQUENCY_TYPE_CODE.
• Modify now rejects explicit null/blank repaymentEvery/repaymentFrequencyType. Reasonable, but it's a behaviour change worth a line in the description.
• The PR description is the unfilled template.