Link the issue a pull request description names when GitHub did not

.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

docs/community-automations.md

@@ -71,13 +71,21 @@ 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.
 
 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

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,

scripts/contributor-pr-reply.js

@@ -1,18 +1,29 @@
-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;
   const { html_url: url, title } = context.payload.pull_request;
   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);
 

scripts/contributor-pr-reply.test.js

@@ -15,24 +15,29 @@ 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' },
     payload: {
       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]);
+});

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 };