Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FINERACT-1981: Advanced payment allocation validation between Loan pr… #3663

Conversation

josehernandezfintecheandomx
Copy link
Contributor

@josehernandezfintecheandomx josehernandezfintecheandomx commented Jan 3, 2024

…oduct and Loan account

Description

Repayment strategy validation applied in the Loan account creation and modification, specifically when the Loan product uses Loan Schedule Type as PROGRESSIVE

Screenshot 2024-01-03 at 4 15 01 p m

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.

@@ -1705,4 +1709,20 @@ private void officeSpecificLoanProductValidation(final Long productId, final Lon
}
}

private void validateTransactionProcessingStrategy(final LoanProduct loanProduct, final String loanRepaymentStrategy) {
if (loanProduct.getRepaymentStrategy().equals(LoanProductConstants.ADVANCED_PAYMENT_ALLOCATION_STRATEGY)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is not necessary true.

Just because the loan product was configured to advanced payment allocation, it does not mean the loan must be as well.

The rule is:

  • If the loan product WAS NOT configured for advanced payment allocation, the loan CANNOT be 'advanced payment allocation`
  • If the loan product WAS configured for advanced payment allocation, the loan CAN be advanced payment allocation OR ANYTHING ELSE, but if something else is selected the loan schedule type MUST be Cumulative.
  • When the loan schedule type is CUMULATIVE, then Loan and loan product repayment strategy CANNOT be "advanced payment allocation".
  • When the loan schedule type is PROGRESSIVE, then the loan and loan product repayment strategy MUST be only "advanced payment allocation".

I hope it helps!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, code updated as:

        // PROGRESSIVE: Repayment strategy MUST be only "advanced payment allocation"
        if (loanProduct.getLoanProductRelatedDetail().getLoanScheduleType().equals(LoanScheduleType.PROGRESSIVE)) {
            if (!transactionProcessingStrategyCode.equals(LoanProductConstants.ADVANCED_PAYMENT_ALLOCATION_STRATEGY)) {
                throw new GeneralPlatformDomainRuleException(
                        "error.msg.loan.repayment.strategy.can.not.be.different.than.advanced.payment.allocation",
                        "Loan repayment strategy can not be different than Advanced Payment Allocation");
            }
            // CUMULATIVE: Repayment strategy CANNOT be "advanced payment allocation"
        } else {
            if (transactionProcessingStrategyCode.equals(LoanProductConstants.ADVANCED_PAYMENT_ALLOCATION_STRATEGY)) {
                throw new GeneralPlatformDomainRuleException(
                        "error.msg.loan.repayment.strategy.can.not.be.equal.to.advanced.payment.allocation",
                        "Loan repayment strategy can not be equal to Advanced Payment Allocation");
            }
        }

Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment!

@josehernandezfintecheandomx josehernandezfintecheandomx force-pushed the fix/repayment_strategy_loan_product_loan_account branch from edb78d5 to 2c4b543 Compare January 3, 2024 22:15
@josehernandezfintecheandomx
Copy link
Contributor Author

Please see my comment!

Done! Code updated and conflicts solved with the latest changes

@josehernandezfintecheandomx josehernandezfintecheandomx force-pushed the fix/repayment_strategy_loan_product_loan_account branch from 2c4b543 to 6dbf461 Compare January 8, 2024 18:02
Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@josehernandezfintecheandomx josehernandezfintecheandomx force-pushed the fix/repayment_strategy_loan_product_loan_account branch 2 times, most recently from 97b7bdb to 2175d85 Compare January 9, 2024 07:27
break;
}
}
if (!existFinancialActivity) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josehernandezfintecheandomx hm?

@adamsaghy That IT was failing when there is already Financial Activity defined but no one of these is for Activity 200, so If the list has values but none one is for Activity 200 then It is created and the boolean flag is for controlling this

@josehernandezfintecheandomx josehernandezfintecheandomx force-pushed the fix/repayment_strategy_loan_product_loan_account branch from 2175d85 to 46b5d62 Compare January 11, 2024 18:41
@adamsaghy
Copy link
Contributor

@josehernandezfintecheandomx Please dont forget to squash your commits! ;)

@josehernandezfintecheandomx josehernandezfintecheandomx force-pushed the fix/repayment_strategy_loan_product_loan_account branch from 46b5d62 to 2adbf2b Compare January 11, 2024 21:02
@adamsaghy adamsaghy merged commit e3a771a into apache:develop Jan 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants