From 5bf0040730dad5232f4ed9bea9228f507788129d Mon Sep 17 00:00:00 2001 From: Qrox Date: Sun, 28 May 2023 18:49:21 +0800 Subject: [PATCH 1/7] Mention non-collaborators to work around Github's restriction of only allowing collaborators to be requested for review --- dist/index.js | 69 ++++++++++++++++++++++++++++++++++++++++----- src/github.js | 69 ++++++++++++++++++++++++++++++++++++++++----- test/github.test.js | 45 +++++++++++++++++++++++++---- 3 files changed, 164 insertions(+), 19 deletions(-) diff --git a/dist/index.js b/dist/index.js index b80ea99..23aab98 100644 --- a/dist/index.js +++ b/dist/index.js @@ -16001,6 +16001,30 @@ async function fetch_changed_files() { return changed_files; } +async function is_collaborator(person) { + const context = get_context(); + const octokit = get_octokit(); + + 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; + } + ); +} + async function assign_reviewers(reviewers) { const context = get_context(); const octokit = get_octokit(); @@ -16008,13 +16032,44 @@ async function assign_reviewers(reviewers) { const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); - return octokit.pulls.requestReviewers({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - reviewers: individuals, - team_reviewers: teams, - }); + const [ collaborators, non_collaborators ] = partition( + await Promise.all(individuals.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: 'Auto-requesting reviews from non-collaborators: ' + non_collaborators.map((person) => '@' + person).join(' '), + event: 'COMMENT', + }); + + return { + request_response: request_response, + mention_response: mention_response, + }; } /* Private */ diff --git a/src/github.js b/src/github.js index 11ff8ad..1b6d7fe 100644 --- a/src/github.js +++ b/src/github.js @@ -96,6 +96,30 @@ async function fetch_changed_files() { return changed_files; } +async function is_collaborator(person) { + const context = get_context(); + const octokit = get_octokit(); + + 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; + } + ); +} + async function assign_reviewers(reviewers) { const context = get_context(); const octokit = get_octokit(); @@ -103,13 +127,44 @@ async function assign_reviewers(reviewers) { const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); - return octokit.pulls.requestReviewers({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - reviewers: individuals, - team_reviewers: teams, - }); + const [ collaborators, non_collaborators ] = partition( + await Promise.all(individuals.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: 'Auto-requesting reviews from non-collaborators: ' + non_collaborators.map((person) => '@' + person).join(' '), + event: 'COMMENT', + }); + + return { + request_response: request_response, + mention_response: mention_response, + }; } /* Private */ diff --git a/test/github.test.js b/test/github.test.js index 7a21ca5..07a933d 100644 --- a/test/github.test.js +++ b/test/github.test.js @@ -124,24 +124,37 @@ describe('github', function() { }); describe('assign_reviewers()', function() { - const spy = sinon.spy(); + const request_spy = sinon.spy(); + const mention_spy = sinon.spy(); + const stub = sinon.stub(); const octokit = { pulls: { - requestReviewers: spy, + requestReviewers: request_spy, + createReview: mention_spy, + }, + repos: { + checkCollaborator: stub, }, }; beforeEach(function() { + request_spy.resetHistory(); + mention_spy.resetHistory(); + stub.reset(); github.getOctokit.resetBehavior(); github.getOctokit.returns(octokit); }); - it('assigns reviewers', async function() { + it('assigns collaborators and teams as reviewers', async function() { + stub.resolves({ status: 204 }); + const reviewers = [ 'mario', 'princess-peach', 'team:koopa-troop' ]; await assign_reviewers(reviewers); - expect(spy.calledOnce).to.be.true; - expect(spy.lastCall.args[0]).to.deep.equal({ + expect(stub.calledTwice).to.be.true; + + expect(request_spy.calledOnce).to.be.true; + expect(request_spy.lastCall.args[0]).to.deep.equal({ owner: 'necojackarc', pull_number: 18, repo: 'auto-request-review', @@ -153,6 +166,28 @@ describe('github', function() { 'koopa-troop', ], }); + + expect(mention_spy.notCalled).to.be.true; + }); + + it('assigns non-collaborators as reviewers', async function() { + stub.rejects({ status: 404 }); + + const reviewers = [ 'mario', 'princess-peach' ]; + await assign_reviewers(reviewers); + + expect(stub.calledTwice).to.be.true; + + expect(request_spy.notCalled).to.be.true; + + expect(mention_spy.calledOnce).to.be.true; + expect(mention_spy.lastCall.args[0]).to.deep.equal({ + owner: 'necojackarc', + pull_number: 18, + repo: 'auto-request-review', + body: 'Auto-requesting reviews from non-collaborators: @mario @princess-peach', + event: 'COMMENT', + }); }); }); }); From 7c4db04512cf03ba63574c7b1fcb8de03ac89da8 Mon Sep 17 00:00:00 2001 From: Qrox Date: Thu, 8 Jun 2023 17:38:07 +0800 Subject: [PATCH 2/7] Do not request review if a person is already requested for review, is already mentioned as a non-collaborator, or has already left a top-level review --- dist/index.js | 45 ++++++++++++++++++++++++++++++++++-- src/github.js | 45 ++++++++++++++++++++++++++++++++++-- test/github.test.js | 56 +++++++++++++++++++++++++++++++++++++-------- 3 files changed, 133 insertions(+), 13 deletions(-) diff --git a/dist/index.js b/dist/index.js index 23aab98..c7eda77 100644 --- a/dist/index.js +++ b/dist/index.js @@ -16032,8 +16032,49 @@ async function assign_reviewers(reviewers) { const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); + 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, + } + ); + // 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), [] + )); + // Only consider top-level reviews + const already_reviewed = new Set(review_list.filter( + (review) => review.user !== null + ).map((review) => review.user.login)); + const [ collaborators, non_collaborators ] = partition( - await Promise.all(individuals.map( + await Promise.all(individuals.filter((person) => ( + !review_requested.has(person) + && !already_mentioned.has(person) + && !already_reviewed.has(person) + && person !== context.payload.pull_request.user.login + )).map( async (person) => ({ person: person, status: await is_collaborator(person), @@ -16062,7 +16103,7 @@ async function assign_reviewers(reviewers) { owner: context.repo.owner, repo: context.repo.repo, pull_number: context.payload.pull_request.number, - body: 'Auto-requesting reviews from non-collaborators: ' + non_collaborators.map((person) => '@' + person).join(' '), + body: comment_prefix + non_collaborators.map((person) => mention_prefix + person).join(' '), event: 'COMMENT', }); diff --git a/src/github.js b/src/github.js index 1b6d7fe..d1d6e01 100644 --- a/src/github.js +++ b/src/github.js @@ -127,8 +127,49 @@ async function assign_reviewers(reviewers) { const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); + 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, + } + ); + // 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), [] + )); + // Only consider top-level reviews + const already_reviewed = new Set(review_list.filter( + (review) => review.user !== null + ).map((review) => review.user.login)); + const [ collaborators, non_collaborators ] = partition( - await Promise.all(individuals.map( + await Promise.all(individuals.filter((person) => ( + !review_requested.has(person) + && !already_mentioned.has(person) + && !already_reviewed.has(person) + && person !== context.payload.pull_request.user.login + )).map( async (person) => ({ person: person, status: await is_collaborator(person), @@ -157,7 +198,7 @@ async function assign_reviewers(reviewers) { owner: context.repo.owner, repo: context.repo.repo, pull_number: context.payload.pull_request.number, - body: 'Auto-requesting reviews from non-collaborators: ' + non_collaborators.map((person) => '@' + person).join(' '), + body: comment_prefix + non_collaborators.map((person) => mention_prefix + person).join(' '), event: 'COMMENT', }); diff --git a/test/github.test.js b/test/github.test.js index 07a933d..6649b6d 100644 --- a/test/github.test.js +++ b/test/github.test.js @@ -126,32 +126,49 @@ describe('github', function() { describe('assign_reviewers()', function() { const request_spy = sinon.spy(); const mention_spy = sinon.spy(); - const stub = sinon.stub(); + const collab_stub = sinon.stub(); + const paginate_stub = sinon.stub(); const octokit = { pulls: { requestReviewers: request_spy, createReview: mention_spy, + listRequestedReviewers: sinon.spy(), + listReviews: sinon.spy(), }, repos: { - checkCollaborator: stub, + checkCollaborator: collab_stub, }, + paginate: paginate_stub, }; beforeEach(function() { request_spy.resetHistory(); mention_spy.resetHistory(); - stub.reset(); + collab_stub.reset(); + paginate_stub.reset(); github.getOctokit.resetBehavior(); github.getOctokit.returns(octokit); }); it('assigns collaborators and teams as reviewers', async function() { - stub.resolves({ status: 204 }); + collab_stub.resolves({ status: 204 }); + const requests_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listRequestedReviewers), + sinon.match.any + ); + requests_call.resolves([]); + const reviews_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listReviews), + sinon.match.any + ); + reviews_call.resolves([]); const reviewers = [ 'mario', 'princess-peach', 'team:koopa-troop' ]; await assign_reviewers(reviewers); - expect(stub.calledTwice).to.be.true; + expect(collab_stub.calledTwice).to.be.true; + expect(requests_call.calledOnce).to.be.true; + expect(reviews_call.calledOnce).to.be.true; expect(request_spy.calledOnce).to.be.true; expect(request_spy.lastCall.args[0]).to.deep.equal({ @@ -171,12 +188,33 @@ describe('github', function() { }); it('assigns non-collaborators as reviewers', async function() { - stub.rejects({ status: 404 }); + collab_stub.rejects({ status: 404 }); + const requests_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listRequestedReviewers), + sinon.match.any + ); + requests_call.resolves([ 'mario' ]); + const reviews_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listReviews), + sinon.match.any + ); + reviews_call.resolves([ + { + body: 'Auto-requesting reviews from non-collaborators: @yoshi', + user: { login: 'bot' }, + }, + { + body: 'Looks good!', + user: { login: 'princess-peach' }, + }, + ]); - const reviewers = [ 'mario', 'princess-peach' ]; + const reviewers = [ 'mario', 'princess-peach', 'luigi', 'yoshi' ]; await assign_reviewers(reviewers); - expect(stub.calledTwice).to.be.true; + expect(collab_stub.calledOnce).to.be.true; + expect(requests_call.calledOnce).to.be.true; + expect(reviews_call.calledOnce).to.be.true; expect(request_spy.notCalled).to.be.true; @@ -185,7 +223,7 @@ describe('github', function() { owner: 'necojackarc', pull_number: 18, repo: 'auto-request-review', - body: 'Auto-requesting reviews from non-collaborators: @mario @princess-peach', + body: 'Auto-requesting reviews from non-collaborators: @luigi', event: 'COMMENT', }); }); From 481e6cd2f5d57b6e914d81ca731211384a76798f Mon Sep 17 00:00:00 2001 From: Qrox Date: Thu, 8 Jun 2023 21:44:07 +0800 Subject: [PATCH 3/7] Do not request review if a person already left a non-top-level review comment --- dist/index.js | 14 +++++++++++++- src/github.js | 14 +++++++++++++- test/github.test.js | 14 ++++++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/dist/index.js b/dist/index.js index c7eda77..b81d0b1 100644 --- a/dist/index.js +++ b/dist/index.js @@ -16051,6 +16051,14 @@ async function assign_reviewers(reviewers) { 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) @@ -16063,16 +16071,20 @@ async function assign_reviewers(reviewers) { ).reduce( (mentions, new_mentions) => mentions.concat(new_mentions), [] )); - // Only consider top-level reviews + // 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) => ({ diff --git a/src/github.js b/src/github.js index d1d6e01..82404fe 100644 --- a/src/github.js +++ b/src/github.js @@ -146,6 +146,14 @@ async function assign_reviewers(reviewers) { 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) @@ -158,16 +166,20 @@ async function assign_reviewers(reviewers) { ).reduce( (mentions, new_mentions) => mentions.concat(new_mentions), [] )); - // Only consider top-level reviews + // 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) => ({ diff --git a/test/github.test.js b/test/github.test.js index 6649b6d..36d3383 100644 --- a/test/github.test.js +++ b/test/github.test.js @@ -134,6 +134,7 @@ describe('github', function() { createReview: mention_spy, listRequestedReviewers: sinon.spy(), listReviews: sinon.spy(), + listReviewComments: sinon.spy(), }, repos: { checkCollaborator: collab_stub, @@ -162,6 +163,11 @@ describe('github', function() { sinon.match.any ); reviews_call.resolves([]); + const review_comments_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listReviewComments), + sinon.match.any + ); + review_comments_call.resolves([]); const reviewers = [ 'mario', 'princess-peach', 'team:koopa-troop' ]; await assign_reviewers(reviewers); @@ -169,6 +175,7 @@ describe('github', function() { expect(collab_stub.calledTwice).to.be.true; expect(requests_call.calledOnce).to.be.true; expect(reviews_call.calledOnce).to.be.true; + expect(review_comments_call.calledOnce).to.be.true; expect(request_spy.calledOnce).to.be.true; expect(request_spy.lastCall.args[0]).to.deep.equal({ @@ -203,6 +210,12 @@ describe('github', function() { body: 'Auto-requesting reviews from non-collaborators: @yoshi', user: { login: 'bot' }, }, + ]); + const review_comments_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listReviewComments), + sinon.match.any + ); + review_comments_call.resolves([ { body: 'Looks good!', user: { login: 'princess-peach' }, @@ -215,6 +228,7 @@ describe('github', function() { expect(collab_stub.calledOnce).to.be.true; expect(requests_call.calledOnce).to.be.true; expect(reviews_call.calledOnce).to.be.true; + expect(review_comments_call.calledOnce).to.be.true; expect(request_spy.notCalled).to.be.true; From c55ae5db64dc9a33eff508e7ded4adb062d121c1 Mon Sep 17 00:00:00 2001 From: Qrox Date: Sun, 28 May 2023 18:49:21 +0800 Subject: [PATCH 4/7] Mention non-collaborators to work around Github's restriction of only allowing collaborators to be requested for review --- dist/index.js | 69 ++++++++++++++++++++++++++++++++++++++++----- src/github.js | 69 ++++++++++++++++++++++++++++++++++++++++----- test/github.test.js | 45 +++++++++++++++++++++++++---- 3 files changed, 164 insertions(+), 19 deletions(-) diff --git a/dist/index.js b/dist/index.js index b80ea99..23aab98 100644 --- a/dist/index.js +++ b/dist/index.js @@ -16001,6 +16001,30 @@ async function fetch_changed_files() { return changed_files; } +async function is_collaborator(person) { + const context = get_context(); + const octokit = get_octokit(); + + 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; + } + ); +} + async function assign_reviewers(reviewers) { const context = get_context(); const octokit = get_octokit(); @@ -16008,13 +16032,44 @@ async function assign_reviewers(reviewers) { const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); - return octokit.pulls.requestReviewers({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - reviewers: individuals, - team_reviewers: teams, - }); + const [ collaborators, non_collaborators ] = partition( + await Promise.all(individuals.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: 'Auto-requesting reviews from non-collaborators: ' + non_collaborators.map((person) => '@' + person).join(' '), + event: 'COMMENT', + }); + + return { + request_response: request_response, + mention_response: mention_response, + }; } /* Private */ diff --git a/src/github.js b/src/github.js index 11ff8ad..1b6d7fe 100644 --- a/src/github.js +++ b/src/github.js @@ -96,6 +96,30 @@ async function fetch_changed_files() { return changed_files; } +async function is_collaborator(person) { + const context = get_context(); + const octokit = get_octokit(); + + 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; + } + ); +} + async function assign_reviewers(reviewers) { const context = get_context(); const octokit = get_octokit(); @@ -103,13 +127,44 @@ async function assign_reviewers(reviewers) { const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); - return octokit.pulls.requestReviewers({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - reviewers: individuals, - team_reviewers: teams, - }); + const [ collaborators, non_collaborators ] = partition( + await Promise.all(individuals.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: 'Auto-requesting reviews from non-collaborators: ' + non_collaborators.map((person) => '@' + person).join(' '), + event: 'COMMENT', + }); + + return { + request_response: request_response, + mention_response: mention_response, + }; } /* Private */ diff --git a/test/github.test.js b/test/github.test.js index 7a21ca5..07a933d 100644 --- a/test/github.test.js +++ b/test/github.test.js @@ -124,24 +124,37 @@ describe('github', function() { }); describe('assign_reviewers()', function() { - const spy = sinon.spy(); + const request_spy = sinon.spy(); + const mention_spy = sinon.spy(); + const stub = sinon.stub(); const octokit = { pulls: { - requestReviewers: spy, + requestReviewers: request_spy, + createReview: mention_spy, + }, + repos: { + checkCollaborator: stub, }, }; beforeEach(function() { + request_spy.resetHistory(); + mention_spy.resetHistory(); + stub.reset(); github.getOctokit.resetBehavior(); github.getOctokit.returns(octokit); }); - it('assigns reviewers', async function() { + it('assigns collaborators and teams as reviewers', async function() { + stub.resolves({ status: 204 }); + const reviewers = [ 'mario', 'princess-peach', 'team:koopa-troop' ]; await assign_reviewers(reviewers); - expect(spy.calledOnce).to.be.true; - expect(spy.lastCall.args[0]).to.deep.equal({ + expect(stub.calledTwice).to.be.true; + + expect(request_spy.calledOnce).to.be.true; + expect(request_spy.lastCall.args[0]).to.deep.equal({ owner: 'necojackarc', pull_number: 18, repo: 'auto-request-review', @@ -153,6 +166,28 @@ describe('github', function() { 'koopa-troop', ], }); + + expect(mention_spy.notCalled).to.be.true; + }); + + it('assigns non-collaborators as reviewers', async function() { + stub.rejects({ status: 404 }); + + const reviewers = [ 'mario', 'princess-peach' ]; + await assign_reviewers(reviewers); + + expect(stub.calledTwice).to.be.true; + + expect(request_spy.notCalled).to.be.true; + + expect(mention_spy.calledOnce).to.be.true; + expect(mention_spy.lastCall.args[0]).to.deep.equal({ + owner: 'necojackarc', + pull_number: 18, + repo: 'auto-request-review', + body: 'Auto-requesting reviews from non-collaborators: @mario @princess-peach', + event: 'COMMENT', + }); }); }); }); From 8bda7c1fa3a3e5b5ecd6f54f5135e1988612bb12 Mon Sep 17 00:00:00 2001 From: Qrox Date: Thu, 8 Jun 2023 17:38:07 +0800 Subject: [PATCH 5/7] Do not request review if a person is already requested for review, is already mentioned as a non-collaborator, or has already left a top-level review --- dist/index.js | 45 ++++++++++++++++++++++++++++++++++-- src/github.js | 45 ++++++++++++++++++++++++++++++++++-- test/github.test.js | 56 +++++++++++++++++++++++++++++++++++++-------- 3 files changed, 133 insertions(+), 13 deletions(-) diff --git a/dist/index.js b/dist/index.js index 23aab98..c7eda77 100644 --- a/dist/index.js +++ b/dist/index.js @@ -16032,8 +16032,49 @@ async function assign_reviewers(reviewers) { const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); + 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, + } + ); + // 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), [] + )); + // Only consider top-level reviews + const already_reviewed = new Set(review_list.filter( + (review) => review.user !== null + ).map((review) => review.user.login)); + const [ collaborators, non_collaborators ] = partition( - await Promise.all(individuals.map( + await Promise.all(individuals.filter((person) => ( + !review_requested.has(person) + && !already_mentioned.has(person) + && !already_reviewed.has(person) + && person !== context.payload.pull_request.user.login + )).map( async (person) => ({ person: person, status: await is_collaborator(person), @@ -16062,7 +16103,7 @@ async function assign_reviewers(reviewers) { owner: context.repo.owner, repo: context.repo.repo, pull_number: context.payload.pull_request.number, - body: 'Auto-requesting reviews from non-collaborators: ' + non_collaborators.map((person) => '@' + person).join(' '), + body: comment_prefix + non_collaborators.map((person) => mention_prefix + person).join(' '), event: 'COMMENT', }); diff --git a/src/github.js b/src/github.js index 1b6d7fe..d1d6e01 100644 --- a/src/github.js +++ b/src/github.js @@ -127,8 +127,49 @@ async function assign_reviewers(reviewers) { const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); + 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, + } + ); + // 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), [] + )); + // Only consider top-level reviews + const already_reviewed = new Set(review_list.filter( + (review) => review.user !== null + ).map((review) => review.user.login)); + const [ collaborators, non_collaborators ] = partition( - await Promise.all(individuals.map( + await Promise.all(individuals.filter((person) => ( + !review_requested.has(person) + && !already_mentioned.has(person) + && !already_reviewed.has(person) + && person !== context.payload.pull_request.user.login + )).map( async (person) => ({ person: person, status: await is_collaborator(person), @@ -157,7 +198,7 @@ async function assign_reviewers(reviewers) { owner: context.repo.owner, repo: context.repo.repo, pull_number: context.payload.pull_request.number, - body: 'Auto-requesting reviews from non-collaborators: ' + non_collaborators.map((person) => '@' + person).join(' '), + body: comment_prefix + non_collaborators.map((person) => mention_prefix + person).join(' '), event: 'COMMENT', }); diff --git a/test/github.test.js b/test/github.test.js index 07a933d..6649b6d 100644 --- a/test/github.test.js +++ b/test/github.test.js @@ -126,32 +126,49 @@ describe('github', function() { describe('assign_reviewers()', function() { const request_spy = sinon.spy(); const mention_spy = sinon.spy(); - const stub = sinon.stub(); + const collab_stub = sinon.stub(); + const paginate_stub = sinon.stub(); const octokit = { pulls: { requestReviewers: request_spy, createReview: mention_spy, + listRequestedReviewers: sinon.spy(), + listReviews: sinon.spy(), }, repos: { - checkCollaborator: stub, + checkCollaborator: collab_stub, }, + paginate: paginate_stub, }; beforeEach(function() { request_spy.resetHistory(); mention_spy.resetHistory(); - stub.reset(); + collab_stub.reset(); + paginate_stub.reset(); github.getOctokit.resetBehavior(); github.getOctokit.returns(octokit); }); it('assigns collaborators and teams as reviewers', async function() { - stub.resolves({ status: 204 }); + collab_stub.resolves({ status: 204 }); + const requests_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listRequestedReviewers), + sinon.match.any + ); + requests_call.resolves([]); + const reviews_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listReviews), + sinon.match.any + ); + reviews_call.resolves([]); const reviewers = [ 'mario', 'princess-peach', 'team:koopa-troop' ]; await assign_reviewers(reviewers); - expect(stub.calledTwice).to.be.true; + expect(collab_stub.calledTwice).to.be.true; + expect(requests_call.calledOnce).to.be.true; + expect(reviews_call.calledOnce).to.be.true; expect(request_spy.calledOnce).to.be.true; expect(request_spy.lastCall.args[0]).to.deep.equal({ @@ -171,12 +188,33 @@ describe('github', function() { }); it('assigns non-collaborators as reviewers', async function() { - stub.rejects({ status: 404 }); + collab_stub.rejects({ status: 404 }); + const requests_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listRequestedReviewers), + sinon.match.any + ); + requests_call.resolves([ 'mario' ]); + const reviews_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listReviews), + sinon.match.any + ); + reviews_call.resolves([ + { + body: 'Auto-requesting reviews from non-collaborators: @yoshi', + user: { login: 'bot' }, + }, + { + body: 'Looks good!', + user: { login: 'princess-peach' }, + }, + ]); - const reviewers = [ 'mario', 'princess-peach' ]; + const reviewers = [ 'mario', 'princess-peach', 'luigi', 'yoshi' ]; await assign_reviewers(reviewers); - expect(stub.calledTwice).to.be.true; + expect(collab_stub.calledOnce).to.be.true; + expect(requests_call.calledOnce).to.be.true; + expect(reviews_call.calledOnce).to.be.true; expect(request_spy.notCalled).to.be.true; @@ -185,7 +223,7 @@ describe('github', function() { owner: 'necojackarc', pull_number: 18, repo: 'auto-request-review', - body: 'Auto-requesting reviews from non-collaborators: @mario @princess-peach', + body: 'Auto-requesting reviews from non-collaborators: @luigi', event: 'COMMENT', }); }); From 321544c3cb11da73e01b81591576fb70e8fd46a1 Mon Sep 17 00:00:00 2001 From: Qrox Date: Thu, 8 Jun 2023 21:44:07 +0800 Subject: [PATCH 6/7] Do not request review if a person already left a non-top-level review comment --- dist/index.js | 14 +++++++++++++- src/github.js | 14 +++++++++++++- test/github.test.js | 14 ++++++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/dist/index.js b/dist/index.js index c7eda77..b81d0b1 100644 --- a/dist/index.js +++ b/dist/index.js @@ -16051,6 +16051,14 @@ async function assign_reviewers(reviewers) { 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) @@ -16063,16 +16071,20 @@ async function assign_reviewers(reviewers) { ).reduce( (mentions, new_mentions) => mentions.concat(new_mentions), [] )); - // Only consider top-level reviews + // 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) => ({ diff --git a/src/github.js b/src/github.js index d1d6e01..82404fe 100644 --- a/src/github.js +++ b/src/github.js @@ -146,6 +146,14 @@ async function assign_reviewers(reviewers) { 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) @@ -158,16 +166,20 @@ async function assign_reviewers(reviewers) { ).reduce( (mentions, new_mentions) => mentions.concat(new_mentions), [] )); - // Only consider top-level reviews + // 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) => ({ diff --git a/test/github.test.js b/test/github.test.js index 6649b6d..36d3383 100644 --- a/test/github.test.js +++ b/test/github.test.js @@ -134,6 +134,7 @@ describe('github', function() { createReview: mention_spy, listRequestedReviewers: sinon.spy(), listReviews: sinon.spy(), + listReviewComments: sinon.spy(), }, repos: { checkCollaborator: collab_stub, @@ -162,6 +163,11 @@ describe('github', function() { sinon.match.any ); reviews_call.resolves([]); + const review_comments_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listReviewComments), + sinon.match.any + ); + review_comments_call.resolves([]); const reviewers = [ 'mario', 'princess-peach', 'team:koopa-troop' ]; await assign_reviewers(reviewers); @@ -169,6 +175,7 @@ describe('github', function() { expect(collab_stub.calledTwice).to.be.true; expect(requests_call.calledOnce).to.be.true; expect(reviews_call.calledOnce).to.be.true; + expect(review_comments_call.calledOnce).to.be.true; expect(request_spy.calledOnce).to.be.true; expect(request_spy.lastCall.args[0]).to.deep.equal({ @@ -203,6 +210,12 @@ describe('github', function() { body: 'Auto-requesting reviews from non-collaborators: @yoshi', user: { login: 'bot' }, }, + ]); + const review_comments_call = paginate_stub.withArgs( + sinon.match.same(octokit.pulls.listReviewComments), + sinon.match.any + ); + review_comments_call.resolves([ { body: 'Looks good!', user: { login: 'princess-peach' }, @@ -215,6 +228,7 @@ describe('github', function() { expect(collab_stub.calledOnce).to.be.true; expect(requests_call.calledOnce).to.be.true; expect(reviews_call.calledOnce).to.be.true; + expect(review_comments_call.calledOnce).to.be.true; expect(request_spy.notCalled).to.be.true; From 070f23693324676521af0630ce8e46dbcf120aaa Mon Sep 17 00:00:00 2001 From: Procyonae <45432782+Procyonae@users.noreply.github.com> Date: Tue, 26 Nov 2024 00:52:18 +0000 Subject: [PATCH 7/7] Verbose info messages and drop strict equality so the codes are matched correctly --- dist/index.js | 10 ++++++++-- src/github.js | 10 ++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/dist/index.js b/dist/index.js index b81d0b1..6f46cf6 100644 --- a/dist/index.js +++ b/dist/index.js @@ -16011,15 +16011,21 @@ async function is_collaborator(person) { username: person, }).then( (response) => { - if (response.status === 204) { + core.info(`is_collaborator(${person}) recieved response code: ${response.status}`); + if (response.status == 204) { + core.info(`${person} is a collaborator`); return true; } + core.info(`Unhandled response code: ${response.status}, ${person} being treated as not a collaborator`); return false; }, (error) => { - if (error.status === 404) { + core.info(`is_collaborator(${person}) recieved error code: ${error.status}`); + if (error.status == 404) { + core.info(`${person} isn't a collaborator`); return false; } + core.info(`Unhandled error code: ${error.status}`); throw error; } ); diff --git a/src/github.js b/src/github.js index 82404fe..9f15d2b 100644 --- a/src/github.js +++ b/src/github.js @@ -106,15 +106,21 @@ async function is_collaborator(person) { username: person, }).then( (response) => { - if (response.status === 204) { + core.info(`is_collaborator(${person}) recieved response code: ${response.status}`); + if (response.status == 204) { + core.info(`${person} is a collaborator`); return true; } + core.info(`Unhandled response code: ${response.status}, ${person} being treated as not a collaborator`); return false; }, (error) => { - if (error.status === 404) { + core.info(`is_collaborator(${person}) recieved error code: ${error.status}`); + if (error.status == 404) { + core.info(`${person} isn't a collaborator`); return false; } + core.info(`Unhandled error code: ${error.status}`); throw error; } );