Implement Review API for Pull Requests (#1648)

* First Iteration Need to finish tests and docs

* Mostly Complete

* Fixing tests and adding review comments

* Added tests for reactive client

* Moved Reviews inside fo the Pull request client for better organization and began initial intigration testing

* Fixing bad recursive function breaking tests

* test fixes

* Add paging support to review comments call

* Fixing recursive function

* Addressing comments from PR

* fixing CI break

* Typo build break

* Fixing Convention Tests

* Adding correct nameof() usage in Ensure

* Small consitancy changes

* Trigger build

* Address PR Comments

* Fixup test naming

* Fix sub client ordering and incorrect URL

* Tidy up comments and remove StringEnum wrapper from Request models as it is only for Response models

* Rename GetReview to Get

* tweak debugger display

* Rework integration tests - implement the easy Get/GetAll ones first...

* Implement integration tests for Create method.
Move helpers to create PR/review into SetupHelper class
Fixed up review status enum to contain correct values
Tests for Approve/RequestChanges currently failing as a user cant approve/request changes on their own PR

* Implement secondary account settings for integration tests and a new [DualAccountTest] attribute for discovery when configured
Change integration test to create PR from the 2nd account, so the main test account is able to perform review actions on the PR

* Add integration tests for Delete, Dismiss and Submit methods
Fixed up API client implementation for delete (was looking for incorrect 201 http status)
Removed unnecessary await/async calls from client implementations that dont need to do anything with the result

* Attempting to add comments as part of a review revealed that we cant use the existing PullRequestReviewCommentCreate class as the API throws a validation error due to the CommitId field
These newer review APIs need a DraftPullRequestReviewComment (that doesnt have a commitId) instead

* add second test account user/password to configure-integration-tests script
This commit is contained in:
Travis Harris
2017-08-16 03:50:25 -07:00
committed by Ryan Gribble
parent 7c170213fd
commit ff9521ce3d
26 changed files with 3381 additions and 78 deletions
+43 -37
View File
@@ -9,12 +9,13 @@ namespace Octokit
/// A client for GitHub's Pull Requests API.
/// </summary>
/// <remarks>
/// See the <a href="http://developer.github.com/v3/activity/notifications/">Pull Requests API documentation</a> for more information.
/// See the <a href="https://developer.github.com/v3/pulls/">Pull Requests API documentation</a> for more information.
/// </remarks>
public class PullRequestsClient : ApiClient, IPullRequestsClient
{
public PullRequestsClient(IApiConnection apiConnection) : base(apiConnection)
{
Review = new PullRequestReviewsClient(apiConnection);
ReviewComment = new PullRequestReviewCommentsClient(apiConnection);
ReviewRequest = new PullRequestReviewRequestsClient(apiConnection);
}
@@ -25,6 +26,11 @@ namespace Octokit
[Obsolete("Please use PullRequestsClient.ReviewComment instead. This method will be removed in a future version")]
public IPullRequestReviewCommentsClient Comment { get { return this.ReviewComment; } }
/// <summary>
/// Client for managing reviews.
/// </summary>
public IPullRequestReviewsClient Review { get; set; }
/// <summary>
/// Client for managing review comments.
/// </summary>
@@ -43,8 +49,8 @@ namespace Octokit
/// </remarks>
public Task<PullRequest> Get(string owner, string name, int number)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
return ApiConnection.Get<PullRequest>(ApiUrls.PullRequest(owner, name, number));
}
@@ -70,8 +76,8 @@ namespace Octokit
/// <param name="name">The name of the repository</param>
public Task<IReadOnlyList<PullRequest>> GetAllForRepository(string owner, string name)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
return GetAllForRepository(owner, name, new PullRequestRequest(), ApiOptions.None);
}
@@ -99,9 +105,9 @@ namespace Octokit
/// <param name="options">Options for changing the API response</param>
public Task<IReadOnlyList<PullRequest>> GetAllForRepository(string owner, string name, ApiOptions options)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNull(options, "options");
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
Ensure.ArgumentNotNull(options, nameof(options));
return GetAllForRepository(owner, name, new PullRequestRequest(), options);
}
@@ -116,7 +122,7 @@ namespace Octokit
/// <param name="options">Options for changing the API response</param>
public Task<IReadOnlyList<PullRequest>> GetAllForRepository(long repositoryId, ApiOptions options)
{
Ensure.ArgumentNotNull(options, "options");
Ensure.ArgumentNotNull(options, nameof(options));
return GetAllForRepository(repositoryId, new PullRequestRequest(), options);
}
@@ -132,9 +138,9 @@ namespace Octokit
/// <param name="request">Used to filter and sort the list of pull requests returned</param>
public Task<IReadOnlyList<PullRequest>> GetAllForRepository(string owner, string name, PullRequestRequest request)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNull(request, "request");
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
Ensure.ArgumentNotNull(request, nameof(request));
return GetAllForRepository(owner, name, request, ApiOptions.None);
}
@@ -149,7 +155,7 @@ namespace Octokit
/// <param name="request">Used to filter and sort the list of pull requests returned</param>
public Task<IReadOnlyList<PullRequest>> GetAllForRepository(long repositoryId, PullRequestRequest request)
{
Ensure.ArgumentNotNull(request, "request");
Ensure.ArgumentNotNull(request, nameof(request));
return GetAllForRepository(repositoryId, request, ApiOptions.None);
}
@@ -166,10 +172,10 @@ namespace Octokit
/// <param name="options">Options for changing the API response</param>
public Task<IReadOnlyList<PullRequest>> GetAllForRepository(string owner, string name, PullRequestRequest request, ApiOptions options)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNull(request, "request");
Ensure.ArgumentNotNull(options, "options");
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
Ensure.ArgumentNotNull(request, nameof(request));
Ensure.ArgumentNotNull(options, nameof(options));
return ApiConnection.GetAll<PullRequest>(ApiUrls.PullRequests(owner, name),
request.ToParametersDictionary(), options);
@@ -186,8 +192,8 @@ namespace Octokit
/// <param name="options">Options for changing the API response</param>
public Task<IReadOnlyList<PullRequest>> GetAllForRepository(long repositoryId, PullRequestRequest request, ApiOptions options)
{
Ensure.ArgumentNotNull(request, "request");
Ensure.ArgumentNotNull(options, "options");
Ensure.ArgumentNotNull(request, nameof(request));
Ensure.ArgumentNotNull(options, nameof(options));
return ApiConnection.GetAll<PullRequest>(ApiUrls.PullRequests(repositoryId),
request.ToParametersDictionary(), options);
@@ -202,9 +208,9 @@ namespace Octokit
/// <param name="newPullRequest">A <see cref="NewPullRequest"/> instance describing the new PullRequest to create</param>
public Task<PullRequest> Create(string owner, string name, NewPullRequest newPullRequest)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNull(newPullRequest, "newPullRequest");
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
Ensure.ArgumentNotNull(newPullRequest, nameof(newPullRequest));
return ApiConnection.Post<PullRequest>(ApiUrls.PullRequests(owner, name), newPullRequest);
}
@@ -217,7 +223,7 @@ namespace Octokit
/// <param name="newPullRequest">A <see cref="NewPullRequest"/> instance describing the new PullRequest to create</param>
public Task<PullRequest> Create(long repositoryId, NewPullRequest newPullRequest)
{
Ensure.ArgumentNotNull(newPullRequest, "newPullRequest");
Ensure.ArgumentNotNull(newPullRequest, nameof(newPullRequest));
return ApiConnection.Post<PullRequest>(ApiUrls.PullRequests(repositoryId), newPullRequest);
}
@@ -233,9 +239,9 @@ namespace Octokit
/// </param>
public Task<PullRequest> Update(string owner, string name, int number, PullRequestUpdate pullRequestUpdate)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNull(pullRequestUpdate, "pullRequestUpdate");
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
Ensure.ArgumentNotNull(pullRequestUpdate, nameof(pullRequestUpdate));
return ApiConnection.Patch<PullRequest>(ApiUrls.PullRequest(owner, name, number), pullRequestUpdate);
}
@@ -250,7 +256,7 @@ namespace Octokit
/// </param>
public Task<PullRequest> Update(long repositoryId, int number, PullRequestUpdate pullRequestUpdate)
{
Ensure.ArgumentNotNull(pullRequestUpdate, "pullRequestUpdate");
Ensure.ArgumentNotNull(pullRequestUpdate, nameof(pullRequestUpdate));
return ApiConnection.Patch<PullRequest>(ApiUrls.PullRequest(repositoryId, number), pullRequestUpdate);
}
@@ -265,9 +271,9 @@ namespace Octokit
/// <param name="mergePullRequest">A <see cref="MergePullRequest"/> instance describing a pull request merge</param>
public async Task<PullRequestMerge> Merge(string owner, string name, int number, MergePullRequest mergePullRequest)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNull(mergePullRequest, "mergePullRequest");
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
Ensure.ArgumentNotNull(mergePullRequest, nameof(mergePullRequest));
try
{
@@ -299,7 +305,7 @@ namespace Octokit
/// <param name="mergePullRequest">A <see cref="MergePullRequest"/> instance describing a pull request merge</param>
public async Task<PullRequestMerge> Merge(long repositoryId, int number, MergePullRequest mergePullRequest)
{
Ensure.ArgumentNotNull(mergePullRequest, "mergePullRequest");
Ensure.ArgumentNotNull(mergePullRequest, nameof(mergePullRequest));
try
{
@@ -331,8 +337,8 @@ namespace Octokit
/// <param name="number">The pull request number</param>
public async Task<bool> Merged(string owner, string name, int number)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
try
{
@@ -375,8 +381,8 @@ namespace Octokit
/// <param name="number">The pull request number</param>
public Task<IReadOnlyList<PullRequestCommit>> Commits(string owner, string name, int number)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
return ApiConnection.GetAll<PullRequestCommit>(ApiUrls.PullRequestCommits(owner, name, number));
}
@@ -401,8 +407,8 @@ namespace Octokit
/// <param name="number">The pull request number</param>
public Task<IReadOnlyList<PullRequestFile>> Files(string owner, string name, int number)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
return ApiConnection.GetAll<PullRequestFile>(ApiUrls.PullRequestFiles(owner, name, number));
}