Handle not enough reviewers (paritytech#29)

Added validation that the amount of available users must not be smaller
than the amount of required reviews.

Closes paritytech#27

Also fixed a small security vulnerability in a dependency.

src/runner.ts

@@ -54,20 +54,26 @@ export class ActionRunner {
    */
   async validatePullRequest({ rules }: ConfigurationFile): Promise<boolean> {
     for (const rule of rules) {
-      // We get all the files that were modified and match the rules condition
-      const files = await this.listFilesThatMatchRuleCondition(rule);
-      // We check if there are any matches
-      if (files.length === 0) {
-        this.logger.debug(`Skipping rule ${rule.name} as no condition matched`);
-        // If there are no matches, we simply skip the check
-        continue;
-      }
-      if (rule.type === "basic") {
-        const [result, missingData] = await this.evaluateCondition(rule);
-        if (!result) {
-          this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`);
-          return false;
+      try {
+        // We get all the files that were modified and match the rules condition
+        const files = await this.listFilesThatMatchRuleCondition(rule);
+        // We check if there are any matches
+        if (files.length === 0) {
+          this.logger.debug(`Skipping rule ${rule.name} as no condition matched`);
+          // If there are no matches, we simply skip the check
+          continue;
+        }
+        if (rule.type === "basic") {
+          const [result, missingData] = await this.evaluateCondition(rule);
+          if (!result) {
+            this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`);
+            return false;
+          }
         }
+      } catch (error: unknown) {
+        // We only throw if there was an unexpected error, not if the check fails
+        this.logger.error(`Rule ${rule.name} failed with error`);
+        throw error;
       }
     }
 
@@ -102,6 +108,19 @@ export class ActionRunner {
       throw new Error("Teams and Users field are not set for rule.");
     }
 
+    if (requiredUsers.length < rule.min_approvals) {
+      this.logger.error(
+        `${rule.min_approvals} approvals are required but only ${requiredUsers.length} user's approval count.`,
+      );
+      if (rule.teams) {
+        this.logger.error(`Allowed teams: ${JSON.stringify(rule.teams)}`);
+      }
+      if (rule.users) {
+        this.logger.error(`Allowed users: ${JSON.stringify(rule.users)}`);
+      }
+      throw new Error("The amount of required approvals is smaller than the amount of available users.");
+    }
+
     // We get the list of users that approved the PR
     const approvals = await this.prApi.listApprovedReviewsAuthors();
 

src/test/runner/conditions.test.ts

@@ -23,6 +23,27 @@ describe("evaluateCondition tests", () => {
     );
   });
 
+  test("should throw if not enough users are available", async () => {
+    await expect(runner.evaluateCondition({ min_approvals: 5, users: ["one-user"] })).rejects.toThrow(
+      "The amount of required approvals is smaller than the amount of available users.",
+    );
+  });
+
+  test("should throw if not enough users in teams are available", async () => {
+    teamsApi.getTeamMembers.mockResolvedValue(["1", "2", "3"]);
+    await expect(runner.evaluateCondition({ min_approvals: 4, teams: ["etcetera"] })).rejects.toThrow(
+      "The amount of required approvals is smaller than the amount of available users.",
+    );
+  });
+
+  test("should throw if not enough users in teams are available and find duplicates", async () => {
+    teamsApi.getTeamMembers.calledWith("a").mockResolvedValue(["1", "2", "3"]);
+    teamsApi.getTeamMembers.calledWith("b").mockResolvedValue(["2", "3", "4"]);
+    await expect(runner.evaluateCondition({ min_approvals: 5, teams: ["a", "b"] })).rejects.toThrow(
+      "The amount of required approvals is smaller than the amount of available users.",
+    );
+  });
+
   describe("users tests", () => {
     const users = ["user-1", "user-2", "user-3"];
     beforeEach(() => {
@@ -104,7 +125,7 @@ describe("evaluateCondition tests", () => {
         teamsApi.getTeamMembers.calledWith(team1.name).mockResolvedValue(team1.users);
         teamsApi.getTeamMembers.calledWith(team2.name).mockResolvedValue(team2.users);
         api.listApprovedReviewsAuthors.mockResolvedValue([]);
-        const [result, report] = await runner.evaluateCondition({ min_approvals: 4, teams: [team1.name, team2.name] });
+        const [result, report] = await runner.evaluateCondition({ min_approvals: 3, teams: [team1.name, team2.name] });
         expect(result).toBeFalsy();
         // Should not send required users more than once
         expect(report?.missingUsers).toEqual([...team1.users, team2.users[0]]);

yarn.lock

@@ -3315,16 +3315,11 @@ safe-regex-test@^1.0.0:
     get-intrinsic "^1.1.3"
     is-regex "^1.1.4"
 
-semver@^6.0.0:
+semver@^6.0.0, semver@^6.3.0:
   version "6.3.1"
   resolved "https://registry.yarnpkg.com/semver/-/semver-6.3.1.tgz#556d2ef8689146e46dcea4bfdd095f3434dffcb4"
   integrity sha512-BR7VvDCVHO+q2xBEWskxS6DJE1qRnb7DxzUrogb71CWoSficBxYsiAGd+Kl0mmq/MprG9yArRkyrQxTO6XjMzA==
 
-semver@^6.3.0:
-  version "6.3.0"
-  resolved "https://registry.yarnpkg.com/semver/-/semver-6.3.0.tgz#ee0a64c8af5e8ceea67687b133761e1becbd1d3d"
-  integrity sha512-b39TBaTSfV6yBrapU89p5fKekE2m/NwnDocOVruQFS1/veMgdzuPcnOM34M6CwxW8jH/lxEa5rBoDeUwu5HHTw==
-
 semver@^7.3.7, semver@^7.5.3:
   version "7.5.4"
   resolved "https://registry.yarnpkg.com/semver/-/semver-7.5.4.tgz#483986ec4ed38e1c6c48c34894a9182dbff68a6e"