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

update models with updated permission enum #2633

Merged
merged 47 commits into from
Jan 20, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
409c586
update models with updated permission enum
notauserx Dec 5, 2022
ce9df33
add suppress message attribute
notauserx Dec 6, 2022
0eed292
update integration tests
notauserx Dec 6, 2022
9b6164a
refactor: new and legacy update teams endpint
notauserx Dec 6, 2022
1ad585e
refactor: add new delete team endpoint
notauserx Dec 6, 2022
9c45e17
Merge branch 'main' into update-team-permissions-enums
kfcampbell Dec 6, 2022
3a3dbdd
use TeamPermission on NewTeam
notauserx Dec 7, 2022
754bece
use updated delete on team context dispose
notauserx Dec 7, 2022
82593bd
Merge branch 'update-team-permissions-enums' of https://github.com/no…
notauserx Dec 7, 2022
b354217
add permission enum for team response object
notauserx Dec 7, 2022
a976530
refactor: remove legacy suffix from method names
notauserx Dec 7, 2022
c22a76d
introduce permissions object on Team
notauserx Dec 7, 2022
c5f0cf4
refactor: rename enum to TeamRepositoryPermission
notauserx Dec 7, 2022
3b16170
fix formatting
notauserx Dec 7, 2022
76cfbff
change Permission to string to match api specs
notauserx Dec 8, 2022
d05c0cd
add TeamRepository
notauserx Dec 8, 2022
baee285
add CheckTeamPermission endpoint support
notauserx Dec 9, 2022
1c6313d
fix convention tests
notauserx Dec 9, 2022
a6be98d
update comments on TeamRepository props
notauserx Dec 9, 2022
d6f345b
add two new endpoints in TeamsClient
notauserx Dec 9, 2022
c01bb60
refactor: rename ApiUrl for TeamPermission
notauserx Dec 9, 2022
d0f4f0c
fix test
notauserx Dec 9, 2022
cba9141
implement methods for new endpoints
notauserx Dec 9, 2022
426abdf
add the integration tests
notauserx Dec 9, 2022
19dd149
fix spelling
notauserx Dec 9, 2022
38b0881
update comments
notauserx Dec 9, 2022
de786e0
refactor: rename method name
notauserx Dec 9, 2022
a76d81a
Merge branch 'main' into update-team-permissions-enums
notauserx Dec 9, 2022
e38ec35
Merge branch 'update-team-permissions-enums' of https://github.com/no…
notauserx Dec 9, 2022
d194b68
fix: add end tag for remarks
notauserx Dec 9, 2022
28a36d6
refactor: remove unused method param
notauserx Dec 9, 2022
662b40a
fix docstring comment
notauserx Dec 11, 2022
2a6497f
the unit tests are in finally
notauserx Dec 11, 2022
d40e981
add docs for teams api
notauserx Dec 11, 2022
f98829f
Merge branch 'main' into update-team-permissions-enums
kfcampbell Dec 12, 2022
a4deb95
split CheckTeamPermissions into two methods
notauserx Dec 13, 2022
f3d6980
Update ObservableTeamsClientTests.cs based on review
notauserx Dec 15, 2022
960382c
add cref to legacy update and delete endpoints
notauserx Dec 15, 2022
62de913
remove editorconfig file
notauserx Dec 16, 2022
7f1bd0e
Merge branch 'main' into update-team-permissions-enums
kfcampbell Jan 3, 2023
b13a43c
Merge branch 'main' into update-team-permissions-enums
kfcampbell Jan 5, 2023
4e2c833
Update Octokit.Tests/Clients/TeamsClientTests.cs
notauserx Jan 6, 2023
aa6788f
remove unused line
notauserx Jan 6, 2023
ca7de3b
rename variable based on review
notauserx Jan 6, 2023
c73545b
rename prop to match constructor param
notauserx Jan 6, 2023
2ec885d
add comment to explain TeamPermission enum values on update
notauserx Jan 7, 2023
31a2e9c
Merge branch 'main' into update-team-permissions-enums
notauserx Jan 16, 2023
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
3 changes: 3 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
root = true
[*.cs]
end_of_line = lf
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 added this file because I was having trouble with inconsistent line ending.

I can remove this file if necessary. But I think having an editorconfig file greatly improves the dev experience. This one is by no means complete.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer that this is added in a separate PR if you don't mind.

Copy link
Contributor Author

@notauserx notauserx Dec 15, 2022

Choose a reason for hiding this comment

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

That sounds reasonable. Removed the file and set a global gitignore for my local editorconfig

19 changes: 19 additions & 0 deletions Octokit.Reactive/Clients/IObservableTeamsClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,34 @@ public interface IObservableTeamsClient
/// <returns>Newly created <see cref="Team"/></returns>
IObservable<Team> Create(string org, NewTeam team);

/// <summary>
/// Updates a team
/// To edit a team, the authenticated user must either be an organization owner or a team maintainer
/// </summary>
/// <returns>updated <see cref="Team" /> for the current org</returns>
IObservable<Team> Update(string org, string teamSlug, UpdateTeam team);

/// <summary>
/// Returns updated <see cref="Team" /> for the current org.
/// This endpoint route is deprecated and will be removed from the Teams API.
/// We recommend migrating your existing code to use the new Update a team endpoint
/// </summary>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns>Updated <see cref="Team"/></returns>
IObservable<Team> Update(int id, UpdateTeam team);

/// <summary>
/// To delete a team, the authenticated user must be an organization owner or team maintainer.
/// If you are an organization owner, deleting a parent team will delete all of its child teams as well.
/// </summary>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns></returns>
IObservable<Unit> Delete(string org, string teamSlug);

/// <summary>
/// Delete a team - must have owner permissions to this
/// This endpoint route is deprecated and will be removed from the Teams API.
/// We recommend migrating your existing code to use the new Delete a team endpoint.
Copy link
Member

Choose a reason for hiding this comment

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

What are your thoughts on providing a link or some other manner to direct users towards the new endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a great suggestion. I will provide a link to the new endpoint in the comments.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, looking forward to it!

Copy link
Contributor Author

@notauserx notauserx Jan 6, 2023

Choose a reason for hiding this comment

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

Added a link

/// </summary>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns></returns>
Expand Down
34 changes: 33 additions & 1 deletion Octokit.Reactive/Clients/ObservableTeamsClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,24 @@ public IObservable<Team> Create(string org, NewTeam team)
return _client.Create(org, team).ToObservable();
}

/// <summary>
/// Updates a team
/// To edit a team, the authenticated user must either be an organization owner or a team maintainer
/// </summary>
/// <returns>updated <see cref="Team" /> for the current org</returns>
public IObservable<Team> Update(string org, string teamSlug, UpdateTeam team)
{
Ensure.ArgumentNotNull(org, nameof(org));
Ensure.ArgumentNotNull(teamSlug, nameof(teamSlug));
Ensure.ArgumentNotNull(team, nameof(team));

return _client.Update(org, teamSlug, team).ToObservable();
}

/// <summary>
/// Returns updated <see cref="Team" /> for the current org.
/// This endpoint route is deprecated and will be removed from the Teams API.
/// We recommend migrating your existing code to use the new Update a team endpoint
/// </summary>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns>Updated <see cref="Team"/></returns>
Expand All @@ -206,7 +222,23 @@ public IObservable<Team> Update(int id, UpdateTeam team)
}

/// <summary>
/// Delete a team - must have owner permissions to this
/// To delete a team, the authenticated user must be an organization owner or team maintainer.
/// If you are an organization owner, deleting a parent team will delete all of its child teams as well.
/// </summary>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns></returns>
public IObservable<Unit> Delete(string org, string teamSlug)
{
Ensure.ArgumentNotNull(org, nameof(org));
Ensure.ArgumentNotNull(teamSlug, nameof(teamSlug));

return _client.Delete(org, teamSlug).ToObservable();
}

/// <summary>
/// Delete a team - must have owner permissions to do this
/// This endpoint route is deprecated and will be removed from the Teams API.
/// We recommend migrating your existing code to use the new Delete a team endpoint.
/// </summary>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns></returns>
Expand Down
45 changes: 43 additions & 2 deletions Octokit.Tests.Integration/Clients/TeamsClientTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
using System;
using System.Linq;
using System.Net;
using System.Threading.Tasks;
using Octokit;
using Octokit.Tests.Integration;
Expand Down Expand Up @@ -33,10 +32,12 @@ public async Task CreatesTeam()
Assert.Equal(teamName, team.Name);
Assert.Equal(teamDescription, team.Description);
Assert.Equal(TeamPrivacy.Closed, team.Privacy);
// Permission defaults to pull when no permission is specified when creating a team
Assert.Equal(TeamRespositoryPermission.Pull, team.Permission);
Assert.Equal(1, team.MembersCount);
Assert.Equal(1, team.ReposCount);

await github.Organization.Team.Delete(team.Id);
await github.Organization.Team.Delete(Helper.Organization, team.Slug);
}
}
}
Expand Down Expand Up @@ -445,10 +446,49 @@ public async Task UpdatesTeam()
{
var teamName = Helper.MakeNameWithTimestamp("updated-team");
var teamDescription = Helper.MakeNameWithTimestamp("updated description");

var update = new UpdateTeam(teamName)
{
Description = teamDescription,
Privacy = TeamPrivacy.Closed,
Permission = TeamPermission.Push,
ParentTeamId = parentTeamContext.TeamId
};

var team = await _github.Organization.Team.Update(Helper.Organization, teamContext.Team.Slug, update);

Assert.Equal(teamName, team.Name);
Assert.Equal(teamDescription, team.Description);
Assert.Equal(TeamPrivacy.Closed, team.Privacy);
Assert.Equal(TeamRespositoryPermission.Push, team.Permission);
Assert.Equal(parentTeamContext.TeamId, team.Parent.Id);
}
}
}

public class TheUpdateLegacyMethod
{
private readonly IGitHubClient _github;

public TheUpdateLegacyMethod()
{
_github = Helper.GetAuthenticatedClient();
}

[OrganizationTest]
public async Task UpdatesTeamLegacy()
{
using (var parentTeamContext = await _github.CreateTeamContext(Helper.Organization, new NewTeam(Helper.MakeNameWithTimestamp("parent-team"))))
using (var teamContext = await _github.CreateTeamContext(Helper.Organization, new NewTeam(Helper.MakeNameWithTimestamp("team-fixture"))))
{
var teamName = Helper.MakeNameWithTimestamp("updated-team");
var teamDescription = Helper.MakeNameWithTimestamp("updated description");

var update = new UpdateTeam(teamName)
{
Description = teamDescription,
Privacy = TeamPrivacy.Closed,
Permission = TeamPermission.Push,
ParentTeamId = parentTeamContext.TeamId
};

Expand All @@ -457,6 +497,7 @@ public async Task UpdatesTeam()
Assert.Equal(teamName, team.Name);
Assert.Equal(teamDescription, team.Description);
Assert.Equal(TeamPrivacy.Closed, team.Privacy);
Assert.Equal(TeamRespositoryPermission.Push, team.Permission);
Assert.Equal(parentTeamContext.TeamId, team.Parent.Id);
}
}
Expand Down
6 changes: 3 additions & 3 deletions Octokit.Tests.Integration/Helper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -190,15 +190,15 @@ public static void DeleteRepo(IConnection connection, string owner, string name)
public static void DeleteTeam(IConnection connection, Team team)
{
if (team != null)
DeleteTeam(connection, team.Id);
DeleteTeam(connection, team.Slug);
}

public static void DeleteTeam(IConnection connection, int teamId)
public static void DeleteTeam(IConnection connection, string slug)
{
try
{
var client = new GitHubClient(connection);
client.Organization.Team.Delete(teamId).Wait(TimeSpan.FromSeconds(15));
client.Organization.Team.Delete(Organization, slug).Wait(TimeSpan.FromSeconds(15));
}
catch { }
}
Expand Down
2 changes: 0 additions & 2 deletions Octokit.Tests.Integration/Helpers/TeamContext.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
kfcampbell marked this conversation as resolved.
Show resolved Hide resolved

namespace Octokit.Tests.Integration.Helpers
{
Expand Down
38 changes: 38 additions & 0 deletions Octokit.Tests.Integration/Reactive/ObservableTeamsClientTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -386,12 +386,50 @@ public async Task UpdatesTeam()
ParentTeamId = parentTeamContext.TeamId
};

var team = await _github.Organization.Team.Update(Helper.Organization, teamContext.Team.Slug, update);

Assert.Equal(teamName, team.Name);
Assert.Equal(teamDescription, team.Description);
Assert.Equal(TeamPrivacy.Closed, team.Privacy);
Assert.Equal(parentTeamContext.TeamId, team.Parent.Id);

_github.Organization.Team.Delete(Helper.Organization, team.Slug);
}
}
}

public class TheUpdateLegacyMethod
{
private readonly IObservableGitHubClient _github;

public TheUpdateLegacyMethod()
{
_github = new ObservableGitHubClient(Helper.GetAuthenticatedClient());
}

[OrganizationTest]
public async Task UpdatesTeamLegacy()
{
using (var parentTeamContext = await _github.CreateTeamContext(Helper.Organization, new NewTeam(Helper.MakeNameWithTimestamp("parent-team"))))
using (var teamContext = await _github.CreateTeamContext(Helper.Organization, new NewTeam(Helper.MakeNameWithTimestamp("team-fixture"))))
{
var teamName = Helper.MakeNameWithTimestamp("updated-team");
var teamDescription = Helper.MakeNameWithTimestamp("updated description");
var update = new UpdateTeam(teamName)
{
Description = teamDescription,
Privacy = TeamPrivacy.Closed,
ParentTeamId = parentTeamContext.TeamId
};

var team = await _github.Organization.Team.Update(teamContext.TeamId, update);

Assert.Equal(teamName, team.Name);
Assert.Equal(teamDescription, team.Description);
Assert.Equal(TeamPrivacy.Closed, team.Privacy);
Assert.Equal(parentTeamContext.TeamId, team.Parent.Id);

_github.Organization.Team.Delete(teamContext.TeamId);
}
}
}
Expand Down
58 changes: 58 additions & 0 deletions Octokit.Tests/Clients/TeamsClientTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,36 @@ public void RequestsTheCorrectUrl()
var client = new TeamsClient(connection);
var team = new UpdateTeam("Octokittens");

var org = "org";
var slug = "slug";
client.Update(org, slug , team);

connection.Received().Patch<Team>(
Arg.Is<Uri>(u => u.ToString() == "orgs/org/teams/slug"),
team);
}

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

await Assert.ThrowsAsync<ArgumentNullException>(() => client.Update(null, "b", new UpdateTeam("update-team")));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.Update("a", null, new UpdateTeam("update-team")));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.Update("a", "b", null));
}
}

public class TheUpdateTeamLegacyMethod
{
[Fact]
public void RequestsTheCorrectUrl()
{
var connection = Substitute.For<IApiConnection>();
var client = new TeamsClient(connection);
var team = new UpdateTeam("Octokittens");

client.Update(1, team);

connection.Received().Patch<Team>(
Expand All @@ -187,6 +217,34 @@ public async Task EnsuresNonNullArguments()
}

public class TheDeleteTeamMethod
{
[Fact]
public void RequestsTheCorrectUrl()
{
var connection = Substitute.For<IApiConnection>();
var client = new TeamsClient(connection);

var org = "org";
var slug = "slug";

client.Delete(org, slug);

connection.Received().Delete(
Arg.Is<Uri>(u => u.ToString() == "orgs/org/teams/slug"));
}

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

await Assert.ThrowsAsync<ArgumentNullException>(() => client.Delete("a", null));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.Delete(null, "a"));
}
}

public class TheDeleteTeamLegacuMethod
notauserx marked this conversation as resolved.
Show resolved Hide resolved
{
[Fact]
public void RequestsTheCorrectUrl()
Expand Down
4 changes: 2 additions & 2 deletions Octokit.Tests/SimpleJsonSerializerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -472,12 +472,12 @@ public void ShouldDeserializeParentTeamWithNullPermission()
var result = new SimpleJsonSerializer().Deserialize<Team>(teamJson);

// original value works as expected
Assert.Equal(PermissionLevel.Admin, result.Permission.Value);
Assert.Equal(TeamRespositoryPermission.Admin, result.Permission.Value);
Assert.Equal("admin", result.Permission.StringValue);

// parent permission is marked as null and cannot be parsed
Assert.Equal("null", result.Parent.Permission.StringValue);
PermissionLevel value;
TeamRespositoryPermission value;
Assert.False(result.Parent.Permission.TryParse(out value));
}
}
Expand Down
21 changes: 20 additions & 1 deletion Octokit/Clients/ITeamsClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,34 @@ public interface ITeamsClient
/// <returns>Newly created <see cref="Team"/></returns>
Task<Team> Create(string org, NewTeam team);

/// <summary>
/// Updates a team
/// To edit a team, the authenticated user must either be an organization owner or a team maintainer
/// </summary>
/// <returns>updated <see cref="Team" /> for the current org</returns>
Task<Team> Update(string org, string teamSlug, UpdateTeam team);

/// <summary>
/// Returns updated <see cref="Team" /> for the current org.
/// This endpoint route is deprecated and will be removed from the Teams API.
/// We recommend migrating your existing code to use the new Update a team endpoint
/// </summary>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns>Updated <see cref="Team"/></returns>
Task<Team> Update(int id, UpdateTeam team);

/// <summary>
/// Delte a team - must have owner permissions to this
/// To delete a team, the authenticated user must be an organization owner or team maintainer.
/// If you are an organization owner, deleting a parent team will delete all of its child teams as well.
/// </summary>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns></returns>
Task Delete(string org, string teamSlug);

/// <summary>
/// Delete a team - must have owner permissions to do this
/// This endpoint route is deprecated and will be removed from the Teams API.
/// We recommend migrating your existing code to use the new Delete a team endpoint.
/// </summary>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns></returns>
Expand Down
Loading