Approved roles

CHANGELOG.md

@@ -2,14 +2,12 @@
 
 ## 1.0.0 (2024-07-29)
 
-
 ### Features
 
-* database update step ([5bad80a](https://github.com/ubiquibot/automated-merging/commit/5bad80a8049890dcf16a5661caadfdacc89fdf2b))
-* set db to be sqlite ([2dbe73b](https://github.com/ubiquibot/automated-merging/commit/2dbe73be10f9ae436050f6b3626890db847c166c))
-
+- database update step ([5bad80a](https://github.com/ubiquibot/automated-merging/commit/5bad80a8049890dcf16a5661caadfdacc89fdf2b))
+- set db to be sqlite ([2dbe73b](https://github.com/ubiquibot/automated-merging/commit/2dbe73be10f9ae436050f6b3626890db847c166c))
 
 ### Bug Fixes
 
-* changed approval requirement check to use the configuration ([e1f50e9](https://github.com/ubiquibot/automated-merging/commit/e1f50e95576f81ce01196bbdc0890b0617bf23df))
-* fixed imports within main ([bb001cf](https://github.com/ubiquibot/automated-merging/commit/bb001cf3204593a79b2d214941940a9a44675c00))
+- changed approval requirement check to use the configuration ([e1f50e9](https://github.com/ubiquibot/automated-merging/commit/e1f50e95576f81ce01196bbdc0890b0617bf23df))
+- fixed imports within main ([bb001cf](https://github.com/ubiquibot/automated-merging/commit/bb001cf3204593a79b2d214941940a9a44675c00))

README.md

@@ -1,6 +1,6 @@
 # `@ubiquibot/automated-merging`
 
-Automatically merge pull-requests based on the reviewer count, the time elapsed since the last activity, depending 
+Automatically merge pull-requests based on the reviewer count, the time elapsed since the last activity, depending
 on the association of the pull-request author.
 
 ## Configuration example
@@ -17,6 +17,10 @@ on the association of the pull-request author.
     mergeTimeout:
       collaborator: "3.5 days" # defaults to 3.5 days
       contributor: "7 days" # defaults to 7 days
+    repos: 
+      monitor: ["ubiquibot/automated-merging"]
+      ignore: ["ubiquibot/automated-merging"]
+    allowedReviewerRoles: ["COLLABORATOR", "MEMBER", "OWNER"]
 ```
 
 ## Testing

manifest.json

@@ -1,5 +1,5 @@
 {
   "name": "Automated merging",
   "description": "Automatically merge pull-requests.",
-  "ubiquity:listeners": [ "push", "issue_comment.created" ]
+  "ubiquity:listeners": ["push", "issue_comment.created"]
 }

package.json

@@ -84,5 +84,6 @@
     "extends": [
       "@commitlint/config-conventional"
     ]
-  }
-}
+  },
+  "packageManager": "yarn@1.22.22"
+}

src/helpers/github.ts

@@ -24,25 +24,32 @@ export interface Requirements {
  * statuses, the longest timeout is chosen.
  */
 export async function getMergeTimeoutAndApprovalRequiredCount(context: Context, authorAssociation: string): Promise<Requirements> {
+  const { config: { mergeTimeout, approvalsRequired } } = context;
   const timeoutCollaborator = {
-    mergeTimeout: context.config.mergeTimeout.collaborator,
-    requiredApprovalCount: context.config.approvalsRequired.collaborator,
+    mergeTimeout: mergeTimeout.collaborator,
+    requiredApprovalCount: approvalsRequired.collaborator,
   };
   const timeoutContributor = {
-    mergeTimeout: context.config.mergeTimeout.contributor,
-    requiredApprovalCount: context.config.approvalsRequired.contributor,
+    mergeTimeout: mergeTimeout.contributor,
+    requiredApprovalCount: approvalsRequired.contributor,
   };
+
+  /**
+   * Hardcoded roles here because we need to determine the timeouts
+   * separate from `allowedReviewerRoles` which introduces 
+   * potential unintended user errors and logic issues.
+   */
   return ["COLLABORATOR", "MEMBER", "OWNER"].includes(authorAssociation) ? timeoutCollaborator : timeoutContributor;
 }
 
-export async function getApprovalCount({ octokit, logger }: Context, { owner, repo, issue_number: pullNumber }: IssueParams) {
+export async function getApprovalCount({ octokit, logger, config: { allowedReviewerRoles } }: Context, { owner, repo, issue_number: pullNumber }: IssueParams) {
   try {
     const { data: reviews } = await octokit.rest.pulls.listReviews({
       owner,
       repo,
       pull_number: pullNumber,
     });
-    return reviews.filter((review) => review.state === "APPROVED").length;
+    return reviews.filter((review) => allowedReviewerRoles.includes(review.author_association)).filter((review) => review.state === "APPROVED").length;
   } catch (e) {
     logger.error(`Error fetching reviews' approvals: ${e}`);
     return 0;

src/types/plugin-inputs.ts

@@ -55,13 +55,18 @@ export const reposSchema = T.Object(
   { default: {} }
 );
 
+const allowedReviewerRoles = T.Array(T.String(), { default: ["COLLABORATOR", "MEMBER", "OWNER"] })
+
 export const pluginSettingsSchema = T.Object({
   approvalsRequired: approvalsRequiredSchema,
   mergeTimeout: mergeTimeoutSchema,
   /**
    * The list of organizations or repositories to watch for updates.
    */
   repos: reposSchema,
+  allowedReviewerRoles: T.Transform(allowedReviewerRoles)
+    .Decode((roles) => roles.map((role) => role.toUpperCase()))
+    .Encode((roles) => roles.map((role) => role.toUpperCase()))
 });
 
 export const pluginSettingsValidator = new StandardValidator(pluginSettingsSchema);

tests/__mocks__/db.ts

@@ -24,5 +24,6 @@ export const db = factory({
   reviews: {
     id: primaryKey(Number),
     state: String,
+    author_association: String,
   },
 });

tests/__mocks__/routes/search-pull-requests.json

@@ -33,15 +33,11 @@
         "type": "User",
         "site_admin": false
       },
-      "labels": [
-
-      ],
+      "labels": [],
       "state": "open",
       "locked": false,
       "assignee": null,
-      "assignees": [
-
-      ],
+      "assignees": [],
       "milestone": null,
       "comments": 24,
       "created_at": "2024-07-02T15:25:13Z",

tests/__mocks__/seed.json

@@ -18,11 +18,23 @@
   "reviews": [
     {
       "id": 1,
-      "state": "APPROVED"
+      "state": "APPROVED",
+      "author_association": "MEMBER"
     },
     {
       "id": 2,
-      "state": "APPROVED"
+      "state": "APPROVED",
+      "author_association": "COLLABORATOR"
+    },
+    {
+      "id": 3,
+      "state": "APPROVED",
+      "author_association": "CONTRIBUTOR"
+    },
+    {
+      "id": 4,
+      "state": "APPROVED",
+      "author_association": "NONE"
     }
   ]
-}
+}

tests/main.test.ts

@@ -62,6 +62,7 @@ describe("Action tests", () => {
             eventName: "push",
             settings: JSON.stringify({
               repos: { monitor: [monitor] },
+              allowedReviewerRoles: ["COLLABORATOR", "MEMBER", "OWNER"],
             }),
             eventPayload: JSON.stringify({
               pull_request: {
@@ -154,6 +155,59 @@ describe("Action tests", () => {
     expect(mergePullRequest).toHaveBeenCalled();
   });
 
+  it("Should not close a PR if non-approved reviews are present", async () => {
+    server.use(
+      http.get(
+        "https://api.github.com/repos/:org/:repo/pulls/:id/reviews",
+        () => {
+          return HttpResponse.json([{ id: 1, state: "COMMENTED", author_association: "CONTRIBUTOR" }, { id: 2, state: "APPROVED", author_association: "NONE" }]);
+        },
+        { once: true }
+      )
+    );
+    jest.mock(actionsGithubPackage, () => ({
+      context: {
+        repo: {
+          owner: {
+            login: "ubiquibot",
+          },
+        },
+        workflow,
+        payload: {
+          inputs: {
+            eventName: "push",
+            settings: JSON.stringify({
+              repos: { monitor: [monitor] },
+            }),
+            eventPayload: JSON.stringify({
+              pull_request: {
+                html_url: htmlUrl,
+              },
+              repository: {
+                owner: "ubiquibot",
+              },
+            }),
+            env: {
+              workflowName: workflow,
+            },
+          },
+        },
+      },
+    }));
+    const mergePullRequest = jest.fn();
+    jest.mock(githubHelpersPath, () => {
+      const actualModule = jest.requireActual(githubHelpersPath) as object;
+      return {
+        __esModule: true,
+        ...actualModule,
+        mergePullRequest,
+      };
+    });
+    const run = (await import("../src/action")).run;
+    await expect(run()).resolves.toMatchObject({ status: 200 });
+    expect(mergePullRequest).not.toHaveBeenCalled();
+  });
+
   it("Should pick the timeout according to the assignees status", async () => {
     const contributorMergeTimeout = "7 days";
     const collaboratorMergeTimeout = "3.5 days";
@@ -177,6 +231,7 @@ describe("Action tests", () => {
           collaborator: collaboratorMinimumApprovalsRequired,
           contributor: contributorMinimumApprovalsRequired,
         },
+        allowedReviewerRoles: ["COLLABORATOR", "MEMBER", "OWNER"],
       },
       octokit: new Octokit(),
     } as unknown as Context;
@@ -228,7 +283,9 @@ describe("Action tests", () => {
         },
       },
       workflow: "other workflow",
-      config: {},
+      config: {
+        allowedReviewerRoles: ["COLLABORATOR", "MEMBER", "OWNER"],
+      },
       octokit: new Octokit(),
       env: {
         workflowName: workflow,

tsconfig.json

@@ -12,8 +12,8 @@
     "target": "ES2022" /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */,
     // "lib": [],                                        /* Specify a set of bundled library declaration files that describe the target runtime environment. */
     // "jsx": "preserve",                                /* Specify what JSX code is generated. */
-    "experimentalDecorators": true,                   /* Enable experimental support for legacy experimental decorators. */
-    "emitDecoratorMetadata": true,                    /* Emit design-type metadata for decorated declarations in source files. */
+    "experimentalDecorators": true /* Enable experimental support for legacy experimental decorators. */,
+    "emitDecoratorMetadata": true /* Emit design-type metadata for decorated declarations in source files. */,
     // "jsxFactory": "",                                 /* Specify the JSX factory function used when targeting React JSX emit, e.g. 'React.createElement' or 'h'. */
     // "jsxFragmentFactory": "",                         /* Specify the JSX Fragment reference used for fragments when targeting React JSX emit e.g. 'React.Fragment' or 'Fragment'. */
     // "jsxImportSource": "",                            /* Specify module specifier used to import the JSX factory functions when using 'jsx: react-jsx*'. */
@@ -80,7 +80,7 @@
     // "strictNullChecks": true,                         /* When type checking, take into account 'null' and 'undefined'. */
     // "strictFunctionTypes": true,                      /* When assigning functions, check to ensure parameters and the return values are subtype-compatible. */
     // "strictBindCallApply": true,                      /* Check that the arguments for 'bind', 'call', and 'apply' methods match the original function. */
-    "strictPropertyInitialization": false,             /* Check for class properties that are declared but not set in the constructor. */
+    "strictPropertyInitialization": false /* Check for class properties that are declared but not set in the constructor. */,
     // "noImplicitThis": true,                           /* Enable error reporting when 'this' is given the type 'any'. */
     // "useUnknownInCatchVariables": true,               /* Default catch clause variables as 'unknown' instead of 'any'. */
     // "alwaysStrict": true,                             /* Ensure 'use strict' is always emitted. */