-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
FINERACT-1958-Rework-Down-Payment-Overlapping-Installment #3467
FINERACT-1958-Rework-Down-Payment-Overlapping-Installment #3467
Conversation
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.
Please kindly see my review!
...apache/fineract/portfolio/loanaccount/loanschedule/domain/AbstractLoanScheduleGenerator.java
Outdated
Show resolved
Hide resolved
bab8840
to
d8ae02c
Compare
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.
It is looking good to me.
minor comment1: in the tests I would have added assertions for the disbursements as well. It's a bit weird that we have gaps in the indexes
loanDetails.getRepaymentSchedule().getPeriods().get(....).getPeriod()
Minor comment 2: I think a helper method which asserts all the attributes of an installments in the testcase could improve the readability.
assertEquals(0.0, loanDetails.getRepaymentSchedule().getPeriods().get(4).getPenaltyChargesPaid()); | ||
assertEquals(true, loanDetails.getRepaymentSchedule().getPeriods().get(4).getComplete()); | ||
|
||
// third period [2023-04-03 to 2023-04-03] overlapping down payment installment |
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.
Just for double checking. We don't want to create two separate entries for the two downpayments happend on the same day, right?
d8ae02c
to
b6d8389
Compare
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.
LGTM
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.
LGTM
Description
Fix for overlapping installments for down payments.
Test fix.
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!
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/legacy-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.