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.

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

Implemented Lock/Unlock Functionality for Issues #1185

Merged
merged 8 commits into from
Apr 5, 2016
+311 −7
Merged

Implemented Lock/Unlock Functionality for Issues #1185

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
171b3c5
Implement Lock/Unlock Issue
prayankmathur Mar 25, 2016
2c167bf
Fixes
prayankmathur Mar 25, 2016
8ff574a
Changes in Accept headers as well as names of the functions
prayankmathur Mar 29, 2016
63bd122
Resolved conflicts
prayankmathur Mar 29, 2016
55f4e72
Changed function names in unit tests
prayankmathur Mar 29, 2016
5173ade
Some more changes in tests
prayankmathur Mar 29, 2016
8e49f04
Added implementation for delete that takes custom media type header a…
prayankmathur Mar 30, 2016
7324039
Minor fixes
prayankmathur Mar 30, 2016
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
23 changes: 22 additions & 1 deletion Octokit.Reactive/Clients/IObservableIssuesClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Diagnostics.CodeAnalysis;
using System.Reactive;

namespace Octokit.Reactive
{
Expand Down Expand Up @@ -156,5 +157,25 @@ public interface IObservableIssuesClient
/// </param>
/// <returns></returns>
IObservable<Issue> Update(string owner, string name, int number, IssueUpdate issueUpdate);

/// <summary>
/// Locks an issue for the specified repository. Issue owners and users with push access can lock an issue.
/// </summary>
/// <remarks>https://developer.github.com/v3/issues/#lock-an-issue</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The issue number</param>
/// <returns></returns>
IObservable<Issue> Lock(string owner, string name, int number);

/// <summary>
/// Unlocks an issue for the specified repository. Issue owners and users with push access can unlock an issue.
/// </summary>
/// <remarks>https://developer.github.com/v3/issues/#unlock-an-issue</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The issue number</param>
/// <returns></returns>
IObservable<Unit> Unlock(string owner, string name, int number);
}
}
}
33 changes: 33 additions & 0 deletions Octokit.Reactive/Clients/ObservableIssuesClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Reactive.Threading.Tasks;
using Octokit.Reactive.Internal;
using System.Reactive;

namespace Octokit.Reactive
{
Expand Down Expand Up @@ -222,5 +223,37 @@ public IObservable<Issue> Update(string owner, string name, int number, IssueUpd

return _client.Update(owner, name, number, issueUpdate).ToObservable();
}

/// <summary>
/// Locks an issue for the specified repository. Issue owners and users with push access can lock an issue.
/// </summary>
/// <remarks>https://developer.github.com/v3/issues/#lock-an-issue</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The issue number</param>
/// <returns></returns>
public IObservable<Issue> Lock(string owner, string name, int number)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");

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

/// <summary>
/// Unlocks an issue for the specified repository. Issue owners and users with push access can unlock an issue.
/// </summary>
/// <remarks>https://developer.github.com/v3/issues/#unlock-an-issue</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The issue number</param>
/// <returns></returns>
public IObservable<Unit> Unlock(string owner, string name, int number)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");

return _client.Unlock(owner, name, number).ToObservable();
}
}
}
19 changes: 19 additions & 0 deletions Octokit.Tests.Integration/Clients/IssuesClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,25 @@ public async Task CanCreateRetrieveAndCloseIssue()
}

[IntegrationTest]
public async Task CanLockAndUnlockIssue()
{
var newIssue = new NewIssue("a test issue") { Body = "A new unassigned issue" };
var issue = await _issuesClient.Create(_context.RepositoryOwner, _context.RepositoryName, newIssue);
var retrieved = await _issuesClient.Get(_context.RepositoryOwner, _context.RepositoryName, issue.Number);
Assert.False(issue.Locked);

await _issuesClient.Lock(_context.RepositoryOwner, _context.RepositoryName, issue.Number);
retrieved = await _issuesClient.Get(_context.RepositoryOwner, _context.RepositoryName, issue.Number);
Copy link
Member

Choose a reason for hiding this comment

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

Actually, you should be returned the updated Issue from Lock and Unlock here - to simplify this test (and do less network calls) you could assign this returned value to retrieved...

EDIT: nevermind, I didn't understand the API correctly. Disregard this, but we need to change the method signature for Lock.

Assert.NotNull(retrieved);
Assert.True(issue.Locked);
Copy link
Member

Choose a reason for hiding this comment

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

We should be checking retrieved.Locked here, right?


await _issuesClient.Unlock(_context.RepositoryOwner, _context.RepositoryName, issue.Number);
retrieved = await _issuesClient.Get(_context.RepositoryOwner, _context.RepositoryName, issue.Number);
Assert.NotNull(retrieved);
Assert.False(issue.Locked);
Copy link
Member

Choose a reason for hiding this comment

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

retrieved.Locked here too

}

[IntegrationTest]
public async Task CanListOpenIssuesWithDefaultSort()
{
var newIssue1 = new NewIssue("A test issue1") { Body = "A new unassigned issue" };
Expand Down
17 changes: 17 additions & 0 deletions Octokit.Tests.Integration/Reactive/ObservableIssuesClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,23 @@ public async Task CanCreateAndUpdateIssues()
Assert.Equal("Modified integration test issue", updateResult.Title);
}

[IntegrationTest]
public async Task CanLockAndUnlockIssues()
{
var newIssue = new NewIssue("Integration Test Issue");

var createResult = await _client.Create(_context.RepositoryOwner, _context.RepositoryName, newIssue);
Assert.False(createResult.Locked);

await _client.Lock(_context.RepositoryOwner, _context.RepositoryName, createResult.Number);
var lockResult = await _client.Get(_context.RepositoryOwner, _context.RepositoryName, createResult.Number);
Assert.True(lockResult.Locked);

await _client.Unlock(_context.RepositoryOwner, _context.RepositoryName, createResult.Number);
var unlockIssueResult = await _client.Get(_context.RepositoryOwner, _context.RepositoryName, createResult.Number);
Assert.False(unlockIssueResult.Locked);
}

public void Dispose()
{
_context.Dispose();
Expand Down
52 changes: 52 additions & 0 deletions Octokit.Tests/Clients/IssuesClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,58 @@ public async Task EnsuresArgumentsNotNull()
}
}

public class TheLockIssueMethod
{
[Fact]
public void PostsToCorrectUrl()
{
var connection = Substitute.For<IApiConnection>();
var client = new IssuesClient(connection);

client.LockIssue("fake", "repo", 42);

connection.Received().Put(Arg.Is<Uri>(u => u.ToString() == "repos/fake/repo/issues/42/lock"));
}

[Fact]
public async Task EnsuresArgumentsNotNull()
{
var connection = Substitute.For<IApiConnection>();
var client = new IssuesClient(connection);

await Assert.ThrowsAsync<ArgumentNullException>(() => client.LockIssue(null, "name", 1));
await Assert.ThrowsAsync<ArgumentException>(() => client.LockIssue("", "name", 1));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.LockIssue("owner", null, 1));
await Assert.ThrowsAsync<ArgumentException>(() => client.LockIssue("owner", "", 1));
}
}

public class TheUnlockIssueMethod
{
[Fact]
public void PostsToCorrectUrl()
{
var connection = Substitute.For<IApiConnection>();
var client = new IssuesClient(connection);

client.UnlockIssue("fake", "repo", 42);

connection.Received().Delete(Arg.Is<Uri>(u => u.ToString() == "repos/fake/repo/issues/42/lock"));
}

[Fact]
public async Task EnsuresArgumentsNotNull()
{
var connection = Substitute.For<IApiConnection>();
var client = new IssuesClient(connection);

await Assert.ThrowsAsync<ArgumentNullException>(() => client.UnlockIssue(null, "name", 1));
await Assert.ThrowsAsync<ArgumentException>(() => client.UnlockIssue("", "name", 1));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.UnlockIssue("owner", null, 1));
await Assert.ThrowsAsync<ArgumentException>(() => client.UnlockIssue("owner", "", 1));
}
}

public class TheCtor
{
[Fact]
Expand Down
50 changes: 50 additions & 0 deletions Octokit.Tests/Reactive/ObservableIssuesClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,56 @@ public void EnsuresArgumentsNotNull()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shiftkey
Why is this function showing changes.
I had opened a seperate PR to change it which has been merged.
The function has changed also here

Had to remove it to remove conflicts.

}

public class TheLockIssueMethod
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this now should be "TheLockMethod"

{
[Fact]
public void LockIssue()
Copy link
Contributor

Choose a reason for hiding this comment

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

"LocksIssue"

{
var gitHubClient = Substitute.For<IGitHubClient>();
var client = new ObservableIssuesClient(gitHubClient);

client.LockIssue("fake", "repo", 42);
gitHubClient.Issue.Received().LockIssue("fake", "repo", 42);
}

[Fact]
public void EnsuresArgumentsNotNull()
{
var gitHubClient = Substitute.For<IGitHubClient>();
var client = new ObservableIssuesClient(gitHubClient);

Assert.Throws<ArgumentNullException>(() => client.LockIssue(null, "name", 42));
Assert.Throws<ArgumentException>(() => client.LockIssue("", "name", 42));
Assert.Throws<ArgumentNullException>(() => client.LockIssue("owner", null, 42));
Assert.Throws<ArgumentException>(() => client.LockIssue("owner", "", 42));
}
}

public class TheUnlockIssueMethod
Copy link
Contributor

Choose a reason for hiding this comment

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

"TheUnlockMethod"

{
[Fact]
public void UnlockIssue()
Copy link
Contributor

Choose a reason for hiding this comment

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

Wording for the unit tests should read TheBlahMethod....DoesSomething

So...
TheUnlockMethod->UnlocksIssue

(note Unlocks rather than Unlock)

{
var gitHubClient = Substitute.For<IGitHubClient>();
var client = new ObservableIssuesClient(gitHubClient);

client.UnlockIssue("fake", "repo", 42);
gitHubClient.Issue.Received().UnlockIssue("fake", "repo", 42);
}

[Fact]
public void EnsuresArgumentsNotNull()
{
var gitHubClient = Substitute.For<IGitHubClient>();
var client = new ObservableIssuesClient(gitHubClient);

Assert.Throws<ArgumentNullException>(() => client.UnlockIssue(null, "name", 42));
Assert.Throws<ArgumentException>(() => client.UnlockIssue("", "name", 42));
Assert.Throws<ArgumentNullException>(() => client.UnlockIssue("owner", null, 42));
Assert.Throws<ArgumentException>(() => client.UnlockIssue("owner", "", 42));
}
}

public class TheCtor
{
[Fact]
Expand Down
24 changes: 22 additions & 2 deletions Octokit/Clients/IIssuesClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,16 +152,36 @@ public interface IIssuesClient
Task<Issue> Create(string owner, string name, NewIssue newIssue);

/// <summary>
/// Creates an issue for the specified repository. Any user with pull access to a repository can create an
/// Updates an issue for the specified repository. Any user with pull access to a repository can update an
/// issue.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/#create-an-issue</remarks>
/// <remarks>http://developer.github.com/v3/issues/#edit-an-issue</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The issue number</param>
/// <param name="issueUpdate">An <see cref="IssueUpdate"/> instance describing the changes to make to the issue
/// </param>
/// <returns></returns>
Task<Issue> Update(string owner, string name, int number, IssueUpdate issueUpdate);

/// <summary>
/// Locks an issue for the specified repository. Issue owners and users with push access can lock an issue.
/// </summary>
/// <remarks>https://developer.github.com/v3/issues/#lock-an-issue</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The issue number</param>
/// <returns></returns>
Task<Issue> Lock(string owner, string name, int number);
Copy link
Member

Choose a reason for hiding this comment

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

This endpoint doesn't return any data, so it needs to return a Task.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the API does return a HTTP 204 NoContent response when "succesful" the way ive seen this implemented in octokit in other places (eg OrganizationMembers.Publicize()) is that the Octokit call returns a Task then internally it calls the Connection.Put and checks the HTTP response. If a 204 was received then "true" is returned, otherwise false (or an exception of course, for 404/500/other stuff handled by the plumbing inside Connection class)

Copy link
Member

Choose a reason for hiding this comment

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

👍


/// <summary>
/// Unlocks an issue for the specified repository. Issue owners and users with push access can unlock an issue.
/// </summary>
/// <remarks>https://developer.github.com/v3/issues/#unlock-an-issue</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The issue number</param>
/// <returns></returns>
Task Unlock(string owner, string name, int number);
}
}
32 changes: 32 additions & 0 deletions Octokit/Clients/IssuesClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,5 +225,37 @@ public Task<Issue> Update(string owner, string name, int number, IssueUpdate iss

return ApiConnection.Patch<Issue>(ApiUrls.Issue(owner, name, number), issueUpdate);
}

/// <summary>
/// Locks an issue for the specified repository. Issue owners and users with push access can lock an issue.
/// </summary>
/// <remarks>https://developer.github.com/v3/issues/#lock-an-issue</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The issue number</param>
/// <returns></returns>
public Task<Issue> Lock(string owner, string name, int number)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");

return ApiConnection.Put<Issue>(ApiUrls.IssueLock(owner, name, number), AcceptHeaders.IssueLockingUnlockingApiPreview);
Copy link
Member

Choose a reason for hiding this comment

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

This is not quite the right overload, and sadly ApiConnection doesn't quite have an easy way to do this.

This works for me:

return ApiConnection.Put<Issue>(ApiUrls.IssueLock(owner, name, number), new object(), null, AcceptHeaders.IssueLockingUnlockingApiPreview);

}

/// <summary>
/// Unlocks an issue for the specified repository. Issue owners and users with push access can unlock an issue.
/// </summary>
/// <remarks>https://developer.github.com/v3/issues/#unlock-an-issue</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The issue number</param>
/// <returns></returns>
public Task Unlock(string owner, string name, int number)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");

return ApiConnection.Delete(ApiUrls.IssueLock(owner, name, number), AcceptHeaders.IssueLockingUnlockingApiPreview);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't the right overload, and we don't have an equivalent that let's us specify an Accepts value.

We should add this to IApiConnection and implement it, something like this (the name is the least-worst option that came to mind given it'll clash with the other 😢):

Task DeleteWith(Uri uri, string accepts);

Copy link
Contributor

Choose a reason for hiding this comment

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

The name is the least-worst option that came to mind given it'll clash with the other

The inconsistency of overloads for Post Put Delete and Get makes me 😢 too!

Rather than introduce the first/only occurence of this "{verb}With" terminology to get around the overload collisions, perhaps in this case we could just go with adding an overload for
Delete(Uri uri, object data, string accepts) and then pass null as the body

ApiConnection.Delete(ApiUrls.IssueLock(owner, name, number), null, AcceptHeaders.IssueLockingUnlockingApiPreview)
To me this seems slightly less ugly than adding in a new terminology

Or alternatively, perhaps the "With" terminology is a good idea, and we could (on a separate PR) introduce a whole new set of "With" overloads, and start to obsolete the old ones

eg

GetWithCustomAccept<T>()
PutWithCustomAccept<T>()
DeleteWithCustomAccept<T>()
PostWithCustomAccept<T>()

Copy link
Member

Choose a reason for hiding this comment

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

we could just go with adding an overload for Delete(Uri uri, object data, string accepts) and then pass null as the body

I don't think we can pass null here, but everything else is 👍 from me

}
}
}
3 changes: 3 additions & 0 deletions Octokit/Helpers/AcceptHeaders.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ public static class AcceptHeaders

public const string StarCreationTimestamps = "application/vnd.github.v3.star+json";

public const string IssueLockingUnlockingApiPreview = "application/vnd.github.the-key-preview+json";

public const string CommitReferenceSha1Preview = "application/vnd.github.chitauri-preview+sha";

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra whitespace line introduced here :)

}
}
12 changes: 12 additions & 0 deletions Octokit/Helpers/ApiUrls.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,18 @@ public static Uri Issue(string owner, string name, int number)
return "repos/{0}/{1}/issues/{2}".FormatUri(owner, name, number);
}

/// <summary>
/// Returns the <see cref="Uri"/> for the specified issue to be locked/unlocked.
/// </summary>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The issue number</param>
/// <returns></returns>
public static Uri IssueLock(string owner, string name, int number)
{
return "repos/{0}/{1}/issues/{2}/lock".FormatUri(owner, name, number);
}

/// <summary>
/// Returns the <see cref="Uri"/> for the comments for all issues in a specific repo.
/// </summary>
Expand Down