Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix of #229 (PsGithub sends invalid POST request) #238

Merged
merged 3 commits into from
May 1, 2015

Conversation

cyberone
Copy link
Contributor

@cyberone cyberone commented May 1, 2015

Fix of #229 (PsGithub sends invalid POST request)

  1. Removed request parameters from URI
  2. Added request parameters to request body

@davvd
Copy link

davvd commented May 1, 2015

@cyberone Thanks, I will find someone to review this PR soon

@davvd
Copy link

davvd commented May 1, 2015

@carlosmiranda review this one, please

@carlosmiranda
Copy link

@cyberone Shouldn't there be a unit test that replicates the bug for this?

@cyberone
Copy link
Contributor Author

cyberone commented May 1, 2015

@carlosmiranda there's already an issue #42 about missing test on PsGithub.

@carlosmiranda
Copy link

@cyberone In that case you should add a puzzle to test this particular scenario. We can't leave it as it is, or nobody will notice and your change will be untested.

@cyberone
Copy link
Contributor Author

cyberone commented May 1, 2015

@carlosmiranda added puzzle. Please re-check.

@@ -51,6 +51,8 @@
* @version $Id$
* @since 0.1
* @checkstyle MultipleStringLiteralsCheck (500 lines)
* @todo #229: PsGithubTest should check that POST request arguments are

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cyberone Where is the estimated time? Should be (for example) #229:30min.

@carlosmiranda
Copy link

@cyberone A couple of comments related to the puzzle above.

@cyberone
Copy link
Contributor Author

cyberone commented May 1, 2015

@carlosmiranda done

@carlosmiranda
Copy link

@cyberone Thanks!

@carlosmiranda
Copy link

@rultor merge please

@rultor
Copy link
Collaborator

rultor commented May 1, 2015

@rultor merge please

@carlosmiranda Thanks for your request. @yegor256 Please confirm this.

@yegor256
Copy link
Owner

yegor256 commented May 1, 2015

@rultor merge

@rultor
Copy link
Collaborator

rultor commented May 1, 2015

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 7e31b15 into yegor256:master May 1, 2015
@rultor
Copy link
Collaborator

rultor commented May 1, 2015

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 11min)

@davvd
Copy link

davvd commented May 3, 2015

@elenavolokhova please, review this task for compliance with our quality rules

@davvd
Copy link

davvd commented May 3, 2015

@rultor deploy pls

@rultor
Copy link
Collaborator

rultor commented May 3, 2015

@rultor deploy pls

@davvd OK, I'll try to deploy now. You can check the progress here

@rultor
Copy link
Collaborator

rultor commented May 3, 2015

@rultor deploy pls

@davvd Done! FYI, the full log is here (took me 7min)

@elenavolokhova
Copy link

@carlosmiranda
According to our quality rules:

Pull request title explains the problem it is fixing.

The title of this request doesn't contain such information. As far as you are the performer of this ticket you are responsible for its quality.
Please fix this issue and make sure that everyone follows this rule in your tickets in the future.

According to our quality rules:

Comments were mostly about design problems, not cosmetic issues.

There were mostly cosmetic issues found during review. Try to pay attention to bigger design problems and the overall quality of the code.
Please confirm that this new rule is clear.

@carlosmiranda
Copy link

@cyberone please rename the ticket title to reflect the actual problem as requested by @elenavolokhova .

@cyberone cyberone changed the title Fix of #229 Fix of #229 (PsGithub sends invalid POST request) May 5, 2015
@cyberone
Copy link
Contributor Author

cyberone commented May 5, 2015

@carlosmiranda done.

@elenavolokhova
Copy link

@cyberone Thank you

@elenavolokhova
Copy link

@davvd Quality is acceptable.

@davvd
Copy link

davvd commented May 5, 2015

@davvd Quality is acceptable.

@elenavolokhova thanks for the review, we'll try better next time

@davvd
Copy link

davvd commented May 6, 2015

@davvd Quality is acceptable.

@elenavolokhova thank you, let's try to make it "good" next time

@davvd
Copy link

davvd commented May 9, 2015

@carlosmiranda I added 10 mins to @elenavolokhova (for QA review) in transaction 56879541

I just added 21 mins to your account, many thanks for your contribution.. 56879557 spent here

you're getting extra minutes here (c=6)

+21 added to your rating, current score is: +4501

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants