adamsaghy commented on PR #6554: URL: https://github.com/apache/fineract/pull/6554#issuecomment-6223272523
> ## Review > Thanks @budaidev, this is a lot better. The migration now normalizes products and unscheduled loans to `DAYS/1`, loans with a schedule keep their stored values and rebuilds read the frequency from the model, so the 30x payment amount problem is gone. The partial override is validated, the daily-only overloads are removed, the internal generate endpoint gives a 400 for YEARS, and the error code constant is reused. Nice. > > A few things are still open: > > `ProjectedAmortizationScheduleModel.java` (`currentFirstPeriodDayOffset` / `calculateAllocationDate`): > > > Still the same as last time: on a monthly loan, a repayment on the disbursement date moves every due date and the maturity one month earlier, and `monthly_repaymentOnTheDisbursementDate_movesEveryDueDateOnePeriodEarlier` locks that in. I haven't seen an answer on whether that's what the product wants or just left over from the daily design. Can you or @adamsaghy confirm? > > `0088_wc_normalize_repayment_frequency_to_daily.xml` / `generateAndSaveAmortizationScheduleOnApproval`: > > > One gap is left. A loan that was approved before the upgrade keeps e.g. `30 DAYS` on the row, and its daily schedule protects it only while that schedule is the one being rebuilt. If someone undoes the approval after the upgrade (or undoes disbursal and then approval) and approves again, `loanFrequency(loan)` takes the `30 DAYS` from the row and the loan gets 30-day periods, and on PAYMENT_AMOUNT the amount is 30x off again. The migration only covers loans that were in status 100 at upgrade time. Maybe on approval we could use the leftover model's frequency when it was stored without one, or document this. What do you think? > > `ProjectedAmortizationScheduleModel.java` (`undoPayment`): > > > Now that `ActualPayment` keeps the `paymentDate` and the fee is booked from it, two payments with the same amount in the same monthly period end up on the same allocation date. `undoPayment` only matches on allocation date + amount, so reversing the 20th's payment can remove the 5th's entry, and the remaining payment is then recognised from the wrong day. Let's match on the payment date as well (with `effectiveDate()` as the fallback for older models). > > `module-changelog-master.xml`: > > > Heads up again on numbering: #6454 got merged as 0087 and you moved to 0088, but #6542 (`0088_wc_adjust-by-delta-transaction-permission.xml`) and #6640 (`0088_put_wc_cob_job_in_the_cob_scheduler_group.xml`) also add a 0088. Whoever goes second will need to renumber. > > The PR description is also still the empty template. Please fill it in, and mention the stricter null/blank handling of `repaymentEvery`/`repaymentFrequencyType` on modify plus the migration behaviour, since both change what existing tenants see. > > Thanks! > > Recommendation: REQUEST_CHANGES I can confirm the Point 1 is fine -- 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]
