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 2021/04/11 11:23:01 UTC
[GitHub] [fineract] josemakara2 opened a new pull request #1693: Fix runreports (FINERACT-1345)
josemakara2 opened a new pull request #1693:
URL: https://github.com/apache/fineract/pull/1693
## Description
See FINERACT-1345
## 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/api-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.
--
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] josemakara2 edited a comment on pull request #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
josemakara2 edited a comment on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-841765803
> To move forward with this, I've made the proposed changes in a separate PR: #1734. Can you check if this fixes the issue for you, and if yes I can then merge it?
:thumbsup: +1: for program to interface, not implementation.
Tested that in my PR ([see changes](https://github.com/apache/fineract/pull/1693/files)) and yes #1734 can be merged after the `format fix` and then we close this PR. I prefer merge squash to re basing.
Many thanks
--
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] josemakara2 closed pull request #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
josemakara2 closed pull request #1693:
URL: https://github.com/apache/fineract/pull/1693
--
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 #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-841751538
Ok - had a look at this and fully agree with the fix.
However can I suggest we keep the logic so that the selection of report type is in getReportType() and not split between two different places. Also we should keep the return value from findReportingProcessService() as the generic type, not specific to DatatableReportingProcessService, as otherwise this may break the possible plug-ins for this (e.g. Pentaho reporting plugin).
To move forward with this, I've made the proposed changes in a separate PR: https://github.com/apache/fineract/pull/1734. Can you check if this fixes the issue for you, and if yes I can then merge it?
--
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 #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-841751538
Ok - had a look at this and fully agree with the fix.
However can I suggest we keep the logic so that the selection of report type is in getReportType() and not split between two different places. Also we should keep the return value from findReportingProcessService() as the generic type, not specific to DatatableReportingProcessService, as otherwise this may break the possible plug-ins for this (e.g. Pentaho reporting plugin).
To move forward with this, I've made the proposed changes in a separate PR: https://github.com/apache/fineract/pull/1734. Can you check if this fixes the issue for you, and if yes I can then merge it?
--
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 #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-818277331
@vorburger I wonder if you might have time to take a quick look at this? At first glance this looks like a regression introduced by the earlier refactoring, but I wonder if there's a better way to fix this - e.g. by having findReportingProcessService return DatatableReportingProcessService? But I'm not that familiar with this part of the code so would value your thoughts...
--
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] josemakara2 commented on pull request #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
josemakara2 commented on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-841765803
> To move forward with this, I've made the proposed changes in a separate PR: #1734. Can you check if this fixes the issue for you, and if yes I can then merge it?
:thumbsup: +1: for program to interface, not implementation.
Tested that in my PR and yes #1734 can be merged after the `format fix` and then we close this PR. I prefer merge squash to re basing.
Many thanks
--
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] josemakara2 closed pull request #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
josemakara2 closed pull request #1693:
URL: https://github.com/apache/fineract/pull/1693
--
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] josemakara2 commented on pull request #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
josemakara2 commented on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-841765803
--
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] bharathcgowda commented on pull request #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
bharathcgowda commented on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-841193149
@BLasan and I were able to test it, and functionality-wise, it fixes the issue.
Could we get the code review done and merge it ASAP, as it is a blocker for reporting needs.
@edcable 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] josemakara2 commented on pull request #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
josemakara2 commented on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-817296285
@ptuomola Code review please. Thank you
--
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 #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-840855909
Thanks @josemakara2
I'm really not familiar with the code & refactoring done here earlier... but let me take a closer look this weekend. If your change seems to fit into the overall pattern I'll merge it in then.
Thanks
--
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] josemakara2 commented on pull request #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
josemakara2 commented on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-841775140
Closing this in favour of #1734
--
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] josemakara2 commented on pull request #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
josemakara2 commented on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-835828557
@ptuomola
I have updated the pull request.
Do you think this can go in and when @vorburger gets a chance to look - if he will be wanting us to refactor further then we can always open another PR? Thank you.
--
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] josemakara2 edited a comment on pull request #1693: Fix runreports (FINERACT-1345)
Posted by GitBox <gi...@apache.org>.
josemakara2 edited a comment on pull request #1693:
URL: https://github.com/apache/fineract/pull/1693#issuecomment-841765803
> To move forward with this, I've made the proposed changes in a separate PR: #1734. Can you check if this fixes the issue for you, and if yes I can then merge it?
:thumbsup: +1: for program to interface, not implementation.
Tested that in my PR ([see changes](https://github.com/apache/fineract/pull/1693/files)) and yes #1734 can be merged after the `format fix` and then we close this PR. I prefer merge squash to re basing.
Many thanks
--
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