alberto-art3ch commented on PR #6546: URL: https://github.com/apache/fineract/pull/6546#issuecomment-6345969886
> @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 `@Setter`s 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. <img width="1214" height="431" alt="Screenshot 2026-10-22 at 11 31 26 AM" src="https://github.com/user-attachments/assets/0fb59773-a579-4740-ab64-d99f0ab67353"; /> -- 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]
