alberto-art3ch commented on PR #6546: URL: https://github.com/apache/fineract/pull/6546#issuecomment-6269710236
> 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. -- 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]
