Skip to content

FINERACT-416 Interest of the whole term#436

Merged
MexinaD merged 1 commit intoapache:developfrom
MexinaD:FINERACT-416
Nov 22, 2018
Merged

FINERACT-416 Interest of the whole term#436
MexinaD merged 1 commit intoapache:developfrom
MexinaD:FINERACT-416

Conversation

@MexinaD
Copy link
Copy Markdown
Contributor

@MexinaD MexinaD commented Dec 29, 2017

@avikganguly01 Can you please preview this PR so that it can be in upcoming release, it is a long waited feature here in Tanzania.
Its jira ticket is https://issues.apache.org/jira/browse/FINERACT-416

@MexinaD
Copy link
Copy Markdown
Contributor Author

MexinaD commented Jan 8, 2018

@nazeer1100126 Can you check this

Copy link
Copy Markdown
Contributor

@avikganguly01 avikganguly01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MexinaD : Hey Mexina, sorry for the delay.

Can you confirm couple of things :-

  1. The loan terms can be overriden at the loan account level right? If that is the case, corresponding changes need to be made at the loan account level to allow to override the product term settings.

  2. Have you checked the usage of PeriodFrequencyType in the codebase and found no obvious cases where usage of this new enum value won't work with other product settings? If so, appropriate exceptions need to be thrown for such combinations.

  3. Are any of the LMS integration tests failing when you run ./gradlew integrationTest?

Once you have addressed all the comments, can you enhance the example you provided at https://issues-test.apache.org/jira/browse/FINERACT-416 with expected results?


switch (repaymentPeriodFrequencyType) {
case DAYS:
defaultAnnualNominalInterestRate = ratePerPeriod.multiply(BigDecimal.valueOf(365));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoudn't this consider the no. of days in a year as per product configuration instead of 365?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed what was already implemented for "DAYS" frequency from the first switch loop.

defaultAnnualNominalInterestRate = interestRatePerPeriod.multiply(BigDecimal.valueOf(1));
break;
case WHOLE_TERM:
final BigDecimal ratePerPeriod = interestRatePerPeriod.divide(BigDecimal.valueOf(numberOfRepayments*repaymentEvery), 8, RoundingMode.HALF_UP);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why divide by 8?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not divided by 8 but i rounded off the result of InterestRatePerPeriod divide by (numberOfRepayment*repayedEvery) to 8 us it was applied in another code in Loan.java "final MathContext mc = new MathContext(8, roundingMode);" So i assumed the roundoff that the system is using is 8.

@MexinaD
Copy link
Copy Markdown
Contributor Author

MexinaD commented Jan 10, 2018

@avikganguly01 Regarding the above cases

  1. The feature is to add another interest method, and since the interest method is primary configured in loan product and that's why the changes are made in loan product level, like any other interest methods

  2. I didn't check the use of PeriodFrequencyType to another product settings at the beginning thinking it wasn't necessary as the feature is only applicable to loan product. if the new change will not apply to other product, is there need to deal with other combinations?

  3. I don't have a knowledge of LMS integrations but when i run that command there are some tests failing and other skipped.

@MexinaD
Copy link
Copy Markdown
Contributor Author

MexinaD commented Jan 10, 2018

@avikganguly01 Note: This is an improvement

@MexinaD
Copy link
Copy Markdown
Contributor Author

MexinaD commented Jan 25, 2018

@avikganguly01 Is there anything to modify?

@MexinaD
Copy link
Copy Markdown
Contributor Author

MexinaD commented Aug 28, 2018

Hello @ShruthiRajaram Can you help to review this too

@MexinaD MexinaD merged commit ccaec28 into apache:develop Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants