adamsaghy commented on PR #6546:
URL: https://github.com/apache/fineract/pull/6546#issuecomment-6344834962
@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 #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. **dev@ discussion (galovics #1).** The reply reads "raised on dev@
([link])". Please share the actual thread; adding a loan-only value to the
shared `PeriodFrequencyType` enum should have list consensus before merge.
3. **Per-loan override rationale (galovics #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.
--
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]
