FINERACT-2102: Loan Product enableDownPayment overrided in the Loan a… #3952

alberto-art3ch · 2024-06-27T05:27:25Z

…pplication

Description

There is need to decide dynamically the loan is downpayment loan or not under the same product constraints. The change here to allow override previously configured enableDownPayment field

FINERACT-2102

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.

adamsaghy · 2024-06-27T09:50:46Z

...-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanAssembler.java

@@ -835,6 +835,12 @@ public Map<String, Object> updateFrom(JsonCommand command, Loan loan) {
            loan.updateLoanRates(rateAssembler.fromParsedJson(command.parsedJson()));
        }

+        if (command.hasParameter(LoanProductConstants.ENABLE_DOWN_PAYMENT)) {


Please follow the same logic as it was done when loan was submitted:

You can disable down payment on loan, when the it was allowed on the loan product, but not the otherwise! It should fail with validation when not allowed action was tried:

user tries to enable down payment but it was not configured on loan product!
Ignoring the field silently that was sent is wrong!

Done! new validation method implemented in the LoanScheduleValidator

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

...va/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java

adamsaghy

Please kindly check my review!

adamsaghy · 2024-07-03T07:59:50Z

...va/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java

@@ -466,7 +467,9 @@ private LoanApplicationTerms assembleLoanApplicationTermsFrom(final JsonElement
        final boolean isPrincipalCompoundingDisabledForOverdueLoans = this.configurationDomainService
                .isPrincipalCompoundingDisabledForOverdueLoans();

-        final boolean isDownPaymentEnabled = loanProduct.getLoanProductRelatedDetail().isEnableDownPayment();
+        final boolean isDownPaymentEnabled = validateDownPaymentAttribute(loanProduct.getLoanProductRelatedDetail().isEnableDownPayment(),


Please move this into the loan validator!

adamsaghy · 2024-07-03T07:59:54Z

...va/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java

@@ -1379,6 +1382,21 @@ public void updateLoanApplicationAttributes(JsonCommand command, Loan loan, Map<
            changes.put(LoanProductConstants.IS_EQUAL_AMORTIZATION_PARAM, newValue);
            loanProductRelatedDetail.setEqualAmortization(newValue);
        }
+
+        final boolean isDownPaymentEnabled = validateDownPaymentAttribute(


Please move this into the loan validator!

adamsaghy · 2024-07-03T08:02:20Z

...va/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java

+                loan.loanProduct().getLoanProductRelatedDetail().isEnableDownPayment(), command.parsedJson());
+        if (isDownPaymentEnabled != loanProductRelatedDetail.isEnableDownPayment()) {
+            changes.put(LoanProductConstants.ENABLE_DOWN_PAYMENT, isDownPaymentEnabled);
+            loanProductRelatedDetail.setEnableDownPayment(isDownPaymentEnabled);


Its more straightforward to set the values as "false" instead of assigning the value of the isDownPaymentEnabled which cannot be anything else but false.

We need to consider a couple of scenarios:

LP with DownPayment enabled and originally the LA with DownPayment enabled, then LA disable DownPayment, and
2.LP with DownPayment enabled and originally the LA with DownPayment disabled, then LA enable DownPayment

This is why we are validating If we have a change in the new value compared with the old value

You are checking the loan product configuration, but what you should check is only the configuration on loan. This method is covering the update loan application part, we already have something configured on the loan, thats the only thing that should be checked!

If on the loan product it was enabled down payment, but it was disabled when the loan was submitted and now when the loan is got updated, we want to enable again, you should not check what was set on the loan product, but check whether on the loan level what was set and now during the update if we wanna change it and enable again, then we can set from the loan product the down payment configurations on the loan level again.

adamsaghy · 2024-07-03T08:04:42Z

...va/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java

+    private boolean validateDownPaymentAttribute(final boolean isDownPaymentEnabledInLoanProduct, final JsonElement element) {
+        boolean isDownPaymentEnabled = isDownPaymentEnabledInLoanProduct;
+        if (!isDownPaymentEnabledInLoanProduct) {
+            if (this.fromApiJsonHelper.parameterExists(LoanProductConstants.ENABLE_DOWN_PAYMENT, element)) {


This is wrong. We should only throw exception if the the down payment is disabled on the loan product (which is correctly checked) and when user tries to set down payment to be enabled during loan submission (this is the wrongly checked part)! If user provide enableDownPayment=false then nothing should happen: No violation!

Done! Logic updated

adamsaghy · 2024-07-03T08:04:53Z

...va/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java

@@ -1446,4 +1464,21 @@ public Pair<Loan, Map<String, Object>> assembleLoanApproval(AppUser currentUser,

        return Pair.of(loan, actualChanges);
    }
+
+    private boolean validateDownPaymentAttribute(final boolean isDownPaymentEnabledInLoanProduct, final JsonElement element) {


Please move the validations into the Validator class!

Done! moved to the validator

adamsaghy

Please kindly see my review!

adamsaghy · 2024-07-04T07:56:53Z

fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/data/LoanSummaryData.java

@@ -91,9 +91,14 @@ public class LoanSummaryData {
    private BigDecimal totalCreditBalanceRefundReversed;
    private BigDecimal totalRepaymentTransaction;
    private BigDecimal totalRepaymentTransactionReversed;
+    private BigDecimal totalInterestRefund;


totalInterestRefund is not yet calculated. Please remove till it got implemented fully.

Done! Removed

adamsaghy · 2024-07-04T07:57:35Z

fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/data/LoanSummaryData.java

    private final Long chargeOffReasonId;
    private final String chargeOffReason;

+    private BigDecimal totalUnpaidAccruedDueInterest;


totalUnpaidAccruedDueInterest and totalUnpaidAccruedNotDueInterest are not set with value. Please compute their value or remove them till it got implemented fully.

Done! Removed

adamsaghy · 2024-07-04T07:58:02Z

...che/fineract/portfolio/loanaccount/domain/transactionprocessor/impl/ChargeOrTransaction.java

@@ -58,7 +58,7 @@ private LocalDate getEffectiveDate() {
    }

    private boolean isBackdatedCharge() {
-        return loanCharge.get().getDueDate().isBefore(loanCharge.get().getSubmittedOnDate());
+        return (loanCharge.get().getDueDate() != null && loanCharge.get().getDueDate().isBefore(loanCharge.get().getSubmittedOnDate()));


This change is unrelated. If this is a fix for something, please raise it in a new PR!

Done! removed, It belongs to an issue when you have a Loan Installment Fee

adamsaghy · 2024-07-04T07:58:13Z

...loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanTransactionType.java

@@ -63,8 +63,7 @@ public enum LoanTransactionType {
    DOWN_PAYMENT(28, "loanTransactionType.downPayment"), //
    REAGE(29, "loanTransactionType.reAge"), //
    REAMORTIZE(30, "loanTransactionType.reAmortize"), //
-    INTEREST_PAYMENT_WAIVER(31, "loanTransactionType.interestPaymentWaiver"), //
-    ;
+    INTEREST_PAYMENT_WAIVER(31, "loanTransactionType.interestPaymentWaiver"); //


This is unrelated change. Please remove it

Done! Removed

adamsaghy · 2024-07-04T07:58:44Z

...der/src/main/java/org/apache/fineract/portfolio/loanaccount/api/LoansApiResourceSwagger.java

@@ -636,6 +636,10 @@ private GetLoansLoanIdFeeFrequency() {}
            @Schema(example = "0.000000")
            public Double totalRepaymentTransaction;
            @Schema(example = "0.000000")
+            public Double totalInterestRefund;


totalInterestRefund is not set properly. Please remove till!

Done! Removed

adamsaghy

Seems like you mixed together different PRs! Please remove the unrelated changes!

adamsaghy · 2024-07-04T07:59:52Z

...java/org/apache/fineract/portfolio/delinquency/service/LoanDelinquencyDomainServiceImpl.java

@@ -64,13 +64,22 @@ public CollectionData getOverdueCollectionData(final Loan loan, List<LoanDelinqu
            return CollectionData.template();
        }

+        BigDecimal delinquentPrincipal = BigDecimal.ZERO;


unrelated changes!

adamsaghy · 2024-07-04T08:02:09Z

...va/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java

@@ -466,7 +468,9 @@ private LoanApplicationTerms assembleLoanApplicationTermsFrom(final JsonElement
        final boolean isPrincipalCompoundingDisabledForOverdueLoans = this.configurationDomainService
                .isPrincipalCompoundingDisabledForOverdueLoans();

-        final boolean isDownPaymentEnabled = loanProduct.getLoanProductRelatedDetail().isEnableDownPayment();
+        final boolean isDownPaymentEnabled = loanScheduleValidator


Validations are called when loan got submitted or updated:
LoanApplicationWritePlatformServiceJpaRepositoryImpl.submitApplication
and
LoanApplicationWritePlatformServiceJpaRepositoryImpl.modifyApplication

Please move the validations into the loanApplicationValidator.validateForCreate and loanApplicationValidator.validateForModify

Done! All the validations are in the LoanScheduleValidator class

adamsaghy · 2024-07-04T08:11:57Z

...main/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanScheduleValidator.java

@@ -176,4 +178,24 @@ private void validateRepaymentsStartingFromDateIsAfterDisbursementDate(final Lis
            dataValidationErrors.add(error);
        }
    }
+
+    public boolean validateDownPaymentAttribute(final boolean isDownPaymentEnabledInLoanProduct, final JsonElement element) {
+        boolean isDownPaymentEnabled = isDownPaymentEnabledInLoanProduct;


This method is overcomplicated:

public void validateDownPaymentAttribute(final boolean isDownPaymentEnabledInLoanProduct, final JsonElement element) { Boolean inputIsDownPaymentEnabled = this.fromApiJsonHelper.extractBooleanNamed(LoanProductConstants.ENABLE_DOWN_PAYMENT, element); if(!isDownPaymentEnabledInLoanProduct && Boolean.TRUE.equals(inputIsDownPaymentEnabled)) { throw new GeneralPlatformDomainRuleException("error.msg.downpayment.is.not.enabled.in.loan.product", "The Loan can not override the downpayment flag because in the Loan Product the downpayment is disabled", inputIsDownPaymentEnabled); } }

What do you think?

P.S.: You can consider passing the whole loan product as parameter and the validation method will decide what it is using from. Similarly the whole command passed as 2nd parameter

adamsaghy · 2024-07-04T08:16:23Z

...va/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java

@@ -1379,6 +1383,22 @@ public void updateLoanApplicationAttributes(JsonCommand command, Loan loan, Map<
            changes.put(LoanProductConstants.IS_EQUAL_AMORTIZATION_PARAM, newValue);
            loanProductRelatedDetail.setEqualAmortization(newValue);
        }
+
+        final boolean isDownPaymentEnabled = loanScheduleValidator


We only need the incoming isDownPaymentEnabled and if it is different than the one on the loan actually set then either set the LP config or reset it (based on we enable or disable it).

Done! In the LoanScheduleAssembler is only the input read If exists, else we set the LP value

adamsaghy · 2024-07-04T19:07:08Z

...java/org/apache/fineract/portfolio/delinquency/service/LoanDelinquencyDomainServiceImpl.java

@@ -110,6 +110,7 @@ public CollectionData getOverdueCollectionData(final Loan loan, List<LoanDelinqu
            collectionData.setDelinquentDate(overdueSinceDate);
        }
        collectionData.setDelinquentAmount(outstandingAmount);
+


Unrelated changes, please remove!

adamsaghy · 2024-07-04T19:07:12Z

fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/data/LoanSummaryData.java

@@ -110,6 +111,8 @@ public static LoanSummaryData withTransactionAmountsSummary(final LoanSummaryDat
        BigDecimal totalCreditBalanceRefundReversed = BigDecimal.ZERO;
        BigDecimal totalRepaymentTransaction = BigDecimal.ZERO;
        BigDecimal totalRepaymentTransactionReversed = BigDecimal.ZERO;
+        BigDecimal totalInterestRefund = BigDecimal.ZERO;


Unrelated changes, please remove!

adamsaghy · 2024-07-04T19:07:16Z

fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/data/LoanSummaryData.java

@@ -131,6 +134,8 @@ public static LoanSummaryData withTransactionAmountsSummary(final LoanSummaryDat
                    loanTransactions);
            totalRepaymentTransaction = computeTotalRepaymentTransactionAmount(loanTransactions);
            totalRepaymentTransactionReversed = computeTotalAmountForReversedTransactions(LoanTransactionType.REPAYMENT, loanTransactions);
+            totalInterestPaymentWaiver = computeTotalAmountForNonReversedTransactions(LoanTransactionType.INTEREST_PAYMENT_WAIVER,


Unrelated changes, please remove!

adamsaghy · 2024-07-04T19:07:19Z

fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/data/LoanSummaryData.java

@@ -163,7 +168,7 @@ public static LoanSummaryData withTransactionAmountsSummary(final LoanSummaryDat
                .totalChargeAdjustment(totalChargeAdjustment).totalChargeAdjustmentReversed(totalChargeAdjustmentReversed)
                .totalChargeback(totalChargeback).totalCreditBalanceRefund(totalCreditBalanceRefund)
                .totalCreditBalanceRefundReversed(totalCreditBalanceRefundReversed).totalRepaymentTransaction(totalRepaymentTransaction)
-                .totalRepaymentTransactionReversed(totalRepaymentTransactionReversed).build();


Unrelated changes, please remove!

adamsaghy · 2024-07-04T19:07:25Z

...che/fineract/portfolio/loanaccount/domain/transactionprocessor/impl/ChargeOrTransaction.java

@@ -58,7 +58,7 @@ private LocalDate getEffectiveDate() {
    }

    private boolean isBackdatedCharge() {
-        return loanCharge.get().getDueDate().isBefore(loanCharge.get().getSubmittedOnDate());
+        return (loanCharge.get().getDueDate().isBefore(loanCharge.get().getSubmittedOnDate()));


Unrelated changes, please remove!

adamsaghy · 2024-07-04T19:07:29Z

...der/src/main/java/org/apache/fineract/portfolio/loanaccount/api/LoansApiResourceSwagger.java

@@ -1004,6 +1004,14 @@ private GetLoansLoanIdDelinquencySummary() {}
            public LocalDate delinquentDate;
            @Schema(example = "100.000000")
            public Double delinquentAmount;
+            @Schema(example = "80.000000")


Unrelated changes, please remove!

adamsaghy · 2024-07-04T19:11:33Z

...va/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java

+        if (isDownPaymentEnabled != loanProductRelatedDetail.isEnableDownPayment()) {
+            changes.put(LoanProductConstants.ENABLE_DOWN_PAYMENT, isDownPaymentEnabled);
+            loanProductRelatedDetail.setEnableDownPayment(isDownPaymentEnabled);
+            loanProductRelatedDetail.setEnableAutoRepaymentForDownPayment(isDownPaymentEnabled);


Just because down payment was enabled, it does not mean auto repayment for down payment should be enabled as well. It should be set based on what was configured on the loan product

Done! We are just changing the enableDownPayment attribute

…pplication

adamsaghy

LGTM

adamsaghy reviewed Jun 27, 2024

View reviewed changes

...org/apache/fineract/integrationtests/AdvancedPaymentAllocationLoanRepaymentScheduleTest.java Show resolved Hide resolved

adamsaghy reviewed Jun 27, 2024

View reviewed changes

...va/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java Show resolved Hide resolved

adamsaghy requested changes Jun 27, 2024

View reviewed changes

alberto-art3ch force-pushed the enhancement/downpayment_override_loan_account branch from 65d1860 to f87752c Compare July 3, 2024 02:40

adamsaghy reviewed Jul 3, 2024

View reviewed changes

adamsaghy requested changes Jul 3, 2024

View reviewed changes

alberto-art3ch force-pushed the enhancement/downpayment_override_loan_account branch from f87752c to 5f5585e Compare July 4, 2024 04:31

adamsaghy reviewed Jul 4, 2024

View reviewed changes

adamsaghy requested changes Jul 4, 2024

View reviewed changes

adamsaghy reviewed Jul 4, 2024

View reviewed changes

alberto-art3ch force-pushed the enhancement/downpayment_override_loan_account branch from 5f5585e to 28ff499 Compare July 4, 2024 18:03

adamsaghy reviewed Jul 4, 2024

View reviewed changes

alberto-art3ch force-pushed the enhancement/downpayment_override_loan_account branch from 28ff499 to 86964b7 Compare July 4, 2024 20:39

FINERACT-2102: Loan Product enableDownPayment overrided in the Loan a…

1077fdd

…pplication

alberto-art3ch force-pushed the enhancement/downpayment_override_loan_account branch from 86964b7 to 1077fdd Compare July 5, 2024 15:44

adamsaghy approved these changes Jul 5, 2024

View reviewed changes

adamsaghy merged commit 5093ca7 into apache:develop Jul 8, 2024
9 checks passed

FINERACT-2102: Loan Product enableDownPayment overrided in the Loan a… #3952

FINERACT-2102: Loan Product enableDownPayment overrided in the Loan a… #3952

Conversation

alberto-art3ch commented Jun 27, 2024

Description

Checklist

adamsaghy Jun 27, 2024 • edited Loading

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

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

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

adamsaghy Jul 4, 2024 • edited Loading

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

adamsaghy Jun 27, 2024 •

edited

Loading

adamsaghy Jul 4, 2024 •

edited

Loading