adamsaghy commented on code in PR #6546:
URL: https://github.com/apache/fineract/pull/6546#discussion_r4386911560
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java:
##########
@@ -328,13 +346,17 @@ private LoanApplicationTerms
assembleLoanApplicationTermsFrom(final JsonElement
*/
if (calculatedRepaymentsStartingFromDate == null) {
LocalDate tmpCalculatedRepaymentsStartingFromDate =
deriveFirstRepaymentDate(loanType, repaymentEvery, expectedDisbursementDate,
- repaymentPeriodFrequencyType, 0, calendar,
submittedOnDate, repaymentStartDateType, monthEndDueDateStrategy);
+ repaymentPeriodFrequencyType, 0, calendar,
submittedOnDate, repaymentStartDateType, semiMonthlyDueDays,
+ monthEndDueDateStrategy);
calculatedRepaymentsStartingFromDate =
deriveFirstRepaymentDate(loanType, repaymentEvery, expectedDisbursementDate,
repaymentPeriodFrequencyType,
loanProduct.getMinimumDaysBetweenDisbursalAndFirstRepayment(), calendar,
submittedOnDate,
- repaymentStartDateType, monthEndDueDateStrategy);
+ repaymentStartDateType, semiMonthlyDueDays,
monthEndDueDateStrategy);
if
(!tmpCalculatedRepaymentsStartingFromDate.equals(calculatedRepaymentsStartingFromDate))
{
repaymentsStartingFromDate =
calculatedRepaymentsStartingFromDate;
}
+ } else if (repaymentPeriodFrequencyType.isSemiMonthly() && calendar ==
null) {
Review Comment:
This check only runs on the JSON path (create / schedule preview). Modify
regenerates the schedule from the loan itself (`LoanAssemblerImpl` →
`regenerateRepaymentSchedule(loan, ...)`), and neither
`validateForModify(command, loan)` nor `validateForModify(loan)` checks the
first repayment date against the due days. So both of these get through:
- `PUT` with `repaymentsStartingFromDate` on a non-due day
- `PUT` that changes the due days so the stored
`expectedFirstRepaymentOnDate` is no longer one of them
The first installment then lands on a non-configured day and the rest follow
the new days. `aFirstRepaymentDateOffTheDueDaysIsRejected` only covers creation.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java:
##########
@@ -690,6 +719,16 @@ private void
validateRepaymentsStartDateWithMeetingDates(final LocalDate repayme
}
}
+ private void validateRepaymentsStartDateIsSemiMonthlyDueDate(final
LocalDate repaymentsStartingFromDate,
+ final SemiMonthlyDueDays semiMonthlyDueDays) {
+ if (!SemiMonthlyScheduleDates.isDueDate(repaymentsStartingFromDate,
semiMonthlyDueDays)) {
+ final String errorMessage = "First repayment date '" +
repaymentsStartingFromDate
+ + "' does not fall on a due day of the semi-monthly
schedule";
+ throw new
LoanApplicationDateException("first.repayment.date.not.a.semi.monthly.due.day",
errorMessage,
Review Comment:
Nit: the PR description says validation errors return 400, but
`LoanApplicationDateException` is a domain-rule exception, so this one is 403
(the IT asserts 403). Either update the description or make it a validation
error.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanApplicationValidator.java:
##########
@@ -1080,7 +1096,21 @@ public void validateForModify(final JsonCommand command,
final Loan loan) {
final Integer repaymentEveryType = this.fromApiJsonHelper
.extractIntegerWithLocaleNamed(LoanApiConstants.repaymentFrequencyTypeParameterName,
element);
baseDataValidator.reset().parameter(LoanApiConstants.repaymentFrequencyTypeParameterName).value(repaymentEveryType)
- .notNull().inMinMaxRange(0, 3);
+
.notNull().isOneOfTheseValues(SUPPORTED_PERIOD_FREQUENCY_TYPES);
+ // A loan being modified keeps the days it already has before
falling back to the product's.
+ final LoanProductRelatedDetail dueDaysFallback =
loan.getLoanRepaymentScheduleDetail().getFirstRepaymentDayOfMonth() != null
+ ? loan.getLoanRepaymentScheduleDetail()
+ : loanProduct.getLoanProductRelatedDetail();
+ validateSemiMonthlyRepayment(element, baseDataValidator,
repaymentEveryType, repaymentEvery,
+ dueDaysFallback.getFirstRepaymentDayOfMonth(),
dueDaysFallback.getSecondRepaymentDayOfMonth());
+ } else if
(this.fromApiJsonHelper.parameterExists(LoanApiConstants.FIRST_REPAYMENT_DAY_OF_MONTH,
element)
+ ||
this.fromApiJsonHelper.parameterExists(LoanApiConstants.SECOND_REPAYMENT_DAY_OF_MONTH,
element)) {
+ // Only the days are changing, so they are checked against the
frequency the loan already has.
Review Comment:
`repaymentEvery` must be 1 is only enforced on modify when the request
carries `repaymentFrequencyType` or one of the due days. A `PUT` with just
`repaymentEvery: 2` (plus a matching `loanTermFrequency`) on a semi-monthly
loan skips both branches and is accepted.
Result: due dates stay half-monthly (`SemiMonthlyScheduleDates.next` ignores
`repaymentEvery`), but interest does not:
- progressive, same-as-repayment: rate factor is `rate * 2 / 24` per
half-month period, i.e. double interest
- cumulative: EMI is solved with `rate * 2 / 24` while each period is
charged `rate / 24`
The same branching also skips the `calendarId` rejection on modify.
Suggest running the semi-monthly check against the effective
(request-or-stored) frequency unconditionally, as
`LoanProductDataValidator#validateForUpdate` already does.
--
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]
