Missing permission parameter

[martinscholz83]

Fixes #1320

Add option to set or update permissions for a repository for teams. I'm still not satisfied with naming of TeamRepositoryUpdate. Any other suggestions?

[ryangribble]

There already exists a RepositoryPermission enum that has the same values so I think you could just use that

[ryangribble]

I'm still not satisfied with naming of TeamRepositoryUpdate. Any other suggestions?

You could go with AddTeamRepositoryRequest or RepositoryPermissionRequest or something like that...

Also it would be nice to specify the AcceptsHeader for this preview functionality, like I did on #1319 so that this functionality works on GHE 2.5 (before the preview period had ended). This also would save the need to add the Put overload on Connection class etc at this time...

[ryangribble]

You shouldn't need to specify [Parameter] tag except in unusual cases. The default serialization will handle converting between github API format and octokit.net's more c# representation

eg:
lowercase_separated_with_underscores <=> LowercaseSeparatedWithUnderscores

Only when a property name on our side doesn't match the above conversion, do you need to explicitly define a [Parameter] tag

[martinscholz83]

@ryangribble, is there something more to do or is this fine?

[ryangribble]

It would be good to add integration tests for the new parameter?

You can use repositorycontext and teamcontext helpers to create repo/teams for the test then they'll be removed when the test finishes

[martinscholz83]

Something like that?

[ryangribble]

Yep that's on the right track... although what if the test user's organisation has no teams created?

Similar to the RepositoryContext There is also an (incorrectly named!) EnterpriseTeamContext helper that can create a team on the fly and then destroy it when it goes out of scope. Using this would make the test more reliable and not require any specially configured test organisation.

Another thing that I like to do in integration tests is not only assert the returned object from that update call, but actually make a "Get" call (in this case, something like client.Team.GetAllRepositories() and check the returned repository in there also indicates the permission level you requested was infact correctly applied.

As an aside, while you're at it you could rename that EnterpriseTeamContext class and helper method to TeamContext rather than EnterpriseTeamContext since they are not actually GitHub Enterprise specific... Im not sure why I named it that!

[ryangribble]

argh sorry @maddin2016 I gave you a bit of a bum steer on that TeamsContext thing, it actually is hardcoded to use EnterpriseHelper class under the covers which means it uses a different set of test environment variables etc. Ive made some changes locally to rejig the integration test helpers so that they can be agnostic to whether github.com or github enterprise is being used. Ill send a PR to your branch in a minute that will hopefully sort everything out! 😀

[ryangribble]

Congrats @maddin2016 on your first merged octokit Pull Request!
We really appreciate the contributions 😀

[martinscholz83]

🎉 😃 @ryangribble many thanks for your help!!