FINERACT-2031: Fix reschedule to next repayment date. by 2vinodhkumar · Pull Request #3656 · apache/fineract

2vinodhkumar · 2023-12-27T12:10:04Z

Schedule should shift all the remaining installments to next repayment date as per the loan frequency.

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!

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.

2vinodhkumar · 2023-12-27T12:16:03Z

@adamsaghy Please review this and let me know if its good to merge.

bharathcgowda · 2023-12-31T07:03:04Z

@adamsaghy Could you please help us review this PR when ever you are available?

adamsaghy · 2023-12-31T11:17:16Z

...ation-tests/src/test/java/org/apache/fineract/integrationtests/SchedulerJobsTestResults.java

+        periods = (ArrayList<LinkedHashMap>) repaymentScheduleHashMap.get("periods");
+        ArrayList<Integer> dateToApplyHolidays = null;
+
+        for (LinkedHashMap period : periods) {


I dont really understand this piece... what exactly are you testing?

I reckon it should be tested, whether all the periods are proper, but this is not testing anything but whether the last period from date is not null.

Please rewrite this to explicitly test the rescheduled periods are correct!

@adamsaghy do we have any slack channel to communicate? I need to understand How to do local testing...

adamsaghy

I dont really understand this piece... what exactly are you testing?

I reckon it should be tested, whether all the periods are proper, but this is not testing anything but whether the last period from date is not null.

Please rewrite this to explicitly test the rescheduled periods are correct!

2vinodhkumar · 2023-12-31T17:54:09Z

@adamsaghy I have updated the test case to fully check all rescheduled date's validity. Please review and let me know if this is good.

adamsaghy · 2023-12-31T18:32:43Z

@adamsaghy I have updated the test case to fully check all rescheduled date's validity. Please review and let me know if this is good.

Please squash your commits!

1 PR, 1 commit ;)

adamsaghy · 2023-12-31T18:41:02Z

...ation-tests/src/test/java/org/apache/fineract/integrationtests/SchedulerJobsTestResults.java

+            if (fromDate != null && Objects.equals(fromDate.get(1), dueDateBeforeRescheduled.get(1))) {
+                dateToApplyHolidays = fromDate;
+            }
+            Assertions.assertNotNull(dateToApplyHolidays);


its still not perfect...

i was wondering whether you could test something like this:

disburse the loan on a date (example: 2023.01.01)
Assert all the periods due date.
do the reschedule, (example: reschedule the 2nd period)
assert all the "new" periods due date:

the 1st period due date is not changed.
the 2nd period due date was changed to the next one.
all the later periods was moved

i hope it gives some insight! :)

@adamsaghy I tried to follow similar approach. Can you please review the latest commit? I will squash the commits from next as I pushed these already

But you did not follow at all... you are still calculating and comparing dates with a logic which might be correct but i am having hard time to read and understand. Since you are using a fixed date: 2023.01.01 where the loan is created and disbursed, the period from and due dates are fixed as well:
2023.02.01
2023.03.01
2023.04.01
etc.

Can you please write assertions which checks whether the above dates are the ones that got used and after the holiday reschedule what are the new dates. Example:

Number of repayments: 3
Period lenght: 1 month

Before holiday reschedule
// 1st period
assertEquals(LocalDate.of(2023,1,1), periods.get(0).getFromDate())
assertEquals(LocalDate.of(2023,2,1), periods.get(0).getDueDate())
// 2nd period
assertEquals(LocalDate.of(2023,2,1), periods.get(1).getFromDate())
assertEquals(LocalDate.of(2023,3,1), periods.get(1).getDueDate())
// 3rd period
assertEquals(LocalDate.of(2023,3,1), periods.get(2).getFromDate())
assertEquals(LocalDate.of(2023,4,1), periods.get(2).getDueDate())

After holiday reschedule

Reschedule from 1st of Feb
// 1st period
assertEquals(LocalDate.of(2023,1,1), periods.get(0).getFromDate())
assertEquals(LocalDate.of(2023,2,1), periods.get(0).getDueDate())
// 2nd period (got rescheduled)
assertEquals(LocalDate.of(2023,2,1), periods.get(1).getFromDate())
assertEquals(LocalDate.of(2023,4,1), periods.get(1).getDueDate())
// 3rd period (got rescheduled)
assertEquals(LocalDate.of(2023,4,1), periods.get(2).getFromDate())
assertEquals(LocalDate.of(2023,5,1), periods.get(2).getDueDate())

This is easy to read and easy to understand what is happening

@adamsaghy Hardcoding these values will not work. On every testing actual rescheduled dates will shift to next dates. If we hard code these values in the first run it will get succeeded but in the next run due dates will get changed but hard coded values will remain same.

@2vinodhkumar you are creating a brand new loan with each execution and checking only the created loan periods. so i dont think it will be different for each execution at all.

@adamsaghy I have tested this from UI/API , seems working as expected. From integration tests, All the reschedules are getting updated though the due date is before the holiday date. Let me recheck with different test cases.

I understand what you say (the old loans might be rescheduled multiple times, but the new is only once), but with each test you are creating a brand new loan and that loan will be rescheduled just once (hence it is brand new). And that loan periods are checked, so it should be always the same. Feel free to test it ;)

@adamsaghy I have pushed new test cases and tested this working as expected. Let me know your comments once review it.

adamsaghy

LGTM

adamsaghy · 2024-01-04T10:24:52Z

@2vinodhkumar Please squash your commits! The rest looks good

… shift all the remaining installments to next repayment date as per the loan frequency. FINERACT-2031: Fix reschedule to next repayment date. Schedule should shift all the remaining installments to next repayment date as per the loan frequency. FINERACT-2031: Fix reschedule to next repayment date. Schedule should shift all the remaining installments to next repayment date as per the loan frequency. FINERACT-2031: Fix reschedule to next repayment date. Schedule should shift all the remaining installments to next repayment date as per the loan frequency. FINERACT-2031: Fix reschedule to next repayment date. Schedule should shift all the remaining installments to next repayment date as per the loan frequency.

2vinodhkumar · 2024-01-04T10:40:24Z

@2vinodhkumar Please squash your commits! The rest looks good

Now squashed all commmits.

adamsaghy

LGTM

2vinodhkumar mentioned this pull request Dec 27, 2023

FINERACT-2031: Fix reschedule to next repayment date. #3655

Closed

6 tasks

adamsaghy reviewed Dec 31, 2023

View reviewed changes

adamsaghy requested changes Dec 31, 2023

View reviewed changes

2vinodhkumar requested a review from adamsaghy December 31, 2023 17:52

adamsaghy requested changes Dec 31, 2023

View reviewed changes

2vinodhkumar force-pushed the feature/FINERACT-2031-with-integration-test branch from 1014e7b to 88855c8 Compare January 4, 2024 10:02

adamsaghy approved these changes Jan 4, 2024

View reviewed changes

2vinodhkumar force-pushed the feature/FINERACT-2031-with-integration-test branch from 88855c8 to b4a7a26 Compare January 4, 2024 10:38

adamsaghy approved these changes Jan 4, 2024

View reviewed changes

adamsaghy merged commit 50eefd2 into apache:develop Jan 4, 2024

Conversation

2vinodhkumar commented Dec 27, 2023

Description

Checklist

Uh oh!

2vinodhkumar commented Dec 27, 2023

Uh oh!

bharathcgowda commented Dec 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

2vinodhkumar commented Dec 31, 2023

Uh oh!

adamsaghy commented Dec 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

adamsaghy commented Jan 4, 2024

Uh oh!

2vinodhkumar commented Jan 4, 2024

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

adamsaghy Jan 4, 2024 •

edited

Loading