-
Notifications
You must be signed in to change notification settings - Fork 2.2k
FINERACT-2412: Full term tranche - Schedule handling and Calculations #5294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
FINERACT-2412: Full term tranche - Schedule handling and Calculations #5294
Conversation
ff77199 to
ee3752c
Compare
ee3752c to
926421f
Compare
| final LocalDate firstDisbursedPeriodStartDate = firstDisbursedPeriod.isPresent() ? firstDisbursedPeriod.get().getFromDate() | ||
| : scheduleModel.getMaturityDate(); | ||
|
|
||
| final LoanApplicationTerms loanApplicationTerms = new LoanApplicationTerms.Builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this LoanAplicationBuilder Logic must be extracted to an new helper function. So that it is easier to read.
and group this chaining maybe like this?
// Loan basics
.currency(...)
.principal(...)
.repaymentsStartingFromDate(...)
// Term & frequency
.loanTermFrequency(...)
.numberOfRepayments(...)
.repaymentEvery(...)
.repaymentPeriodFrequencyType(...)
// Interest configuration
.interestRatePerPeriod(...)
.annualNominalInterestRate(...)
.interestRatePeriodFrequencyType(...)
.interestMethod(...)
// Day count conventions
.daysInMonthType(...)
.daysInYearType(...)
.daysInYearCustomStrategy(...)
// Flags & options
.isDownPaymentEnabled(false)
.allowPartialPeriodInterestCalculation(...)
// Technical
.mc(mc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
926421f to
cdaf5bc
Compare
Aman-Mittal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code wise seems ok, Not reviewed based on e2e tests due to my lack of functional knowlegede.
Message to Maintainer: Please cross-check the tests before merge
cdaf5bc to
8ddfbe7
Compare
8ddfbe7 to
1c4cb36
Compare
Description
Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.