You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/05/22 10:06:24 UTC
[GitHub] [fineract] percyashu opened a new pull request #924: FINERACT-609 TopUp
percyashu opened a new pull request #924:
URL: https://github.com/apache/fineract/pull/924
Build from https://github.com/apache/fineract/pull/574
## Description
Describe the changes made and why they were made. Ignore if these details are present on the associated Jira ticket https://issues.apache.org/jira/browse/FINERACT-609
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.
- [ ] Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.
- [ ] API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm has been updated with details of any API changes.
- [ ] Integration tests have been created/updated for verifying the changes made.
- [ ] All Integrations tests are passing with the new commits.
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)
Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #924: FINERACT-609 TopUp
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #924:
URL: https://github.com/apache/fineract/pull/924#discussion_r429377378
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/accountdetails/service/AccountDetailsReadPlatformServiceJpaRepositoryImpl.java
##########
@@ -171,6 +172,12 @@ public AccountSummaryCollectionData retrieveGroupAccountDetails(final Long group
return this.jdbcTemplate.query(sql, rm, new Object[]{groupId , glimAccount});
}
+ @Override
+ public Collection<LoanAccountSummaryData> retrieveGroupActiveLoanAccountSummary(final Long groupId) {
+ final String loanWhereClause = " where l.group_id = ? and l.loan_status_id = 300 ";
Review comment:
This still doesn't seem to take the code review feedback that @vishwasbabu had provided in #574 into account?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vishwasbabu merged pull request #924: FINERACT-609 TopUp
Posted by GitBox <gi...@apache.org>.
vishwasbabu merged pull request #924:
URL: https://github.com/apache/fineract/pull/924
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #924: FINERACT-609 TopUp
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #924:
URL: https://github.com/apache/fineract/pull/924#issuecomment-635542461
@vishwasbabu I'll let you review (and merge, if/when OK..) this one, and ignore it; I've got (more than enough..) other PRs to tend to! ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #924: FINERACT-609 TopUp
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #924:
URL: https://github.com/apache/fineract/pull/924#discussion_r429407242
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/accountdetails/service/AccountDetailsReadPlatformServiceJpaRepositoryImpl.java
##########
@@ -171,6 +172,12 @@ public AccountSummaryCollectionData retrieveGroupAccountDetails(final Long group
return this.jdbcTemplate.query(sql, rm, new Object[]{groupId , glimAccount});
}
+ @Override
+ public Collection<LoanAccountSummaryData> retrieveGroupActiveLoanAccountSummary(final Long groupId) {
+ final String loanWhereClause = " where l.group_id = ? and l.loan_status_id = 300 ";
Review comment:
@vishwasbabu does this LGTY?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #924: FINERACT-609 TopUp
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #924:
URL: https://github.com/apache/fineract/pull/924#discussion_r429396101
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/accountdetails/service/AccountDetailsReadPlatformServiceJpaRepositoryImpl.java
##########
@@ -171,6 +172,12 @@ public AccountSummaryCollectionData retrieveGroupAccountDetails(final Long group
return this.jdbcTemplate.query(sql, rm, new Object[]{groupId , glimAccount});
}
+ @Override
+ public Collection<LoanAccountSummaryData> retrieveGroupActiveLoanAccountSummary(final Long groupId) {
+ final String loanWhereClause = " where l.group_id = ? and l.loan_status_id = 300 ";
Review comment:
From
> (since you check if client ID is not null in the service method), it might introduce bugs in the future when someone tries to reuse this method. I would suggest returning only loans of type 3.
I removed the conditional statement to check if client ID is null from LoanReadPlatformServiceImpl
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vishwasbabu commented on a change in pull request #924: FINERACT-609 TopUp
Posted by GitBox <gi...@apache.org>.
vishwasbabu commented on a change in pull request #924:
URL: https://github.com/apache/fineract/pull/924#discussion_r431960044
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/accountdetails/service/AccountDetailsReadPlatformServiceJpaRepositoryImpl.java
##########
@@ -171,6 +172,12 @@ public AccountSummaryCollectionData retrieveGroupAccountDetails(final Long group
return this.jdbcTemplate.query(sql, rm, new Object[]{groupId , glimAccount});
}
+ @Override
+ public Collection<LoanAccountSummaryData> retrieveGroupActiveLoanAccountSummary(final Long groupId) {
+ final String loanWhereClause = " where l.group_id = ? and l.loan_status_id = 300 ";
Review comment:
I would append a clause to the effect of "l.client_id is NULL", this ensures that only group loans are fetched by the query all JLG loans are ignored (loans which are associated with clients and groups and fetched as a part of the retrieveClientActiveLoanAccountSummary method)
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanReadPlatformServiceImpl.java
##########
@@ -1428,15 +1428,15 @@ public LoanAccountData retrieveLoanProductDetailsTemplate(final Long productId,
}
}
- Collection<LoanAccountSummaryData> clientActiveLoanOptions = null;
- if(loanProduct.canUseForTopup() && clientId != null){
- clientActiveLoanOptions = this.accountDetailsReadPlatformService.retrieveClientActiveLoanAccountSummary(clientId);
Review comment:
We shouldn't be removing the existing condition which checks for client loans, rather we should be adding an else if for additionally checking for group loans
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on pull request #924: FINERACT-609 TopUp
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #924:
URL: https://github.com/apache/fineract/pull/924#issuecomment-635530174
@vorburger and @vishwasbabu LGTY?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org