Skip to content
This repository has been archived by the owner on Mar 14, 2023. It is now read-only.

User is collaborator check #111

Merged
merged 3 commits into from
Mar 18, 2018

Conversation

davidalber
Copy link
Collaborator

@davidalber davidalber commented Mar 16, 2018

GitHub (now) has an endpoint to check if a user is a collaborator. Currently, Highfive uses the list collaborators endpoint and then checks the response to see if the expected user is there. I assume this response is subject to being paginated in projects with lots of collaborators. Highfive, however, isn't handling pagination in this case. I don't know if that's an issue (i.e., I don't know how many contributors rust-lang/rust has; a problem would manifest as Highfive ignoring "r? @" comments from some collaborators), but it seems like an issue waiting to happen.

This PR modifies Highfive to check if a specific user is a reviewer.

In addition to the unit tests, I manually verified that is_collaborator is working as expected. Here's the log of that:

>>> from newpr import is_collaborator
>>> is_collaborator('blah', 'davidalber', 'highfive-test-repo', 'davidalber', OAUTH_TOKEN)
False

>>> is_collaborator('davidalber', 'davidalber', 'highfive-test-repo', 'davidalber', OAUTH_TOKEN)
True

>>> is_collaborator('nrc', 'davidalber', 'highfive-test-repo', 'davidalber', OAUTH_TOKEN)
False

Here is some justification for caution: I haven't tested this on a Highfive instance running against a repository. (I'm working on this now, but I keep accidentally opening my PR here. Rather than closing this again and making more noise, I am just going to update this comment when I'm done.)

@davidalber davidalber changed the title Tactical collaborator check User is collaborator check Mar 16, 2018
@davidalber
Copy link
Collaborator Author

Sorry about #110. GitHub tricked me (er...I wasn't paying enough attention) twice to open a PR in this repo, when I meant to put it in my fork of this repo.

Anyway, I tested the new_comment code path that calls is_collaborator in davidalber#2. Looks good!

Thus, with the unit tests and the manual end-to-end test, this appears to be working correctly to me.

@nrc
Copy link
Member

nrc commented Mar 18, 2018

This seems like a better approach, thanks!

@nrc nrc merged commit f80398d into rust-lang:master Mar 18, 2018
@davidalber davidalber deleted the tactical-collaborator-check branch March 19, 2018 02:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants