You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/07/17 20:06:22 UTC
[GitHub] [fineract] francisguchie opened a new pull request #1186: FINERACT-1052 global config for Loan OverPayment
francisguchie opened a new pull request #1186:
URL: https://github.com/apache/fineract/pull/1186
## Description
https://issues.apache.org/jira/browse/FINERACT-1052
## 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
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] github-actions[bot] closed pull request #1186: FINERACT-1052 global config for Loan OverPayment
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #1186:
URL: https://github.com/apache/fineract/pull/1186
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola edited a comment on pull request #1186: FINERACT-1052 global config for Loan OverPayment
Posted by GitBox <gi...@apache.org>.
ptuomola edited a comment on pull request #1186:
URL: https://github.com/apache/fineract/pull/1186#issuecomment-663814472
Hi
My suggestion on this would be:
- 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)
- 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
That way we a) preserve the current functionality for those who are relying on it, b) preserve the tests that test the current functionality, and c) add the ability to disable the overpayments and the relevant tests for this
Personally I'm a bit surprised that we want to do this as a global configuration - wouldn't it make more sense to configure this per-product? But the original JIRA / requirements docs are very focused on making this a global setting...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] francisguchie commented on pull request #1186: FINERACT-1052 global config for Loan OverPayment
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1186:
URL: https://github.com/apache/fineract/pull/1186#issuecomment-662707400
> Alternatively, should (will YOU) change the test, as part of this PR?
Yes @vorburger i have to do changes on the test to cater for the "disabled overpayment"
@ptuomola it would be nice to get some help on this when you have time
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1186: FINERACT-1052 global config for Loan OverPayment
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1186:
URL: https://github.com/apache/fineract/pull/1186#issuecomment-663814472
Hi
My suggestion on this would be:
- 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 add the flag as true
- The existing integration tests should be kept-as is and they should test the current behaviour
- 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
That way we a) preserve the current functionality for those who are relying on it, b) preserve the tests that test the current functionality, and c) add the ability to disable the overpayments and the relevant tests for this
Personally I'm a bit surprised that we want to do this as a global configuration - wouldn't it make more sense to configure this per-product? But the original JIRA / requirements docs are very focused on making this a global setting...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] github-actions[bot] commented on pull request #1186: FINERACT-1052 global config for Loan OverPayment
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1186:
URL: https://github.com/apache/fineract/pull/1186#issuecomment-693738249
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] edcable commented on pull request #1186: FINERACT-1052 global config for Loan OverPayment
Posted by GitBox <gi...@apache.org>.
edcable commented on pull request #1186:
URL: https://github.com/apache/fineract/pull/1186#issuecomment-673643112
@bharathc27 could you do a functional review on this in line with comments from @ptuomola
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1186: FINERACT-1052 global config for Loan OverPayment
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1186:
URL: https://github.com/apache/fineract/pull/1186#issuecomment-662702555
> if over_payment is disabled, then this test should be skipped please advise
@francisguchie it's your responsibility to make this PR pass the build.
I don't understand enough about the functionality here, but are we changing the default here? Should the SQL script insert TRUE instead of FALSE to preserve current behaviour, so that this becomes an explicit "opt-in"? Alternatively, should (will YOU) change the test, as part of this PR?
@ptuomola don't know if you have any interest to engage on this one, some time (no rush).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] francisguchie commented on pull request #1186: FINERACT-1052 global config for Loan OverPayment
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1186:
URL: https://github.com/apache/fineract/pull/1186#issuecomment-660547728
@nikpawar and @awasum
this PR is failing at line 712
org.apache.fineract.integrationtests.AccountingScenarioIntegrationTest.checkPeriodicAccrualAccountingFlow_OVER_PAYMENT(AccountingScenarioIntegrationTest.java:712)
while testing
AccountingScenarioIntegrationTest > checkPeriodicAccrualAccountingFlow_OVER_PAYMENT()
for details look at lines 467 to 511 of the test at https://travis-ci.org/github/apache/fineract/builds/709311599
if over_payment is disabled, then this test should be skipped please advise
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org