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/09/22 19:28:22 UTC
[GitHub] [fineract] thesmallstar opened a new pull request #1344: Updated Report-name Regex
thesmallstar opened a new pull request #1344:
URL: https://github.com/apache/fineract/pull/1344
I got this issue in the email thread, no jira Ticket:
The report names with "(" threw SQL injection error, I identified the mistake to be incorrect Regex, I am not sure how this worked in the past.
Still testing this.
----------------------------------------------------------------
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 #1344: Updated Report-name Regex
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-696941435
> I got this issue in the email thread
Which email thread? :smile:
> no jira Ticket:
It would be great to create a JIRA issue, describing the problem, with the error message. With text, so that it's searchable, instead of an image. It helps when we look for it again in the future, when a user reports the same error on the mailing list later.
> I am not sure how this worked in the past.
It's possible that it hasn't worked in a long time and was just broken... :sweat_smile:
Would it be possible to add a simple integration test for this?
----------------------------------------------------------------
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 #1344: Updated Report-name Regex (FINERACT-1156)
Posted by GitBox <gi...@apache.org>.
edcable commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-698695751
@bharathc27 have you had a chance to test yet?
----------------------------------------------------------------
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 #1344: Updated Report-name Regex (FINERACT-1156)
Posted by GitBox <gi...@apache.org>.
edcable commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-699035713
@francisguchie are you able to test this locally from your side? @thesmallstar @vorburger is it possible to merge this so @bharathc27 could test against fineract.dev - he doesn't have a local build of Fineract running on his current device.
----------------------------------------------------------------
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] thesmallstar commented on pull request #1344: Updated Report-name Regex
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-696944880
+1 for the integration test as well.
----------------------------------------------------------------
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] thesmallstar commented on pull request #1344: Updated Report-name Regex (FINERACT-1156)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-699037862
@edcable Let me try to setup community app locally (Somehow it is not working), I will give an update soon. It will be better if we test and merge this.
@vorburger just an idea, can we do something to host a particular branch/PR whenever required online? It will be so much handy to test the PRs that way, should I make an issue for this?
----------------------------------------------------------------
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] thesmallstar commented on pull request #1344: Updated Report-name Regex
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-698587123
@edcable @bharathc27 I have tested this for not SQL errors anymore, will you like to test this locally with the community app? If that looks we can add an integration test and merge this?
----------------------------------------------------------------
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] thesmallstar commented on pull request #1344: Updated Report-name Regex
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-698587123
@edcable @bharathc27 I have tested this for not SQL errors anymore, will you like to test this locally with the community app? If that looks we can add an integration test and merge this?
----------------------------------------------------------------
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] thesmallstar edited a comment on pull request #1344: Updated Report-name Regex
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-698587123
----------------------------------------------------------------
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 #1344: Updated Report-name Regex
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-696941435
> I got this issue in the email thread
Which email thread? :smile:
> no jira Ticket:
It would be great to create a JIRA issue, describing the problem, with the error message. With text, so that it's searchable, instead of an image. It helps when we look for it again in the future, when a user reports the same error on the mailing list later.
> I am not sure how this worked in the past.
It's possible that it hasn't worked in a long time and was just broken... :sweat_smile:
Would it be possible to add a simple integration test for this?
----------------------------------------------------------------
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 #1344: Updated Report-name Regex (FINERACT-1156)
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #1344:
URL: https://github.com/apache/fineract/pull/1344
----------------------------------------------------------------
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 #1344: Updated Report-name Regex (FINERACT-1156)
Posted by GitBox <gi...@apache.org>.
edcable commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-698695751
@bharathc27 have you had a chance to test yet?
----------------------------------------------------------------
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] thesmallstar commented on pull request #1344: Updated Report-name Regex
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-696944033
----------------------------------------------------------------
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] thesmallstar edited a comment on pull request #1344: Updated Report-name Regex (FINERACT-1156)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-698587123
@edcable @bharathc27 I have tested this, not getting SQL injection errors anymore, will you like to test this locally with the community app? If that looks we can add an integration test and merge this?
----------------------------------------------------------------
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] thesmallstar commented on pull request #1344: Updated Report-name Regex
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-696944033
My bad :P
Let me quickly make a jira ticket for this explaining the entire issue.
----------------------------------------------------------------
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 #1344: Updated Report-name Regex (FINERACT-1156)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-700180249
> @thesmallstar @vorburger is it possible to merge this so @bharathc27 could test against fineract.dev
sure, let's just merge this as-is now. @thesmallstar you can add the IT in a follow-up PR.
> @edcable Let me try to setup community app locally (Somehow it is not working), I will give an update soon. It will be better if we test and merge this.
@thesmallstar if you have some JS issue, just FYI you could technically use https://cui.fineract.dev with a `baseApiUrl` to `localhost`. But cui.fineract.dev is not auto updated - watch https://github.com/openMF/community-app/issues/3309 about that.
@vorburger just an idea, can we do something to host a particular branch/PR whenever required online? It will be so much handy to test the PRs that way, should I make an issue for this?
That's... not that easy. I thought about it as well. But running a server instance costs. Running a server for every open PR is... non-negligeable. We sure could have some Bot, with some command, with some timeout that "tears down" per-PR servers, but.. this is a amount of work I don't have the time for to spend, unfortunately.
However, I think it's acceptable to sometimes merge PRs so that people can test things on demo.fineract.dev. If things are broken, then follow-up PRs can add further fixes. (The clean thing to do in theory would actually a PR with a `git revert` and then the proper 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] thesmallstar edited a comment on pull request #1344: Updated Report-name Regex
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on pull request #1344:
URL: https://github.com/apache/fineract/pull/1344#issuecomment-698587123
@edcable @bharathc27 I have tested this for not SQL injection errors anymore, will you like to test this locally with the community app? If that looks we can add an integration test and merge this?
----------------------------------------------------------------
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