Request review from non-collaborators by mentioning them #99
Conversation
… allowing collaborators to be requested for review
… already mentioned as a non-collaborator, or has already left a top-level review
src/github.js
Outdated
| return await octokit.repos.checkCollaborator({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| username: person, | ||
| }).then( | ||
| (response) => { | ||
| if (response.status === 204) { | ||
| return true; | ||
| } | ||
| return false; | ||
| }, | ||
| (error) => { | ||
| if (error.status === 404) { | ||
| return false; | ||
| } | ||
| throw error; | ||
| } | ||
| ); |
There was a problem hiding this comment.
In JavaScript, when you return a promise, you don't need to add await. Also, because we use async and await, except for some rare cases, you can simply use async and await:
| return await octokit.repos.checkCollaborator({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| username: person, | |
| }).then( | |
| (response) => { | |
| if (response.status === 204) { | |
| return true; | |
| } | |
| return false; | |
| }, | |
| (error) => { | |
| if (error.status === 404) { | |
| return false; | |
| } | |
| throw error; | |
| } | |
| ); | |
| try { | |
| const response = await octokit.repos.checkCollaborator({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| username: person, | |
| }); | |
| if (response.status === 204) { | |
| return true; | |
| } | |
| return false; | |
| } catch (error) { | |
| if (error.status === 404) { | |
| return false; | |
| } | |
| throw error; | |
| } |
There was a problem hiding this comment.
Thanks for your suggestion, I'll test it later.
| const comment_prefix = 'Auto-requesting reviews from non-collaborators: '; | ||
| const mention_prefix = '@'; | ||
|
|
||
| const review_requested = new Set(await octokit.paginate( | ||
| octokit.pulls.listRequestedReviewers, | ||
| { | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.payload.pull_request.number, | ||
| } | ||
| )); | ||
| const review_list = await octokit.paginate( | ||
| octokit.pulls.listReviews, | ||
| { | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.payload.pull_request.number, | ||
| } | ||
| ); | ||
| const review_comments = await octokit.paginate( | ||
| octokit.pulls.listReviewComments, | ||
| { | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.payload.pull_request.number, | ||
| } | ||
| ); | ||
| // Only consider mentions starting with the prefix | ||
| const already_mentioned = new Set(review_list.filter((review) => ( | ||
| review.body.startsWith(comment_prefix) | ||
| )).map( | ||
| (review) => review.body.substring(comment_prefix.length).split(' ').filter( | ||
| (mention) => mention.startsWith(mention_prefix) | ||
| ).map( | ||
| (mention) => mention.substring(mention_prefix.length) | ||
| ) | ||
| ).reduce( | ||
| (mentions, new_mentions) => mentions.concat(new_mentions), [] | ||
| )); | ||
| // Review and review comments | ||
| const already_reviewed = new Set(review_list.filter( | ||
| (review) => review.user !== null | ||
| ).map((review) => review.user.login)); | ||
| const already_commented_review = new Set(review_comments.filter( | ||
| (review) => review.user.login !== null | ||
| ).map((review) => review.user.login)); | ||
|
|
||
| const [ collaborators, non_collaborators ] = partition( | ||
| await Promise.all(individuals.filter((person) => ( | ||
| !review_requested.has(person) | ||
| && !already_mentioned.has(person) | ||
| && !already_reviewed.has(person) | ||
| && !already_commented_review.has(person) | ||
| && person !== context.payload.pull_request.user.login | ||
| )).map( | ||
| async (person) => ({ | ||
| person: person, | ||
| status: await is_collaborator(person), | ||
| }) | ||
| )), | ||
| ({ status }) => status | ||
| ).map( | ||
| (list) => list.map( | ||
| ({ person }) => person | ||
| ) | ||
| ); | ||
|
|
||
| const request_response = (collaborators.length === 0 && teams.length === 0) | ||
| ? null | ||
| : await octokit.pulls.requestReviewers({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.payload.pull_request.number, | ||
| reviewers: collaborators, | ||
| team_reviewers: teams, | ||
| }); | ||
|
|
||
| const mention_response = non_collaborators.length === 0 | ||
| ? null | ||
| : await octokit.pulls.createReview({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.payload.pull_request.number, | ||
| body: comment_prefix + non_collaborators.map((person) => mention_prefix + person).join(' '), | ||
| event: 'COMMENT', | ||
| }); | ||
|
|
||
| return { | ||
| request_response: request_response, | ||
| mention_response: mention_response, | ||
| }; |
There was a problem hiding this comment.
I have to say this function does now a bit too much. It should be broken down into smaller functions each of which does just one thing if possible.
I believe what you wanted to add is:
mentions the non-collaborators with a review comment instead. To avoid repeatedly mentioning the same people, it now checks the review comments for people who already left a review or were pinged by the action and skip them along with the already requested reviewers when requesting new reviews.
right?
I think it'd be better to have a separate function just for this, at least.
Also, am I right that if you have only collaborators, the behaviour of this action won't change? Right now, as it's all in one function, it's a bit hard to tell if that's the case to me.
There was a problem hiding this comment.
I agree the function has become a bit complex, I'll break it into smaller functions.
Also, am I right that if you have only collaborators, the behaviour of this action won't change? Right now, as it's all in one function, it's a bit hard to tell if that's the case to me.
Yes, collaborators are still assigned with review requests. Only non-collaborators are pinged via mentioning.
… allowing collaborators to be requested for review
… already mentioned as a non-collaborator, or has already left a top-level review
Update from upstream and fix is_collaborator
Fixes #98. Github restricts requested reviewers to people having write access to a repo (collaborators), but people may actually want to request non-collaborators for review.
This change separates non-collaborators from collaborators and mentions the non-collaborators with a review comment instead. To avoid repeatedly mentioning the same people, it now checks the review comments for people who already left a review or were pinged by the action and skip them along with the already requested reviewers when requesting new reviews. It also skips the PR author in case the author is not a collaborator.
A new test case is added to test the newly added functions. I also tested them in Qrox/Cataclysm-DDA#14 and Qrox/Cataclysm-DDA#15 (more detail in CleverRaven/Cataclysm-DDA#66072).