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/09 23:03:01 UTC
[GitHub] [fineract] francisguchie opened a new pull request #1166: FINERACT-1054 fixed sql grammar at loan repayment
francisguchie opened a new pull request #1166:
URL: https://github.com/apache/fineract/pull/1166
## Description
see https://issues.apache.org/jira/browse/FINERACT-1054
## 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 pull request #1166: FINERACT-1054 fixed sql grammar at loan repayment
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1166:
URL: https://github.com/apache/fineract/pull/1166#issuecomment-660646730
@ptuomola do you have any interest in reviewing this? My SQL Foo is... limited. Feel free to ignore this request and I'll just merge on my next round.
----------------------------------------------------------------
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] francisguchie commented on pull request #1166: FINERACT-1054 fixed sql grammar at loan repayment
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1166:
URL: https://github.com/apache/fineract/pull/1166#issuecomment-657873337
> This change looks OK to me, if you say that it fixes the error you described in the JIRA.. I'm just curious why none of our ITs (tests) hit this- is this not covered by any tests? Or does the problem which this fixes only appear on some (old?) MySQL version?
This appears on mysSQL 5.7 and above in which full_group_by comes as a default for 5.7 and above
This is a "systematic" error in the code the SQL used before works well for MySQL < 5.7
there is need to make compatibility tests especially where there is change in version especially the database engine.
Backward compatibility could also become an issue. a number of times, newer SQL versions come with newer keywords which when used the older versions would not recognize
----------------------------------------------------------------
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] francisguchie commented on pull request #1166: FINERACT-1054 fixed sql grammar at loan repayment
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1166:
URL: https://github.com/apache/fineract/pull/1166#issuecomment-659649163
@awasum, it might take me longer than expected to come up with an integration test and raise a PR for this test. I would not want to delay the team
----------------------------------------------------------------
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] francisguchie commented on pull request #1166: FINERACT-1054 fixed sql grammar at loan repayment
Posted by GitBox <gi...@apache.org>.
francisguchie commented on pull request #1166:
URL: https://github.com/apache/fineract/pull/1166#issuecomment-656434888
@awasum
This one passed
----------------------------------------------------------------
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 #1166: FINERACT-1054 fixed sql grammar at loan repayment
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1166:
URL: https://github.com/apache/fineract/pull/1166#issuecomment-657826139
This change looks OK to me, if you say that it fixes the error you described in the JIRA.. I'm just curious why none of our ITs (tests) hit this- is this not covered by any tests? Or does the problem which this fixes only appear on some (old?) MySQL version?
----------------------------------------------------------------
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 pull request #1166: FINERACT-1054 fixed sql grammar at loan repayment
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1166:
URL: https://github.com/apache/fineract/pull/1166#issuecomment-660794602
@vorburger Unfortunately I'm not familiar with the business functionality here - i.e. in what scenarios will we end up with multiple rows for the same due date that need to be grouped - and when we do, how do these rows differ?
I agree that this change should fix the SQL error. But just not sure if there are any cases where we would end up with different behaviour than before: i.e. previously we would have had one row returned with values picked at random from the rows being grouped, and now we would end up with multiple rows being returned. If we know that in all cases the rows with the same due date will also have the same other values, then this will naturally never happen...
Having said that, at the moment it clearly does not work at all but throws an SQL error - so this is definitely an improvement regardless!
----------------------------------------------------------------
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 merged pull request #1166: FINERACT-1054 fixed sql grammar at loan repayment
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #1166:
URL: https://github.com/apache/fineract/pull/1166
----------------------------------------------------------------
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