Update permissions

[notauserx]

Resolves #2323

---

Behavior

Before the change?

• App Installations and Collaborators endpoints do not contain the correct permissions on the request and response contracts.

After the change?

• Permissions are updated to match GitHub's open API specs

---

Additional info

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Added the appropriate label for the given change

Does this introduce a breaking change?

• Yes (Please add the Type: Breaking change label)
• No

If Yes, what's the impact:

• Permissions have been updated for App Installation and Collaborator endpoints to match the updated spec

Pull request type

• Bugfix: Type: Bug Type: Breaking Change

---

[notauserx]

There are some integration tests failing for IRepoCollaboratorsClient's GetAll method. The tests are adding a collaborator and expecting the collaborator count to be updated. However, adding a collaborator will send an invitation and the collaborator has to accept the invitation before the count will be updated.

The integration tests for ReviewPermission also follow the same route, a collaborator is added with 'write' permission and the test expects a collaborator with 'write' permission, without waiting for the collaborator to accept the invitation.

In these tests octokitnet-test1 and m-zuber-octokit-integration-tests accounts are used for adding a collaborator, and I don't have their creds to accept the invitation.

This probably is out of scope for this issue, but we probably should create a new issue and think about possible workarounds.

For app installation permissions, there are some permissions that have only 'read' as a value, some have only 'write', most with 'read' and 'write', and some with 'read', 'write' and 'admin'.

I have named the enums InstallationReadPermissionLevel, InstallationWritePermissionLevel, InstallationReadWritePermissionLevel, and InstallationReadWriteAdminPermissionLevel respectively.

I have also moved the Permission.cs to the common folder because it houses all the permissions from the API surface.

[kfcampbell]

@notauserx is the [wip] description in the PR still valid?

[notauserx]

@kfcampbell I have completed updating all references the old Permission enums and made them context-specific. I was also planning to update some endpoints that use the updated permission in request/response.

However, the integration tests are failing for some of these endpoints, as I've mentioned in my previous comments. I do need some feedback on how to proceed.

[kfcampbell]

The integration testing question with the invites is a tough one. As far as I know, we don't have a convenient way to do that. I'll have to think about that more and ask my team.

[nickfloyd]

Hey @notauserx, Thanks again for these contributions ❤️ . As you've detailed above, we need to rethink some of the strategies in our integration tests. I cannot be 100% certain, but the workflow for the ones you referenced has changed since they were originally implemented.

We have a few options here:

1. We can skip to unblock this by adding the skip attribute i.e., [Fact(Skip = "Reference to this PR")]. That will give us more time to figure out what we want to do without looking at code context clues or outright deleting them.
2. We can delete the tests, looking at the ones here some of these seems a bit contrived and fragile (as you discovered).
3. We can see if there is an alternate strategy and implement that - at its most basic, GetAll could check for something else or check that it pulled back the one contributor it already has.
4. We could rework things so that it is dependent on less fragile and more public fixtures - though I think we'd probably still run into what you are running into around account access.

I don't want to get into philosophical aspects of how useful functional tests are - especially since there is already pretty good unit test coverage on RepoCollaboratorsClient, for instance - so I'll leave the decision on how to proceed up to you. We trust your judgment. I see this as a valuable change and would like to get it in.

I appreciate you highlighting this fragile part of this SDK's coverage. We're working daily to try to improve this for everyone, and I'm glad you've decided to come alongside us and do the same ❤️ !

[notauserx]

@nickfloyd thank you for sharing your thoughts. Among the options you suggested, I am more biased toward either removing or skipping the failing tests.

I do understand that with GitHub updating their REST APIs, there will be times when this SDK gets out of sync, and with breaking changes and workflow updates some workflows do get outdated.

I will think through your suggestions and incorporate them into this PR, which I plan to resume next week.

And for the contributions, I am glad to offer my PRs.

[notauserx]

This PR is now ready for review. Please give me some feedback and I will update the PR accordingly.

[nickfloyd]

Thank you for getting this in ❤️