diff --git a/.github/workflows/contributor-pr-reply.yml b/.github/workflows/contributor-pr-reply.yml index 071dace..cb22b75 100644 --- a/.github/workflows/contributor-pr-reply.yml +++ b/.github/workflows/contributor-pr-reply.yml @@ -38,7 +38,7 @@ jobs: with: app-id: ${{ secrets.LE_BOT_APP_ID }} private-key: ${{ secrets.LE_BOT_PRIVATE_KEY }} - permission-issues: read + permission-issues: write permission-pull-requests: write - name: Checkout .github repository uses: actions/checkout@v7 diff --git a/docs/community-automations.md b/docs/community-automations.md index 2af3466..6a9cbea 100644 --- a/docs/community-automations.md +++ b/docs/community-automations.md @@ -71,6 +71,12 @@ What happens depends on who the linked issue is assigned to: An issue counts as linked only when the description closes it, for example `Fixes #123`. Assignments on any other issue are ignored. If the author is assigned to any linked issue, the pull request is not closed. Close contributors are never closed or labelled `unlinked`. +When nothing is linked, the automation reads the description for open issues in the same repository, as `#123`, `owner/repo#123`, or an issue URL, outside code and HTML comments, and links some itself: + +- on a base other than the default branch, where GitHub ignores closing keywords: every issue after a closing keyword, whoever it is assigned to +- otherwise, the one mentioned issue assigned to the author; the reply names it +- several mentioned issues assigned to the author link none; the reply names them and asks which one + It runs on `opened`, and again on `edited` when the description changes. An edit sends no reply: it closes, labels, or removes `unlinked` by the rules above, and starts review only on a pull request that carried `unlinked`. The weekly `unassign-inactive-issues` run closes every open pull request labelled `unlinked` for longer than `PR_UNLINKED_MAX_DAYS` days, except a close contributor's. It re-reads the links first, because a link added from the Development sidebar fires no `edited` event: a pull request now linked to its author's issue loses the label and starts review instead. @@ -78,6 +84,8 @@ The weekly `unassign-inactive-issues` run closes every open pull request labelle In `scripts/constants.js` set: - `BOT_MESSAGE_PULL_REQUEST`: Message text - `BOT_MESSAGE_LINK_ISSUE`: Text added when no issue is linked +- `BOT_MESSAGE_LINKED_ISSUE`: Text added when the automation linked the author's issue +- `BOT_MESSAGE_LINK_WHICH_ISSUE`: Text added when several mentioned issues are assigned to the author - `BOT_MESSAGE_PULL_REQUEST_CLOSED`: Closing message when the issue is assigned to someone else - `BOT_MESSAGE_PULL_REQUEST_UNASSIGNED`: Closing message when the issue is assigned to no one - `BOT_MESSAGE_PULL_REQUEST_UNLINKED_CLOSED`: Closing message from the sweep when still no issue is linked diff --git a/scripts/constants.js b/scripts/constants.js index 3675fff..cb84506 100644 --- a/scripts/constants.js +++ b/scripts/constants.js @@ -166,7 +166,15 @@ const BOT_MESSAGE_PULL_REQUEST = author => const LABEL_UNLINKED = 'unlinked'; const PR_UNLINKED_MAX_DAYS = 7; -const BOT_MESSAGE_LINK_ISSUE = `**📎 We couldn't find an issue linked to this pull request.** Please edit the description and link the issue assigned to you under **References**, for example \`Fixes #123\` or \`Closes #123\`. A plain \`#123\` mention doesn't link the PR to the issue. **Pull requests with no linked issue are closed after ${PR_UNLINKED_MAX_DAYS} days.**`; +const BOT_MESSAGE_UNLINKED_DEADLINE = `**Pull requests with no linked issue are closed after ${PR_UNLINKED_MAX_DAYS} days.**`; + +const BOT_MESSAGE_LINK_ISSUE = `**📎 We couldn't find an issue linked to this pull request.** Please edit the description and link the issue assigned to you under **References**, for example \`Fixes #123\` or \`Closes #123\`. A plain \`#123\` mention doesn't link the PR to the issue. ${BOT_MESSAGE_UNLINKED_DEADLINE}\n\n**Open an issue and get assigned to it before you open a pull request.** Visit [Contributing guidelines](https://learningequality.org/contributing-to-our-open-code-base) to learn about the contributing process.`; + +const BOT_MESSAGE_LINKED_ISSUE = number => + `**📎 We linked this pull request to #${number}, the issue assigned to you.** Next time, link it yourself with \`Closes #${number}\` under **References**.`; + +const BOT_MESSAGE_LINK_WHICH_ISSUE = numbers => + `**📎 This pull request mentions more than one issue assigned to you: ${numbers.map(number => `#${number}`).join(', ')}.** Please edit the description and add \`Closes #123\` under **References** to say which one this pull request closes. ${BOT_MESSAGE_UNLINKED_DEADLINE}`; const BOT_MESSAGE_PULL_REQUEST_UNASSIGNED = (author, issueUrl) => `👋 Hi @${author}, thanks for your interest! \n\n **We're closing this pull request because ${issueUrl} isn't assigned to you.** We check and assign issues before work on them starts. Comment on the issue to ask to be assigned, and open a new pull request once you are. Visit [Contributing guidelines](https://learningequality.org/contributing-to-our-open-code-base) to learn about the contributing process.${GSOC_NOTE}`; @@ -254,6 +262,8 @@ module.exports = { BOT_MESSAGE_KEYWORD_GOOD_FIRST_ISSUE, BOT_MESSAGE_PULL_REQUEST, BOT_MESSAGE_LINK_ISSUE, + BOT_MESSAGE_LINKED_ISSUE, + BOT_MESSAGE_LINK_WHICH_ISSUE, BOT_MESSAGE_PULL_REQUEST_CLOSED, BOT_MESSAGE_PULL_REQUEST_UNASSIGNED, BOT_MESSAGE_PULL_REQUEST_UNLINKED_CLOSED, diff --git a/scripts/contributor-pr-reply.js b/scripts/contributor-pr-reply.js index 78b7021..e1774b9 100644 --- a/scripts/contributor-pr-reply.js +++ b/scripts/contributor-pr-reply.js @@ -1,6 +1,11 @@ -const { BOT_MESSAGE_PULL_REQUEST, BOT_MESSAGE_LINK_ISSUE } = require('./constants'); +const { + BOT_MESSAGE_PULL_REQUEST, + BOT_MESSAGE_LINK_ISSUE, + BOT_MESSAGE_LINKED_ISSUE, + BOT_MESSAGE_LINK_WHICH_ISSUE, +} = require('./constants'); const { sendBotMessage, getLinkedIssues, escapeIssueTitleForSlackMessage } = require('./utils'); -const { linkStatus, planLink, applyLink } = require('./pull-request-link'); +const { linkReferences, linkStatus, planLink, applyLink } = require('./pull-request-link'); function slackLine(context, text) { const { repo } = context.repo; @@ -8,11 +13,17 @@ function slackLine(context, text) { return `*[${repo}] ${text}: <${url}|${escapeIssueTitleForSlackMessage(title)}>*`; } -async function sendReply({ github, context, core }, unlinked) { +function linkNote(link, { named, ambiguous }) { + if (named) return BOT_MESSAGE_LINKED_ISSUE(named.number); + if (link.status !== 'unlinked') return null; + if (ambiguous.length) return BOT_MESSAGE_LINK_WHICH_ISSUE(ambiguous.map(issue => issue.number)); + return BOT_MESSAGE_LINK_ISSUE; +} + +async function sendReply({ github, context, core }, note) { const { number, user } = context.payload.pull_request; - const body = unlinked - ? `${BOT_MESSAGE_PULL_REQUEST(user.login)}\n\n${BOT_MESSAGE_LINK_ISSUE}` - : BOT_MESSAGE_PULL_REQUEST(user.login); + const reply = BOT_MESSAGE_PULL_REQUEST(user.login); + const body = note ? `${reply}\n\n${note}` : reply; const botMessageUrl = await sendBotMessage(number, body, { github, context, core }); core.setOutput( 'slack_notification', @@ -63,15 +74,18 @@ module.exports = async ({ github, context, core }) => { const lookup = await getLinkedIssues(number, { github, context, core }); if (lookup === null) { if (opening) { - await sendReply({ github, context, core }, false); + await sendReply({ github, context, core }, null); } return; } - const link = linkStatus(lookup, author); + const found = lookup.length + ? { linked: lookup, named: null, ambiguous: [] } + : await linkReferences(owner, repo, pullRequest, { github, core }); + const link = linkStatus(found.linked, author); const first = await plan(link); if (opening && first.action !== 'close') { - await sendReply({ github, context, core }, link.status === 'unlinked'); + await sendReply({ github, context, core }, linkNote(link, found)); } await act({ github, context, core }, first, link); diff --git a/scripts/contributor-pr-reply.test.js b/scripts/contributor-pr-reply.test.js index 6013989..656d418 100644 --- a/scripts/contributor-pr-reply.test.js +++ b/scripts/contributor-pr-reply.test.js @@ -15,7 +15,7 @@ const CLOSE_CONTRIBUTOR = CLOSE_CONTRIBUTORS[0]; function fakeContext( author = AUTHOR, title = 'Fix it', - { action = 'opened', state = 'open', labels = [] } = {}, + { action = 'opened', state = 'open', labels = [], body = '', base = 'develop' } = {}, ) { return { repo: { owner: 'learningequality', repo: 'kolibri' }, @@ -23,16 +23,21 @@ function fakeContext( action, pull_request: { number: 42, + node_id: 'PR_42', html_url: 'https://github.com/learningequality/kolibri/pull/42', title, + body, state, labels: labels.map(name => ({ name })), user: { login: author }, + base: { ref: base, repo: { default_branch: 'develop' } }, }, }, }; } +const describing = (body, options = {}) => fakeContext(AUTHOR, 'Fix it', { body, ...options }); + const edited = (author = AUTHOR, options = {}) => fakeContext(author, 'Fix it', { action: 'edited', ...options }); @@ -57,18 +62,66 @@ function fakeCore() { }; } -function fakeGithub(linkedIssues = [], { graphqlError, relinked } = {}) { - const calls = { comments: [], updates: [], reviewers: [], labels: [], removed: [], order: [] }; +function referencedNode(number, { assignees = [], state = 'OPEN', type = 'Issue' } = {}) { + if (type !== 'Issue') { + return { __typename: type }; + } + return { + __typename: type, + id: `I_${number}`, + number, + url: `https://github.com/learningequality/kolibri/issues/${number}`, + state, + assignees: { nodes: assignees.map(login => ({ login })) }, + }; +} + +function lookUpReferences(query, referenced) { + const repository = {}; + for (const [, alias, number] of query.matchAll(/(\w+): issueOrPullRequest\(number: (\d+)\)/g)) { + const issue = referenced[number]; + repository[alias] = issue ? referencedNode(Number(number), issue) : null; + } + if (Object.values(repository).includes(null)) { + throw Object.assign(new Error('Could not resolve to an issue'), { data: { repository } }); + } + return { repository }; +} + +function fakeGithub( + linkedIssues = [], + { graphqlError, relinked, referenced = {}, linkError } = {}, +) { + const calls = { + comments: [], + updates: [], + reviewers: [], + labels: [], + removed: [], + links: [], + order: [], + }; const current = new Set(); let lookups = 0; return { calls, current, paginate: async (fn, params) => fn(params), - graphql: async () => { + graphql: async (query, variables) => { if (graphqlError) { throw new Error(graphqlError); } + if (query.includes('addCloseIssueReferences')) { + if (linkError) { + throw new Error(linkError); + } + calls.links.push(variables); + calls.order.push('link'); + return {}; + } + if (query.includes('issueOrPullRequest')) { + return lookUpReferences(query, referenced); + } lookups += 1; const issues = lookups > 1 && relinked ? relinked : linkedIssues; return { @@ -374,3 +427,122 @@ test('a failed lookup does not ask the author to link an issue', async () => { assert.doesNotMatch(github.calls.comments[0].body, /link the issue/); }); + +test('a mentioned issue assigned to the author is linked and named', async () => { + const github = fakeGithub([], { referenced: { 7: { assignees: [AUTHOR] } } }); + await run( + github, + describing('Fix for [the bug](https://github.com/learningequality/kolibri/issues/7)'), + ); + + assert.deepEqual(github.calls.links, [{ issueId: 'I_7', pullRequestIds: ['PR_42'] }]); + assert.match(github.calls.comments[0].body, /We linked this pull request to #7/); + assert.match(github.calls.comments[0].body, /`Closes #7`/); + assert.doesNotMatch(github.calls.comments[0].body, /couldn't find an issue/); + assert.deepEqual(github.calls.reviewers, [RTIBBLESBOT_USERNAME]); + assert.deepEqual(github.calls.labels, [LABEL_COMMUNITY_REVIEW]); +}); + +test('a mentioned issue the author cannot claim is not linked', async () => { + const cases = [ + { assignees: ['someone-else'] }, + { assignees: [] }, + { assignees: [AUTHOR], state: 'CLOSED' }, + { type: 'PullRequest' }, + ]; + for (const issue of cases) { + const github = fakeGithub([], { referenced: { 7: issue } }); + await run(github, describing('See #7')); + + assert.deepEqual(github.calls.links, []); + assert.deepEqual(github.calls.updates, []); + assert.deepEqual(github.calls.labels, [LABEL_UNLINKED]); + assert.match(github.calls.comments[0].body, /couldn't find an issue/); + } +}); + +test('two mentioned issues assigned to the author are named and neither is linked', async () => { + const github = fakeGithub([], { + referenced: { 7: { assignees: [AUTHOR] }, 8: { assignees: [AUTHOR] } }, + }); + await run(github, describing('See #7 and #8')); + + assert.deepEqual(github.calls.links, []); + assert.deepEqual(github.calls.labels, [LABEL_UNLINKED]); + assert.match(github.calls.comments[0].body, /#7, #8/); + assert.match(github.calls.comments[0].body, /which one this pull request closes/); +}); + +test('a missing issue number does not hide the other mentions', async () => { + const github = fakeGithub([], { referenced: { 7: { assignees: [AUTHOR] } } }); + const core = await run(github, describing('See #7 and #9999')); + + assert.equal(github.calls.links.length, 1); + assert.deepEqual(core.failures, []); +}); + +test('an unlinked pull request is told the issue comes first', async () => { + const github = fakeGithub([]); + await run(github, describing('A drive-by fix')); + + assert.match(github.calls.comments[0].body, /Open an issue and get assigned to it/); +}); + +test('a closing keyword on another base links the issue the way the default branch would', async () => { + const github = fakeGithub([], { referenced: { 7: { assignees: ['someone-else'] } } }); + await run(github, describing('Fixes #7', { base: 'release-v0.20.x' })); + + assert.deepEqual(github.calls.links, [{ issueId: 'I_7', pullRequestIds: ['PR_42'] }]); + assert.deepEqual(github.calls.updates, [{ pull_number: 42, state: 'closed' }]); + assert.match(github.calls.comments[0].body, /issues\/7 is assigned to someone else/); +}); + +test('a closing keyword on another base starts review without naming the link', async () => { + const github = fakeGithub([], { referenced: { 7: { assignees: [AUTHOR] } } }); + await run(github, describing('Fixes #7', { base: 'release-v0.20.x' })); + + assert.equal(github.calls.links.length, 1); + assert.deepEqual(github.calls.reviewers, [RTIBBLESBOT_USERNAME]); + assert.doesNotMatch(github.calls.comments[0].body, /We linked/); +}); + +test('a closing keyword on the default branch is left to GitHub', async () => { + const github = fakeGithub([], { referenced: { 7: { assignees: ['someone-else'] } } }); + await run(github, describing('Fixes #7')); + + assert.deepEqual(github.calls.links, []); + assert.deepEqual(github.calls.updates, []); +}); + +test('a pull request that already links an issue reads no mentions', async () => { + const github = fakeGithub([{ number: 7, assignees: [AUTHOR] }], { + referenced: { 8: { assignees: [AUTHOR] } }, + }); + await run(github, describing('Fixes #7, see #8')); + + assert.deepEqual(github.calls.links, []); +}); + +test('a refused link falls back to asking', async () => { + const github = fakeGithub([], { + referenced: { 7: { assignees: [AUTHOR] } }, + linkError: 'Resource not accessible', + }); + const core = await run(github, describing('See #7')); + + assert.match(github.calls.comments[0].body, /couldn't find an issue/); + assert.doesNotMatch(github.calls.comments[0].body, /We linked/); + assert.deepEqual(github.calls.labels, [LABEL_UNLINKED]); + assert.match(core.warnings.join('\n'), /Resource not accessible/); + assert.deepEqual(core.failures, []); +}); + +test('an edit mentioning the issue assigned to the author links it and starts review', async () => { + const github = fakeGithub([], { referenced: { 7: { assignees: [AUTHOR] } } }); + await run(github, edited(AUTHOR, { labels: [LABEL_UNLINKED], body: 'See #7' })); + + assert.equal(github.calls.links.length, 1); + assert.deepEqual(github.calls.comments, []); + assert.deepEqual(github.calls.removed, [LABEL_UNLINKED]); + assert.deepEqual(github.calls.reviewers, [RTIBBLESBOT_USERNAME]); +}); diff --git a/scripts/pull-request-link.js b/scripts/pull-request-link.js index 12ec65d..9fa6aa4 100644 --- a/scripts/pull-request-link.js +++ b/scripts/pull-request-link.js @@ -5,7 +5,40 @@ const { LABEL_UNLINKED, RTIBBLESBOT_USERNAME, } = require('./constants'); -const { addLabel, removeLabel, isCloseContributor } = require('./utils'); +const { + addLabel, + removeLabel, + isCloseContributor, + findIssueReferences, + getReferencedIssues, + addClosingIssueReference, +} = require('./utils'); + +function chooseReferences(issues, author, defaultBase) { + const closing = defaultBase ? [] : issues.filter(issue => issue.keyword); + if (closing.length) return { link: closing, named: false, ambiguous: [] }; + const assigned = issues.filter(issue => issue.assignees.includes(author)); + if (assigned.length === 1) return { link: assigned, named: true, ambiguous: [] }; + return { link: [], named: false, ambiguous: assigned.length > 1 ? assigned : [] }; +} + +async function linkReferences(owner, repo, pullRequest, { github, core }) { + const references = findIssueReferences(pullRequest.body, owner, repo); + if (!references.length) return { linked: [], named: null, ambiguous: [] }; + const issues = await getReferencedIssues(owner, repo, references, { github, core }); + const defaultBase = pullRequest.base.ref === pullRequest.base.repo.default_branch; + const { link, named, ambiguous } = chooseReferences(issues, pullRequest.user.login, defaultBase); + const linked = []; + for (const issue of link) { + try { + await addClosingIssueReference(issue.id, pullRequest.node_id, github); + linked.push(issue); + } catch (error) { + core.warning(`Failed to link #${issue.number} to #${pullRequest.number}: ${error.message}`); + } + } + return { linked, named: named && linked.length ? linked[0] : null, ambiguous }; +} function linkStatus(linkedIssues, author) { if (!linkedIssues.length) return { status: 'unlinked' }; @@ -86,4 +119,4 @@ async function applyLink(owner, repo, number, plan, { github, core }) { return null; } -module.exports = { linkStatus, planLink, applyLink }; +module.exports = { linkReferences, linkStatus, planLink, applyLink }; diff --git a/scripts/utils.js b/scripts/utils.js index 494c7ae..d1e9f02 100644 --- a/scripts/utils.js +++ b/scripts/utils.js @@ -305,6 +305,94 @@ async function getLinkedIssues(prNumber, { github, context, core }) { } } +const UNREAD_MARKDOWN = /|(`{3,}|~{3,})[\s\S]*?(?:\1|$)|`[^`\n]*`/g; +const ISSUE_REFERENCE = new RegExp( + String.raw`https?://github\.com/([\w.-]+)/([\w.-]+)/issues/(\d+)` + + String.raw`|(? ` + issue${number}: issueOrPullRequest(number: ${number}) { + __typename + ... on Issue { + id + number + url + state + assignees(first: 10) { + nodes { + login + } + } + } + }`, + ) + .join(''); + const query = ` + query ($owner: String!, $repo: String!) { + repository(owner: $owner, name: $repo) {${fields} + } + }`; + + let repository; + try { + ({ repository } = await github.graphql(query, { owner, repo })); + } catch (error) { + repository = error.data?.repository; + if (!repository) { + core.warning(`Failed to fetch referenced issues: ${error.message}`); + return []; + } + } + return references + .map(({ number, keyword }) => ({ issue: repository[`issue${number}`], keyword })) + .filter(({ issue }) => issue?.__typename === 'Issue' && issue.state === 'OPEN') + .map(({ issue, keyword }) => ({ + id: issue.id, + number: issue.number, + url: issue.url, + keyword, + assignees: issue.assignees.nodes.map(assignee => assignee.login), + })); +} + +async function addClosingIssueReference(issueId, pullRequestId, github) { + await github.graphql( + `mutation ($issueId: ID!, $pullRequestIds: [ID!]!) { + addCloseIssueReferences(input: { issueId: $issueId, pullRequestIds: $pullRequestIds }) { + clientMutationId + } + }`, + { issueId, pullRequestIds: [pullRequestId] }, + ); +} + /** * Fetches issues assigned to an assignee in given repositories. */ @@ -456,6 +544,9 @@ module.exports = { addLabel, removeLabel, getLinkedIssues, + findIssueReferences, + getReferencedIssues, + addClosingIssueReference, getIssues, getPullRequests, deleteBotComments, diff --git a/scripts/utils.test.js b/scripts/utils.test.js index 6355ad4..997d830 100644 --- a/scripts/utils.test.js +++ b/scripts/utils.test.js @@ -1,7 +1,69 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { escapeIssueTitleForSlackMessage } = require('./utils'); +const { escapeIssueTitleForSlackMessage, findIssueReferences } = require('./utils'); + +const references = body => findIssueReferences(body, 'learningequality', 'kolibri'); + +test('every reference form names an issue in the same repository', () => { + const body = [ + 'See #1.', + 'Also learningequality/kolibri#2,', + 'https://github.com/learningequality/kolibri/issues/3 and', + '[the issue](https://github.com/LearningEquality/Kolibri/issues/4)', + ].join('\n'); + + assert.deepEqual( + references(body).map(r => r.number), + [1, 2, 3, 4], + ); +}); + +test('a reference to another repository is ignored', () => { + const body = 'learningequality/studio#5 and https://github.com/learningequality/studio/issues/6'; + + assert.deepEqual(references(body), []); +}); + +test('a reference in code or a comment is ignored', () => { + const body = [ + 'Use `#1` here.', + '```', + 'Fixes #2', + '```', + '', + '~~~js', + '#4', + '~~~', + ].join('\n'); + + assert.deepEqual(references(body), []); +}); + +test('a hash inside a word, an entity or an anchor is not a reference', () => { + const body = 'issue#1  https://github.com/learningequality/kolibri/pull/3#issuecomment-4'; + + assert.deepEqual(references(body), []); +}); + +test('a closing keyword marks the reference', () => { + const body = 'Fixes #1, closes: #2, Resolved [#3](url), see #4'; + + assert.deepEqual(references(body), [ + { number: 1, keyword: true }, + { number: 2, keyword: true }, + { number: 3, keyword: true }, + { number: 4, keyword: false }, + ]); +}); + +test('a repeated reference appears once, closing if any mention closes it', () => { + assert.deepEqual(references('See #7. Fixes #7'), [{ number: 7, keyword: true }]); +}); + +test('an empty description has no references', () => { + assert.deepEqual(references(null), []); +}); function payload(title) { return `{\n "text": "on issue: "\n}`;