[alberto-art3ch]

Description

Adds a semi-monthly repayment frequency (repaymentFrequencyType = 6, SEMI_MONTHLY): a loan repaid twice every calendar month on two configured days.

Fineract supports daily, weekly, every-n-weeks, monthly and yearly repayments, but not two repayments within the same month on fixed days (e.g. payroll on the 15th and the last day of the month).

Configuration

• Loan product: firstRepaymentDayOfMonth (1–27) and secondRepaymentDayOfMonth (2–31, greater than the first). A second day the month lacks falls on its last day, so 31 means "last day of the month". Both days are sent together; repaymentEvery must be 1.
• Loan: may override the product's days (same rules); otherwise it inherits them. On any other frequency the days are rejected if sent and cleared if left over.
• Available for both CUMULATIVE and PROGRESSIVE schedules.

Schedule and interest

• Due dates alternate between the two days; the first repayment date, if given, must be one of them.
• Interest "same as repayment period" charges 1/24 of the annual rate per period; daily interest (cumulative) or actual days in month (progressive) charge the actual days of each period.
• The EMI is derived from a regular half-month period (1/24 of the year), so uneven pairs such as 2/17 still give equal installments.

API

• GET /loanproducts/{id} (also with ?template=true), GET /loans/template and GET /loans/{id} return both days.
• Data validation errors return 400 with specific codes (listed in the docs). A first repayment date that is not one of the due days (on create and on modify) is a business rule violation and returns 403 with error.msg.loan.application.first.repayment.date.not.a.semi.monthly.due.day.

Limitations

• Not supported for loans synchronised with a group/center meeting calendar.
• Re-aging accepts only DAYS, WEEKS, MONTHS and YEARS; SEMI_MONTHLY is rejected with 400.

FINERACT-1322

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.

[adamsaghy]

@alberto-art3ch Please review below my concerns:

1. The schedule is wrong end to end, and CI shows it

All 8 schedule-generation tests in the PR's own LoanSemiMonthlyRepaymentScheduleTest fail on MariaDB, MySQL and PostgreSQL. In every one, the first due date is disbursement + 1 year. For example, a loan disbursed on 02 Jan 2023 is expected to fall due on 2023-01-10 but falls due on 2024-01-02.

• Cause: LoanScheduleAssembler.deriveFirstRepaymentDate (around line 1195) branches on daily, weekly and monthly, and everything else falls into else { // yearly loan } → seedDate.plusYears(repaymentEvery). The PR adds no semi-monthly branch, so the first repayment date is wrong before the new SemiMonthlyScheduleDates.next() is ever called.
• Why nothing caught it: SemiMonthlyScheduleDatesTest passes, but it only tests the helper on its own.
• What this breaks in the spec: Rule 6 (disbursed 12 Jan should give 15 Jan; 20 Jan should give 1 Feb), Example 1, Example 2, and the acceptance criterion "two repayment dates per calendar month".
• When a first repayment date is supplied: if the caller passes repaymentsStartingFromDate, that date is used as-is and is not moved onto a configured day.

2. The configuration doesn't match the spec

Spec	PR
Two fields: first day 1–28, second day 2–28 or "Last Day"	One field, firstRepaymentDayOfMonth, allowed 1–15. The second day is derived: 1→15, 15→last day, anything else→day+15
Rule 1 (second > first) and Rule 3 (no duplicates) are checked on user input	Always true by construction, because there is no second field to validate
Rule 2: second day must be 2–28 or Last Day	firstRepaymentDayOfMonth=14 gives the 29th in every month except non-leap February, which the spec disallows

The only pairs you can set are 1&15, 2&17 … 13&28, 14&29, and 15&Last. Many pairs the spec allows can't be configured at all: 1 & Last Day, 1 & 20, 10 & 20, 5 & 25, 20 & Last Day, or any first day above 15. The spec's own examples (1&15, 15&Last, 10&25) happen to be reachable. This is a design choice the author needs to justify or change, not a bug.

3. What matches the spec

• A new frequency type, SEMI_MONTHLY(6), is added after INVALID, so existing rows are unaffected.
• Last Day resolution is correct in the helper: 28/29 Feb, 30 Apr, 31 Jul (Rules 4 and 5, unit-tested).
• 24 periods per year is used for the APR and periods-per-year calculations.
• The existing daily, weekly, monthly and yearly paths look untouched. The switch statements that gained a SEMI_MONTHLY case either do nothing or throw on purpose.

4. Smaller gaps against the spec

• Progressive schedules are blocked, but only at product create. effectiveLoanScheduleType(element) defaults to CUMULATIVE when loanScheduleType isn't in the request. So a PUT that sets only repaymentFrequencyType=6 on an existing progressive product passes the check. The loan application validator has no such check at all. I haven't traced what the progressive engine then does. Note that the spec's "declining balance/EIR" wording implies progressive should be supported, not rejected.
• Spec Example 3 can't be entered as written. LoanScheduleValidator requires loanTermFrequencyType to equal repaymentFrequencyType, so "Term 6 Months, 12 installments" must be entered as a term of 12 semi-monthly periods.
• A loan's missing day is silently defaulted. A loan can switch to repaymentFrequencyType=6 without a firstRepaymentDayOfMonth (only the product validator requires it), and the code then quietly uses 15, i.e. the 15th & last day.

Suggested asks for the author

1. Add semi-monthly handling to deriveFirstRepaymentDate and get the integration tests green.
2. Either implement the spec's two fields (first 1–28, second 2–28 or Last Day, with validation for order and duplicates), or get the spec changed to the derived-day model. If the derived model stays, disallow 14, since it produces the 29th.
3. Make the progressive guard use the product's stored schedule type on update, and add the same guard to the loan application.

I reviewed the PR against its head commit in a temporary worktree; your current branch wasn't touched. I can post this as a PR comment if you want.

[alberto-art3ch]

@alberto-art3ch Please review below my concerns:

1. The schedule is wrong end to end, and CI shows it

All 8 schedule-generation tests in the PR's own LoanSemiMonthlyRepaymentScheduleTest fail on MariaDB, MySQL and PostgreSQL. In every one, the first due date is disbursement + 1 year. For example, a loan disbursed on 02 Jan 2023 is expected to fall due on 2023-01-10 but falls due on 2024-01-02.

• Cause: LoanScheduleAssembler.deriveFirstRepaymentDate (around line 1195) branches on daily, weekly and monthly, and everything else falls into else { // yearly loan } → seedDate.plusYears(repaymentEvery). The PR adds no semi-monthly branch, so the first repayment date is wrong before the new SemiMonthlyScheduleDates.next() is ever called.
• Why nothing caught it: SemiMonthlyScheduleDatesTest passes, but it only tests the helper on its own.
• What this breaks in the spec: Rule 6 (disbursed 12 Jan should give 15 Jan; 20 Jan should give 1 Feb), Example 1, Example 2, and the acceptance criterion "two repayment dates per calendar month".
• When a first repayment date is supplied: if the caller passes repaymentsStartingFromDate, that date is used as-is and is not moved onto a configured day.

2. The configuration doesn't match the spec

Spec PR
Two fields: first day 1–28, second day 2–28 or "Last Day" One field, firstRepaymentDayOfMonth, allowed 1–15. The second day is derived: 1→15, 15→last day, anything else→day+15
Rule 1 (second > first) and Rule 3 (no duplicates) are checked on user input Always true by construction, because there is no second field to validate
Rule 2: second day must be 2–28 or Last Day firstRepaymentDayOfMonth=14 gives the 29th in every month except non-leap February, which the spec disallows
The only pairs you can set are 1&15, 2&17 … 13&28, 14&29, and 15&Last. Many pairs the spec allows can't be configured at all: 1 & Last Day, 1 & 20, 10 & 20, 5 & 25, 20 & Last Day, or any first day above 15. The spec's own examples (1&15, 15&Last, 10&25) happen to be reachable. This is a design choice the author needs to justify or change, not a bug.

3. What matches the spec

• A new frequency type, SEMI_MONTHLY(6), is added after INVALID, so existing rows are unaffected.
• Last Day resolution is correct in the helper: 28/29 Feb, 30 Apr, 31 Jul (Rules 4 and 5, unit-tested).
• 24 periods per year is used for the APR and periods-per-year calculations.
• The existing daily, weekly, monthly and yearly paths look untouched. The switch statements that gained a SEMI_MONTHLY case either do nothing or throw on purpose.

4. Smaller gaps against the spec

• Progressive schedules are blocked, but only at product create. effectiveLoanScheduleType(element) defaults to CUMULATIVE when loanScheduleType isn't in the request. So a PUT that sets only repaymentFrequencyType=6 on an existing progressive product passes the check. The loan application validator has no such check at all. I haven't traced what the progressive engine then does. Note that the spec's "declining balance/EIR" wording implies progressive should be supported, not rejected.
• Spec Example 3 can't be entered as written. LoanScheduleValidator requires loanTermFrequencyType to equal repaymentFrequencyType, so "Term 6 Months, 12 installments" must be entered as a term of 12 semi-monthly periods.
• A loan's missing day is silently defaulted. A loan can switch to repaymentFrequencyType=6 without a firstRepaymentDayOfMonth (only the product validator requires it), and the code then quietly uses 15, i.e. the 15th & last day.

Suggested asks for the author

1. Add semi-monthly handling to deriveFirstRepaymentDate and get the integration tests green.
2. Either implement the spec's two fields (first 1–28, second 2–28 or Last Day, with validation for order and duplicates), or get the spec changed to the derived-day model. If the derived model stays, disallow 14, since it produces the 29th.
3. Make the progressive guard use the product's stored schedule type on update, and add the same guard to the loan application.

I reviewed the PR against its head commit in a temporary worktree; your current branch wasn't touched. I can post this as a PR comment if you want.

@adamsaghy Thanks for the thorough review — all points confirmed and addressed:

1. First due date (+1 year). deriveFirstRepaymentDate now has a semi-monthly branch: the first due date is the next configured due day after the seed date. If minimumDaysBetweenDisbursalAndFirstRepayment pushes it further, it moves to the next configured due day on/after the gap (never an arbitrary day). A caller-supplied repaymentsStartingFromDate must be a configured due day, otherwise it is rejected. Added integration tests for both cases.
2. Single derived day vs. the spec's two fields. Kept by design, for simplicity and control: capping the first day at 15 and deriving the second keeps exactly two due dates in the same month ~15 days apart, so the ordering/duplicate rules can't be violated and don't need validating. Interest is charged on actual days per period, so balanced periods keep the two monthly installments even. The spec's own examples (1&15, 15&Last, 10&25) are all reachable. The trade-off (e.g. 1&Last isn't configurable) and the rationale are documented in the new fineract-doc chapter.
3. Progressive guard. Product update now falls back to the stored loanScheduleType, and the same guard was added to the loan application.
4. Silent default day. A loan that becomes semi-monthly must resolve a day (request → loan's stored day → product), otherwise it's rejected. On modify, a new day is now persisted and regenerates the schedule; omitting it keeps the loan's own day.

Example 3 (term in months with semi-monthly installments) is left as is: loanTermFrequencyType must equal repaymentFrequencyType for every frequency, so it's entered as 12 semi-monthly periods. This is documented.

[adamsaghy]

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

[alberto-art3ch]

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

[galovics]

Hi @alberto-art3ch, thanks for the PR. Big one, but well covered at every level, and SemiMonthlyScheduleDates is clean. Questions:

• PeriodFrequencyType is a shared core enum. Has adding a loan-only value been discussed on the dev list? It forced dummy case SEMI_MONTHLY: branches in savings and shares and a special-case filter in integerValues(). A loan-specific frequency type would avoid leaking into every switch.
• DefaultScheduledDateGenerator.getRepaymentPeriodDate throws UnsupportedOperationException for SEMI_MONTHLY. The re-age callers are guarded, but can a charge with fee frequency 6 reach it (LoanChargeWritePlatformServiceImpl)? Pls add a test or a validation.
• validateSemiMonthlyRepayment / validateSemiMonthlyDueDays in LoanApplicationValidator is a near copy of LoanProductDataValidator. Can we share one helper next to SemiMonthlyScheduleDates so the 1-27 / 2-31 rules cannot drift?
• toRecurrence for a second day of 29 or 30: the Javadoc admits iCalendar skips the day in short months while next() uses the last day, so the rest/compounding date would not match the due date in February. Map a capped second day to -1, reject "same as repayment period" for semi-monthly, or at least add a test with 30.
• Why a per-loan override of the days at all? A payroll loan is a product-level concern. The fallback chain (request, loan, product) adds a lot of surface; could we ship product-only first?
• @Setter on both days plus setFirstRepaymentDayOfMonth(...) after construction in every factory is easy to forget at one call site. Builder or constructor?
• Housekeeping: e2e shard 12 is red, the 0260 changeset will collide if another lands first, and pls squash.

[alberto-art3ch]

Hi @alberto-art3ch, thanks for the PR. Big one, but well covered at every level, and SemiMonthlyScheduleDates is clean. Questions:

• PeriodFrequencyType is a shared core enum. Has adding a loan-only value been discussed on the dev list? It forced dummy case SEMI_MONTHLY: branches in savings and shares and a special-case filter in integerValues(). A loan-specific frequency type would avoid leaking into every switch.
• DefaultScheduledDateGenerator.getRepaymentPeriodDate throws UnsupportedOperationException for SEMI_MONTHLY. The re-age callers are guarded, but can a charge with fee frequency 6 reach it (LoanChargeWritePlatformServiceImpl)? Pls add a test or a validation.
• validateSemiMonthlyRepayment / validateSemiMonthlyDueDays in LoanApplicationValidator is a near copy of LoanProductDataValidator. Can we share one helper next to SemiMonthlyScheduleDates so the 1-27 / 2-31 rules cannot drift?
• toRecurrence for a second day of 29 or 30: the Javadoc admits iCalendar skips the day in short months while next() uses the last day, so the rest/compounding date would not match the due date in February. Map a capped second day to -1, reject "same as repayment period" for semi-monthly, or at least add a test with 30.
• Why a per-loan override of the days at all? A payroll loan is a product-level concern. The fallback chain (request, loan, product) adds a lot of surface; could we ship product-only first?
• @Setter on both days plus setFirstRepaymentDayOfMonth(...) after construction in every factory is easy to forget at one call site. Builder or constructor?
• Housekeeping: e2e shard 12 is red, the 0260 changeset will collide if another lands first, and pls squash.

Thanks @galovics:

1. Shared enum — raised on dev@ ([link]); keeping it fenced off until there is consensus.
2. Fee frequency 6 — ChargeDefinitionCommandFromApiJsonDeserializer already restricts feeFrequency to 0–3 on create and update; added ChargeDefinitionCommandFromApiJsonDeserializerTest to pin the 400.
3. Validation drift — extracted SemiMonthlyDueDaysValidator, used by both validators.
4. toRecurrence 29/30 — now BYMONTHDAY=d1,28..d2;BYSETPOS=1,-1, i.e. the day or the last day of the month; a test compares the ical4j dates with next() across leap and non-leap Februaries. (For "same as repayment period" the cumulative generator derives dates through next(), so schedules were not affected.)
5. Per-loan override — kept: one product can serve borrowers with different pay days instead of one product per pay-day pair. The rationale will be in the spec. (@bharathc27 please)
6. Setters — the days go through the constructor; @Setter removed.
7. Changeset renumbered to 0261; shard 12 was a test setup issue (LP1's partial-period interest combined with daily interest calculation), fixed; squashed.

[bharathcgowda]

@galovics @adamsaghy can anyone help please review this PR?
@alberto-art3ch has completed the previous review comments.

[adamsaghy]

@alberto-art3ch Thanks for the updates. I re-reviewed the current head (412bc3a5f4). The code is close: I found no new correctness bugs, and the points from my two earlier rounds are all fixed. What is left is mostly follow-ups to @galovics' review.

Verified as fixed

• deriveFirstRepaymentDate has a semi-monthly branch; it respects the minimum gap, and a supplied repaymentsStartingFromDate must be a due day.
• Cumulative EMI with daily interest: with isForPMT the nominal period is days-in-year / 24.
• The LoanProductData copy constructor carries both days, so ?template=true returns them.
• Re-age (command and preview) only accepts the basic frequencies, so SEMI_MONTHLY gets a 400.
• Product and loan both clear the days when they move to another frequency.
• repaymentEvery must be 1, and a semi-monthly loan with a calendarId is rejected.
• calculateLoanSchedule goes through validateForCreate, so bad days get a 400 there too.
• SemiMonthlyDueDaysValidator is shared by the product and loan validators.
• toRecurrence uses BYSETPOS=1,-1 for second days of 29/30.
• Fee frequency stays limited to 0–3, so a charge can't reach the UnsupportedOperationException.
• Changeset renumbered to 0261, ids in order.

I also checked that working-day and holiday shifts don't make later due dates drift: the next due date is generated from the unshifted date, and only "reschedule to next repayment date" moves that base, onto a real due day.

Still open

1. Setters (galovics No.6). The reply says "the days go through the constructor; @Setter removed", but that is only true for LoanApplicationTerms. The days are still set after construction in:

   • LoanProductAssembler (~line 360), on the new LoanProduct;
   • LoanApplicationTerms.toLoanProductRelatedDetail() (~line 1794);
   • LoanProductReadPlatformServiceImpl (~line 617); LoanProductData gained non-final @Setter fields.

   LoanProductRelatedDetail keeps its class-level @Setter. Every construction site sets the days today, so nothing is broken, but the risk galovics raised is still there. Please either move the days into LoanProductRelatedDetail.createFrom and the LoanProduct constructor, or correct the reply.

2. galovics No.1 We should not force something onto other products where it is not supported (savings, share, etc.)

3. Per-loan override rationale (galovics No.5). It was promised in the spec. The fineract-doc chapter describes what the override does, but not why it is needed.

Nit

• A product PUT that resends repaymentFrequencyType: 6 without the days is rejected with "cannot be blank", even when the product is already semi-monthly and has stored days. A loan modify falls back to its stored days in the same situation. The doc does state the product rule, but consider letting the product fall back to its stored days when the frequency isn't changing.

[alberto-art3ch]

@alberto-art3ch Thanks for the updates. I re-reviewed the current head (412bc3a5f4). The code is close: I found no new correctness bugs, and the points from my two earlier rounds are all fixed. What is left is mostly follow-ups to @galovics' review.

Verified as fixed

• deriveFirstRepaymentDate has a semi-monthly branch; it respects the minimum gap, and a supplied repaymentsStartingFromDate must be a due day.
• Cumulative EMI with daily interest: with isForPMT the nominal period is days-in-year / 24.
• The LoanProductData copy constructor carries both days, so ?template=true returns them.
• Re-age (command and preview) only accepts the basic frequencies, so SEMI_MONTHLY gets a 400.
• Product and loan both clear the days when they move to another frequency.
• repaymentEvery must be 1, and a semi-monthly loan with a calendarId is rejected.
• calculateLoanSchedule goes through validateForCreate, so bad days get a 400 there too.
• SemiMonthlyDueDaysValidator is shared by the product and loan validators.
• toRecurrence uses BYSETPOS=1,-1 for second days of 29/30.
• Fee frequency stays limited to 0–3, so a charge can't reach the UnsupportedOperationException.
• Changeset renumbered to 0261, ids in order.

I also checked that working-day and holiday shifts don't make later due dates drift: the next due date is generated from the unshifted date, and only "reschedule to next repayment date" moves that base, onto a real due day.

Still open

1. Setters (galovics No.6). The reply says "the days go through the constructor; @Setter removed", but that is only true for LoanApplicationTerms. The days are still set after construction in:

   • LoanProductAssembler (~line 360), on the new LoanProduct;
   • LoanApplicationTerms.toLoanProductRelatedDetail() (~line 1794);
   • LoanProductReadPlatformServiceImpl (~line 617); LoanProductData gained non-final @Setter fields.

   LoanProductRelatedDetail keeps its class-level @Setter. Every construction site sets the days today, so nothing is broken, but the risk galovics raised is still there. Please either move the days into LoanProductRelatedDetail.createFrom and the LoanProduct constructor, or correct the reply.

2. galovics No.1 We should not force something onto other products where it is not supported (savings, share, etc.)

3. Per-loan override rationale (galovics No.5). It was promised in the spec. The fineract-doc chapter describes what the override does, but not why it is needed.

Nit

• A product PUT that resends repaymentFrequencyType: 6 without the days is rejected with "cannot be blank", even when the product is already semi-monthly and has stored days. A loan modify falls back to its stored days in the same situation. The doc does state the product rule, but consider letting the product fall back to its stored days when the frequency isn't changing.

@adamsaghy thanks for re-checking. The three open points and the nit are addressed in the new commit:

1. Setters (galovics No.6). You're right, my earlier reply only covered LoanApplicationTerms. The two days are now constructor parameters everywhere: LoanProductRelatedDetail (constructor and createFrom), LoanProduct, and LoanProductData (both fields are final, the @Setters are gone). LoanProductAssembler, LoanApplicationTerms.toLoanProductRelatedDetail() and the read-service mapper pass them in; there is no post-construction assignment left. I left the pre-existing class-level @Setter of LoanProductRelatedDetail alone, since it predates this PR and is used by the update path for every other field.

2. Shared enum (galovics No.1). I raised it on dev@ (see the screenshot attached). I agree a loan-only frequency type is the cleaner shape, but repaymentPeriodFrequencyType is a PeriodFrequencyType across the whole loan domain (LoanProductRelatedDetail, LoanApplicationTerms, the schedule generators, EMI/APR calculators, calendar utils), so changing its type is a refactor of its own and I'd rather do it in a follow-up PR than grow this one. What this PR guarantees is that no other product can be forced into it: every savings and deposit validator limits the frequency to inMinMaxRange(0, 3), share lock-in does the same, and interest rate charts validate against PeriodFrequencyType.integerValues(), which excludes 6. The case SEMI_MONTHLY branches outside loans are therefore unreachable by validation; they only exist to keep the switches exhaustive. If you prefer, I can open the follow-up ticket now and link it here.

3. Per-loan override rationale (galovics No.5). Added to the fineract-doc chapter: the product carries the default days and the loan the days that match the borrower's income; a payroll lender serves employers paying on different days (1st & 15th, 10th & 25th, 15th & last day) with otherwise identical product terms, so without the override it would need one product per pay-day pair. It follows the existing product-defaults / loan-override pattern.

Nit. Done: a product PUT that resends repaymentFrequencyType: 6 without the days keeps its stored days; the days are only required when the frequency actually changes. Integration test aProductThatStaysSemiMonthlyKeepsItsDueDaysWhenTheyAreNotResent and the doc rule updated accordingly.

[adamsaghy]

@alberto-art3ch Please resolve the conflicts.

[alberto-art3ch]

@alberto-art3ch Please resolve the conflicts.

Conflicts solved, thanks!

[adamsaghy]

@bharathcgowda before merge, please confirm (or update the spec page to match) three deliberate deviations from spec v4: (1) first repayment day is 1–27, not 1–28, because 28 & Last Day collide on 28 Feb in non-leap years (Rule 3); (2) the second day accepts 29 and 30, falling back to the last day in shorter months (Rule 2 allows only 2–28 or Last Day); (3) re-aging rejects the SEMI_MONTHLY frequency, and loans synced to a meeting calendar aren't supported. @alberto-art3ch please open the follow-up ticket for the loan-only frequency type and link it here, and update the JIRA summary from "Bi Monthly" to "Semi-Monthly".

[alberto-art3ch]

@bharathcgowda before merge, please confirm (or update the spec page to match) three deliberate deviations from spec v4: (1) first repayment day is 1–27, not 1–28, because 28 & Last Day collide on 28 Feb in non-leap years (Rule 3); (2) the second day accepts 29 and 30, falling back to the last day in shorter months (Rule 2 allows only 2–28 or Last Day); (3) re-aging rejects the SEMI_MONTHLY frequency, and loans synced to a meeting calendar aren't supported. @alberto-art3ch please open the follow-up ticket for the loan-only frequency type and link it here, and update the JIRA summary from "Bi Monthly" to "Semi-Monthly".

@adamsaghy thanks. Confirming the three deviations from spec v4 as deliberate; the code, the fineract-doc chapter and the Swagger descriptions all state them the same way:

1. First day 1–27. The second day is capped at the length of the month, so with a first day of 28 every valid second day (29, 30, 31/Last Day) falls on the 28th in a non-leap February and both repayments of that month would land on the same date, which Rule 3 forbids. That includes the spec's own "28 & Last Day" pair, so 27 is the highest first day that keeps the two dates distinct in every month (SemiMonthlyScheduleDates.MAX_FIRST_DAY_OF_MONTH, chapter section "Why the first day stops at 27").
2. Second day 29 and 30. Accepted and resolved to the last day of shorter months, the same way 31 resolves to the last day; the recurrence rule uses BYSETPOS=1,-1 so the rest/compounding dates match the due dates. Rule 2 only allows 2–28 or Last Day, so this is a superset of the spec.
3. Re-aging and meeting calendars. Re-age and re-age preview accept DAYS, WEEKS, MONTHS and YEARS and reject SEMI_MONTHLY with 400, since re-aging derives due dates from a period length; a semi-monthly loan can still be re-aged with one of those. A semi-monthly loan with a calendarId is rejected with 400 because meeting calendars have no semi-monthly recurrence. Both are listed under "Limitations" in the chapter.

@bharathcgowda if you update the spec page, the chapter at fineract-doc/src/docs/en/chapters/features/semi-monthly-repayment-frequency.adoc has the wording for all three.

Follow-up ticket for the loan-only frequency type: FINERACT-2992. It keeps the persisted values (0–3 and 6) so it needs no migration and no API change; it retypes repaymentPeriodFrequencyType across the loan domain and removes SEMI_MONTHLY from PeriodFrequencyType together with the dummy branches in savings, deposits, shares and interest rate charts.

JIRA summary of FINERACT-1322 renamed from "Bi Monthly" to "Semi-Monthly".

[bharathcgowda]

@adamsaghy Thank you for reviewing and sharing the feedback.
I have updated the Spec page as discussed and implemented.
cc @alberto-art3ch

[bharathcgowda]

@adamsaghy, could you please continue reviewing this PR, as the shared concerns have now been addressed?

[adamsaghy]

Reviewed against the PR description. Create/read paths match; two gaps on loan modification and one description nit, see inline.

[alberto-art3ch]

Reviewed against the PR description. Create/read paths match; two gaps on loan modification and one description nit, see inline.

Changes requested solved, Thanks! @adamsaghy

[adamsaghy]

I ran two lifecycle cases on a local build of this head (progressive loans, interest recalculation on). Numbers below are from those runs.

[adamsaghy]

Re-aged monthly installments are charged half the interest.

This affects a progressive semi-monthly loan with interest recalculation and interest calculated "same as repayment period". Re-aging can't use SEMI_MONTHLY, so it always switches the loan to another frequency (here months). The re-aged periods are attached in attachTemporaryScheduleModelReAgedPeriodsToExistingModel with the loan's own configuration, so their interest is worked out here: 1/24 of the annual rate per period, even though each re-aged period is a month.

I ran it on this head. All three loans have 24% interest and were re-aged on 20 March 2023, before maturity, into 3 monthly installments starting 15 April:

Loan	2nd re-aged installment	3rd re-aged installment
Semi-monthly 15/31, same as repayment	7.03 on 703.15 (1%)	3.53 on 353.33 (1%)
Monthly, same as repayment (control)	14.26 on 712.85 (2%)	7.20 on 359.96 (2%)
Semi-monthly 15/31, daily interest	13.79 (30 days)	7.19 (31 days)

The EMI is computed at the halved rate too (356.85 against 367.15 on the monthly loan). The loan still amortises, but it charges half the interest. Daily interest is fine.

Could the re-aged periods be charged by their own frequency? Alternatively, could re-aging a semi-monthly loan be restricted to combinations that charge correctly, with a test for a re-age with MONTHS?

[adamsaghy]

@bharathcgowda @alberto-art3ch Please validate my findings

[adamsaghy]

A one-day first period is charged a full half-month by default.

Without a pinned first repayment date, the first due date is simply the next due day after disbursement, so it can be a day away. I ran this on this head with due days 1 and 16, disbursed on 15 January 2023, 1000 at 24% over 4 installments:

Interest	1st installment (15 → 16 January)	Interest in it
Same as repayment	256.28	10.00 (a full 1/24)
Daily	253.90	0.66

This happens even with allowPartialPeriodInterestCalculation on. A monthly loan behaves the same if its first date is pinned one day out (20.00 for one day). The difference is that a monthly loan's default first date is a full month away, while a semi-monthly loan gets the short period without anyone asking for it. Only minimumDaysBetweenDisbursalAndFirstRepayment avoids it.

Is that intended? If not, the default first due date could skip a due day that is closer than some minimum gap, or the docs could say to set the minimum gap for semi-monthly products.

[adamsaghy]

@bharathcgowda @alberto-art3ch Please validate my findings

[bharathcgowda]

When allowPartialPeriodInterestCalculation is enabled for same as repayment, interest should be calculated only for the number of days between disbursement and repayment.

A repayment made the day after disbursement is valid under this configuration, but interest should still be calculated for that one day.

However, if minimumDaysBetweenDisbursalAndFirstRepayment is configured, the system should prioritize and enforce this setting.

outdated

[alberto-art3ch]

I ran two lifecycle cases on a local build of this head (progressive loans, interest recalculation on). Numbers below are from those runs.

Confirmed, thanks for running it. With interest "same as repayment period" the rate factor only looks at the loan's own repaymentPeriodFrequencyType (ProgressiveEMICalculator#calculateRateFactorPerPeriod), never at the length of the period, and the re-aged periods are attached with the loan's configuration, so each monthly re-aged period gets 1/24. Your control numbers match the same code path at 1/12.

Charging a re-aged period by its own frequency needs a frequency per period in the model, and the same mechanism today prices a monthly loan re-aged into weeks at 1/12 per week (the isMonthly() / isWeekly() branches are pre-existing), so that is an engine change of its own: follow-up FINERACT-XXXX.

For this PR I took your second option. LoanReAgingValidator now rejects the re-age, and its preview, on a semi-monthly loan whose interest is calculated "same as repayment period" (403, error.msg.loan.reage.not.allowed.on.semi.monthly.loan.with.interest.per.repayment.period). Daily interest with actual days in month charges each re-aged period on its own days and stays allowed.

Chapter updated: error table and Limitations now state that a semi-monthly loan is re-aged only with daily interest, and why.

[adamsaghy]

@alberto-art3ch Please review the failing IT tests and let me know once all fixed and i can review again