-
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
Holiday Rescheduling (FINERACT-1313) #1843
Conversation
Holiday rescheduling feature
@francisguchie and @bharathc27 Please can you find time to test this one functionally? Looks interesting. |
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
Is there a UI component to this? @ptuomola Do you have an opinion? |
// Account for a Bimonthly Loan Product | ||
// if (loanApplicationTerms.getRepaymentPeriodFrequencyType().isSemiMonthly()) { | ||
// if (rescheduleToDate.isAfter(loanRepaymentScheduleInstallment.getDueDate()) | ||
// && rescheduleToDate.isBefore(loanRepaymentScheduleInstallment.getDueDate().plusDays(15))) { |
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.
Why are you commenting all these lines if its not needed maybe we remove them? is it important?
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.
The commented code is more for a future proof for an upcoming feature that I think is still WIP progress at Muellners around a bi-monthly product.
The commented code is meant to retrofit the holiday rescheduling to new feature once it's rolled out.
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.
thanks Kaze for focusing on detail here. very thoughtful!
1 similar comment
For this feature, there is no actual need for a UI change as it just an improvement on the loan rescheduling algorithm and nothing around the API. The improvement is more at the level of improving the holiday rescheduling algorithm to work better with long holidays as in the past, the feature would accumulate unpaid loans during holiday periods to a single on the next working day. So same UI from before works for this. |
I have not used the reschedule before but if this is what is being updated, its a +1 from me - This is really a good improvement |
This feature is part of a strategy of keeping Fineract vaccinated against pandemic. Read more here.. https://research.muellners.org/holiday-rescheduling/ |
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
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/api-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.