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 Pull Request Review Request API. #1588

Merged
merged 25 commits into from
May 14, 2017
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ce66988
Add Pull Request Review Request API.
gdziadkiewicz Apr 6, 2017
1e3512f
Merge branch 'master' into PullRequestReviewRequestAPI
gdziadkiewicz Apr 19, 2017
2624676
Add Reactive Pull Request Review Request API.
gdziadkiewicz Apr 20, 2017
91ada35
Add PullRequestReviewRequestClient tests.
gdziadkiewicz Apr 21, 2017
5169aec
Add ObservablePullRequestReviewRequestClient tests.
gdziadkiewicz Apr 21, 2017
36e6f02
Fix sub-client property naming.
gdziadkiewicz Apr 23, 2017
55deff2
Remove redundant model and update PullRequest model.
gdziadkiewicz Apr 26, 2017
896cfae
Add repositoryId based methods and missing Observable documentation.
gdziadkiewicz Apr 26, 2017
59056ec
Add missing parameter to PullRequest ctor.
Apr 27, 2017
cae3701
Merge branch 'master' into PullRequestReviewRequestAPI
gdziadkiewicz May 5, 2017
bc9a8d0
Add integration tests for PullRequestReviewRequest.
gdziadkiewicz May 6, 2017
7cf3331
Upgrade PullRequestReviewRequest integration tests.
gdziadkiewicz May 6, 2017
2ee3be3
Add integration tests for repositoryId methods and fix url bug.
gdziadkiewicz May 7, 2017
0be9f19
Add missing unit tests and fix PR issues.
gdziadkiewicz May 7, 2017
995d4d8
Add pagination support for PullRequestReviewRequst.GetAll and tests f…
gdziadkiewicz May 7, 2017
fab5a03
Merge branch 'master' into PullRequestReviewRequestAPI
gdziadkiewicz May 8, 2017
bfcd1de
Revert changes on `PullRequestReviewCommentsClientTests.cs`
gdziadkiewicz May 8, 2017
4b348be
Small upgrades - remove unused using and compress property to express…
gdziadkiewicz May 8, 2017
b62601a
Revert use of expression body in property.
gdziadkiewicz May 8, 2017
802022e
Add pagination tests for PullRequestReviewRequest.GetAll.
gdziadkiewicz May 9, 2017
d6b7161
Change pagination tests to use 2 users.
May 11, 2017
5c9d9da
Correct class/file name
ryangribble May 11, 2017
a093315
Reword the integration test names for consistency
ryangribble May 11, 2017
57d0d49
Fix DebuggerDisplay of requested reviewers
ryangribble May 12, 2017
e194682
fix reviewRequestToCreate parameter to be consistent
ryangribble May 12, 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
using System;
using System.Collections.Generic;
using System.Reactive;
using System.Threading.Tasks;

namespace Octokit.Reactive
{
/// <summary>
/// A client for GitHub's Pull Request Review Requests API.
/// </summary>
/// <remarks>
/// See the <a href="https://developer.github.com/v3/pulls/review_requests/">Review Requests API documentation</a> for more information.
/// </remarks>
public interface IObservablePullRequestReviewRequestsClient
{
/// <summary>
/// Gets review requests for a specified pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#list-review-requests</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The pull request number</param>
IObservable<User> GetAll(string owner, string name, int number);

/// <summary>
/// Gets review requests for a specified pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#list-review-requests</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="number">The pull request number</param>
IObservable<User> GetAll(long repositoryId, int number);

/// <summary>
/// Creates review requests on a pull request for specified users.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#create-a-review-request</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The Pull Request number</param>
/// <param name="users">List of logins of user will be requested for review</param>
IObservable<PullRequest> Create(string owner, string name, int number, PullRequestReviewRequest users);

/// <summary>
/// Creates review requests on a pull request for specified users.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#create-a-review-request</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="number">The Pull Request number</param>
/// <param name="users">List of logins of user will be requested for review</param>
IObservable<PullRequest> Create(long repositoryId, int number, PullRequestReviewRequest users);

/// <summary>
/// Deletes review request for given users on a pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#delete-a-review-request</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The pull request review comment number</param>
/// <param name="users">List of logins of users that will be not longer requested for review</param>
IObservable<Unit> Delete(string owner, string name, int number, PullRequestReviewRequest users);

/// <summary>
/// Deletes review request for given users on a pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#delete-a-review-request</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="number">The pull request review comment number</param>
/// <param name="users">List of logins of users that will be not longer requested for review</param>
IObservable<Unit> Delete(long repositoryId, int number, PullRequestReviewRequest users);
}
}
5 changes: 5 additions & 0 deletions Octokit.Reactive/Clients/IObservablePullRequestsClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ public interface IObservablePullRequestsClient
/// </summary>
IObservablePullRequestReviewCommentsClient ReviewComment { get; }

/// <summary>
/// Client for managing review requests.
/// </summary>
IObservablePullRequestReviewRequestsClient ReviewRequest { get; }

/// <summary>
/// Gets a single Pull Request by number.
/// </summary>
Expand Down
109 changes: 109 additions & 0 deletions Octokit.Reactive/Clients/ObservablePullRequestReviewRequestsClient.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
using System;
using System.Reactive;
using System.Reactive.Threading.Tasks;
using Octokit.Reactive.Internal;

namespace Octokit.Reactive
{
public class ObservablePullRequestReviewRequestsClient : IObservablePullRequestReviewRequestsClient
{
readonly IPullRequestReviewRequestsClient _client;
readonly IConnection _connection;

public ObservablePullRequestReviewRequestsClient(IGitHubClient client)
{
Ensure.ArgumentNotNull(client, "client");

_client = client.PullRequest.ReviewRequest;
_connection = client.Connection;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI that we add the xmldoc comments to the interfaces and the client implementations, so once you are done with the changes please add the xmldoc comments to all the conrete methods as well 👍

/// <summary>
/// Gets review requests for a specified pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#list-review-requests</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The pull request number</param>
public IObservable<User> GetAll(string owner, string name, int number)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");

return _connection.GetAndFlattenAllPages<User>(ApiUrls.PullRequestReviewRequests(owner, name, number), null, AcceptHeaders.PullRequestReviewsApiPreview);
}

/// <summary>
/// Gets review requests for a specified pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#list-review-requests</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="number">The pull request number</param>
public IObservable<User> GetAll(long repositoryId, int number)
{
return _connection.GetAndFlattenAllPages<User>(ApiUrls.PullRequestReviewRequests(repositoryId, number), null, AcceptHeaders.PullRequestReviewsApiPreview);
}

/// <summary>
/// Creates review requests on a pull request for specified users.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#create-a-review-request</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The Pull Request number</param>
/// <param name="users">List of logins of user will be requested for review</param>
public IObservable<PullRequest> Create(string owner, string name, int number, PullRequestReviewRequest users)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNull(users, "users");

return _client.Create(owner, name, number, users).ToObservable();
}

/// <summary>
/// Creates review requests on a pull request for specified users.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#create-a-review-request</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="number">The Pull Request number</param>
/// <param name="users">List of logins of user will be requested for review</param>
public IObservable<PullRequest> Create(long repositoryId, int number, PullRequestReviewRequest users)
{
Ensure.ArgumentNotNull(users, "users");

return _client.Create(repositoryId, number, users).ToObservable();
}

/// <summary>
/// Deletes review request for given users on a pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#delete-a-review-request</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The pull request review comment number</param>
/// <param name="users">List of logins of users that will be not longer requested for review</param>
public IObservable<Unit> Delete(string owner, string name, int number, PullRequestReviewRequest users)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNull(users, "users");

return _client.Delete(owner, name, number, users).ToObservable();
}

/// <summary>
/// Deletes review request for given users on a pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/review_requests/#delete-a-review-request</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="number">The pull request review comment number</param>
/// <param name="users">List of logins of users that will be not longer requested for review</param>
public IObservable<Unit> Delete(long repositoryId, int number, PullRequestReviewRequest users)
{
Ensure.ArgumentNotNull(users, "users");

return _client.Delete(repositoryId, number, users).ToObservable();
}
}
}
6 changes: 6 additions & 0 deletions Octokit.Reactive/Clients/ObservablePullRequestsClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,19 @@ public class ObservablePullRequestsClient : IObservablePullRequestsClient
/// </summary>
public IObservablePullRequestReviewCommentsClient ReviewComment { get; private set; }

/// <summary>
/// Client for managing review requests.
/// </summary>
public IObservablePullRequestReviewRequestsClient ReviewRequest { get; private set; }

public ObservablePullRequestsClient(IGitHubClient client)
{
Ensure.ArgumentNotNull(client, "client");

_client = client.Repository.PullRequest;
_connection = client.Connection;
ReviewComment = new ObservablePullRequestReviewCommentsClient(client);
ReviewRequest = new ObservablePullRequestReviewRequestsClient(client);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Octokit;
using Octokit.Tests.Integration;
using Octokit.Tests.Integration.Helpers;
using Xunit;

public class PullRequestsReviewRequestClientTests : IDisposable
{
private readonly IGitHubClient _github;
private readonly IPullRequestReviewRequestsClient _client;
private readonly RepositoryContext _context;

const string branchName = "new-branch";
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be a class level const since it's only used in 1 place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inlined it.

const string collaboratorLogin = "m-zuber-octokit-integration-tests";
Copy link
Contributor

Choose a reason for hiding this comment

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

In other integration tests we use "octocat" as our contributor user, so it'd be good to use it here too. The added bonus is that M-Zuber would be able to run these tests lol 😛

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 change it :) But in my defense: I've take it from RepositoryCollaboratorClientTests.cs and ran the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

But, but, then I won't know when someone runs the integration tests 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha 😀

yeah as I said there are a fair few inconsistencies with how integration tests are being done across the code base so I just try to focus on new integration tests being done in the preferred way, and perhaps one day we could even go back and restyle the existing ones!

const string path = "CONTRIBUTING.md";
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto with this one, if it's not re-used multiple times it doesn't need to be a class level const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inlined it.


public PullRequestsReviewRequestClientTests()
{
_github = Helper.GetAuthenticatedClient();

_client = _github.PullRequest.ReviewRequest;

_context = _github.CreateRepositoryContext("test-repo").Result;
_github.Repository.Collaborator.Add(Helper.UserName, _context.RepositoryName, collaboratorLogin);
Copy link
Contributor

Choose a reason for hiding this comment

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

When refering to the created RepositoryContext I'd prefer we use _context.RepositoryOwner rather than Helper.UserName - just to keep from making assumptions about the internal workings of repository context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it.

}

[IntegrationTest]
public async Task CanGetAllWhenNone()
{
var pullRequest = await CreatePullRequest(_context);

var reviewRequests = await _client.GetAll(Helper.UserName, _context.RepositoryName, pullRequest.Number);

Assert.NotNull(reviewRequests);
Assert.Empty(reviewRequests);
}

[IntegrationTest]
public async Task CanCreateAndThenGetAll()
{
var pullRequest = await CreatePullRequest(_context);
var reviewers = new List<string> { collaboratorLogin };
var reviewRequestToCreate = new PullRequestReviewRequest(reviewers);

await _client.Create(Helper.UserName, _context.RepositoryName, pullRequest.Number, reviewRequestToCreate);
var reviewRequests = await _client.GetAll(Helper.UserName, _context.RepositoryName, pullRequest.Number);

Assert.Equal(reviewers, reviewRequests.Select(rr => rr.Login));
}

[IntegrationTest]
public async Task CanCreate()
{
var pullRequest = await CreatePullRequest(_context);
var reviewers = new List<string> { collaboratorLogin };
var reviewRequestToCreate = new PullRequestReviewRequest(reviewers);

var pr = await _client.Create(Helper.UserName, _context.RepositoryName, pullRequest.Number, reviewRequestToCreate);

Assert.Equal(reviewers, pr.RequestedReviewers.Select(rr => rr.Login));
}

[IntegrationTest]
public async Task CanCreateAndDelete()
{
var pullRequest = await CreatePullRequest(_context);
var reviewers = new List<string> { collaboratorLogin };
var reviewRequestToCreate = new PullRequestReviewRequest(reviewers);

await _client.Create(Helper.UserName, _context.RepositoryName, pullRequest.Number, reviewRequestToCreate);
var reviewRequestsAfterCreate = await _client.GetAll(Helper.UserName, _context.RepositoryName, pullRequest.Number);
await _client.Delete(Helper.UserName, _context.RepositoryName, pullRequest.Number, reviewRequestToCreate);
var reviewRequestsAfterDelete = await _client.GetAll(Helper.UserName, _context.RepositoryName, pullRequest.Number);

Assert.Equal(reviewers, reviewRequestsAfterCreate.Select(rr => rr.Login));
Assert.Empty(reviewRequestsAfterDelete);
}

public void Dispose()
{
_context.Dispose();
}

/// <summary>
/// Creates the base state for testing (creates a repo, a commit in master, a branch, a commit in the branch and a pull request)
/// </summary>
/// <returns></returns>
async Task<PullRequestData> CreatePullRequest(RepositoryContext context, string branch = branchName)
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency it would be nice to create the base state using the same process/code that the CreateTheWorld() functions in the PullRequestsClientTests use: https://github.com/octokit/octokit.net/blob/master/Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs#L931-L993

That way we have consistency in how we create Pull Requests for integration tests

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've changed the method name and it's code to be like PullRequestClientTests.CreateTheWorld, but without second branch creation and with baked-in pull request creation.

{
string branchHead = "heads/" + branch;
string branchRef = "refs/" + branchHead;

var repoName = context.RepositoryName;

// Creating a commit in master

var createdCommitInMaster = await CreateCommit(repoName, "Hello World!", "README.md", "heads/master", "A master commit message");

// Creating a branch

var newBranch = new NewReference(branchRef, createdCommitInMaster.Sha);
await _github.Git.Reference.Create(Helper.UserName, repoName, newBranch);

// Creating a commit in the branch

var createdCommitInBranch = await CreateCommit(repoName, "Hello from the fork!", path, branchHead, "A branch commit message");

// Creating a pull request

var pullRequest = new NewPullRequest("Nice title for the pull request", branch, "master");
var createdPullRequest = await _github.PullRequest.Create(Helper.UserName, repoName, pullRequest);

var data = new PullRequestData
{
Sha = createdCommitInBranch.Sha,
Number = createdPullRequest.Number
};

return data;
}

async Task<Commit> CreateCommit(string repoName, string blobContent, string treePath, string reference, string commitMessage)
{
// Creating a blob
var blob = new NewBlob
{
Content = blobContent,
Encoding = EncodingType.Utf8
};

var createdBlob = await _github.Git.Blob.Create(Helper.UserName, repoName, blob);

// Creating a tree
var newTree = new NewTree();
newTree.Tree.Add(new NewTreeItem
{
Type = TreeType.Blob,
Mode = FileMode.File,
Path = treePath,
Sha = createdBlob.Sha
});

var createdTree = await _github.Git.Tree.Create(Helper.UserName, repoName, newTree);
var treeSha = createdTree.Sha;

// Creating a commit
var parent = await _github.Git.Reference.Get(Helper.UserName, repoName, reference);
var commit = new NewCommit(commitMessage, treeSha, parent.Object.Sha);

var createdCommit = await _github.Git.Commit.Create(Helper.UserName, repoName, commit);
await _github.Git.Reference.Update(Helper.UserName, repoName, reference, new ReferenceUpdate(createdCommit.Sha));

return createdCommit;
}

class PullRequestData
{
public int Number { get; set; }
public string Sha { get; set; }
}
}
Loading