diff --git a/.github/scripts/project-status-sweep.js b/.github/scripts/project-status-sweep.js index 4789172c5..8cff03641 100644 --- a/.github/scripts/project-status-sweep.js +++ b/.github/scripts/project-status-sweep.js @@ -8,7 +8,10 @@ * * Todo issue with no assignee and no trusted open linked PR * In Progress issue assigned or with a trusted open linked PR; any PR that - * is a draft or has an outstanding changes-requested verdict + * is a draft + * Changes requested + * ready PR with a changes-requested verdict from a reviewer + * who has not been asked to review again: waits on the author * Needs review PR that is ready (not a draft) and waits on a reviewer: no * review from another person yet, or its changes-requested * reviewers have all been asked to review again @@ -24,7 +27,7 @@ * It never touches: * - closed or merged items (the board's built-in workflows set Done) * - draft (board-only), inaccessible and archived items - * - a card whose Status is not one of the four above (e.g. Done, Blocked) + * - a card whose Status is not one of the five above (e.g. Done, Blocked) * - a card whose "Auto status" field is "Off" (a permanent hold) * - a card a person moved by hand, until the issue, the PR, or the issue's * trusted linked PR changes again. "By hand" is read from the item's @@ -54,7 +57,7 @@ const HOLD_FIELD = 'Auto status'; const HOLD_OFF = 'Off'; const HEARTBEAT_PREFIX = 'Board sweep:'; // Statuses the sweep manages, lowest to highest. -const MANAGED = ['Todo', 'In Progress', 'Needs review', 'Review']; +const MANAGED = ['Todo', 'In Progress', 'Changes requested', 'Needs review', 'Review']; const TRUSTED = new Set(['OWNER', 'MEMBER', 'COLLABORATOR']); const REVIEW_STATES = new Set(['APPROVED', 'CHANGES_REQUESTED', 'COMMENTED', 'DISMISSED']); // Built-in board workflows that also set Status and would fight the sweep. @@ -206,7 +209,9 @@ function prStatus(pr) { const blocking = verdicts.filter( (r) => r.state === 'CHANGES_REQUESTED' && !requested.has(r.author.login), ); - if (blocking.length > 0) return 'In Progress'; + // A reviewer asked for changes and the author has not asked them back yet: + // the ball is with the author. + if (blocking.length > 0) return 'Changes requested'; // Every changes-requested reviewer was asked to look again: the ball is // back with a reviewer. if (verdicts.some((r) => r.state === 'CHANGES_REQUESTED')) return 'Needs review'; diff --git a/.github/scripts/project-status-sweep.test.js b/.github/scripts/project-status-sweep.test.js index 8504b4806..cbaf978c0 100644 --- a/.github/scripts/project-status-sweep.test.js +++ b/.github/scripts/project-status-sweep.test.js @@ -87,7 +87,7 @@ function fakeGithub({ pages, workflows = [], failUpdates = 0, linkedById = {}, f }; } -const OPTIONS = ['Todo', 'In Progress', 'Needs review', 'Review', 'Done'].map((name) => ({ id: `OPT_${name}`, name })); +const OPTIONS = ['Todo', 'In Progress', 'Changes requested', 'Needs review', 'Review', 'Done'].map((name) => ({ id: `OPT_${name}`, name })); const boardPage = (nodes, { hasNextPage = false, remaining = 4000, field = { id: 'F', options: OPTIONS } } = {}) => ({ rateLimit: { cost: 9, remaining }, @@ -151,19 +151,38 @@ test('an approval moves a PR to Review', () => { assert.equal(prStatus(pr({ verdicts: { nodes: [verdict(user('bob'), 'APPROVED')] } })), 'Review'); }); -test('changes requested sends a PR back to In Progress', () => { +test('changes requested moves a PR to Changes requested', () => { const p = pr({ verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] }, reviews: { nodes: [review(user('bob'), 'CHANGES_REQUESTED'), review(user('alice'), 'COMMENTED')] }, }); - assert.equal(prStatus(p), 'In Progress'); + assert.equal(prStatus(p), 'Changes requested'); }); test('one approval does not outweigh another reviewer requesting changes', () => { const p = pr({ verdicts: { nodes: [verdict(user('bob'), 'APPROVED'), verdict(user('cat'), 'CHANGES_REQUESTED')] } }); + assert.equal(prStatus(p), 'Changes requested'); +}); + +test('a draft PR with changes requested stays In Progress', () => { + const p = pr({ isDraft: true, verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] } }); assert.equal(prStatus(p), 'In Progress'); }); +test('a bot requesting changes does not move a PR to Changes requested', () => { + assert.equal(prStatus(pr({ verdicts: { nodes: [verdict(bot('some-bot'), 'CHANGES_REQUESTED')] } })), 'Needs review'); +}); + +test("the author's own changes-requested verdict does not count", () => { + assert.equal(prStatus(pr({ verdicts: { nodes: [verdict(user('alice'), 'CHANGES_REQUESTED')] } })), 'Needs review'); +}); + +test('a dismissed changes-requested review (no longer a verdict) is Review, not Changes requested', () => { + // GitHub drops a dismissed review from latestOpinionatedReviews; it stays in reviews. + const p = pr({ reviews: { nodes: [review(user('bob'), 'DISMISSED')] } }); + assert.equal(prStatus(p), 'Review'); +}); + test('re-requesting review from the reviewer who asked for changes moves it to Needs review', () => { const p = pr({ verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] }, @@ -184,12 +203,12 @@ test('a draft PR with no reviews stays In Progress', () => { assert.equal(prStatus(pr({ isDraft: true })), 'In Progress'); }); -test('re-requesting only some blocking reviewers keeps In Progress', () => { +test('re-requesting only some blocking reviewers keeps Changes requested', () => { const p = pr({ verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED'), verdict(user('cat'), 'CHANGES_REQUESTED')] }, reviewRequests: { nodes: [request('bob')] }, }); - assert.equal(prStatus(p), 'In Progress'); + assert.equal(prStatus(p), 'Changes requested'); }); test('a stale changes-requested verdict is not lost behind many later comments', () => { @@ -197,7 +216,7 @@ test('a stale changes-requested verdict is not lost behind many later comments', verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] }, reviews: { nodes: Array.from({ length: 30 }, () => review(user('bob'), 'COMMENTED')) }, }); - assert.equal(prStatus(p), 'In Progress'); + assert.equal(prStatus(p), 'Changes requested'); }); test('a PR whose only review was dismissed counts as reviewed', () => { @@ -244,6 +263,17 @@ test('an issue takes the furthest of its linked PRs', () => { assert.equal(issueStatus(i), 'Review'); }); +test('issue follows its trusted linked PR into Changes requested', () => { + const blocked = pr({ verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] } }); + assert.equal(issueStatus(issue({ assignees: { totalCount: 1 }, linked: linked(blocked) })), 'Changes requested'); +}); + +test('an issue with one PR waiting on a reviewer and one on its author shows Needs review', () => { + const blocked = pr({ id: 'PR_2', verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] } }); + const i = issue({ linked: { totalCount: 2, nodes: [blocked, pr()] } }); + assert.equal(issueStatus(i), 'Needs review'); +}); + test('a merged or closed linked PR is ignored', () => { assert.equal(issueStatus(issue({ linked: linked(pr({ state: 'MERGED' })) })), 'Todo'); }); @@ -359,6 +389,7 @@ test('the public log names no issue, PR, status or draft; the private card has t item(pr({ number: 501, verdicts: { nodes: [verdict(user('bob'), 'APPROVED')] } }), 'Todo'), item(issue({ number: 602, assignees: { totalCount: 1 } }), 'Todo'), item(pr({ number: 703 }), 'Todo'), + item(pr({ number: 804, verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] } }), 'Todo'), item({ __typename: 'DraftIssue', id: 'D1', title: 'Secret launch plan' }, 'Todo'), item(null, 'Todo'), ]; @@ -369,11 +400,11 @@ test('the public log names no issue, PR, status or draft; the private card has t }); const { core } = await run(gh); const publicLog = [...core.info, ...core.warning, ...core.failed].join('\n'); - for (const leak of ['#501', '#602', '#703', 'Todo', 'In Progress', 'Needs review', 'Review', 'review', 'draft', 'Secret', 'linked to issue', 'inaccessible', 'boom']) { + for (const leak of ['#501', '#602', '#703', '#804', 'Todo', 'In Progress', 'Changes requested', 'Needs review', 'Review', 'review', 'draft', 'Secret', 'linked to issue', 'inaccessible', 'boom']) { assert.ok(!publicLog.includes(leak), `public log leaked "${leak}": ${publicLog}`); } const card = gh.writes.find((w) => w.kind === 'heartbeat-add'); - for (const detail of ['PR #501', 'issue #602', 'PR #703: Todo -> Needs review', 'Pull request linked to issue', 'draft item 1', 'boom']) { + for (const detail of ['PR #501', 'issue #602', 'PR #703: Todo -> Needs review', 'PR #804: Todo -> Changes requested', 'Pull request linked to issue', 'draft item 1', 'boom']) { assert.ok(card.body.includes(detail), `card is missing "${detail}"`); } }); @@ -418,6 +449,28 @@ test('a renamed status option is reported, not fatal', async () => { assert.equal(core.failed.length, 0); }); +test('before the Changes requested column exists, those cards stay put and the run is degraded', async () => { + const field = { id: 'F', options: OPTIONS.filter((o) => o.name !== 'Changes requested') }; + const blocked = pr({ number: 5, verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] } }); + const gh = fakeGithub({ pages: [boardPage([item(blocked, 'In Progress'), item(pr({ number: 6 }), 'Todo')], { field })] }); + const { result, core } = await run(gh); + assert.equal(result.outcome, 'degraded'); + assert.ok(result.problems.some((m) => m.includes('PR #5') && m.includes('no "Changes requested" status option'))); + assert.deepEqual(gh.writes.filter((w) => w.kind === 'status').map((w) => w.item), ['ITEM_6']); + assert.equal(core.failed.length, 0); +}); + +test('a card already in Changes requested is managed, and moves on once review is re-requested', async () => { + const back = pr({ + number: 5, + verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] }, + reviewRequests: { nodes: [request('bob')] }, + }); + const gh = fakeGithub({ pages: [boardPage([item(back, 'Changes requested')])] }); + const { result } = await run(gh); + assert.deepEqual(result.changes.map((c) => [c.label, c.from, c.to]), [['PR #5', 'Changes requested', 'Needs review']]); +}); + test('a missing Status field changes nothing and is reported', async () => { const gh = fakeGithub({ pages: [boardPage([item(pr({ number: 5 }), 'Todo')], { field: null })] }); const { result } = await run(gh);