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/07/16 20:08:40 UTC
[GitHub] [fineract] percyashu opened a new pull request #1184: FINERACT-1089 removing the rs.previous()
percyashu opened a new pull request #1184:
URL: https://github.com/apache/fineract/pull/1184
## 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-1089
## 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 #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#discussion_r493251227
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountInterestRateChartReadPlatformServiceImpl.java
##########
@@ -305,12 +305,8 @@ public DepositAccountInterestRateChartSlabData extractData(ResultSet rs) throws
if (incentiveData != null) {
chartSlabData.addIncentives(incentiveData);
}
- } else {
- rs.previous();
Review comment:
Nope, I unfortunately don't have any functional knowledge about this either, so a non regression test case indeed seems like our best bet here.
----------------------------------------------------------------
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] ptuomola commented on a change in pull request #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#discussion_r477002949
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountInterestRateChartReadPlatformServiceImpl.java
##########
@@ -305,12 +305,8 @@ public DepositAccountInterestRateChartSlabData extractData(ResultSet rs) throws
if (incentiveData != null) {
chartSlabData.addIncentives(incentiveData);
}
- } else {
- rs.previous();
Review comment:
@percyashu Yes as far as I can see, the easiest solution would be to have all the logic advancing in the record set in the calling function - so not to move anywhere (forward or backward) in the called functions. That would mean changing them to mapRow (as they are operating on this one row only) rather than extractData (which operates on the whole resultset).
But my problem in the previous comment is: I don't really understand why there's logic to move forward / backward anyway? Typically that's there to e.g. combine multiple rows into one. But in this case it returns just a single row and seems to ignore all the others? Is this just to ensure the last record is returned or what's the intention?
So my suggestion would be to set up a test that checks the results being returned (and covers this scenario where rs.next()/rs.previous() is triggered) - and then refactor the method, with the test ensuring that the result remains the same. Of course that assumes the current logic is actually correct... which would be great for someone with more functional knowledge of the data model to confirm!
----------------------------------------------------------------
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 #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#issuecomment-674180330
@edcable am still addressing it, had some difficulties in understanding the appropriate test case for the extractData and the subsequent mapRow replacement.
----------------------------------------------------------------
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 #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#discussion_r493177869
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountInterestRateChartReadPlatformServiceImpl.java
##########
@@ -305,12 +305,8 @@ public DepositAccountInterestRateChartSlabData extractData(ResultSet rs) throws
if (incentiveData != null) {
chartSlabData.addIncentives(incentiveData);
}
- } else {
- rs.previous();
Review comment:
@vorburger Yeah I will make time, just the issues of setting up a test case to ensure the output stays unchanged and if you can confirm the functional knowledge of the data model.
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#issuecomment-715645442
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.
----------------------------------------------------------------
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] ptuomola commented on a change in pull request #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#discussion_r462080648
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountInterestRateChartReadPlatformServiceImpl.java
##########
@@ -305,12 +305,8 @@ public DepositAccountInterestRateChartSlabData extractData(ResultSet rs) throws
if (incentiveData != null) {
chartSlabData.addIncentives(incentiveData);
}
- } else {
- rs.previous();
Review comment:
Having looked at this some more... I think the easiest thing would be just to "collapse" the two functions into a single one: i.e. given the extractData only returns one record, why not just make it a RowMapper and call it in the right place in the calling function? The logic to move in the ResultSet would then be in the calling function only, not split across two functions.
I'm not really sure about the business logic here in the first place: despite the whole rs.next() logic, the method only returns a single record. So in fact all rows except the last one are ignored as far as I can see. Is that really correct: in what cases will we have multiple rows, and how would we expect them to be handled?
To change this, I think the first step would be to set up a test case with known output. We can then refactor the code and ensure the output stays unchanged. I don't think we have any test cases for this at the moment?
----------------------------------------------------------------
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] ptuomola commented on a change in pull request #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#discussion_r456741939
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountInterestRateChartReadPlatformServiceImpl.java
##########
@@ -305,12 +305,8 @@ public DepositAccountInterestRateChartSlabData extractData(ResultSet rs) throws
if (incentiveData != null) {
chartSlabData.addIncentives(incentiveData);
}
- } else {
- rs.previous();
Review comment:
OK so looks like we have two calls to rs.previous(), not just one.
As far as I can see, this function is trying to combine all rows with the same interestRateChartSlabId to a single chartSlabData. So when you get a row with a different ID, then it backs the cursor by one to ensure the calling function gets the right row when it calls next(). Just removing this rs.previous() will not work, as then the cursor is in the wrong position.
We need to think how to fix this. Personally I think this "manipulating ResultsSet in different functions and hoping that each function returns it in the right state" is not a very clean approach, but I'm not sure it's worth a complete rewrite - probably just easier to find some quick fix.
----------------------------------------------------------------
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 #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#discussion_r493177869
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountInterestRateChartReadPlatformServiceImpl.java
##########
@@ -305,12 +305,8 @@ public DepositAccountInterestRateChartSlabData extractData(ResultSet rs) throws
if (incentiveData != null) {
chartSlabData.addIncentives(incentiveData);
}
- } else {
- rs.previous();
Review comment:
@vorburger Yeah I will make time, just the issues of setting up a test case to ensure the output stays unchanged and if you can confirm the functional knowledge of the data model.
----------------------------------------------------------------
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] edcable commented on pull request #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
edcable commented on pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#issuecomment-673649440
@percyashu are you still addressing comments from @ptuomola - looked like next step was a test case with known output.
----------------------------------------------------------------
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 #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#discussion_r492927273
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountInterestRateChartReadPlatformServiceImpl.java
##########
@@ -305,12 +305,8 @@ public DepositAccountInterestRateChartSlabData extractData(ResultSet rs) throws
if (incentiveData != null) {
chartSlabData.addIncentives(incentiveData);
}
- } else {
- rs.previous();
Review comment:
@percyashu will you have time to do that @ptuomola suggests above? /Cc @awasum FYI.
----------------------------------------------------------------
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] github-actions[bot] closed pull request #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #1184:
URL: https://github.com/apache/fineract/pull/1184
----------------------------------------------------------------
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 #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#discussion_r472165457
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountInterestRateChartReadPlatformServiceImpl.java
##########
@@ -305,12 +305,8 @@ public DepositAccountInterestRateChartSlabData extractData(ResultSet rs) throws
if (incentiveData != null) {
chartSlabData.addIncentives(incentiveData);
}
- } else {
- rs.previous();
Review comment:
@ptuomola concerning making extractData in to mapRow does this entails removing whole the rs.next() and using the ircIndex of the calling function as row number of the two RowMappers incentiveMapper and chartSlabsMapper.
----------------------------------------------------------------
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 #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#issuecomment-659659832
@ptuomola @vorburger 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] vorburger commented on a change in pull request #1184: FINERACT-1089 removing the rs.previous()
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#discussion_r492927273
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountInterestRateChartReadPlatformServiceImpl.java
##########
@@ -305,12 +305,8 @@ public DepositAccountInterestRateChartSlabData extractData(ResultSet rs) throws
if (incentiveData != null) {
chartSlabData.addIncentives(incentiveData);
}
- } else {
- rs.previous();
Review comment:
@percyashu will you have time to do that @ptuomola suggests above? /Cc @awasum FYI.
----------------------------------------------------------------
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