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/01/26 16:56:06 UTC
[GitHub] [fineract] angelboxes opened a new pull request #700: Fineract-820:
Fix for LoanReschedulingWithinCenterTest fail on Sundays
angelboxes opened a new pull request #700: Fineract-820: Fix for LoanReschedulingWithinCenterTest fail on Sundays
URL: https://github.com/apache/fineract/pull/700
Did an extra validation made in another test, tested in my PC and went OK, made this pull request just to verify if it is fixed with Travis. I will close it if it doesn't work.
## Description
Describe the changes made and why they were made. Ignore if these details are present on the associated Jira ticket
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [X] Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.
- [x] Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.
- [ ] API documentation at https://github.com/apache/fineract/blob/develop/api-docs/apiLive.htm has been updated with details of any API changes.
- [ ] Integration tests have been created/updated for verifying the changes made.
- [x] 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
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on issue #700: Fineract-820: Fix for
LoanReschedulingWithinCenterTest fail on Sundays
Posted by GitBox <gi...@apache.org>.
vorburger commented on issue #700: Fineract-820: Fix for LoanReschedulingWithinCenterTest fail on Sundays
URL: https://github.com/apache/fineract/pull/700#issuecomment-578547028
@angelboxes thanks a lot for raising this PR! Given that [the develop branch failed to build today on Sunday](https://travis-ci.org/apache/fineract/builds/641997405) (after the requested revert of e9e0bbc930ded7e1b3be942d237e228bacdb52b8, because of that minor Checkstyle problem), and given that this PR passed, it appears this fix is the correct solution (contrary to the initial one which myself and @awasum tried on Jan 5/6th, see JIRA), and I'm therefore in favour of this ASAP.
Before we click Merge, just one thing seems worth clarifying: Was this PR inspired by e9e0bbc930ded7e1b3be942d237e228bacdb52b8 from @nazeer1100126 ? (See https://lists.apache.org/thread.html/r853c0b8ad9387d9925f89da611bb31a93139153e2b364b552fca8d5e%40%3Cdev.fineract.apache.org%3E).
If you did look at that code, it would be "correct" to attribute @nazeer1100126 as the (original) author of this commit - would you like to do that? Just in cases you never did anything like this before, FYI it's easy with git, you can just `git commit --amend --author="Shaik Nazeer Hussain <na...@gmail.com>"; git push --force`. (Unless @nazeer1100126 states here that he doesn't care and is OK if we merge this with @angelboxes as that commit's author.)
If you came up with the same solution without having looked at that code, and independently found the same solution, then you can just state that here, and it would seem fair to merge this and attribute you instead.
The only other and much smaller point would be if, after we merged this, you would be interested to do a follow-up with the minor refactoring proposed in e9e0bbc930ded7e1b3be942d237e228bacdb52b8 to extract the same from ClientLoanIntegrationTest.java into some common place such as Utils.java.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] angelboxes edited a comment on issue #700: Fineract-820:
Fix for LoanReschedulingWithinCenterTest fail on Sundays
Posted by GitBox <gi...@apache.org>.
angelboxes edited a comment on issue #700: Fineract-820: Fix for LoanReschedulingWithinCenterTest fail on Sundays
URL: https://github.com/apache/fineract/pull/700#issuecomment-578549495
> @angelboxes thanks a lot for raising this PR! Given that [the develop branch failed to build today on Sunday](https://travis-ci.org/apache/fineract/builds/641997405) (after the requested revert of [e9e0bbc](https://github.com/apache/fineract/commit/e9e0bbc930ded7e1b3be942d237e228bacdb52b8), because of that minor Checkstyle problem), and given that this PR passed, it appears this fix is the correct solution (contrary to the initial one which myself and @awasum tried on Jan 5/6th, see JIRA), and I'm therefore in favour of this ASAP.
>
> Before we click Merge, just one thing seems worth clarifying: Was this PR inspired by [e9e0bbc](https://github.com/apache/fineract/commit/e9e0bbc930ded7e1b3be942d237e228bacdb52b8) from @nazeer1100126 ? (See https://lists.apache.org/thread.html/r853c0b8ad9387d9925f89da611bb31a93139153e2b364b552fca8d5e%40%3Cdev.fineract.apache.org%3E).
>
> If you did look at that code, it would be "correct" to attribute @nazeer1100126 as the (original) author of this commit - would you like to do that? Just in cases you never did anything like this before, FYI it's easy with git, you can just `git commit --amend --author="Shaik Nazeer Hussain <na...@gmail.com>"; git push --force`. (Unless @nazeer1100126 states here that he doesn't care and is OK if we merge this with @angelboxes as that commit's author.)
>
> If you came up with the same solution without having looked at that code, and independently found the same solution, then you can just state that here, and it would seem fair to merge this and attribute you instead.
>
> The only other and much smaller point would be if, after we merged this, you would be interested to do a follow-up with the minor refactoring proposed in [e9e0bbc](https://github.com/apache/fineract/commit/e9e0bbc930ded7e1b3be942d237e228bacdb52b8) to extract the same from ClientLoanIntegrationTest.java into some common place such as Utils.java.
Hi, well actually this is not based on Nazeer commit and I did comment about this solution on Jiira on Tuesday, which at the time it was assigned to you, I didn't reassigned to myself for that reason and also I was waiting for today just to be sure that this would run on Travis. I got no response and saw that Nazeer assigned to himself some days after that and guessed that he would fix this issue. As you can see on Jiira comments I just did this pull request just to show that what I pointed Iniitially was the problem and that we shouldn't gave up so easily on this issue. I have no problem if this gets closed and Nazeer recommits his code.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] vorburger merged pull request #700: Fineract-820: Fix
for LoanReschedulingWithinCenterTest fail on Sundays
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #700: Fineract-820: Fix for LoanReschedulingWithinCenterTest fail on Sundays
URL: https://github.com/apache/fineract/pull/700
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] nazeer1100126 commented on issue #700: Fineract-820: Fix
for LoanReschedulingWithinCenterTest fail on Sundays
Posted by GitBox <gi...@apache.org>.
nazeer1100126 commented on issue #700: Fineract-820: Fix for LoanReschedulingWithinCenterTest fail on Sundays
URL: https://github.com/apache/fineract/pull/700#issuecomment-578570152
@angelboxes I was in hurry for fineract release didn't check much in that JIRA comments. Apologies.
I have reverted the changes yesterday.
@vorburger I am OK with @angelboxes being the PR author.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fineract] angelboxes commented on issue #700: Fineract-820: Fix
for LoanReschedulingWithinCenterTest fail on Sundays
Posted by GitBox <gi...@apache.org>.
angelboxes commented on issue #700: Fineract-820: Fix for LoanReschedulingWithinCenterTest fail on Sundays
URL: https://github.com/apache/fineract/pull/700#issuecomment-578549495
> @angelboxes thanks a lot for raising this PR! Given that [the develop branch failed to build today on Sunday](https://travis-ci.org/apache/fineract/builds/641997405) (after the requested revert of [e9e0bbc](https://github.com/apache/fineract/commit/e9e0bbc930ded7e1b3be942d237e228bacdb52b8), because of that minor Checkstyle problem), and given that this PR passed, it appears this fix is the correct solution (contrary to the initial one which myself and @awasum tried on Jan 5/6th, see JIRA), and I'm therefore in favour of this ASAP.
>
> Before we click Merge, just one thing seems worth clarifying: Was this PR inspired by [e9e0bbc](https://github.com/apache/fineract/commit/e9e0bbc930ded7e1b3be942d237e228bacdb52b8) from @nazeer1100126 ? (See https://lists.apache.org/thread.html/r853c0b8ad9387d9925f89da611bb31a93139153e2b364b552fca8d5e%40%3Cdev.fineract.apache.org%3E).
>
> If you did look at that code, it would be "correct" to attribute @nazeer1100126 as the (original) author of this commit - would you like to do that? Just in cases you never did anything like this before, FYI it's easy with git, you can just `git commit --amend --author="Shaik Nazeer Hussain <na...@gmail.com>"; git push --force`. (Unless @nazeer1100126 states here that he doesn't care and is OK if we merge this with @angelboxes as that commit's author.)
>
> If you came up with the same solution without having looked at that code, and independently found the same solution, then you can just state that here, and it would seem fair to merge this and attribute you instead.
>
> The only other and much smaller point would be if, after we merged this, you would be interested to do a follow-up with the minor refactoring proposed in [e9e0bbc](https://github.com/apache/fineract/commit/e9e0bbc930ded7e1b3be942d237e228bacdb52b8) to extract the same from ClientLoanIntegrationTest.java into some common place such as Utils.java.
Hi, well actually this is not based on Nazeer commit and I did comment about this solution on Jiira on Tuesday, which at the time it was assigned to you, I didn't assigned to myself for that reason and also I was waiting for today just to ve sure that this would run on Travis. I got no responde and saw that Nazeer assigned to himself some days after that and guessed that he would fix this issue. As You can see on Jiira comments I just did this pull request just to show that what I pointed Iniitially was the problem and that wey shouldn't gave up so easily on this. I have no problem if this gets closed and Nazeer recommits his cose.
----------------------------------------------------------------
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
With regards,
Apache Git Services