block-loan-overpayment (FINERACT-1052)

[francisguchie]

FINERACT-1052

Description

#1264

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!

• 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/api-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.

[francisguchie]

@vorburger @rrpawar96 I have done a functional test and this is working fine

[vorburger]

@ptuomola if I understand this one correctly, this is "take II" of #1264, for FINERACT-1052. Would you like to (re)review this?

[ptuomola]

Hmm... not quite sure I understand this. Seems that this code allows repayments to be used to pay written off amounts. Whilst this may be right (though should we not deduct the amount transferred from writtenOffBalance?), why/how is this linked to the functionality to stop overpayments?

In other words, why does turning off overpayments for loans also change how we handle written off balances?

[rrpawar96]

@ptuomola, yes, we are considering the written-off amounts because usually when
we repay the loan, it deducts from the outstanding-balance but,
when a user written-off the loan, the outstanding-balance straight away becomes 0.
because the exact same amount is transferred to the Written-off.
So that's why (when the block-overpayment flag is enabled.) we will be repaying the money until it gets equal to the written-off amounts(or outstanding-balance)
otherwise, if the flag is not enabled(default condition) it will not stop even after the amount gets equal and will continue to get overpaid.

[francisguchie]

@rrpawar96
thanks for sharing these details

[avikganguly01]

@francisguchie @rrpawar96 :
"So that's why (when the block-overpayment flag is enabled.) we will be repaying the money until it gets equal to the written-off amounts(or outstanding-balance)" - Block overpayment flag is more than welcome as overpayment creates some issues. But this doesn't look good to me.

• A written off status loan should have an outstanding balance. Leads to state inconsistencies compared to ledger.
• If you are changing the loan status, you have to reverse any writeoff journal entries, post accruals till date, etc - not sure if this is the route you want to go to.
• If you want to continue doing recovery payments without going into overpayment, is this the right place in code to handle recovery repayments?
• In either case, you have to enhance the test to handle the impact of any change in this code snippet like explicitly state writeoff balance, verify if it's recovery repayment or writeoff reversal.
• Mayble also include the logic in test for the dry run you are doing. Ex:- Writeoff balance 1000, Total Recovery Repaid - 800, Repayment - 100. This IF condition is satisfied. But why should outstandingBalance become 1000 and not 100? Please correct me if I am wrong regarding assuming recovery repayments don't reduce writeoff balance.

[francisguchie]

@avikganguly01 - I hope i understand your comments well please see my comments below for every bullet

• A written off status loan should have an outstanding balance. Leads to state inconsistencies compared to ledger.
o When a loan is written-off, on the face value (loan summary you have no balance seen, however, the loan still maintains a balance (in the background) that is why one can still make a payment against it which payment is sent to incomes for that particular period. As-is, Over-payment is allowed, thus fineract does not consider the loan balance (written-off amount) to restrict the user. So for fineract to restrict over-payment, we need to supply the written-off amount to it. So that it can alert user when they hit that value.
o What we are doing here does not in any-way affect the Ledger balances or how the payments are treated

• If you are changing the loan status, you have to reverse any write-off journal entries, post accruals till date, etc. - not sure if this is the route you want to go to.
o We are not in anyway changing the loan status- please make me understand this better – what I know is we are only keeping in mind what was written-off

• If you want to continue doing recovery payments without going into overpayment, is this the right place in code to handle recovery repayments?
o This is the same place loan repayment transactions are being addressed, I think putting a condition in the same area of code would be nice. Nevertheless, help me with some options (please note am a beginner)

• In either case, you have to enhance the test to handle the impact of any change in this code snippet like explicitly state write-off balance, verify if it's recovery repayment or write-off reversal.
o At the moment, we do not have an option to reverse a write-off so we shall only have to look at recovery repayment.

• Mayble also include the logic in test for the dry run you are doing. Ex:- Write-off balance 1000, Total Recovery Repaid - 800, Repayment - 100. This IF condition is satisfied. But why should outstanding Balance become 1000 and not 100? Please correct me if I am wrong regarding assuming recovery repayments don't reduce write-off balance.
o Once a loan is written-off, the write-off amount (accounting standards) never changes it remains as such and during the period which this write-off is done, it is recorded as a loss. (never changes)
o So as we recover, we look at how much has been recovered as against what was written-off so in your example Write-off balance 1,000, Total Recovery Repaid - 800, Repayment – 100, we simply keep use the approach
Write-off balance 1,000
Total recovery so far 800
new recovery repayment 100
so we test 100+800 vs write-off balance (1,000)

[vorburger]

NOK, 81 commits.. somehow Git got confused. Try rebase, or start over.

[francisguchie]

NOK, 81 commits.. somehow Git got confused. Try rebase, or start over.

I will close and open new PR for this

[francisguchie]

Starting afresh for this take 2