Add support for the read half of the PR Review API #139

mscharley · 2017-05-19T05:21:54Z

Just putting this up to solicit some initial feedback before I get too deep in the rest of this API.

Fixes GH-133

BenFradet

Looks like a good start 👍

juanpedromoreno · 2017-05-19T17:27:38Z

@mscharley Indeed, it's looking nice 👏

juanpedromoreno · 2017-05-21T12:46:31Z

github4s/shared/src/main/scala/github4s/free/domain/PullRequest.scala

+    pull_request_url: String)
+
+sealed abstract class PullRequestReviewState(val value: String)
+case class PRRStateApprove()        extends PullRequestReviewState("APPROVE")


We could change these case classes by case objects, what do you think?

Good point, I'll do that.

mscharley · 2017-05-22T00:03:29Z

This is ready for final review now.

mscharley · 2017-05-22T00:06:11Z

I've limited the scope of this PR to the read-only portion of the review API as this is all I need right now, I'm on a deadline, and the write half needs some special logic around draft reviews.

juanpedromoreno

It looks great, I've added some minor comments though, that would need to be addressed. Thanks!

juanpedromoreno · 2017-05-22T08:55:52Z

docs/src/main/tut/pull_request.md

@@ -128,6 +130,57 @@ createPullRequestIssue.exec[cats.Id, HttpResponse[String]]() match {

 See [the API doc](https://developer.github.com/v3/pulls/#create-a-pull-request) for full reference.

+## List pull requests


List reviews?

juanpedromoreno · 2017-05-22T09:01:20Z

docs/src/main/tut/pull_request.md

+
+## Get an individual review
+
+You can list the reviews for a pull request using `listReviews`, it takes as arguments:


-You can list the reviews for a pull request using `listReviews` +You can get an individual review for a pull request using `getReview`

juanpedromoreno · 2017-05-22T09:22:30Z

github4s/shared/src/test/scala/github4s/utils/TestData.scala

+    user = user,
+    body = validCommentBody,
+    commit_id = validCommitSha,
+    state = PRRStateCommented(),


This won't compile since PRRStateCommented is now a case object.

BenFradet

LGTM, I have a few minors regarding doc and tests

BenFradet · 2017-05-22T10:09:00Z

docs/src/main/tut/pull_request.md

+
+As an example, if we wanted to see all the reviews for pull request 139 of `47deg/github4s`:
+
+```scala


You can use tut:silent instead of scala for code examples that do not create stuff.
That way your examples will be compiled when you run makeMicrosite.

BenFradet · 2017-05-22T10:10:28Z

docs/src/main/tut/pull_request.md

+
+As an example, if we wanted to see review 39355613 for pull request 139 of `47deg/github4s`:
+
+```scala


BenFradet · 2017-05-22T10:14:30Z

docs/src/main/tut/pull_request.md

+The `result` on the right is the matching [PullRequestReview][pr-scala].
+
+
+See [the API doc](https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request) for full reference.


We usually have a link to the gh doc per endpoint, so this line should be in the # List reviews section and here the link should be changed to https://developer.github.com/v3/pulls/reviews/#get-a-single-review

BenFradet · 2017-05-22T10:16:35Z

docs/src/main/tut/pull_request.md

@@ -128,6 +130,57 @@ createPullRequestIssue.exec[cats.Id, HttpResponse[String]]() match {

 See [the API doc](https://developer.github.com/v3/pulls/#create-a-pull-request) for full reference.



Since Reviews are a "sub-API" of the pull request API, we usually do subsections, so you'll end up with:

Pull request

List pull requests

...

Reviews

List reviews

Get an individual review

BenFradet · 2017-05-22T10:17:42Z

github4s/jvm/src/test/scala/github4s/utils/MockGithubApiServer.scala

+        .withMethod("GET")
+        .withPath(s"/repos/$validRepoOwner/$validRepoName/pulls/$validPullRequestNumber/reviews")
+        .withHeader(not("Authorization")))
+    .respond(response.withStatusCode(unauthorizedStatusCode).withBody(unauthorizedResponse))


If you're not using this endpoint in your tests, you can remove it 👍

BenFradet · 2017-05-22T10:17:48Z

github4s/jvm/src/test/scala/github4s/utils/MockGithubApiServer.scala

+        .withPath(
+          s"/repos/$validRepoOwner/$validRepoName/pulls/$validPullRequestNumber/reviews/$validPullRequestReviewNumber")
+        .withHeader(not("Authorization")))
+    .respond(response.withStatusCode(unauthorizedStatusCode).withBody(unauthorizedResponse))


mscharley · 2017-05-22T11:22:31Z

github4s/jvm/src/test/scala/github4s/utils/MockGithubApiServer.scala

@@ -637,7 +673,7 @@ trait MockGithubApiServer extends MockServerService with FakeResponses with Test
      request
        .withMethod("POST")
        .withPath(s"/repos/$validRepoOwner/$validRepoName/issues")
-        .withHeader(not("Authorization")))
+        .withHeader("Authorization", tokenHeader))


Where did this come from? This should fail.

Did you sort this out in the end?

Yeah, same as above: the difference here are strictly correct but don't reflect how I actually just added a block of text, instead trying to find the way to represent that with the minimum of changes.

I'll double check when I get home just to be sure though.

mscharley · 2017-05-22T11:23:03Z

github4s/jvm/src/test/scala/github4s/utils/MockGithubApiServer.scala

@@ -598,29 +598,65 @@ trait MockGithubApiServer extends MockServerService with FakeResponses with Test
         """.stripMargin)))
    .respond(response.withStatusCode(createdStatusCode).withBody(validCreatePullRequest))

-  //Issues >> list
+  //PullRequests >> listReviews


I think I screwed something up big here.

BenFradet · 2017-05-22T11:12:09Z

docs/src/main/tut/pull_request.md

@@ -11,6 +11,8 @@ with Github4s, you can:
 - [List pull requests](#list-pull-requests)
 - [List the files in a pull request](#list-the-files-in-a-pull-request)
 - [Create a pull request](#create-a-pull-request)
+- [List reviews](#list-reviews)


This link won't work since you changed your heading to "List pull request reviews"

BenFradet · 2017-05-22T11:16:30Z

github4s/jvm/src/test/scala/github4s/utils/MockGithubApiServer.scala

        .withPath(s"/repos/$validRepoOwner/$invalidRepoName/issues")
-        .withHeader(not("Authorization")))
+        .withHeader("Authorization", tokenHeader))
    .respond(response.withStatusCode(notFoundStatusCode).withBody(notFoundResponse))


Hmmm I think things got mixed up because Issues >> List is testing for request without auth token: https://github.com/47deg/github4s/blob/master/github4s/jvm/src/test/scala/github4s/unit/ApiSpec.scala#L778-L782

Oh yeah, there's a corresponding add for >>Issues >> Create lower down. Derp. Sometimes I hate how diffs work.

juanpedromoreno · 2017-05-22T12:10:13Z

github4s/shared/src/main/scala/github4s/free/domain/PullRequest.scala

+
+case class PullRequestReview(
+    id: Int,
+    user: User,


This field should be Option[User]. Recently, we've discovered when one user is removed, it's replaced by https://github.com/ghost (the Github API will return null).

Fix for the rest of the Apis:
#144

…degreesGH-133

juanpedromoreno · 2017-05-22T13:59:30Z

github4s/shared/src/test/scala/github4s/integration/GHPullRequestsSpec.scala

+  it should "return error when an invalid repo name is passed" in {
+    val response =
+      Github(accessToken).pullRequests
+        .getReview(validRepoOwner, validRepoName, validPullRequestNumber, validPullRequestReviewNumber)


Shouldn't be in this way?

-.getReview(validRepoOwner, validRepoName, validPullRequestNumber, validPullRequestReviewNumber) +.getReview(validRepoOwner, invalidRepoName, validPullRequestNumber, validPullRequestReviewNumber)

The test is failing since it returns success.

Doh. I had some failing tests from outside what I was working on, even with a Github key so it was hard to see 'everything green'. Thanks for catching this.

BenFradet

doc nits 👍

BenFradet · 2017-05-22T18:32:48Z

docs/src/main/tut/pull_request.md

+  139)
+
+listReviews.exec[cats.Id, HttpResponse[String]]() match {
+  case Left(e) => println("Something went wrong: s{e.getMessage}")


should be:

case Left(e) => println(s"Something went wrong: ${e.getMessage}")

BenFradet · 2017-05-22T18:32:59Z

docs/src/main/tut/pull_request.md

+  39355613)
+
+review.exec[cats.Id, HttpResponse[String]]() match {
+  case Left(e) => println("Something went wrong: s{e.getMessage}")


juanpedromoreno · 2017-05-22T21:26:52Z

Please, also see this one #139 (review) ;)

juanpedromoreno

For changes added in commit abb7a87, it's needed to add the s interpolator at the beginning ;)

juanpedromoreno · 2017-05-22T21:29:16Z

docs/src/main/tut/pull_request.md

@@ -49,7 +49,7 @@ val prFilters = List(PRFilterOpen, PRFilterSortPopularity)
 val listPullRequests = Github(accessToken).pullRequests.list("scala", "scala", prFilters)

 listPullRequests.exec[cats.Id, HttpResponse[String]]() match {
-  case Left(e) => println("Something went wrong: s{e.getMessage}")
+  case Left(e) => println("Something went wrong: ${e.getMessage}")


-case Left(e) => println("Something went wrong: ${e.getMessage}") +case Left(e) => println(s"Something went wrong: ${e.getMessage}")

The s interpolator is needed at the beginning of the String :P

juanpedromoreno · 2017-05-22T21:29:41Z

docs/src/main/tut/pull_request.md

@@ -71,7 +71,7 @@ To list the files for a pull request:
 val listPullRequestFiles = Github(accessToken).pullRequests.listFiles("47deg", "github4s", 102)

 listPullRequestFiles.exec[cats.Id, HttpResponse[String]]() match {
-  case Left(e) => println("Something went wrong: s{e.getMessage}")
+  case Left(e) => println("Something went wrong: ${e.getMessage}")


mscharley · 2017-05-22T21:32:35Z

@juanpedromoreno That's already fixed, I noticed that when I did the merge.

juanpedromoreno

Great job, LGTM!

juanpedromoreno · 2017-05-22T21:43:34Z

Before merging, let's wait until final @BenFradet 's 👍 , many thanks @mscharley !

BenFradet

A couple of nits regarding the doc and a Q regarding the tests and we're good to go 👍

BenFradet · 2017-05-23T07:52:31Z

docs/src/main/tut/pull_request.md

+
+## List pull request reviews
+
+You can list the reviews for a pull request using `listReviews`, it takes as arguments:


reflecting #146:
listReviews; it takes

BenFradet · 2017-05-23T07:53:11Z

docs/src/main/tut/pull_request.md

+
+## Get an individual review
+
+You can get an individual review for a pull request using `getReview`, it takes as arguments:


BenFradet · 2017-05-23T07:55:08Z

github4s/jvm/src/test/scala/github4s/utils/MockGithubApiServer.scala


  mockServer
    .when(
      request
        .withMethod("GET")
-        .withPath(s"/repos/$validRepoOwner/$validRepoName/issues")
+        .withPath(s"/repos/$validRepoOwner/$validRepoName/pulls/$validPullRequestNumber/reviews")
        .withHeader(not("Authorization")))
    .respond(response.withStatusCode(unauthorizedStatusCode).withBody(unauthorizedResponse))


This is still unused

BenFradet · 2017-05-23T07:55:34Z

github4s/jvm/src/test/scala/github4s/utils/MockGithubApiServer.scala

@@ -637,7 +673,7 @@ trait MockGithubApiServer extends MockServerService with FakeResponses with Test
      request
        .withMethod("POST")
        .withPath(s"/repos/$validRepoOwner/$validRepoName/issues")
-        .withHeader(not("Authorization")))
+        .withHeader("Authorization", tokenHeader))


Did you sort this out in the end?

BenFradet

LGTM, thanks a lot 👍

mscharley · 2017-05-23T08:59:46Z

Still not sure how this happened, looks like it was actually a regression. Is someone able to run the tests where they're known to be working before merging this? I'm still getting one failure locally but they look to me like they're related to my key rather than the test... but I could easily be wrong too.

[info] Activity >> Set a thread subscription                                            
[info] - should return the provided gist                            
[info] - should return a list of references              
[info] - should return at least one reference                             
[info] - should return an error when an invalid repository name is passed              
[info] - should return one commit                                          
[info] - should return an error when an invalid repository name is passed  
[info] - should return a list of issues                         
[info] - should return at least one issue for a valid query             
[info] - should return an empty result for a non existent query string
[info] - should edit the specified issue *** FAILED ***   
[info]   false was not equal to true (BaseIntegrationSpec.scala:47)

BenFradet · 2017-05-23T09:04:14Z

The tests are passing locally for me.

mscharley · 2017-05-23T09:05:14Z

👍

BenFradet · 2017-05-23T09:06:29Z

Merging when green 👍 (or red I should say)

mscharley · 2017-05-23T09:10:29Z

The tests don't seem to work on Travis because I'm not a contributor so Travis valiantly refuses to give me your keys for running tests in case i'm evil and send them on somewhere else. There's a bunch of 401 (auth required) errors in the integration tests as a result.

juanpedromoreno · 2017-05-23T09:12:46Z

Yeah, I'm not sure how could we make it work for project forks. We usually test the PRs locally. And yours are working fine 👍

mscharley added 4 commits May 19, 2017 15:19

Initial pass to add the read-only PR review API.

3db6512

Add inline docs

80839bb

Remove defunct object

a6bf4a3

Add a getReview endpoint

71e9b89

mscharley changed the title ~~Initial pass to add the read-only PR review API.~~ Add support for the PR Review API May 19, 2017

BenFradet reviewed May 19, 2017

View reviewed changes

juanpedromoreno reviewed May 21, 2017

View reviewed changes

mscharley added 2 commits May 22, 2017 09:50

Add the rest of the tests for this issue

da817c3

Review state case class => case object

11408bc

mscharley changed the title ~~Add support for the PR Review API~~ Add support for the read half of the PR Review API May 22, 2017

Add documentation for this PR

ea2934a

juanpedromoreno reviewed May 22, 2017

View reviewed changes

BenFradet reviewed May 22, 2017

View reviewed changes

Fix various issues with the PR

ec8fd76

mscharley commented May 22, 2017

View reviewed changes

BenFradet reviewed May 22, 2017

View reviewed changes

mscharley and others added 2 commits May 22, 2017 21:28

Fix derp in the links

3fe4cf2

Fixes ghost users associated with pull requests, issues and comments

04eb6e4

juanpedromoreno reviewed May 22, 2017

View reviewed changes

mscharley added 2 commits May 22, 2017 22:19

Fix model to allow for ghostly users

48bb78a

Merge branch 'jp-fix-null-users' of github.com:47deg/github4s into 47…

7ac88b5

…degreesGH-133

juanpedromoreno reviewed May 22, 2017

View reviewed changes

BenFradet reviewed May 22, 2017

View reviewed changes

mscharley added 2 commits May 23, 2017 07:19

Fix copy paste woes

abb7a87

Merge branch 'master' of github.com:47deg/github4s into 47degreesGH-133

8c59b89

Another minor testing fix

9784ec6

juanpedromoreno reviewed May 22, 2017

View reviewed changes

Fix a regression that somehow made it in

dbbb7b9

juanpedromoreno approved these changes May 22, 2017

View reviewed changes

BenFradet reviewed May 23, 2017

View reviewed changes

More minor fixes

8652b4b

BenFradet approved these changes May 23, 2017

View reviewed changes

Fix regression

737ac9f

mscharley mentioned this pull request May 23, 2017

Integration tests create a bunch of gists #149

Closed

juanpedromoreno merged commit b8bb8e4 into 47degrees:master May 23, 2017

		@@ -128,6 +130,57 @@ createPullRequestIssue.exec[cats.Id, HttpResponse[String]]() match {

		See [the API doc](https://developer.github.com/v3/pulls/#create-a-pull-request) for full reference.

		## List pull requests


		## Get an individual review

		You can list the reviews for a pull request using `listReviews`, it takes as arguments:


		As an example, if we wanted to see all the reviews for pull request 139 of `47deg/github4s`:

		```scala


		As an example, if we wanted to see review 39355613 for pull request 139 of `47deg/github4s`:

		```scala

		The `result` on the right is the matching [PullRequestReview][pr-scala].


		See [the API doc](https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request) for full reference.


		## List pull request reviews

		You can list the reviews for a pull request using `listReviews`, it takes as arguments:


		## Get an individual review

		You can get an individual review for a pull request using `getReview`, it takes as arguments:

Add support for the read half of the PR Review API #139

Add support for the read half of the PR Review API #139

Conversation

mscharley commented May 19, 2017

BenFradet left a comment

Choose a reason for hiding this comment

juanpedromoreno commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscharley commented May 22, 2017

mscharley commented May 22, 2017

juanpedromoreno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenFradet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pull request

List pull requests

Reviews

List reviews

Get an individual review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanpedromoreno May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenFradet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanpedromoreno commented May 22, 2017

juanpedromoreno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscharley commented May 22, 2017

juanpedromoreno left a comment

Choose a reason for hiding this comment

juanpedromoreno commented May 22, 2017 • edited Loading

BenFradet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenFradet left a comment

Choose a reason for hiding this comment

mscharley commented May 23, 2017 • edited Loading

BenFradet commented May 23, 2017

mscharley commented May 23, 2017

BenFradet commented May 23, 2017 • edited Loading

mscharley commented May 23, 2017

juanpedromoreno commented May 23, 2017 • edited Loading

juanpedromoreno May 22, 2017 •

edited

Loading

juanpedromoreno commented May 22, 2017 •

edited

Loading

mscharley commented May 23, 2017 •

edited

Loading

BenFradet commented May 23, 2017 •

edited

Loading

juanpedromoreno commented May 23, 2017 •

edited

Loading