fix: add default reviewers to manual backports (#274)

* fix: add default reviewers to manual backport

* test: add test to updateManualBackport

* fix: pass undefined user for manual backport

* chore: remove .only and clean up imports

* test: use const for context variable in updateManualbackport test

---------

Co-authored-by: George Xu <[email protected]>

Author: alicelovescake

spec/fixtures/backport_pull_request.closed.json

@@ -12,6 +12,12 @@
       "head": {
         "ref": "123456789iuytdxcvbnjhfdriuyfedfgy54escghjnbg"
       },
+      "base": {
+        "ref": "main",
+        "repo": {
+          "default_branch": "main"
+        }
+      },
       "body": "Backport of #14\nSee that PR for details.\nNotes: <!-- One-line Change Summary Here-->",
       "created_at": "2018-11-01T17:29:51Z",
       "merged_at": "2018-11-01T17:30:11Z",

spec/fixtures/backport_pull_request.merged.json

@@ -12,6 +12,12 @@
       "head": {
         "ref": "123456789iuytdxcvbnjhfdriuyfedfgy54escghjnbg"
       },
+      "base": {
+        "ref": "main",
+        "repo": {
+          "default_branch": "main"
+        }
+      },
       "body": "Backport of #14\nSee that PR for details.\nNotes: <!-- One-line Change Summary Here-->",
       "created_at": "2018-11-01T17:29:51Z",
       "merged_at": "2018-11-01T17:30:11Z",

spec/fixtures/backport_pull_request.opened.json

@@ -4,6 +4,7 @@
     "action": "opened",
     "number": 7,
     "pull_request": {
+      "number": 7,
       "url": "my_cool_url",
       "title": "CHANGE README",
       "merged": true,

spec/index.spec.ts

@@ -392,6 +392,12 @@ Notes: <!-- One-line Change Summary Here-->`,
         head: {
           ref: '123456789iuytdxcvbnjhfdriuyfedfgy54escghjnbg',
         },
+        base: {
+          ref: 'main',
+          repo: {
+            default_branch: 'main',
+          },
+        },
         labels: [
           {
             color: 'ededed',
@@ -429,6 +435,12 @@ Notes: <!-- One-line Change Summary Here-->`,
         head: {
           ref: '123456789iuytdxcvbnjhfdriuyfedfgy54escghjnbg',
         },
+        base: {
+          ref: 'main',
+          repo: {
+            default_branch: 'main',
+          },
+        },
         labels: [
           {
             color: 'ededed',

spec/operations.spec.ts

@@ -3,8 +3,12 @@ import * as fs from 'fs-extra';
 import * as os from 'os';
 import * as path from 'path';
 import simpleGit from 'simple-git';
+
+import { PRChange } from '../src/enums';
 import { initRepo } from '../src/operations/init-repo';
 import { setupRemotes } from '../src/operations/setup-remotes';
+import { updateManualBackport } from '../src/operations/update-manual-backport';
+import { tagBackportReviewers } from '../src/utils';
 
 let dirObject: { dir?: string } | null = null;
 
@@ -13,6 +17,22 @@ const saveDir = (o: { dir: string }) => {
   return o.dir;
 };
 
+const backportPRClosedEvent = require('./fixtures/backport_pull_request.closed.json');
+const backportPRMergedEvent = require('./fixtures/backport_pull_request.merged.json');
+const backportPROpenedEvent = require('./fixtures/backport_pull_request.opened.json');
+
+jest.mock('../src/utils', () => ({
+  tagBackportReviewers: jest.fn().mockReturnValue(Promise.resolve()),
+  isSemverMinorPR: jest.fn().mockReturnValue(false),
+}));
+
+jest.mock('../src/utils/label-utils', () => ({
+  labelExistsOnPR: jest.fn().mockResolvedValue(true),
+  getSemverLabel: jest.fn().mockResolvedValue(false),
+  addLabels: jest.fn(),
+  removeLabel: jest.fn(),
+}));
+
 describe('runner', () => {
   jest.setTimeout(30000);
   console.error = jest.fn();
@@ -101,4 +121,50 @@ describe('runner', () => {
       }
     });
   });
+
+  describe('updateManualBackport()', () => {
+    const octokit = {
+      pulls: {
+        get: jest.fn().mockReturnValue(Promise.resolve({})),
+      },
+      issues: {
+        createComment: jest.fn().mockReturnValue(Promise.resolve({})),
+        listComments: jest.fn().mockReturnValue(Promise.resolve({ data: [] })),
+      },
+    };
+
+    it('tags reviewers on manual backport creation', async () => {
+      const context = {
+        ...backportPROpenedEvent,
+        octokit,
+        repo: jest.fn(),
+      };
+      await updateManualBackport(context, PRChange.OPEN, 1234);
+      expect(tagBackportReviewers).toHaveBeenCalled();
+      expect(tagBackportReviewers).toHaveBeenCalledWith({
+        context,
+        targetPrNumber: 7,
+      });
+    });
+
+    it('does not tag reviewers on merged PRs', async () => {
+      const context = {
+        ...backportPRMergedEvent,
+        octokit,
+        repo: jest.fn(),
+      };
+      await updateManualBackport(context, PRChange.MERGE, 1234);
+      expect(tagBackportReviewers).not.toHaveBeenCalled();
+    });
+
+    it('does not tag reviewers on closed PRs', async () => {
+      const context = {
+        ...backportPRClosedEvent,
+        octokit,
+        repo: jest.fn(),
+      };
+      await updateManualBackport(context, PRChange.CLOSE, 1234);
+      expect(tagBackportReviewers).not.toHaveBeenCalled();
+    });
+  });
 });

spec/queue.spec.ts

@@ -3,8 +3,6 @@ import * as sinon from 'sinon';
 
 import { ExecutionQueue } from '../src/Queue';
 
-const noop = async () => {};
-
 const waitForEvent = (emitter: EventEmitter, event: string) => {
   return new Promise<void>((resolve) => {
     emitter.once(event, resolve);

src/operations/update-manual-backport.ts

@@ -1,13 +1,13 @@
-import * as labelUtils from '../utils/label-utils';
-import { log } from '../utils/log-util';
-import { PRChange, PRStatus, LogLevel } from '../enums';
 import {
   BACKPORT_LABEL,
   BACKPORT_REQUESTED_LABEL,
   SKIP_CHECK_LABEL,
 } from '../constants';
-import { isSemverMinorPR } from '../utils';
+import { PRChange, PRStatus, LogLevel } from '../enums';
 import { WebHookPRContext } from '../types';
+import { isSemverMinorPR, tagBackportReviewers } from '../utils';
+import * as labelUtils from '../utils/label-utils';
+import { log } from '../utils/log-util';
 
 /**
  * Updates the labels on a backport's original PR as well as comments with links
@@ -135,6 +135,12 @@ please check out #${pr.number}`;
         }),
       );
     }
+
+    // Tag default reviewers to manual backport
+    await tagBackportReviewers({
+      context,
+      targetPrNumber: pr.number,
+    });
   } else if (type === PRChange.MERGE) {
     log(
       'updateManualBackport',

src/utils.ts

@@ -397,6 +397,38 @@ const createBackportComment = async (
   return body;
 };
 
+export const tagBackportReviewers = async ({
+  context,
+  targetPrNumber,
+  user,
+}: {
+  context: SimpleWebHookRepoContext;
+  targetPrNumber: number;
+  user?: string;
+}) => {
+  const reviewers = [];
+
+  if (DEFAULT_BACKPORT_REVIEW_TEAM) {
+    reviewers.push(DEFAULT_BACKPORT_REVIEW_TEAM);
+  }
+
+  if (user) {
+    const hasWrite = await checkUserHasWriteAccess(context, user);
+    // Optionally request a default review team for backports.
+    // If the PR author has write access, also request their review.
+    if (hasWrite) reviewers.push(user);
+  }
+
+  if (reviewers.length > 0) {
+    await context.octokit.pulls.requestReviewers(
+      context.repo({
+        pull_number: targetPrNumber,
+        reviewers,
+      }),
+    );
+  }
+};
+
 export const backportImpl = async (
   robot: Probot,
   context: SimpleWebHookRepoContext,
@@ -561,24 +593,11 @@ export const backportImpl = async (
           }),
         );
 
-        const reviewers = [];
-        const hasWrite = await checkUserHasWriteAccess(context, pr.user.login);
-
-        // Optionally request a default review team for backports.
-        // If the PR author has write access, also request their review.
-        if (hasWrite) reviewers.push(pr.user.login);
-        if (DEFAULT_BACKPORT_REVIEW_TEAM) {
-          reviewers.push(DEFAULT_BACKPORT_REVIEW_TEAM);
-        }
-
-        if (reviewers.length > 0) {
-          await context.octokit.pulls.requestReviewers(
-            context.repo({
-              pull_number: newPr.number,
-              reviewers,
-            }),
-          );
-        }
+        await tagBackportReviewers({
+          context,
+          targetPrNumber: newPr.number,
+          user: pr.user.login,
+        });
 
         log(
           'backportImpl',