FINERACT-1052 global config loan overpayment leaving situation as is #1264
Conversation
ptuomola
left a comment
ptuomola
left a comment
There was a problem hiding this comment.
Some review comments - please have a look at let me know your thoughts. Also would be good to have some tests for this before merging - maybe you could add a few?
| openapi: 3.0.3 | ||
| info: | ||
| version: release-1.4.0-561-g9bad487-dirty | ||
| title: Apache Fineract |
There was a problem hiding this comment.
We should not check in this file as it is autogenerated
| // customization configure ability to overpay loan | ||
| final Money outstandingBalance = Money.of(loan.getCurrency(), loan.getSummary().getTotalOutstanding()); | ||
| if (outstandingBalance.minus(repaymentAmount).isLessThanZero() && !isLoanOverpaymentEnabled) { | ||
| baseDataValidator.reset().failWithCode("Repayment exceeding outstanding balance"); |
There was a problem hiding this comment.
failWithCode should take as parameter the error code property, not freetext - you can take a look at the other calls to it in this file
| baseDataValidator.reset().failWithCode("Repayment exceeding outstanding balance"); | ||
| if (!dataValidationErrors.isEmpty()) { | ||
| throw new PlatformApiDataValidationException(dataValidationErrors); | ||
| } |
There was a problem hiding this comment.
To be consistent with the other exceptions thrown in this class, there are a couple of other parameters that should be passed to PlatformApiDataValidationException - take a look at the other throws
| final List<ApiParameterError> dataValidationErrors = new ArrayList<>(); | ||
| final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors).resource("loan.transaction"); | ||
| final GlobalConfigurationPropertyData configuration = this.configurationReadPlatformService | ||
| .retrieveGlobalConfiguration("Enable-Loan-Overpayment"); |
There was a problem hiding this comment.
To make the current behaviour default, would it not make sense to have a configuration value that works the other way around - i.e. call it "block-loan-overpayment", and if it is present & enabled then overpayments are blocked.
There was a problem hiding this comment.
thanks @ptuomola i did not think of this suggested approach. let me try just that
|
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days. |
|
@francisguchie will you update this PR based on @ptuomola code review feedback? If you don't, it will auto-close soon. |
Yes @vorburger, will do this weekend |
|
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days. |
|
@awasum, @vorburger , @ptuomola I am going to make sure this will be done asap. |
|
new pull request made |
3 participants
Description
#1186
Addressing the following comments from @ptuomola
Addressing these comments below
The current behaviour should be kept as the default - i.e. without a configuration present in the database, the same behaviour should remain (overpayments allowed)
The default configuration added to this should also keep the current behaviour as-is - i.e. the default Flyway script should create the configuration in such way that it allows overpayments
The existing integration tests should be kept-as is and they should continue to be used to test the current behaviour (i.e. overpayment allowed)
but Not able to answer this
We should create a new integration test (or multiple tests) to test the scenario where overpayments are not allowed. These tests should first set the flag to disable overpayments, then carry out the tests to confirm that overpayments fail in different scenarios, and then reset the flag back to enabling overpayments
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.
Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.
API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm has been updated with details of any API changes.
Integration tests have been created/updated for verifying the changes made.
All Integrations tests are passing with the new commits.
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)
Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide