update models with updated permission enum #2633

notauserx · 2022-12-05T03:23:05Z

Resolves #2323

Behavior

Before the change?

NewTeam, UpdateTeam on the request side and the Team on the response side uses incorrect permission enums

After the change?

Based on the schema

Fix permission enum for NewTeam(line 11249), UpdateTeam(line 11418).
Update "Permission" and "Permissions" properties on Team.cs (line 62414)

Other information

I have also added the following endpoints from the teams api because TeamsClient was using the legacy ones and are closely related to Team Permissions

Additional info

Pull request checklist

Tests for the changes have been added
Docs have been reviewed and added/updated if needed
Added the appropriate label for the given change
Decide what to do with the UpdateTeam model, looks like it uses the legacy update team schema
Mark areas with breaking changes
Update integration tests.

Does this introduce a breaking change?

Yes (Please add the Type: Breaking change label)

Changes to the request(NewTeam.cs, UpdateTeam.cs) and response(Team.cs) model
Changed Permission property type from Permission to TeamPermission on NewTeam and UpdateTeam request models
Changed Permission property type from StringEnum to string

Pull request type

Type: Bug
Type: Breaking Change

notauserx · 2022-12-05T03:58:13Z

I have added two new enums, NewTeamPermission and TeamPermission to model the enum values from the openapi schema. Also, updated the NewTeam.cs to use the NewTeamPermission, and UpdateTeam.cs and Team.cs to use TeamPermission.

I am having some trouble running the integration tests for TeamsClientTests, all the tests are marked with OrganizationTest. I have added an organization to my test account for integration, is there a setup doc that I am missing?

Also, I see that the permission is marked as Deprecated on the docs as well as the rest open api schema. Are these permissions getting deprecated?

kfcampbell · 2022-12-05T21:45:41Z

I am having some trouble running the integration tests for TeamsClientTests, all the tests are marked with OrganizationTest. I have added an organization to my test account for integration, is there a setup doc that I am missing?

How are you attempting to run the tests? What platform are you doing it on? The only doc I'm aware of that's relevant is this one.

Also, I see that the permission is marked as Deprecated on the docs as well as the rest open api schema. Are these permissions getting deprecated?

I don't have any cool insider information here, sorry. I would guess if they're marked as deprecated, they will be deprecated, but I'm not sure of the timeframe.

notauserx · 2022-12-06T03:42:36Z

@kfcampbell, thanks for your response, I guess I did not provide enough context for my problem.

Figured out the issue, I had not set the OCTOKIT_GITHUBORGANIZATION environment variable and the OrganizationTests requires that variable be set.

notauserx · 2022-12-06T08:46:15Z

Octokit.Tests.Integration/Clients/TeamsClientTests.cs

@@ -445,10 +446,14 @@ public async Task UpdatesTeam()
            {
                var teamName = Helper.MakeNameWithTimestamp("updated-team");
                var teamDescription = Helper.MakeNameWithTimestamp("updated description");
+
+                // setting TeamPermission.Admin fails with Octokit.ApiValidationException : Setting team permission to admin is no longer supported
+                var teamPermission = TeamPermission.Push;


It seems to me that admin is not being accepted as a permission value, even though the docs say it can be.

I can get this test passing with Pull and Push values, but passing admin just fails with

setting TeamPermission.Admin fails with Octokit.ApiValidationException : Setting team permission to admin is no longer supported

curl also gets me the same error

{ "message": "Setting team permission to admin is no longer supported", "documentation_url": "https://docs.github.com/rest/reference/teams/#update-a-team-legacy" }

notauserx · 2022-12-06T14:23:39Z

What I propose is we make TeamPermission enum with pull and push values, and use that in NewTeam, UpdateTeam and Team classes. I can see that when you edit and Organization from the web UI, there is no option to set the permission, I am guessing since the permission is marked as deprecated, they don't a.

I have kept both the current and legacy versions for Update a team and Delete a team, as can be seen in the ITeamsClient, I have renamed the Update and Delete contract as UpdateLegacy and DeleteLegacy respectively, and created Update and Delete contracts that exposes the current endpoint. Not sure whether to keep the legacy endpoints exposed but I think we can leave that on the ITeamsContract.

Also, not dealing with RepositoryPermissions (also called ReviewPermissions) for collaborators as @nickfloyd mentioned in the issue, want to tackle it in a separate PR.

Hopefully, I am moving in the right direction with this PR, let me know what you think.

kfcampbell · 2022-12-06T18:10:22Z

What I propose is we make TeamPermission enum with pull and push values, and use that in NewTeam, UpdateTeam and Team classes.

This sounds like a reasonable solution to me. I always get confused by the push/pull and read/write permission splits.

Not sure whether to keep the legacy endpoints exposed

My vote is to keep the legacy behavior around in the SDK until it's dropped by the API.

Also, not dealing with RepositoryPermissions (also called ReviewPermissions) for collaborators as @nickfloyd mentioned in the issue, want to tackle it in a separate PR.

👍

This direction makes sense to me.

…tauserx/octokit.net into update-team-permissions-enums

notauserx · 2022-12-07T18:09:57Z

Here's a summary of my changes so far

Created TeamPermission enum for creating and updating team request model.
Found that both update and delete teams were using legacy endpoints, so added support for current endpoints for updating and deleting a team.
Found a schema inconsistency with the update of a team endpoint where "admin" was not being supported as a permission value, contrary to what the doc says. Created an issue.
Added TeamRepositoryPermission enum and TeamRepositoryPermissions object to model the permission and permissions property of the team response model. These were based on the schema
Deleted TeamRepositoryPermission enum and changed the type of Permission property on Team.cs as string to reflect the schema

      "permission": {
        "type": "string"
      },
      "permissions": {
        "type": "object",
        "properties": {
          "pull": {
            "type": "boolean"
          },
          "triage": {
            "type": "boolean"
          },
          "push": {
            "type": "boolean"
          },
          "maintain": {
            "type": "boolean"
          },
          "admin": {
            "type": "boolean"
          }
        },
        "required": [
          "pull",
          "triage",
          "push",
          "maintain",
          "admin"
        ]
      },

Ran the integration test hitting RepositoriesClient.GetAllTeams() and verified that "permission" can have any of "pull", "triage", "push", "maintain" or "admin" value.
Added the following endpoints because they are closely related to Team permissions and TeamsClient was using the legacy endpoint.
Check team permissions for a repository
Add or update team repository permissions
Remove a repository from a team

notauserx · 2022-12-07T18:14:44Z

.editorconfig

@@ -0,0 +1,3 @@
+root = true
+[*.cs]
+end_of_line = lf


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.

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

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

kfcampbell · 2022-12-14T18:20:52Z

is there a naming convention that we follow for naming the methods, or what to return for a 204 status code on the response?

Not to my knowledge, though perhaps @nickfloyd has opinions on this matter?

I think we could choose either the "operationId" or the "summary" value to name the method in the clients, I personally prefer the "summary" because that's more accessible from the docs . What do you think?

👍

how/where do I mark the breaking changes?

I've added the "Type: Breaking Change" label to this PR. When releasing a new version, we'll have to be careful to increment to v5.0.0 as well.

kfcampbell

Thank you for contributing! I have several naive questions, I hope that's okay.

kfcampbell · 2022-12-14T18:21:28Z

.editorconfig

@@ -0,0 +1,3 @@
+root = true
+[*.cs]
+end_of_line = lf


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

kfcampbell · 2022-12-14T18:25:05Z

Octokit.Reactive/Clients/IObservableTeamsClient.cs

+        /// <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.


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

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

Sounds good, looking forward to it!

Added a link

kfcampbell · 2022-12-14T18:26:17Z

Octokit.Tests.Conventions/PreviewsTests.cs

@@ -70,7 +70,8 @@ public void NoStalePreviews()
                // https://developer.github.com/v3/repos/commits/#get-a-single-commit
                "application/vnd.github.v3.sha",
                // https://developer.github.com/v3/activity/starring/#alternative-response-with-star-creation-timestamps
-                "application/vnd.github.v3.star+json"
+                "application/vnd.github.v3.star+json",
+                "application/vnd.github.v3.repository+json"


Do you mind explaining to me the reason for this change?

I added RepositoryContentMediaType to AcceptHeaders for passing repository media type in CheckTeamPermissionsForARepositoryWithCustomAcceptHeader. This caused this convention test to fail because now it expects "application/vnd.github.v3.repository+json" in defaultHeaders

Octokit.Tests.Integration/Helpers/TeamContext.cs

Octokit.Tests.Integration/Reactive/ObservableTeamsClientTests.cs

kfcampbell · 2022-12-14T18:31:22Z

Octokit/Clients/TeamsClient.cs

        /// </summary>
        /// <param name="id">The team identifier.</param>
        /// <param name="organization">Org to associate the repo with.</param>
        /// <param name="repoName">Name of the repo.</param>
        /// <param name="permission">The permission to grant the team on this repository.</param>
        /// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
        /// <returns></returns>
-        [ManualRoute("PUT", "/orgs/{org}/team/{team_slug}/repos/{owner}/{repo}")]
+        [ManualRoute("PUT", "/teams/{team_id}/repos/{owner}/{repo}")]


What's the reason you went with the strategy of changing routes here and adding new ones with a different method name to the client?

I'm naively thinking it might be easier to avoid a breaking change here by isolating the new functionality to new methods.

This is the legacy endpoint that is currently implemented in the project, however, it gets the endpoint uri from ApiUrls.TeamRepository which returns the /teams/{team_id}/repos/{owner}/{repo}, so I've decided to update the ManualRoute attribute value to match the correct url from the docs.

Ahh, thanks for explaining! @nickfloyd has mentioned to me that ManualRoute is used for documentation generation rather than implementation, so this makes sense.

kfcampbell · 2022-12-14T18:32:51Z

Octokit/Http/ApiConnection.cs

+        /// <param name="uri">URI of the API resource to put</param>
+        /// <param name="data">Object that describes the API resource; this will be serialized and used as the request's body</param>
+        /// <returns>A <see cref="Task"/> for the request's execution.</returns>
+        public Task Put(Uri uri, object data)


Do you mind ELI5ing the reason for this helper?

I needed a non-generic Put method where I could pass data for the request's body for AddOrUpdateTeamRepositoryPermissions method on TeamsClients, which returns a 204.

Co-authored-by: Keegan Campbell <[email protected]>

notauserx · 2022-12-15T05:01:10Z

Thank you for contributing! I have several naive questions, I hope that's okay.

I am glad you took the time to review and give me some feedback. Appreciate it!

kfcampbell

Thank you for your patience! @nickfloyd and I had a few more questions on this review for you before we get this wrapped up. Looking forward to getting the PR merged and released!

kfcampbell · 2023-01-05T19:07:22Z

Octokit.Reactive/Clients/IObservableTeamsClient.cs

+        /// <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.


Sounds good, looking forward to it!

Octokit/Models/Request/UpdateTeam.cs

Octokit.Tests.Integration/Clients/TeamsClientTests.cs

Octokit.Tests/Clients/TeamsClientTests.cs

Octokit/Clients/TeamsClient.cs

kfcampbell · 2023-01-05T19:30:51Z

Octokit/Clients/TeamsClient.cs

        /// </summary>
        /// <param name="id">The team identifier.</param>
        /// <param name="organization">Org to associate the repo with.</param>
        /// <param name="repoName">Name of the repo.</param>
        /// <param name="permission">The permission to grant the team on this repository.</param>
        /// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
        /// <returns></returns>
-        [ManualRoute("PUT", "/orgs/{org}/team/{team_slug}/repos/{owner}/{repo}")]
+        [ManualRoute("PUT", "/teams/{team_id}/repos/{owner}/{repo}")]


Ahh, thanks for explaining! @nickfloyd has mentioned to me that ManualRoute is used for documentation generation rather than implementation, so this makes sense.

kfcampbell · 2023-01-05T19:47:10Z

Octokit/Http/Connection.cs

        /// <returns>The returned <seealso cref="HttpStatusCode"/></returns>
-        public async Task<HttpStatusCode> Put(Uri uri, string accepts)
+        public async Task<HttpStatusCode> Put(Uri uri, object body)


Would you mind explaining why accepts has been removed here? I'm following that you want to PUT the dynamic object similarly to in ApiConnection.cs, but I'm unsure why that means accepts is no longer desirable/needed.

I've double-checked with the current main branch, and this method has only one reference to IConnection.cs.

Since I needed to pass an object, initially I added the object body as an additional parameter. Then I saw that the string accepts is not being passed from anywhere in the codebase, so I decided to remove that argument.

Understood, thank you.

Octokit/Models/Response/Team.cs

Co-authored-by: Keegan Campbell <[email protected]>

kfcampbell · 2023-01-06T18:39:54Z

I'll be 👍 on landing this after a small comment update to link to the REST API docs issue!

notauserx · 2023-01-16T15:28:01Z

@kfcampbell I have added the changes based on your review. Do you think we can get this merged?

kfcampbell

Thanks for working with me on this change!

matt-richardson · 2023-01-18T01:21:11Z

🎉 Great to see 🎉. What's the plans around merging and releasing?

kfcampbell · 2023-01-20T18:49:43Z

v5.0.0 has been released with this change in it!

notauserx · 2023-01-21T04:13:41Z

@kfcampbell thank you for merging my changes.

matt-richardson · 2023-01-31T02:44:11Z

Octokit/Models/Response/Team.cs

@@ -71,9 +72,14 @@ public Team(string url, string htmlUrl, int id, string nodeId, string slug, stri
        public StringEnum<TeamPrivacy> Privacy { get; private set; }

        /// <summary>
-        /// permission attached to this team
+        /// Deprecated. The permission that new repositories will be added to the team with when none is specified


I'm not sure this is right. This model is returned from gitHubClient.Repository.GetAllTeams(repositoryOwner, repositoryName), so the permission is actually "what permission does this team have for this repo".

(Previously PermissionLevel was wrong too)

@matt-richardson you are correct! This comment is more suitable for NewTeam.cs and UpdateTeam.cs than the Team.cs.

From the API reference here, the permission has a description of "Permission that the team will have for its repositories", which I think would be a better comment for this property. I can submit a quick patch updating the comment if we can agree on that.

I am currently working on a PR that takes care of all the other outdated permissions.

update models with updated permission enum

409c586

notauserx changed the title ~~update models with updated permission enum~~ [wip] update models with updated permission enum Dec 5, 2022

notauserx added 2 commits December 6, 2022 14:39

add suppress message attribute

ce9df33

update integration tests

0eed292

notauserx commented Dec 6, 2022

View reviewed changes

notauserx added 2 commits December 6, 2022 16:22

refactor: new and legacy update teams endpint

9b6164a

refactor: add new delete team endpoint

1ad585e

Merge branch 'main' into update-team-permissions-enums

9c45e17

notauserx added 7 commits December 7, 2022 10:59

use TeamPermission on NewTeam

3a3dbdd

use updated delete on team context dispose

754bece

Merge branch 'update-team-permissions-enums' of https://github.com/no…

82593bd

…tauserx/octokit.net into update-team-permissions-enums

add permission enum for team response object

b354217

refactor: remove legacy suffix from method names

a976530

introduce permissions object on Team

c22a76d

refactor: rename enum to TeamRepositoryPermission

c5f0cf4

fix formatting

3b16170

notauserx commented Dec 7, 2022

View reviewed changes

notauserx added 7 commits December 8, 2022 09:15

change Permission to string to match api specs

76cfbff

add TeamRepository

d05c0cd

add CheckTeamPermission endpoint support

baee285

fix convention tests

1c6313d

update comments on TeamRepository props

a6be98d

add two new endpoints in TeamsClient

d6f345b

refactor: rename ApiUrl for TeamPermission

c01bb60

notauserx and others added 3 commits December 11, 2022 18:52

add docs for teams api

d40e981

Merge branch 'main' into update-team-permissions-enums

f98829f

split CheckTeamPermissions into two methods

a4deb95

kfcampbell added the Type: Breaking change Used to note any change that requires a major version bump label Dec 14, 2022

kfcampbell reviewed Dec 14, 2022

View reviewed changes

Update ObservableTeamsClientTests.cs based on review

f3d6980

Co-authored-by: Keegan Campbell <[email protected]>

notauserx added 2 commits December 15, 2022 11:46

add cref to legacy update and delete endpoints

960382c

remove editorconfig file

62de913

notauserx changed the title ~~[wip] update models with updated permission enum~~ update models with updated permission enum Dec 17, 2022

Merge branch 'main' into update-team-permissions-enums

7f1bd0e

notauserx mentioned this pull request Jan 5, 2023

Team model uses wrong Permission enum #2323

Closed

Merge branch 'main' into update-team-permissions-enums

b13a43c

kfcampbell requested changes Jan 5, 2023

View reviewed changes

notauserx and others added 4 commits January 6, 2023 08:21

Update Octokit.Tests/Clients/TeamsClientTests.cs

4e2c833

Co-authored-by: Keegan Campbell <[email protected]>

remove unused line

aa6788f

rename variable based on review

ca7de3b

rename prop to match constructor param

c73545b

notauserx and others added 2 commits January 7, 2023 21:43

add comment to explain TeamPermission enum values on update

2ec885d

Merge branch 'main' into update-team-permissions-enums

31a2e9c

kfcampbell approved these changes Jan 18, 2023

View reviewed changes

kfcampbell merged commit 891015c into octokit:main Jan 20, 2023

notauserx deleted the update-team-permissions-enums branch January 21, 2023 03:33

matt-richardson reviewed Jan 31, 2023

View reviewed changes

update models with updated permission enum #2633

update models with updated permission enum #2633

Conversation

notauserx commented Dec 5, 2022 • edited Loading

Behavior

Before the change?

After the change?

Other information

Additional info

Pull request checklist

Does this introduce a breaking change?

Pull request type

notauserx commented Dec 5, 2022

kfcampbell commented Dec 5, 2022

notauserx commented Dec 6, 2022

notauserx Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

notauserx commented Dec 6, 2022

kfcampbell commented Dec 6, 2022

notauserx commented Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notauserx Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

kfcampbell commented Dec 14, 2022

kfcampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notauserx Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notauserx commented Dec 15, 2022

kfcampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfcampbell commented Jan 6, 2023

notauserx commented Jan 16, 2023

kfcampbell left a comment

Choose a reason for hiding this comment

matt-richardson commented Jan 18, 2023

kfcampbell commented Jan 20, 2023

notauserx commented Jan 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notauserx Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

notauserx commented Dec 5, 2022 •

edited

Loading

notauserx Dec 6, 2022 •

edited

Loading

notauserx commented Dec 7, 2022 •

edited

Loading

notauserx Dec 15, 2022 •

edited

Loading

notauserx Jan 6, 2023 •

edited

Loading

notauserx Jan 31, 2023 •

edited

Loading