From 902a5d765f6a129df42a72786e3f7098c557a33f Mon Sep 17 00:00:00 2001 From: Ryan Gribble Date: Thu, 25 Aug 2016 07:38:24 +1000 Subject: [PATCH] Tweak naming of classes to be consistent Change team and user lists to specific classes derived from Collection so we can offer better configuration of BranchProtectionPushRestriction via multiple ctors (teams only, users only, teams and users, etc) Add another ctor to BranchProtectionPushRestrictions for the case where no teams/users are specified (ie admin only) Change organization update tests to use this new ctor and assert we get empty lists rather than no push restrictions --- .../Clients/RepositoryBranchesClientTests.cs | 10 +- .../OrganizationRepositoryWithTeamContext.cs | 6 +- .../Models/Request/BranchProtectionUpdate.cs | 100 +++++++++++++++--- Octokit/Models/Response/BranchProtection.cs | 10 +- 4 files changed, 102 insertions(+), 24 deletions(-) diff --git a/Octokit.Tests.Integration/Clients/RepositoryBranchesClientTests.cs b/Octokit.Tests.Integration/Clients/RepositoryBranchesClientTests.cs index b00bc5ff..39542e61 100644 --- a/Octokit.Tests.Integration/Clients/RepositoryBranchesClientTests.cs +++ b/Octokit.Tests.Integration/Clients/RepositoryBranchesClientTests.cs @@ -442,7 +442,7 @@ public class RepositoryBranchesClientTests var repoName = _orgRepoContext.RepositoryContext.RepositoryName; var update = new BranchProtectionSettingsUpdate( new BranchProtectionRequiredStatusChecksUpdate(false, false, new[] { "new" }), - null); + new BranchProtectionPushRestrictionsUpdate()); var protection = await _client.UpdateBranchProtection(repoOwner, repoName, "master", update); @@ -450,7 +450,8 @@ public class RepositoryBranchesClientTests Assert.False(protection.RequiredStatusChecks.Strict); Assert.Equal(1, protection.RequiredStatusChecks.Contexts.Count); - Assert.Null(protection.Restrictions); + Assert.Empty(protection.Restrictions.Teams); + Assert.Empty(protection.Restrictions.Users); } [IntegrationTest] @@ -459,7 +460,7 @@ public class RepositoryBranchesClientTests var repoId = _orgRepoContext.RepositoryContext.RepositoryId; var update = new BranchProtectionSettingsUpdate( new BranchProtectionRequiredStatusChecksUpdate(false, false, new[] { "new" }), - null); + new BranchProtectionPushRestrictionsUpdate()); var protection = await _client.UpdateBranchProtection(repoId, "master", update); @@ -467,7 +468,8 @@ public class RepositoryBranchesClientTests Assert.False(protection.RequiredStatusChecks.Strict); Assert.Equal(1, protection.RequiredStatusChecks.Contexts.Count); - Assert.Null(protection.Restrictions); + Assert.Empty(protection.Restrictions.Teams); + Assert.Empty(protection.Restrictions.Users); } public void Dispose() diff --git a/Octokit.Tests.Integration/Helpers/OrganizationRepositoryWithTeamContext.cs b/Octokit.Tests.Integration/Helpers/OrganizationRepositoryWithTeamContext.cs index 49bcedae..67600606 100644 --- a/Octokit.Tests.Integration/Helpers/OrganizationRepositoryWithTeamContext.cs +++ b/Octokit.Tests.Integration/Helpers/OrganizationRepositoryWithTeamContext.cs @@ -55,10 +55,10 @@ namespace Octokit.Tests.Integration.Helpers new RepositoryPermissionRequest(Permission.Push)); // Protect master branch - var orgUpdate = new BranchProtectionSettingsUpdate( + var protection = new BranchProtectionSettingsUpdate( new BranchProtectionRequiredStatusChecksUpdate(true, true, new[] { "build", "test" }), - new ProtectedBranchRestrictionsUpdate(new[] { contextOrgTeam.TeamName }, new string[0])); - await client.Repository.Branch.UpdateBranchProtection(contextOrgRepo.RepositoryOwner, contextOrgRepo.RepositoryName, "master", orgUpdate); + new BranchProtectionPushRestrictionsUpdate(new BranchProtectionTeamCollection { contextOrgTeam.TeamName })); + await client.Repository.Branch.UpdateBranchProtection(contextOrgRepo.RepositoryOwner, contextOrgRepo.RepositoryName, "master", protection); return new OrganizationRepositoryWithTeamContext { diff --git a/Octokit/Models/Request/BranchProtectionUpdate.cs b/Octokit/Models/Request/BranchProtectionUpdate.cs index 76ca4465..46cb2de5 100644 --- a/Octokit/Models/Request/BranchProtectionUpdate.cs +++ b/Octokit/Models/Request/BranchProtectionUpdate.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Collections.ObjectModel; using System.Diagnostics; using System.Globalization; using Octokit.Internal; @@ -30,7 +31,7 @@ namespace Octokit /// /// Specifies the requested status check settings. Pass null to disable status checks /// Specifies the requested push access restrictions (applies only to Organization owned repositories). Pass null to disable push access restrictions - public BranchProtectionSettingsUpdate(BranchProtectionRequiredStatusChecksUpdate requiredStatusChecks, ProtectedBranchRestrictionsUpdate restrictions) + public BranchProtectionSettingsUpdate(BranchProtectionRequiredStatusChecksUpdate requiredStatusChecks, BranchProtectionPushRestrictionsUpdate restrictions) { RequiredStatusChecks = requiredStatusChecks; Restrictions = restrictions; @@ -46,13 +47,16 @@ namespace Octokit /// Push access restrictions for the protected branch /// [SerializeNull] - public ProtectedBranchRestrictionsUpdate Restrictions { get; protected set; } + public BranchProtectionPushRestrictionsUpdate Restrictions { get; protected set; } internal string DebuggerDisplay { get { - return String.Format(CultureInfo.InvariantCulture, "StatusChecks: {0} Restrictions: {1}", RequiredStatusChecks.DebuggerDisplay, Restrictions.DebuggerDisplay); + return String.Format(CultureInfo.InvariantCulture, + "StatusChecks: {0} Restrictions: {1}", + RequiredStatusChecks == null ? "disabled" : RequiredStatusChecks.DebuggerDisplay, + Restrictions == null ? "disabled" : Restrictions.DebuggerDisplay); } } } @@ -101,17 +105,50 @@ namespace Octokit } /// - /// Specifies people or teams allowed to push to the protected branch. Required status checks will still prevent these people from merging if the checks fail + /// Specifies teams and/or people allowed to push to the protected branch. Required status checks will still prevent these people from merging if the checks fail /// [DebuggerDisplay("{DebuggerDisplay,nq}")] - public class ProtectedBranchRestrictionsUpdate + public class BranchProtectionPushRestrictionsUpdate { /// - /// Specify people or teams (in addition to Administrators) allowed to push to this branch. Required status checks will still prevent these people from merging if the checks fail + /// Specify only administrators are allowed to push to this branch. Required status checks will still prevent these people from merging if the checks fail /// - /// Teams allowed to push to this branch (pass empty array if no teams) - /// Users allowed to push to this branch (pass empty array if no users) - public ProtectedBranchRestrictionsUpdate(IReadOnlyList teams, IReadOnlyList users) + public BranchProtectionPushRestrictionsUpdate() + { + Teams = new BranchProtectionTeamCollection(); + Users = new BranchProtectionUserCollection(); + } + + /// + /// Specify teams (in addition to Administrators) allowed to push to this branch. Required status checks will still prevent these people from merging if the checks fail + /// + /// Teams allowed to push to this branch + public BranchProtectionPushRestrictionsUpdate(BranchProtectionTeamCollection teams) + { + Ensure.ArgumentNotNull(teams, "teams"); + + Teams = teams; + Users = new BranchProtectionUserCollection(); + } + + /// + /// Specify people (in addition to Administrators) allowed to push to this branch. Required status checks will still prevent these people from merging if the checks fail + /// + /// Users allowed to push to this branch + public BranchProtectionPushRestrictionsUpdate(BranchProtectionUserCollection users) + { + Ensure.ArgumentNotNull(users, "users"); + + Teams = new BranchProtectionTeamCollection(); + Users = users; + } + + /// + /// Specify teams and/or people (in addition to Administrators) allowed to push to this branch. Required status checks will still prevent these people from merging if the checks fail + /// + /// Teams allowed to push to this branch + /// Users allowed to push to this branch + public BranchProtectionPushRestrictionsUpdate(BranchProtectionTeamCollection teams, BranchProtectionUserCollection users) { Ensure.ArgumentNotNull(teams, "teams"); Ensure.ArgumentNotNull(users, "users"); @@ -123,18 +160,57 @@ namespace Octokit /// /// Teams allowed to push to this branch /// - public IReadOnlyList Teams { get; private set; } + public BranchProtectionTeamCollection Teams { get; private set; } /// /// Users allowed to push to this branch /// - public IReadOnlyList Users { get; private set; } + public BranchProtectionUserCollection Users { get; private set; } internal string DebuggerDisplay { get { - return String.Format(CultureInfo.InvariantCulture, "Teams: {0} Users: {1}", Teams == null ? "" : String.Join(",", Teams), Users == null ? "" : String.Join(",", Users)); + return String.Format(CultureInfo.InvariantCulture, + "Teams: {0} Users: {1}", + Teams == null ? "" : Teams.DebuggerDisplay, + Users == null ? "" : Users.DebuggerDisplay); + } + } + } + + [DebuggerDisplay("{DebuggerDisplay,nq}")] + public class BranchProtectionTeamCollection : Collection + { + public BranchProtectionTeamCollection() + { } + + public BranchProtectionTeamCollection(IList list) : base(list) + { } + + internal string DebuggerDisplay + { + get + { + return string.Format(CultureInfo.InvariantCulture, String.Join(", ", this)); + } + } + } + + [DebuggerDisplay("{DebuggerDisplay,nq}")] + public class BranchProtectionUserCollection : Collection + { + public BranchProtectionUserCollection() + { } + + public BranchProtectionUserCollection(IList list) : base(list) + { } + + internal string DebuggerDisplay + { + get + { + return string.Format(CultureInfo.InvariantCulture, String.Join(", ", this)); } } } diff --git a/Octokit/Models/Response/BranchProtection.cs b/Octokit/Models/Response/BranchProtection.cs index 98225ae6..3ef2d06f 100644 --- a/Octokit/Models/Response/BranchProtection.cs +++ b/Octokit/Models/Response/BranchProtection.cs @@ -108,7 +108,7 @@ namespace Octokit { public BranchProtectionSettings() { } - public BranchProtectionSettings(BranchProtectionRequiredStatusChecks requiredStatusChecks, ProtectedBranchRestrictions restrictions) + public BranchProtectionSettings(BranchProtectionRequiredStatusChecks requiredStatusChecks, BranchProtectionPushRestrictions restrictions) { RequiredStatusChecks = requiredStatusChecks; Restrictions = restrictions; @@ -122,7 +122,7 @@ namespace Octokit /// /// Push access restrictions for the protected branch /// - public ProtectedBranchRestrictions Restrictions { get; protected set; } + public BranchProtectionPushRestrictions Restrictions { get; protected set; } internal string DebuggerDisplay { @@ -183,11 +183,11 @@ namespace Octokit /// Specifies people or teams allowed to push to the protected branch. Required status checks will still prevent these people from merging if the checks fail /// [DebuggerDisplay("{DebuggerDisplay,nq}")] - public class ProtectedBranchRestrictions + public class BranchProtectionPushRestrictions { - public ProtectedBranchRestrictions() { } + public BranchProtectionPushRestrictions() { } - public ProtectedBranchRestrictions(IReadOnlyList teams, IReadOnlyList users) + public BranchProtectionPushRestrictions(IReadOnlyList teams, IReadOnlyList users) { Teams = teams; Users = users;