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

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

Merged
merged 18 commits into from
May 23, 2017
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
56 changes: 56 additions & 0 deletions docs/src/main/tut/pull_request.md
Original file line number Diff line number Diff line change
Expand Up @@ -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-pull-request-reviews)
- [Get a review](#get-an-individual-review)

The following examples assume the following imports and token:

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

# Review API

## List pull request reviews

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

Choose a reason for hiding this comment

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

reflecting #146:
listReviews; it takes


- the repository coordinates (`owner` and `name` of the repository).
- the pull request id.

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

```tut:silent
val listReviews = Github(accessToken).pullRequests.listReviews(
"47deg",
"github4s",
139)

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

Choose a reason for hiding this comment

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

should be:

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

case Right(r) => println(r.result)
}
```

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

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

## Get an individual review

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

Choose a reason for hiding this comment

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

same


- the repository coordinates (`owner` and `name` of the repository).
- the pull request id.
- the review id.

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

```tut:silent
val review = Github(accessToken).pullRequests.getReview(
"47deg",
"github4s",
139,
39355613)

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

Choose a reason for hiding this comment

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

same

case Right(r) => println(r.result)
}
```

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

See [the API doc](https://developer.github.com/v3/pulls/reviews/#get-a-single-review) for full reference.

As you can see, a few features of the pull request endpoint are missing. As a result, if you'd like
to see a feature supported, feel free to create an issue and/or a pull request!

Expand Down
42 changes: 42 additions & 0 deletions github4s/jvm/src/test/scala/github4s/unit/ApiSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,48 @@ class ApiSpec
response should be('left)
}

"PullRequests >> List PullRequestReviews" should "return a list of reviews when valid data is provided" in {
val response = pullRequests.listReviews(
accessToken,
headerUserAgent,
validRepoOwner,
validRepoName,
validPullRequestNumber)
response should be('right)
}

it should "return an error when invalid data is passed" in {
val response = pullRequests.listReviews(
accessToken,
headerUserAgent,
validRepoOwner,
invalidRepoName,
validPullRequestNumber)
response should be('left)
}

"PullRequests >> Get PullRequestReview" should "return a single review when valid data is provided" in {
val response = pullRequests.getReview(
accessToken,
headerUserAgent,
validRepoOwner,
validRepoName,
validPullRequestNumber,
validPullRequestReviewNumber)
response should be('right)
}

it should "return an error when invalid data is passed" in {
val response = pullRequests.getReview(
accessToken,
headerUserAgent,
validRepoOwner,
invalidRepoName,
validPullRequestNumber,
validPullRequestReviewNumber)
response should be('left)
}

"Issues >> List" should "return the expected issues when a valid owner/repo is provided" in {
val response =
issues.list(accessToken, headerUserAgent, validRepoOwner, validRepoName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -598,29 +598,65 @@ trait MockGithubApiServer extends MockServerService with FakeResponses with Test
""".stripMargin)))
.respond(response.withStatusCode(createdStatusCode).withBody(validCreatePullRequest))

//Issues >> list
//PullRequests >> listReviews
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I screwed something up big here.

mockServer
.when(
request
.withMethod("GET")
.withPath(s"/repos/$validRepoOwner/$validRepoName/issues")
.withPath(s"/repos/$validRepoOwner/$validRepoName/pulls/$validPullRequestNumber/reviews")
.withHeader("Authorization", tokenHeader))
.respond(response.withStatusCode(okStatusCode).withBody(listIssuesValidResponse))
.respond(response.withStatusCode(okStatusCode).withBody(listReviewsValidResponse))

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still unused


mockServer
.when(
request
.withMethod("GET")
.withPath(s"/repos/$validRepoOwner/$invalidRepoName/pulls/$validPullRequestNumber/reviews")
.withHeader("Authorization", tokenHeader))
.respond(response.withStatusCode(notFoundStatusCode).withBody(notFoundResponse))

//PullRequests >> getReview
mockServer
.when(
request
.withMethod("GET")
.withPath(
s"/repos/$validRepoOwner/$validRepoName/pulls/$validPullRequestNumber/reviews/$validPullRequestReviewNumber")
.withHeader("Authorization", tokenHeader))
.respond(response.withStatusCode(okStatusCode).withBody(getReviewValidResponse))

mockServer
.when(
request
.withMethod("GET")
.withPath(
s"/repos/$validRepoOwner/$invalidRepoName/pulls/$validPullRequestNumber/reviews/$validPullRequestReviewNumber")
.withHeader("Authorization", tokenHeader))
.respond(response.withStatusCode(notFoundStatusCode).withBody(notFoundResponse))

//Issues >> list
mockServer
.when(
request
.withMethod("GET")
.withPath(s"/repos/$validRepoOwner/$validRepoName/issues")
.withHeader("Authorization", tokenHeader))
.respond(response.withStatusCode(okStatusCode).withBody(listIssuesValidResponse))

mockServer
.when(
request
.withMethod("GET")
.withPath(s"/repos/$validRepoOwner/$invalidRepoName/issues")
.withHeader(not("Authorization")))
.withHeader("Authorization", tokenHeader))
.respond(response.withStatusCode(notFoundStatusCode).withBody(notFoundResponse))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


//Issues >> create
Expand All @@ -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))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where did this come from? This should fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you sort this out in the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

.respond(response.withStatusCode(unauthorizedStatusCode).withBody(unauthorizedResponse))

mockServer
Expand Down
8 changes: 8 additions & 0 deletions github4s/shared/src/main/scala/github4s/Decoders.scala
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,14 @@ object Decoders {
}
}

implicit val decodePrrStatus: Decoder[PullRequestReviewState] =
Decoder.decodeString.map {
case "APPROVE" => PRRStateApprove
case "REQUEST_CHANGES" => PRRStateRequestChanges
case "COMMENTED" => PRRStateCommented
case "PENDING" => PRRStatePending
}

implicit val decodeGist: Decoder[Gist] = Decoder.instance { c ⇒
for {
url ← c.downField("url").as[String]
Expand Down
2 changes: 2 additions & 0 deletions github4s/shared/src/main/scala/github4s/Encoders.scala
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,6 @@ object Encoders {
case d: CreatePullRequestData => d.asJson
case d: CreatePullRequestIssue => d.asJson
}

implicit val encodePrrStatus: Encoder[PullRequestReviewState] = Encoder.encodeString.contramap(_.value)
}
15 changes: 15 additions & 0 deletions github4s/shared/src/main/scala/github4s/GithubAPIs.scala
Original file line number Diff line number Diff line change
Expand Up @@ -315,4 +315,19 @@ class GHPullRequests(accessToken: Option[String] = None)(implicit O: PullRequest
maintainerCanModify: Option[Boolean] = Some(true)
): GHIO[GHResponse[PullRequest]] =
O.createPullRequest(owner, repo, newPullRequest, head, base, maintainerCanModify, accessToken)

def listReviews(
owner: String,
repo: String,
pullRequest: Int
): GHIO[GHResponse[List[PullRequestReview]]] =
O.listPullRequestReviews(owner, repo, pullRequest, accessToken)

def getReview(
owner: String,
repo: String,
pullRequest: Int,
review: Int
): GHIO[GHResponse[PullRequestReview]] =
O.getPullRequestReview(owner, repo, pullRequest, review, accessToken)
}
38 changes: 38 additions & 0 deletions github4s/shared/src/main/scala/github4s/api/PullRequests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -118,4 +118,42 @@ class PullRequests[C, M[_]](
httpClient
.post[PullRequest](accessToken, s"repos/$owner/$repo/pulls", headers, data.asJson.noSpaces)
}

/**
* List pull request reviews.
*
* @param accessToken Token to identify the authenticated user
* @param headers Optional user header to include in the request
* @param owner Owner of the repo
* @param repo Name of the repo
* @param pullRequest ID number of the PR to get reviews for.
*/
def listReviews(
accessToken: Option[String] = None,
headers: Map[String, String] = Map(),
owner: String,
repo: String,
pullRequest: Int): M[GHResponse[List[PullRequestReview]]] = {
httpClient.get[List[PullRequestReview]](accessToken, s"repos/$owner/$repo/pulls/$pullRequest/reviews", headers)
}

/**
* Get a specific pull request review.
*
* @param accessToken Token to identify the authenticated user
* @param headers Optional user header to include in the request
* @param owner Owner of the repo
* @param repo Name of the repo
* @param pullRequest ID number of the PR to get reviews for
* @param review ID number of the review to retrieve.
*/
def getReview(
accessToken: Option[String] = None,
headers: Map[String, String] = Map(),
owner: String,
repo: String,
pullRequest: Int,
review: Int): M[GHResponse[PullRequestReview]] = {
httpClient.get[PullRequestReview](accessToken, s"repos/$owner/$repo/pulls/$pullRequest/reviews/$review", headers)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,21 @@ final case class CreatePullRequest(
accessToken: Option[String] = None
) extends PullRequestOp[GHResponse[PullRequest]]

final case class ListPullRequestReviews(
owner: String,
repo: String,
pullRequest: Int,
accessToken: Option[String] = None
) extends PullRequestOp[GHResponse[List[PullRequestReview]]]

final case class GetPullRequestReview(
owner: String,
repo: String,
pullRequest: Int,
review: Int,
accessToken: Option[String] = None
) extends PullRequestOp[GHResponse[PullRequestReview]]

/**
* Exposes Pull Request operations as a Free monadic algebra that may be combined with other
* Algebras via Coproduct
Expand Down Expand Up @@ -83,6 +98,20 @@ class PullRequestOps[F[_]](implicit I: Inject[PullRequestOp, F]) {
Free.inject[PullRequestOp, F](
CreatePullRequest(owner, repo, newPullRequest, head, base, maintainerCanModify, accessToken))

def listPullRequestReviews(
owner: String,
repo: String,
pullRequest: Int,
accessToken: Option[String] = None): Free[F, GHResponse[List[PullRequestReview]]] =
Free.inject[PullRequestOp, F](ListPullRequestReviews(owner, repo, pullRequest, accessToken))

def getPullRequestReview(
owner: String,
repo: String,
pullRequest: Int,
review: Int,
accessToken: Option[String] = None): Free[F, GHResponse[PullRequestReview]] =
Free.inject[PullRequestOp, F](GetPullRequestReview(owner, repo, pullRequest, review, accessToken))
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ case class Issue(
html_url: String,
number: Int,
state: String,
user: User,
user: Option[User],
assignee: Option[User],
labels: List[Label] = List.empty,
locked: Option[Boolean],
Expand Down Expand Up @@ -73,7 +73,7 @@ case class Comment(
url: String,
html_url: String,
body: String,
user: User,
user: Option[User],
created_at: String,
updated_at: String)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ case class PullRequestBase(
label: Option[String],
ref: String,
sha: String,
user: User,
user: Option[User],
repo: Option[Repository])

case class PullRequestFile(
Expand Down Expand Up @@ -101,3 +101,18 @@ case object PRFilterOrderDesc extends PRFilterDirection("desc")
sealed trait NewPullRequest
case class NewPullRequestData(title: String, body: String) extends NewPullRequest
case class NewPullRequestIssue(issue: Int) extends NewPullRequest

case class PullRequestReview(
id: Int,
user: Option[User],
body: String,
commit_id: String,
state: PullRequestReviewState,
html_url: String,
pull_request_url: String)

sealed abstract class PullRequestReviewState(val value: String)
case object PRRStateApprove extends PullRequestReviewState("APPROVE")
case object PRRStateRequestChanges extends PullRequestReviewState("REQUEST_CHANGES")
case object PRRStateCommented extends PullRequestReviewState("COMMENTED")
case object PRRStatePending extends PullRequestReviewState("PENDING")
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,10 @@ class Interpreters[M[_], C](
head,
base,
maintainerCanModify)
case ListPullRequestReviews(owner, repo, pullRequest, accessToken) ⇒
pullRequests.listReviews(accessToken, headers, owner, repo, pullRequest)
case GetPullRequestReview(owner, repo, pullRequest, review, accessToken) ⇒
pullRequests.getReview(accessToken, headers, owner, repo, pullRequest, review)
}
}
}
Expand Down
Loading