FINERACT-2065: Schedule handling for fixed length configuration #3808

alberto-art3ch · 2024-03-19T06:26:40Z

Description

Following with the Loan Product Fixed Length configuration we need to consider the schedule generation with this new setting.

We will get this with the next considerations:

Should be supported for zero interest bearing loan accounts only
Should be available for “advance payment allocation“
Validation should be in place when the fixed length cannot derive the no of repayments entered - system should not approve such config and we are creating a new BadFixedLengthException

FINERACT-2065.

Fixed_Length_Loan_Account.mov

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

reluxa

Could you please add some unit / integration tests?

adamsaghy · 2024-03-20T09:35:07Z

...ract/portfolio/loanaccount/loanschedule/domain/AbstractProgressiveLoanScheduleGenerator.java

@@ -58,6 +61,7 @@ public LoanScheduleModel generate(final MathContext mc, final LoanApplicationTer
        final ApplicationCurrency applicationCurrency = loanApplicationTerms.getApplicationCurrency();
        // generate list of proposed schedule due dates
        LocalDate loanEndDate = getScheduledDateGenerator().getLastRepaymentDate(loanApplicationTerms, holidayDetailDTO);
+        log.info("loanEndDate {}", loanEndDate);


Please remove this info logger...

Done, Removed!

adamsaghy · 2024-03-20T09:35:10Z

...ract/portfolio/loanaccount/loanschedule/domain/AbstractProgressiveLoanScheduleGenerator.java

@@ -102,6 +106,19 @@ public LoanScheduleModel generate(final MathContext mc, final LoanApplicationTer
            // Loan Schedule Exceptions that need to be applied for Loan Account
            LoanTermVariationParams termVariationParams = applyLoanTermVariations(loanApplicationTerms, scheduleParams, scheduledDueDate);
            scheduledDueDate = termVariationParams.scheduledDueDate();
+            log.info("scheduledDueDate {}", scheduledDueDate);


Please remove this info logger...

Done, Removed!

adamsaghy · 2024-03-20T09:35:16Z

...ract/portfolio/loanaccount/loanschedule/domain/AbstractProgressiveLoanScheduleGenerator.java

+
+                isNextRepaymentAvailable = DateUtils.isBefore(scheduledDueDate, loanEndDate);
+            }
+            log.info("scheduledDueDate {}", scheduledDueDate);


Please remove this info logger...

Done, Removed!

Removed from this method and class

adamsaghy · 2024-03-20T09:38:22Z

...ract/portfolio/loanaccount/loanschedule/domain/AbstractProgressiveLoanScheduleGenerator.java

+
+                // Fixed Length Validations - If the Number or Repayments requested is greater than current/last
+                // Installment number
+                if (loanApplicationTerms.getFixedLength() != null


This should be validated in the validator, not during the schedule generation.

We have talked about this, that validation will be keep it here

Removed from this method and class

adamsaghy · 2024-03-20T09:39:11Z

...ract/portfolio/loanaccount/loanschedule/domain/AbstractProgressiveLoanScheduleGenerator.java

+            log.info("scheduledDueDate {}", scheduledDueDate);
+
+            // Fixed Length Validations - Ensure the last repayment date equal to Fixed Length
+            if (loanApplicationTerms.getFixedLength() != null) {


I dont fully understand this here...

We have talked about this in the session

adamsaghy · 2024-03-20T09:40:24Z

...apache/fineract/portfolio/loanaccount/loanschedule/domain/DefaultScheduledDateGenerator.java

        }
        lastRepaymentDate = adjustRepaymentDate(lastRepaymentDate, loanApplicationTerms, holidayDetailDTO).getChangedScheduleDate();
+        // Fixed Length validation - When the Number of Repayments is lower than Fixed Length
+        if (maxDateForFixedLength != null && DateUtils.isAfter(maxDateForFixedLength, lastRepaymentDate)) {


Why do we do it twice?

We have talked about this in the session

See https://github.com/apache/fineract/pull/3808/files#r1532214898

adamsaghy · 2024-03-20T09:40:34Z

...apache/fineract/portfolio/loanaccount/loanschedule/domain/DefaultScheduledDateGenerator.java

+        if (maxDateForFixedLength != null && DateUtils.isAfter(maxDateForFixedLength, lastRepaymentDate)) {
+            lastRepaymentDate = maxDateForFixedLength;
+        }
+        log.info("lastRepaymentDate {}", lastRepaymentDate);


Please remove the info logger

Done, Removed!

adamsaghy · 2024-03-20T09:41:26Z

...ache/fineract/portfolio/loanaccount/loanschedule/exception/BadFixedLengthSetupException.java

+
+import org.apache.fineract.infrastructure.core.exception.AbstractPlatformDomainRuleException;
+
+public class BadFixedLengthSetupException extends AbstractPlatformDomainRuleException {


We dont need a new exception. Please use the GenericPlatformDomainRuleException

Done! removed

adamsaghy · 2024-03-20T09:42:18Z

...org/apache/fineract/integrationtests/AdvancedPaymentAllocationLoanRepaymentScheduleTest.java

+            validateRepaymentPeriod(loanDetails, 2, LocalDate.of(2023, 12, 22), 33.0, 0.0, 33.0, 0.0, 0.0);
+            validateRepaymentPeriod(loanDetails, 3, LocalDate.of(2024, 1, 01), 34.0, 0.0, 34.0, 0.0, 0.0);
+            assertEquals(loanDetails.getNumberOfRepayments(), 3);
+            assertEquals(Utils.getDifferenceInDays(loanDetails.getTimeline().getActualDisbursementDate(), LocalDate.of(2024, 1, 01)),


You should test the due date of the very last installment instead of hard coding a date...

Done, validations updated with the due date of the last period

adamsaghy · 2024-03-20T09:43:04Z

...org/apache/fineract/integrationtests/AdvancedPaymentAllocationLoanRepaymentScheduleTest.java

+            validateRepaymentPeriod(loanDetails, 2, LocalDate.of(2023, 12, 20), 33.0, 0.0, 33.0, 0.0, 0.0);
+            validateRepaymentPeriod(loanDetails, 3, LocalDate.of(2023, 12, 27), 34.0, 0.0, 34.0, 0.0, 0.0);
+            assertEquals(loanDetails.getNumberOfRepayments(), 3);
+            assertEquals(Utils.getDifferenceInWeeks(loanDetails.getTimeline().getActualDisbursementDate(), LocalDate.of(2023, 12, 27)),


Same as above

Done, validations updated with the due date of the last period

adamsaghy · 2024-03-20T09:43:29Z

...org/apache/fineract/integrationtests/AdvancedPaymentAllocationLoanRepaymentScheduleTest.java

+        });
+    }
+
+    // UC127: Advanced payment allocation with Fixed Length for 11 months and Loan Term for 12 months (6 repayments


Please add a test case when the fixed lenght is longer than the loan term.

TC uc128 created

adamsaghy · 2024-03-20T09:44:10Z

...org/apache/fineract/integrationtests/AdvancedPaymentAllocationLoanRepaymentScheduleTest.java

+            loanTransactionHelper.disburseLoan(loanResponse.getLoanId(),
+                    new PostLoansLoanIdRequest().actualDisbursementDate("22 November 2023").dateFormat(DATETIME_PATTERN)
+                            .transactionAmount(BigDecimal.valueOf(100.0)).locale("en"));
+


Please add test cases where you are testing negative scenarios:

Providing invalid fixed length parameters

adamsaghy

Please kindly check my comments!

adamsaghy · 2024-03-20T14:44:06Z

...apache/fineract/portfolio/loanaccount/loanschedule/domain/DefaultScheduledDateGenerator.java

        boolean isFirstRepayment = true;
        for (int repaymentPeriod = 1; repaymentPeriod <= numberOfRepayments; repaymentPeriod++) {
            lastRepaymentDate = generateNextRepaymentDate(lastRepaymentDate, loanApplicationTerms, isFirstRepayment);
            isFirstRepayment = false;
+            // Fixed Length validation - When the Number of Repayments is greater than Fixed Length


Please correct these comments. This has nothing to do with the number of repayments... it checks whether the last repaymentDate is greater than the fixedLengthDate...

adamsaghy · 2024-03-20T14:44:20Z

...apache/fineract/portfolio/loanaccount/loanschedule/domain/DefaultScheduledDateGenerator.java

+                break;
+            }
+        }
+        // Fixed Length validation - When the Number of Repayments is lower than Fixed Length


Please correct these comments. This has nothing to do with the number of repayments... it checks whether the last repaymentDate is lower than the fixedLengthDate...

adamsaghy · 2024-03-20T14:46:25Z

...apache/fineract/portfolio/loanaccount/loanschedule/domain/DefaultScheduledDateGenerator.java

+            // Fixed Length validation - When the Number of Repayments is greater than Fixed Length
+            if (maxDateForFixedLength != null && DateUtils.isAfter(lastRepaymentDate, maxDateForFixedLength)) {
+                lastRepaymentDate = maxDateForFixedLength;
+                break;


Would you mind moving this condition out of the cycle, it does not require do to anything there. After the cycle finished you will have the lastRepaymentDate set, which is the due date of the last installment and after you can decide whether it needs to be changed or not:
if lastRepaymentDate is not equal with the fixedLengthDate, override it with the fixedLengthDate.

...apache/fineract/portfolio/loanaccount/loanschedule/domain/DefaultScheduledDateGenerator.java

adamsaghy · 2024-03-22T09:47:49Z

...apache/fineract/portfolio/loanaccount/loanschedule/domain/DefaultScheduledDateGenerator.java

-            lastRepaymentDate = generateNextRepaymentDate(lastRepaymentDate, loanApplicationTerms, isFirstRepayment);
-            isFirstRepayment = false;
+        LocalDate lastRepaymentDate = loanApplicationTerms.getRepaymentStartDate();
+        // When exists defined a Fixed Length value


We dont really need this..

Please revert and just adjust the lastRepaymentDate after it got calculated (the after the for cycle) in the following manner:
if the lastRepaymentDate is not equals with calculatedMaxDateForFixedLenght than override the lastRepaymentDate with it.

Done! updated

adamsaghy

Kindly check my review and please add the requested negative test scenarios!

alberto-art3ch · 2024-03-25T13:05:43Z

Kindly check my review and please add the requested negative test scenarios!

The last change was already done and the negative case is the uc128

adamsaghy · 2024-03-25T14:21:02Z

...org/apache/fineract/integrationtests/AdvancedPaymentAllocationLoanRepaymentScheduleTest.java

+                    .numberOfRepayments(6).repaymentEvery(1).repaymentFrequencyType(repaymentFrequencyType.longValue()).fixedLength(6);
+            PostLoanProductsResponse loanProductResponse = loanProductHelper.createLoanProduct(product);
+
+            loanTransactionHelper.applyLoanWithError(new PostLoansRequest().clientId(client.getClientId())


You should check whether the provided exception and message is correct or not.

adamsaghy

Please see my comments and kindly fix the conflicts!

adamsaghy · 2024-03-26T09:00:51Z

@alberto-art3ch Please fix the code format error!

adamsaghy

LGTM

adamsaghy · 2024-03-26T17:11:51Z

...n/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java

+                            interestRatePerPeriod);
+                }
+
+                if (fixedLength != null && numberOfRepayments.compareTo(fixedLength) > 0) {


This is incorrect...
Example which should work:

number of repayments: 3

Repay every: 15 DAYS

Loan term: 45 DAYS

Fixed term length: 40 days

adamsaghy

https://github.com/apache/fineract/pull/3808/files#r1539759545

adamsaghy

LGTM

reluxa reviewed Mar 19, 2024

View reviewed changes

alberto-art3ch force-pushed the enhancement/schedule_handling_for_fixed_length_configuration branch from f0f79ad to 15f0e4d Compare March 20, 2024 08:34

adamsaghy reviewed Mar 20, 2024

View reviewed changes

adamsaghy requested changes Mar 20, 2024

View reviewed changes

alberto-art3ch force-pushed the enhancement/schedule_handling_for_fixed_length_configuration branch 2 times, most recently from b956079 to 4bd1621 Compare March 20, 2024 14:32

adamsaghy reviewed Mar 20, 2024

View reviewed changes

alberto-art3ch force-pushed the enhancement/schedule_handling_for_fixed_length_configuration branch from 4bd1621 to 88e5bd8 Compare March 22, 2024 06:27

adamsaghy reviewed Mar 22, 2024

View reviewed changes

...apache/fineract/portfolio/loanaccount/loanschedule/domain/DefaultScheduledDateGenerator.java Show resolved Hide resolved

adamsaghy reviewed Mar 22, 2024

View reviewed changes

adamsaghy requested changes Mar 22, 2024

View reviewed changes

alberto-art3ch force-pushed the enhancement/schedule_handling_for_fixed_length_configuration branch from 88e5bd8 to c0b758f Compare March 25, 2024 06:25

adamsaghy reviewed Mar 25, 2024

View reviewed changes

adamsaghy requested changes Mar 25, 2024

View reviewed changes

alberto-art3ch force-pushed the enhancement/schedule_handling_for_fixed_length_configuration branch 2 times, most recently from ec1cf2a to c2894da Compare March 25, 2024 19:36

alberto-art3ch force-pushed the enhancement/schedule_handling_for_fixed_length_configuration branch from c2894da to b416bfc Compare March 26, 2024 09:12

adamsaghy approved these changes Mar 26, 2024

View reviewed changes

alberto-art3ch force-pushed the enhancement/schedule_handling_for_fixed_length_configuration branch from b416bfc to a38273b Compare March 26, 2024 14:31

adamsaghy reviewed Mar 26, 2024

View reviewed changes

adamsaghy requested changes Mar 26, 2024

View reviewed changes

alberto-art3ch force-pushed the enhancement/schedule_handling_for_fixed_length_configuration branch from a38273b to 23627a4 Compare March 26, 2024 20:33

FINERACT-2065: Schedule handling for fixed length configuration

160c5a2

alberto-art3ch force-pushed the enhancement/schedule_handling_for_fixed_length_configuration branch from 23627a4 to 160c5a2 Compare March 27, 2024 03:28

adamsaghy approved these changes Mar 27, 2024

View reviewed changes

adamsaghy merged commit b64b4b7 into apache:develop Mar 27, 2024
9 checks passed


		import org.apache.fineract.infrastructure.core.exception.AbstractPlatformDomainRuleException;

		public class BadFixedLengthSetupException extends AbstractPlatformDomainRuleException {

FINERACT-2065: Schedule handling for fixed length configuration #3808

FINERACT-2065: Schedule handling for fixed length configuration #3808

Conversation

alberto-art3ch commented Mar 19, 2024

Description

Checklist

reluxa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsaghy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsaghy left a comment

Choose a reason for hiding this comment

alberto-art3ch commented Mar 25, 2024

Choose a reason for hiding this comment

adamsaghy left a comment

Choose a reason for hiding this comment

adamsaghy commented Mar 26, 2024

adamsaghy left a comment

Choose a reason for hiding this comment

adamsaghy Mar 26, 2024 • edited

Choose a reason for hiding this comment

adamsaghy left a comment

Choose a reason for hiding this comment

adamsaghy left a comment

Choose a reason for hiding this comment

adamsaghy Mar 26, 2024 •

edited