Skip to content

Added dismissal review mandatory "message" parameter #606

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 25, 2017
Merged

Added dismissal review mandatory "message" parameter #606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a8a22a
Added dismissal review mandatory "message" parameter
Geolim4 Jun 24, 2017
d674e1d
Ahhh the tests x)
Geolim4 Jun 24, 2017
18e88bb
Fixed flipped statement in Review API
Geolim4 Jun 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions lib/Github/Api/PullRequest/Review.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,22 @@ public function submit($username, $repository, $pullRequest, $id, array $params
* @param string $repository the repository
* @param int $pullRequest the pull request number
* @param int $id the review id
* @param string $message a mandatory dismissal message
*
* @return array|string
*/
public function dismiss($username, $repository, $pullRequest, $id)
public function dismiss($username, $repository, $pullRequest, $id, $message)
Copy link
Collaborator

@Nyholm Nyholm Jun 25, 2017

Choose a reason for hiding this comment

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

This is technically a BC break. But i think we should consider it as a bugfix since the method as has been unusable before this PR.

Copy link
Contributor Author

@Geolim4 Geolim4 Jun 25, 2017

Choose a reason for hiding this comment

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

Yeah, I was getting a 500 Internal Error due to the missing required parameter $message :)

Copy link
Collaborator

@Nyholm Nyholm Jun 25, 2017

Choose a reason for hiding this comment

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

Did you get 500 from the API server?

Copy link
Contributor Author

@Geolim4 Geolim4 Jun 25, 2017

Choose a reason for hiding this comment

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

If the message is not provided it seem that Github does not like it very well. I was expecting something like a 422 Error, but I got a 500 instead (A ruby nil error seem to be uncaught by Github).

Copy link
Collaborator

@acrobat acrobat Jun 25, 2017

Choose a reason for hiding this comment

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

I was also in doubt if this was a BC break but indeed the message is required. It could be this changed in the preview period. So this should be documented but it's a correct change. See https://developer.github.com/v3/pulls/reviews/#input-2

Copy link
Collaborator

@acrobat acrobat Jun 25, 2017

Choose a reason for hiding this comment

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

👍 I would also report this to github so they can fix their error in this case!

Copy link
Contributor Author

@Geolim4 Geolim4 Jun 25, 2017

Choose a reason for hiding this comment

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

I already did it :)

Copy link
Contributor Author

@Geolim4 Geolim4 Jun 25, 2017

Choose a reason for hiding this comment

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

My mistake, it's not always a 500 error, it look completely random, sometimes I got a 400, sometimes a 422 O.O

Copy link
Collaborator

@acrobat acrobat Jun 25, 2017

Choose a reason for hiding this comment

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

That's possibly a github error, but this PR is right. The message is required, so we should change the method

Copy link
Contributor Author

@Geolim4 Geolim4 Jun 25, 2017

Choose a reason for hiding this comment

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

@acrobat Let's add your review, so we can merge this asap (if you agree w/ this one).

Thanks :)

{
return $this->put('/repos/'.rawurlencode($username).'/'.rawurlencode($repository).'/pulls/'.$pullRequest.'/reviews/'.$id.'/dismissals');
if (!is_string($message)) {
throw new InvalidArgumentException(sprintf('"message" must be a valid string ("%s" given).', gettype($message)));
}

if (empty($message)) {
throw new InvalidArgumentException('"message" is mandatory and cannot be empty');
}

return $this->put('/repos/'.rawurlencode($username).'/'.rawurlencode($repository).'/pulls/'.$pullRequest.'/reviews/'.$id.'/dismissals', [
'message' => $message
]);
}
}
2 changes: 1 addition & 1 deletion test/Github/Tests/Api/PullRequest/ReviewTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ public function shouldDismissReview()
->with('/repos/octocat/Hello-World/pulls/12/reviews/80/dismissals')
->willReturn($expectedValue);

$this->assertSame($expectedValue, $api->dismiss('octocat', 'Hello-World', 12, 80));
$this->assertSame($expectedValue, $api->dismiss('octocat', 'Hello-World', 12, 80, 'Dismiss reason'));
}

protected function getApiClass()
Expand Down