diff --git a/.github/actions/release-pr-bot/manage-release-pr.js b/.github/actions/release-pr-bot/manage-release-pr.js index 05d7fab..ba4d635 100644 --- a/.github/actions/release-pr-bot/manage-release-pr.js +++ b/.github/actions/release-pr-bot/manage-release-pr.js @@ -143,12 +143,24 @@ async function updateReleasePR({ github, owner, repo, pr, body, reviewers }) { }), ]) - const excludedLogins = new Set([ - ...currentReviews.users.map((u) => u.login), - ...submittedReviews - .filter((review) => review.state === 'APPROVED') - .map((review) => review.user?.login), - ]) + const excludedLogins = new Set(currentReviews.users.map((u) => u.login)) + const staleApprovers = new Set() + + const latestReviewByLogin = submittedReviews.reduce((map, review) => { + const login = review.user?.login + if (login) map.set(login, review) + return map + }, new Map()) + + Array.from(latestReviewByLogin.entries()).forEach(([login, review]) => { + if (review.state !== 'APPROVED') return + if (review.commit_id === pr.head.sha) { + excludedLogins.add(login) + } else { + staleApprovers.add(login) + } + }) + const newReviewers = reviewers.filter((r) => !excludedLogins.has(r)) if (newReviewers.length > 0) { @@ -158,6 +170,12 @@ async function updateReleasePR({ github, owner, repo, pr, body, reviewers }) { pull_number: pr.number, reviewers: newReviewers, }) + const reRequested = newReviewers.filter((r) => staleApprovers.has(r)) + if (reRequested.length > 0) { + console.log( + `Re-requested review from ${reRequested.join(', ')} (new commits since their approval)`, + ) + } console.log(`Added reviewers: ${newReviewers.join(', ')}`) } diff --git a/.github/actions/release-pr-bot/manage-release-pr.test.js b/.github/actions/release-pr-bot/manage-release-pr.test.js index a90b0d1..53f2ba5 100644 --- a/.github/actions/release-pr-bot/manage-release-pr.test.js +++ b/.github/actions/release-pr-bot/manage-release-pr.test.js @@ -94,13 +94,15 @@ describe('manage-release-pr', () => { expect(github.rest.pulls.requestReviewers).not.toHaveBeenCalled() }) - it('does not re-request reviews from people who already approved', async () => { + it('does not re-request reviews from people who already approved the current commit', async () => { const github = makeGithub({ commits: [{ sha: 'abc123' }], mergedPRs: [unreleasedPR], - openPRs: [{ number: 42 }], + openPRs: [{ number: 42, head: { sha: 'head-sha-1' } }], currentReviewers: [], - submittedReviews: [{ user: { login: 'alice' }, state: 'APPROVED' }], + submittedReviews: [ + { user: { login: 'alice' }, state: 'APPROVED', commit_id: 'head-sha-1' }, + ], }) await run({ github, context: makeContext() }) @@ -108,6 +110,71 @@ describe('manage-release-pr', () => { expect(github.rest.pulls.requestReviewers).not.toHaveBeenCalled() }) + it('re-requests review from someone who approved a stale commit', async () => { + const github = makeGithub({ + commits: [{ sha: 'abc123' }], + mergedPRs: [unreleasedPR], + openPRs: [{ number: 42, head: { sha: 'head-sha-2' } }], + currentReviewers: [], + submittedReviews: [ + { user: { login: 'alice' }, state: 'APPROVED', commit_id: 'head-sha-1' }, + ], + }) + + await run({ github, context: makeContext() }) + + expect(github.rest.pulls.requestReviewers).toHaveBeenCalledWith( + expect.objectContaining({ pull_number: 42, reviewers: ['alice'] }), + ) + }) + + it('uses the latest review per person when they reviewed more than once', async () => { + const github = makeGithub({ + commits: [{ sha: 'abc123' }], + mergedPRs: [unreleasedPR], + openPRs: [{ number: 42, head: { sha: 'head-sha-1' } }], + currentReviewers: [], + submittedReviews: [ + { user: { login: 'alice' }, state: 'APPROVED', commit_id: 'stale-sha' }, + { user: { login: 'alice' }, state: 'APPROVED', commit_id: 'head-sha-1' }, + ], + }) + + await run({ github, context: makeContext() }) + + expect(github.rest.pulls.requestReviewers).not.toHaveBeenCalled() + }) + + it('re-requests review only from the person whose approval went stale after a new commit', async () => { + const bobPR = { + number: 2, + title: 'Add feature', + merged_at: '2024-01-02T00:00:00Z', + merge_commit_sha: 'def456', + user: { login: 'bob' }, + } + + const github = makeGithub({ + commits: [{ sha: 'abc123' }, { sha: 'def456' }], + mergedPRs: [unreleasedPR, bobPR], + openPRs: [{ number: 42, head: { sha: 'head-sha-2' } }], + currentReviewers: [], + submittedReviews: [ + // alice reviewed before her second commit landed - now stale + { user: { login: 'alice' }, state: 'APPROVED', commit_id: 'head-sha-1' }, + // bob reviewed after that commit landed - still current + { user: { login: 'bob' }, state: 'APPROVED', commit_id: 'head-sha-2' }, + ], + }) + + await run({ github, context: makeContext() }) + + expect(github.rest.pulls.requestReviewers).toHaveBeenCalledTimes(1) + expect(github.rest.pulls.requestReviewers).toHaveBeenCalledWith( + expect.objectContaining({ pull_number: 42, reviewers: ['alice'] }), + ) + }) + it('excludes bots from reviewers', async () => { const github = makeGithub({ commits: [{ sha: 'abc123' }],