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/05/22 17:50:46 UTC
[GitHub] [fineract] maektwain opened a new pull request #926: FINERACT-977
maektwain opened a new pull request #926:
URL: https://github.com/apache/fineract/pull/926
## Description
Probable fix, please test before or replay , though these scenario are typically because of null values stored in the data structure .
Need to come up with better way to have default values in such case and calculation criteria.
## 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] maektwain commented on a change in pull request #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
maektwain commented on a change in pull request #926:
URL: https://github.com/apache/fineract/pull/926#discussion_r429437269
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/domain/AbstractLoanScheduleGenerator.java
##########
@@ -517,9 +517,11 @@ private LoanScheduleModelPeriod handleRecalculationForTransactions(final MathCon
scheduleParams.getTotalCumulativePrincipal().plus(
currentPeriodParams.getPrincipalForThisPeriod().minus(principalProcessed)),
scheduleParams.getPeriodNumber() + 1, mc));
- if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment()
- && fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount()) != 0) {
- currentPeriodParams.setEmiAmountChanged(true);
+ if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment())
+ if(fixedEmiAmount != null) {
+ if (fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount()) != 0) {
Review comment:
This part is based on the fact that `fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount()) != 0)`
fixedEmiAmount is not going to be null ?
It will fail even if loanApplicationTerms.getFixedEmiAmount() this is going to be null, since the first hand requires not to be null.
We need to broadly think of the null checks in the application, this way we can make code more clean and easier to such edge cases,
In my opinion there are many places like this where null values are sometimes checked and if the field dependent to some fields is not logically set it fails at null exception
----------------------------------------------------------------
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 a change in pull request #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #926:
URL: https://github.com/apache/fineract/pull/926#discussion_r429613205
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/domain/AbstractLoanScheduleGenerator.java
##########
@@ -517,9 +517,11 @@ private LoanScheduleModelPeriod handleRecalculationForTransactions(final MathCon
scheduleParams.getTotalCumulativePrincipal().plus(
currentPeriodParams.getPrincipalForThisPeriod().minus(principalProcessed)),
scheduleParams.getPeriodNumber() + 1, mc));
- if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment()
- && fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount()) != 0) {
- currentPeriodParams.setEmiAmountChanged(true);
+ if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment())
+ if(fixedEmiAmount != null) {
+ if (fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount()) != 0) {
Review comment:
why did you use the && style below as discussed, but kept if/if/if style here? This is inconsistent, please change.
----------------------------------------------------------------
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 a change in pull request #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #926:
URL: https://github.com/apache/fineract/pull/926#discussion_r429425458
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/domain/AbstractLoanScheduleGenerator.java
##########
@@ -517,9 +517,11 @@ private LoanScheduleModelPeriod handleRecalculationForTransactions(final MathCon
scheduleParams.getTotalCumulativePrincipal().plus(
currentPeriodParams.getPrincipalForThisPeriod().minus(principalProcessed)),
scheduleParams.getPeriodNumber() + 1, mc));
- if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment()
- && fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount()) != 0) {
- currentPeriodParams.setEmiAmountChanged(true);
+ if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment())
+ if(fixedEmiAmount != null) {
+ if (fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount()) != 0) {
Review comment:
The 3 nested `if` are not necessary, and this should instead be written like so (correctly formatted).
```suggestion
if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment()
&& fixedEmiAmount != null
&& fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount()) != 0) {
```
More importantly, while I don't understand much about this functionality, it seems to me that as-is we may be missing a subtle corner case... what if one of `fixedEmiAmount` and `loanApplicationTerms.getFixedEmiAmount()` are `null` but (respectively) the other is not (anymore?) - should that also trigger `setEmiAmountChanged(true)` ? I don't know details about this, but that would kind of seem "logical", to me. If you agree, then the check should take that case into account.
BTW we are assuming here that it is valid for an "Emi Amount" (whatever that is???) to be `null`. Again, I don't know anything about this functionality, so unless you know more, I guess it's a fair assumption. If someone know actually understands the details of this can chime in here with knowledge if it's expected to (sometimes) be null, or if that is really an indication of another problem, that could be interesting, but if we don't learn more, let's just go for making it null safe.
In an ideal world, an integration test for this would be nice, but I'd rather merge it without one than not having this fixed... ;-)
The same feedback as above should be applied below as well.
Lastly, the commit message here needs to be improved, instead of "Fix for Null Values" e.g. "fix possible NPE in AbstractLoanScheduleGenerator [FINERACT-977]" would be better.
@pramodn02 according to Git Blame log, these lines of code were introduced by you, long ago, for [MIFOSX-579 ](https://mifosforge.jira.com/browse/MIFOSX-579)... if you still have any interest in this project, perhaps you'd like to chime in here!
----------------------------------------------------------------
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 #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #926:
URL: https://github.com/apache/fineract/pull/926#issuecomment-636496559
@maektwain respectfully pinging you re. this, after 1 week. Are you planning to wrap this up?
----------------------------------------------------------------
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] maektwain commented on pull request #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
maektwain commented on pull request #926:
URL: https://github.com/apache/fineract/pull/926#issuecomment-636815411
Does it makes sense to you ?
----------------------------------------------------------------
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] maektwain commented on pull request #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
maektwain commented on pull request #926:
URL: https://github.com/apache/fineract/pull/926#issuecomment-636923938
Let me reopen a new PR
----------------------------------------------------------------
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 #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #926:
URL: https://github.com/apache/fineract/pull/926#issuecomment-637068084
#960 @maektwain FYI you don't actually have to open a new PR, you can just "force push" (next 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] vorburger commented on pull request #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #926:
URL: https://github.com/apache/fineract/pull/926#issuecomment-636826609
@maektwain LGTM but as previously requested, all commits must be squashed into 1 please.
----------------------------------------------------------------
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] maektwain closed pull request #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
maektwain closed pull request #926:
URL: https://github.com/apache/fineract/pull/926
----------------------------------------------------------------
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] maektwain commented on a change in pull request #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
maektwain commented on a change in pull request #926:
URL: https://github.com/apache/fineract/pull/926#discussion_r429436062
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/domain/AbstractLoanScheduleGenerator.java
##########
@@ -517,9 +517,11 @@ private LoanScheduleModelPeriod handleRecalculationForTransactions(final MathCon
scheduleParams.getTotalCumulativePrincipal().plus(
currentPeriodParams.getPrincipalForThisPeriod().minus(principalProcessed)),
scheduleParams.getPeriodNumber() + 1, mc));
- if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment()
- && fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount()) != 0) {
- currentPeriodParams.setEmiAmountChanged(true);
+ if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment())
+ if(fixedEmiAmount != null) {
+ if (fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount()) != 0) {
Review comment:
I thought about this, @vorburger , in case where fixedAmount is null , then the third condition will simply throw null exception. Don't you think?
Let me simplify the case for you.
Given a loan scenario where loan product type Amortisation is set as Equal Instalment so, you could set the fixedEmi Amount which is your repayment as when you create a loan product you can set this boolean value here
Allow fixing of the installment amount []
if this and other condition is true of equal instalment then while making a loan you can set the EMI of your own choice like in case of 100 USD loan you can choose to set every month 10 USD instalment then system is forced to have this as every month due.
Now in the above case there are two parts, when we decide to set the FixedEMI or not and when we don't then there is null value and then you can imagine what might happen.
Now in code level, we should actually take care of null values since we know the cases, but it was not taken care here.
It won't trigger `setEmiAmountChanged (true)`if fixedEMI is null, and in the edge case its null, so better we check if fixedEmi is null and then apply the method .
----------------------------------------------------------------
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] maektwain edited a comment on pull request #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
maektwain edited a comment on pull request #926:
URL: https://github.com/apache/fineract/pull/926#issuecomment-636815411
Does it makes sense to you ? oh didn't realise I had this merge on top?
----------------------------------------------------------------
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 #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #926:
URL: https://github.com/apache/fineract/pull/926#issuecomment-632895537
*@maektwain* Java expressions evaluation respects parentheses (and && ||
Precedenc), so it first checks "fixedEmiAmount != null" and if that is
false, it does not evaluate "fixedEmiAmount.compareTo()" anymore, so this
is safe.
----------------------------------------------------------------
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 #926: FINERACT-977
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #926:
URL: https://github.com/apache/fineract/pull/926#issuecomment-632875104
@nnatarajan following along the code review conversation in this Pull Request could interest you.
----------------------------------------------------------------
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