Skip to content

Comments

FINERACT-1109 appropriate-interest-larger-than-emi#1220

Merged
avikganguly01 merged 1 commit intoapache:developfrom
fynmanoj:AL-14
Aug 19, 2020
Merged

FINERACT-1109 appropriate-interest-larger-than-emi#1220
avikganguly01 merged 1 commit intoapache:developfrom
fynmanoj:AL-14

Conversation

@fynmanoj
Copy link
Contributor

@fynmanoj fynmanoj commented Jul 31, 2020

Description

Describe the changes made and why they were made. Ignore if these details are present on the associated Jira ticket
https://mifosforge.jira.com/browse/AL-14

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide

@xurror
Copy link
Contributor

xurror commented Jul 31, 2020

You got checkstyle violations. Run ./gradlew build and ./gradlew check and make sure everything works fine.

@vorburger
Copy link
Member

@avikganguly01 knowing that @fynmanoj works with you, and assuming that you are still an active committer to this project (are you?), I was wondering if you would like to engage on code review feedback and possible evental merge of PRs such as this one? (I don't want to be a bottleneck on this project, and won't have the spare time to help review changes like this. If there are other active commiters such as @awasum @ptuomola @xurror or others reading this who want to engage on this PR, please don't let me hold you up / wait for me - I'll ignore this one, and try to help on others.)

@vorburger
Copy link
Member

@fynmanoj this PR needs to have a JIRA issue on the ASF JIRA, instead of referencing an issue on https://mifosforge.jira.com.

@edcable edcable changed the title AL-14-approppritae-interest-larger-than-emi FINERACT-1109 approppriate-interest-larger-than-emi Aug 4, 2020
@edcable edcable changed the title FINERACT-1109 approppriate-interest-larger-than-emi FINERACT-1109 appropriate-interest-larger-than-emi Aug 4, 2020
@edcable edcable requested a review from avikganguly01 August 4, 2020 22:50
@edcable
Copy link
Contributor

edcable commented Aug 10, 2020

@vorburger I opened up tickets on JIRA for these pull requests and referenced them in the title of the PR. I tagged @avikganguly01 as a reviewer and assume he can do the needful in reviewing and merging the PRs as we are going to start testing this development branch with these pull requests merged on the user's test server.

@fynmanoj we also have to address the failing build for this PR.

Copy link
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.

Rebase required. Missing integration test for this scenario.

https://travis-ci.org/github/apache/fineract/builds/713781505 - Tranche loan creation is failing. Can you check if it's a valid regression?

Have you tested same scenario for a new loan where there is a long gap between disbursement and first repayment date? If this is resolved, we can create and mention it in a separate bug blocked by resolution of this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should have been part of PR FIN-1106.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is tranche loan approval failing due to flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a different bug got squashed here. What effects could this bug have been producing? Shall we create a different ticket to track this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, found this while testing the new feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the add Interest method principle was added to the total due instead of the interest. which is wrong, as the added interest must reflect in the total due.

@fynmanoj fynmanoj force-pushed the AL-14 branch 7 times, most recently from e53eb23 to db98023 Compare August 18, 2020 12:23
@edcable
Copy link
Contributor

edcable commented Aug 18, 2020

@avikganguly01 Did the recent changes made by @fynmanoj address the feedback you provided?

@avikganguly01 avikganguly01 merged commit 899eab8 into apache:develop Aug 19, 2020
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.

5 participants