[GitHub] [fineract] maektwain commented on a change in pull request #926: FINERACT-977

[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