adamsaghy commented on code in PR #6588:
URL: https://github.com/apache/fineract/pull/6588#discussion_r4308325446
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java:
##########
@@ -312,15 +317,21 @@ private LoanApplicationTerms
assembleLoanApplicationTermsFrom(final JsonElement
}
}
+ MonthEndDueDateStrategy monthEndDueDateStrategy =
loanProduct.getLoanProductRelatedDetail().getMonthEndDueDateStrategy();
+ if
(this.fromApiJsonHelper.parameterExists(LoanApiConstants.monthEndDueDateStrategyParameterName,
element)) {
Review Comment:
**Question – scope.** The story says the attribute is set on the product and
that the loan *inherits* it at submission. This change also accepts
`monthEndDueDateStrategy` on the loan request, so a loan can override its
product at submission and on modify (Swagger: "Overrides the loan product's
month end due date strategy for this loan", e2e UC8).
Was the loan-level override agreed with the product owner? If yes, please
add it to the story/ACs so it is documented and supported. If not, I'd drop it
here, in `LoanApplicationValidator`'s supported-parameters set, in
`updateLoanApplicationAttributes`, and in the two Swagger request fields, and
keep the loan strictly inheriting from the product.
##########
fineract-e2e-tests-runner/src/test/java/org/apache/fineract/test/initializer/global/LoanProductGlobalInitializerStep.java:
##########
@@ -5298,6 +5298,106 @@ public void initialize() throws Exception {
responseLoanProductsRequestLP2AdvPmntIntEmi36030IntRecalcDailyMultiDisburseAutoDownpaymentAccelerateMaturity);
});
+ tasks.add(() -> {
+ // LP1 monthly with actual/actual interest and LAST_DAY_OF_MONTH
strategy
+ String name181 =
DefaultLoanProduct.LP1_MONTHLY_END_DUE_DATE_CLAMP_CUMULATIVE.getName();
Review Comment:
**Change request – CI is red because of this.**
`run-regression-safety-db-changes` starts the *baseline* (develop) backend and
runs this PR's e2e runner against it. The global initializer runs before tag
filtering, so it tries to create these new products on a backend that does not
know the field. Every one of them fails with `The parameter
monthEndDueDateStrategy is not supported` (400), and the run aborts with
`Parallel loan cleanup failed` ([job
log](https://github.com/apache/fineract/actions/runs/38872202557/job/116886463967)).
Could you create these products where the feature needs them (for example in
the feature's own Background / step defs) instead of in the global initializer,
or otherwise make the job pass? As it stands, the check fails for the PR
itself, not because of something flaky.
##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/transactionprocessor/impl/AdvancedPaymentScheduleTransactionProcessor.java:
##########
@@ -4159,7 +4172,8 @@ private void
handleReAgeWithCommonStrategy(LoanTransaction loanTransaction, Comm
reAgedInstallmentIndex++;
for (int i = 1; i < numberOfReAgeInstallments; i++) {
final LocalDate calculatedDueDate =
scheduledDateGenerator.getRepaymentPeriodDate(loanReAgeParameter.getFrequencyType(),
- loanReAgeParameter.getFrequencyNumber() * i,
loanReAgeParameter.getStartDate());
+ loanReAgeParameter.getFrequencyNumber() * i,
loanReAgeParameter.getStartDate(),
+
LoanConfigurationDetailsMapper.monthEndDueDateStrategy(loan,
loanMeetingCalendarService));
Review Comment:
Nit: on a loan with `FIRST_DAY_OF_NEXT_MONTH`,
`LoanConfigurationDetailsMapper.monthEndDueDateStrategy(...)` runs a
`calendarInstanceRepository` query on every call, and here it runs once per
re-aged installment. Please resolve it once before the loop.
More generally, `LoanConfigurationDetailsMapper.map(loan, ...)` now runs
this query on every reprocessing/model load for these loans. It is probably
fine, but it's worth keeping in mind for COB.
##########
fineract-core/src/main/java/org/apache/fineract/portfolio/calendar/service/CalendarUtils.java:
##########
@@ -90,13 +90,7 @@ public static LocalDate getNextRecurringDate(final String
recurringRule, final L
}
public static Temporal adjustDate(final Temporal date, final Temporal
seedDate, final PeriodFrequencyType frequencyType) {
- if (frequencyType.isMonthly() &&
seedDate.get(ChronoField.DAY_OF_MONTH) > 28 &&
date.get(ChronoField.DAY_OF_MONTH) >= 28) {
- int noOfDaysInCurrentMonth = YearMonth.from(date).lengthOfMonth();
- int seedDay = seedDate.get(ChronoField.DAY_OF_MONTH);
- int adjustedDay = Math.min(noOfDaysInCurrentMonth, seedDay);
- return date.with(ChronoField.DAY_OF_MONTH, adjustedDay);
- }
- return date;
+ return MonthlyDueDateResolver.adjustDate(date, seedDate,
frequencyType, MonthEndDueDateStrategy.LAST_DAY_OF_MONTH);
Review Comment:
Nit: the hard-coded `LAST_DAY_OF_MONTH` is correct, because after this
change the only callers left are on the meeting-calendar paths, where the
calendar decides the date (AC 8). But it looks like an oversight when you read
it on its own. A one-line comment saying why would stop someone from "fixing"
it later.
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/domain/LoanApplicationTerms.java:
##########
@@ -1195,17 +1212,22 @@ public BigDecimal calculatePeriodsBetweenDates(final
LocalDate startDate, final
.add(BigDecimal.valueOf((double) daysLeftAfterWeeks /
7));
break;
case MONTHS:
- int numberOfMonths = DateUtils.getExactDifference(startDate,
endDate, ChronoUnit.MONTHS);
+ // a 1st after a month too short for the anchor day is that
month's date when rolling forward, so it
Review Comment:
Nit: spotless broke this comment across lines in the middle of a sentence
(`so it` / `counts` / `whole months ...`). Please rewrite it so it wraps
cleanly.
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/MonthEndDueDateStrategyTest.java:
##########
@@ -0,0 +1,931 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import java.util.List;
+import java.util.Objects;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+import java.util.function.UnaryOperator;
+import org.apache.fineract.client.models.GetLoanProductsProductIdResponse;
+import org.apache.fineract.client.models.GetLoansLoanIdRepaymentPeriod;
+import org.apache.fineract.client.models.PostHolidaysRequest;
+import org.apache.fineract.client.models.PostHolidaysRequestOffices;
+import org.apache.fineract.client.models.PostLoanProductsRequest;
+import org.apache.fineract.client.models.PostLoansRequest;
+import org.apache.fineract.client.models.PutGlobalConfigurationsRequest;
+import org.apache.fineract.client.models.PutLoanProductsProductIdRequest;
+import org.apache.fineract.client.models.PutLoansLoanIdRequest;
+import org.apache.fineract.client.models.StringEnumOptionData;
+import org.apache.fineract.client.models.WorkingDaysData;
+import org.apache.fineract.client.models.WorkingDaysUpdateRequest;
+import
org.apache.fineract.infrastructure.configuration.api.GlobalConfigurationConstants;
+import org.apache.fineract.integrationtests.client.feign.FeignLoanTestBase;
+import
org.apache.fineract.integrationtests.client.feign.helpers.FeignGroupHelper;
+import
org.apache.fineract.integrationtests.client.feign.helpers.FeignRawHttpHelper;
+import
org.apache.fineract.integrationtests.client.feign.modules.LoanRequestBuilders;
+import org.apache.fineract.integrationtests.client.feign.modules.LoanTestData;
+import
org.apache.fineract.integrationtests.client.feign.modules.LoanTestData.DaysInMonthType;
+import
org.apache.fineract.integrationtests.client.feign.modules.LoanTestData.DaysInYearType;
+import org.apache.fineract.integrationtests.common.CalendarHelper;
+import org.apache.fineract.integrationtests.common.FineractFeignClientHelper;
+import org.apache.fineract.integrationtests.common.HolidayHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.apache.fineract.integrationtests.common.WorkingDaysHelper;
+import org.junit.jupiter.api.Test;
+
+/**
+ * A monthly due date whose anchor day (the disbursement's day of month) is
missing from the target month - 30 February,
+ * 31 April - is clamped back to the last day of the month by default, or
rolled forward to the first day of the next
+ * month when the product says so. Either way the anchor day comes back as
soon as a month has it.
+ */
+public class MonthEndDueDateStrategyTest extends FeignLoanTestBase {
+
+ private static final String LAST_DAY_OF_MONTH = "LAST_DAY_OF_MONTH";
+ private static final String FIRST_DAY_OF_NEXT_MONTH =
"FIRST_DAY_OF_NEXT_MONTH";
+
+ private static final Integer DISBURSEMENT_DATE = 1;
+ private static final Integer SUBMITTED_ON_DATE = 2;
+ private static final Integer MOVE_TO_NEXT_WORKING_DAY = 2;
+ private static final Integer RESCHEDULE_TO_NEXT_REPAYMENT_DATE = 1;
+ private static final Integer RESCHEDULE_TO_SPECIFIED_DATE = 2;
+
+ private static final double PRINCIPAL = 10000.0;
+ private static final double INTEREST_RATE = 12.0;
+
+ private final FeignGroupHelper groupHelper = new
FeignGroupHelper(FineractFeignClientHelper.getFineractFeignClient());
+
+ @Test
+ public void testProductExposesAndUpdatesTheStrategy() {
+ final Long unsetProductId = createLoanProduct(create4IProgressive());
+
assertNull(retrieveLoanProduct(unsetProductId).getMonthEndDueDateStrategy(),
"An unset strategy reads back as unset");
+
+ for (final PostLoanProductsRequest template :
List.of(create4IProgressive(), create4ICumulative())) {
+ final Long productId = createLoanProduct(
+
template.monthEndDueDateStrategy(PostLoanProductsRequest.MonthEndDueDateStrategyEnum.FIRST_DAY_OF_NEXT_MONTH));
+ assertEquals(FIRST_DAY_OF_NEXT_MONTH,
strategyOf(retrieveLoanProduct(productId)), template.getLoanScheduleType());
+
+ updateLoanProduct(productId, new PutLoanProductsProductIdRequest()
+
.monthEndDueDateStrategy(PutLoanProductsProductIdRequest.MonthEndDueDateStrategyEnum.LAST_DAY_OF_MONTH));
+ assertEquals(LAST_DAY_OF_MONTH,
strategyOf(retrieveLoanProduct(productId)), template.getLoanScheduleType());
+ }
+
+ final List<StringEnumOptionData> options =
getLoanProductTemplate(false).getMonthEndDueDateStrategyOptions();
+ assertNotNull(options, "The template offers the strategies");
+ assertEquals(List.of(LAST_DAY_OF_MONTH, FIRST_DAY_OF_NEXT_MONTH),
options.stream().map(StringEnumOptionData::getId).toList());
+ }
+
+ @Test
+ public void testProductStrategyCanBeClearedAndRejectsUnknownValues() {
+ final Long productId = createLoanProduct(
+
create4IProgressive().monthEndDueDateStrategy(PostLoanProductsRequest.MonthEndDueDateStrategyEnum.FIRST_DAY_OF_NEXT_MONTH));
+
+ final RuntimeException exception = assertThrows(RuntimeException.class,
+ () -> FeignRawHttpHelper.put("/loanproducts/" + productId, """
+ {"monthEndDueDateStrategy": "SOMEWHERE_IN_BETWEEN",
"locale": "en"}
+ """));
+ assertTrue(exception.getMessage().startsWith("HTTP 400"),
exception.getMessage());
+ assertEquals(FIRST_DAY_OF_NEXT_MONTH,
strategyOf(retrieveLoanProduct(productId)), "A rejected update changes
nothing");
+
+ FeignRawHttpHelper.put("/loanproducts/" + productId, """
+ {"monthEndDueDateStrategy": null, "locale": "en"}
+ """);
+
assertNull(retrieveLoanProduct(productId).getMonthEndDueDateStrategy(),
"Sending null clears the strategy");
+ }
+
+ @Test
+ public void testLoanInheritsTheProductStrategy() {
+ final Long clientId = createClient();
+ runAt("31 January 2025", () -> {
+ final Long progressiveLoanId = progressiveLoan(clientId,
product(create4IProgressive(), FIRST_DAY_OF_NEXT_MONTH),
+ "31 January 2025", 3);
+ final Long cumulativeLoanId = cumulativeLoan(clientId,
product(create4ICumulative(), FIRST_DAY_OF_NEXT_MONTH),
+ "31 January 2025", 3);
+ final Long unsetLoanId = progressiveLoan(clientId,
product(create4IProgressive(), null), "31 January 2025", 3);
+
+ assertEquals(FIRST_DAY_OF_NEXT_MONTH,
getLoanDetails(progressiveLoanId).getMonthEndDueDateStrategy().getId());
+ assertEquals(FIRST_DAY_OF_NEXT_MONTH,
getLoanDetails(cumulativeLoanId).getMonthEndDueDateStrategy().getId());
+
assertNull(getLoanDetails(unsetLoanId).getMonthEndDueDateStrategy());
+ });
+ }
+
+ /**
+ * The ticket's worked example: disbursed 31 January 2025, three monthly
periods, Actual/Actual. February has no
+ * 31st and neither has April; March does. Interest is 12% a year on the
declining balance over the actual days of
+ * each period - 29, 30 and 31 days when rolled against 28, 31 and 30 when
clamped - so the rolled schedule bills
+ * 95.34 = 10,000.00 * 12% * 29/365 in its first period against 92.05 =
10,000.00 * 12% * 28/365, and pays a
+ * slightly higher EMI because its money is out for a day longer before
the first repayment.
+ */
+ @Test
+ public void testWorkedExampleOnProgressiveLoan() {
+ final Long clientId = createClient();
+ runAt("31 January 2025", () -> {
+ final ScheduleType progressive =
progressive(MonthEndDueDateStrategyTest::actualActual);
+ verifyRepaymentSchedule(disbursedLoan(clientId, progressive,
LAST_DAY_OF_MONTH, "31 January 2025", 3), //
+ installment(PRINCIPAL, null, "31 January 2025"), //
+ installment(3305.77, 92.05, 3397.82, false, "28 February
2025"), //
+ installment(3329.59, 68.23, 3397.82, false, "31 March
2025"), //
+ installment(3364.64, 33.19, 3397.83, false, "30 April
2025"));
+ verifyRepaymentSchedule(disbursedLoan(clientId, progressive,
FIRST_DAY_OF_NEXT_MONTH, "31 January 2025", 3), //
+ installment(PRINCIPAL, null, "31 January 2025"), //
+ installment(3303.22, 95.34, 3398.56, false, "01 March
2025"), //
+ installment(3332.51, 66.05, 3398.56, false, "31 March
2025"), //
+ installment(3364.27, 34.29, 3398.56, false, "01 May
2025"));
+ });
+ }
+
+ /**
+ * The worked example on a cumulative product with no meeting calendar
lands on the same dates. The cumulative EMI
+ * is worked out from the nominal monthly rate, so it does not move; the
interest split does, following the actual
+ * days of each period, and the last instalment absorbs the difference.
+ */
+ @Test
+ public void testWorkedExampleOnCumulativeLoan() {
+ withInterestChargedFromDisbursementDate(() -> {
+ final Long clientId = createClient();
+ runAt("31 January 2025", () -> {
+ final ScheduleType cumulative =
cumulative(MonthEndDueDateStrategyTest::actualActual);
+ verifyRepaymentSchedule(disbursedLoan(clientId, cumulative,
LAST_DAY_OF_MONTH, "31 January 2025", 3), //
+ installment(PRINCIPAL, null, "31 January 2025"), //
+ installment(3307.25, 92.05, 3399.30, false, "28
February 2025"), //
+ installment(3331.09, 68.21, 3399.30, false, "31 March
2025"), //
+ installment(3361.66, 33.16, 3394.82, false, "30 April
2025"));
+ verifyRepaymentSchedule(disbursedLoan(clientId, cumulative,
FIRST_DAY_OF_NEXT_MONTH, "31 January 2025", 3), //
+ installment(PRINCIPAL, null, "31 January 2025"), //
+ installment(3303.96, 95.34, 3399.30, false, "01 March
2025"), //
+ installment(3333.26, 66.04, 3399.30, false, "31 March
2025"), //
+ installment(3362.78, 34.27, 3397.05, false, "01 May
2025"));
+ });
+ });
+ }
+
+ /**
+ * Under Actual/Actual interest follows the actual days. Disbursed 30
January 2025: the rolled first period (30
+ * January - 1 March, 30 days) is a day longer than the clamped one (30
January - 28 February, 29 days), and the
+ * second (1 March - 30 March, 29 days) a day shorter than its clamped
counterpart (28 February - 30 March, 30
+ * days).
+ */
+ @Test
+ public void
testRolledDueDateMovesInterestBetweenPeriodsUnderActualActual() {
+ withInterestChargedFromDisbursementDate(() -> {
+ final Long clientId = createClient();
+ runAt("30 January 2025", () -> {
+ for (final ScheduleType type :
scheduleTypes(MonthEndDueDateStrategyTest::actualActual)) {
+ final Long clampedLoanId = disbursedLoan(clientId, type,
LAST_DAY_OF_MONTH, "30 January 2025", 3);
+ final Long rolledLoanId = disbursedLoan(clientId, type,
FIRST_DAY_OF_NEXT_MONTH, "30 January 2025", 3);
+
+ assertEquals(List.of(LocalDate.of(2025, 2, 28),
LocalDate.of(2025, 3, 30), LocalDate.of(2025, 4, 30)),
+ dueDates(clampedLoanId), type.name());
+ assertEquals(List.of(LocalDate.of(2025, 3, 1),
LocalDate.of(2025, 3, 30), LocalDate.of(2025, 4, 30)),
+ dueDates(rolledLoanId), type.name());
+
+ final List<BigDecimal> clamped = interests(clampedLoanId);
+ final List<BigDecimal> rolled = interests(rolledLoanId);
+ assertTrue(rolled.get(0).compareTo(clamped.get(0)) > 0,
Review Comment:
Nit: this is the business use case (disbursed 30 Jan, Actual/Actual), but
the test only asserts which way the interest moves. Could you also pin the
exact amounts? For example, period 1 should be 10,000 × 12% × 30/365 =
**98.63** when rolled (30 Jan → 1 Mar), against 95.34 when clamped (29 days),
and period 2 should be billed on 29 days instead of 30. Then a later change to
the day count can't slip through while the direction still holds.
--
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]
