implemented action on itself (#31)

Resolves #6 

## Changes
- Fixed validation of rules not setting default values.
- Fixed basic rule amount of review to be a positive integer (min value
1)
    - Added a test to verify this
- Implemented GitHub Action to run on Pull Requests.
- Added a log which reports what teams and users are missing.
- Fixed a bug where the approvals wasn’t being properly filtered.
- Fixed a bug where the system asked the author to review the PR
- Fixed the wrong default name of the config file
  - Was set to `review.yml` instead of `review-bot.yml`
- Changed rule type validation to have forced string values.
- Added logs which inform when a rule is running and if it fails.
- Rules are executed in order, so the logs will always correspond to the
current rule.
- Created tickets #32, #33 and #34 as foreseeable problems.
- Created utility method to concatenate two arrays and removing
duplicates.
- @mutantcornholio I don’t know if `EngAutomation/js` has this (or if we
want to add it to it)
- Fixed a bug where the users who had approved the PR was not caching.
- Fixed a glitch where the required reviews could be 0 or a negative
number.
- Fixed a glitch where the rule type could be invalid.

.github/workflows/review-bot.yml

@@ -0,0 +1,24 @@
+name: Review PR
+on:
+  pull_request:
+    types:
+      - opened
+      - reopened
+      - synchronize
+      - review_requested
+      - review_request_removed
+      - ready_for_review
+  pull_request_review:
+
+permissions:
+  contents: read
+
+jobs:
+  review-approvals:
+    runs-on: ubuntu-latest
+    steps:
+      # TODO: Set this to main
+      - uses: paritytech/review-bot@self-implementation
+        with:
+          repo-token: ${{ secrets.GITHUB_TOKEN }}
+          team-token: ${{ secrets.TEST_TEAM_TOKEN }}

action.yml

@@ -14,7 +14,7 @@ inputs:
   config-file:
     description: 'Location of the configuration file'
     required: false
-    default: '.github/review.yml'
+    default: '.github/review-bot.yml'
 outputs:
   repo:
     description: 'The name of the repo in owner/repo pattern'

src/file/validator.ts

@@ -2,7 +2,7 @@ import { validate } from "@eng-automation/js";
 import Joi from "joi";
 
 import { ActionLogger } from "../github/types";
-import { BasicRule, ConfigurationFile, Rule } from "./types";
+import { BasicRule, ConfigurationFile, DebugRule, Rule, RuleTypes } from "./types";
 
 /** For the users or team schema. Will be recycled A LOT
  * Remember to add `.or("users", "teams")` to force at least one of the two to be defined
@@ -22,7 +22,7 @@ const ruleSchema = Joi.object<Rule & { type: string }>().keys({
     include: Joi.array().items(Joi.string()).required(),
     exclude: Joi.array().items(Joi.string()).optional().allow(null),
   }),
-  type: Joi.string().required(),
+  type: Joi.string().valid(RuleTypes.Basic, RuleTypes.Debug).required(),
 });
 
 /** General Configuration schema.
@@ -38,7 +38,7 @@ export const generalSchema = Joi.object<ConfigurationFile>().keys({
  * This rule is quite simple as it only has the min_approvals field and the required reviewers
  */
 export const basicRuleSchema = Joi.object<BasicRule>()
-  .keys({ min_approvals: Joi.number().empty(1), ...reviewersObj })
+  .keys({ min_approvals: Joi.number().min(1).default(1), ...reviewersObj })
   .or("users", "teams");
 
 /**
@@ -54,19 +54,19 @@ export const validateConfig = (config: ConfigurationFile): ConfigurationFile | n
     message: "Configuration file is invalid",
   });
 
-  for (const rule of validatedConfig.rules) {
+  for (let i = 0; i < validatedConfig.rules.length; i++) {
+    const rule = validatedConfig.rules[i];
     const { name, type } = rule;
     const message = `Configuration for rule '${rule.name}' is invalid`;
     if (type === "basic") {
-      validate<BasicRule>(rule, basicRuleSchema, { message });
+      validatedConfig.rules[i] = validate<BasicRule>(rule, basicRuleSchema, { message });
     } else if (type === "debug") {
-      validate<Rule>(rule, ruleSchema, { message });
+      validatedConfig.rules[i] = validate<DebugRule>(rule, ruleSchema, { message });
     } else {
       // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
       throw new Error(`Rule ${name} has an invalid type: ${type}`);
     }
   }
-
   return validatedConfig;
 };
 

src/github/pullRequest.ts

@@ -20,6 +20,7 @@ export class PullRequestApi {
   private usersThatApprovedThePr: string[] | null = null;
 
   async getConfigFile(configFilePath: string): Promise<string> {
+    this.logger.info(`Fetching config file in ${configFilePath}`);
     const { data } = await this.api.rest.repos.getContent({
       owner: this.pr.base.repo.owner.login,
       repo: this.pr.base.repo.name,
@@ -53,9 +54,18 @@ export class PullRequestApi {
     if (!this.usersThatApprovedThePr) {
       const request = await this.api.rest.pulls.listReviews({ ...this.repoInfo, pull_number: this.number });
       const reviews = request.data as PullRequestReview[];
-      const approvals = reviews.filter((review) => review.state === "approved");
+      this.logger.debug(`List of reviews: ${JSON.stringify(reviews)}`);
+      const approvals = reviews.filter(
+        (review) => review.state.localeCompare("approved", undefined, { sensitivity: "accent" }) === 0,
+      );
       this.usersThatApprovedThePr = approvals.map((approval) => approval.user.login);
     }
+    this.logger.debug(`PR approvals are ${JSON.stringify(this.usersThatApprovedThePr)}`);
     return this.usersThatApprovedThePr;
   }
+
+  /** Returns the login of the PR's author */
+  getAuthor(): string {
+    return this.pr.user.login;
+  }
 }

src/github/teams.ts

@@ -31,10 +31,12 @@ export class GitHubTeamsApi implements TeamApi {
 
   async getTeamMembers(teamName: string): Promise<string[]> {
     // We first verify that this information hasn't been fetched yet
-    if (this.teamsCache.has(teamName)) {
-      return this.teamsCache.get(teamName) as string[];
+    if (!this.teamsCache.has(teamName)) {
+      this.logger.debug(`Fetching team '${teamName}'`);
+      const { data } = await this.api.rest.teams.listMembersInOrg({ org: this.org, team_slug: teamName });
+      const members = data.map((d) => d.login);
+      this.teamsCache.set(teamName, members);
     }
-    const { data } = await this.api.rest.teams.listMembersInOrg({ org: this.org, team_slug: teamName });
-    return data.map((d) => d.login);
+    return this.teamsCache.get(teamName) as string[];
   }
 }

src/runner.ts

@@ -54,52 +54,71 @@ export class ActionRunner {
    * @returns a true/false statement if the rule failed. This WILL BE CHANGED for an object with information (see issue #26)
    */
   async validatePullRequest({ rules }: ConfigurationFile): Promise<boolean> {
+    const errorReports: ReviewReport[] = [];
     for (const rule of rules) {
       try {
+        this.logger.info(`Validating rule '${rule.name}'`);
         // 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`);
+          this.logger.info(`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;
+            errorReports.push(missingData);
           }
         }
       } 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`);
+        this.logger.error(`Rule '${rule.name}' failed with error`);
         throw error;
       }
+      this.logger.info(`Finish validating '${rule.name}'`);
+    }
+    if (errorReports.length > 0) {
+      const finalReport = this.aggregateReports(errorReports);
+      // Preview, this will be improved in a future commit
+      this.logger.warn(`Missing reviews: ${JSON.stringify(finalReport)}`);
+      return false;
     }
-
     // TODO: Convert this into a list of users/teams missing and convert the output into a nice summary object -> Issue #26
     return true;
   }
 
+  /** Aggregates all the reports and generate a final combined one */
+  aggregateReports(reports: ReviewReport[]): ReviewReport {
+    const finalReport: ReviewReport = { missingReviews: 0, missingUsers: [], teamsToRequest: [], usersToRequest: [] };
+
+    for (const report of reports) {
+      finalReport.missingReviews += report.missingReviews;
+      finalReport.missingUsers = concatArraysUniquely(finalReport.missingUsers, report.missingUsers);
+      finalReport.teamsToRequest = concatArraysUniquely(finalReport.teamsToRequest, report.teamsToRequest);
+      finalReport.usersToRequest = concatArraysUniquely(finalReport.usersToRequest, report.usersToRequest);
+    }
+
+    return finalReport;
+  }
+
   /** Evaluates if the required reviews for a condition have been meet
    * @param rule Every rule check has this values which consist on the min required approvals and the reviewers.
    * @returns a [bool, error data] tuple which evaluates if the condition (not the rule itself) has fulfilled the requirements
    * @see-also ReviewError
    */
   async evaluateCondition(rule: { min_approvals: number } & Reviewers): Promise<ReviewState> {
+    this.logger.debug(JSON.stringify(rule));
+
     // This is a list of all the users that need to approve a PR
     let requiredUsers: string[] = [];
     // If team is set, we fetch the members of such team
     if (rule.teams) {
       for (const team of rule.teams) {
         const members = await this.teamApi.getTeamMembers(team);
-        for (const member of members) {
-          // simple check to stop us from having duplicates
-          if (requiredUsers.indexOf(member) < 0) {
-            requiredUsers.push(member);
-          }
-        }
+        requiredUsers = concatArraysUniquely(requiredUsers, members);
       }
       // If, instead, users are set, we simply push them to the array as we don't need to scan a team
     }
@@ -125,6 +144,7 @@ export class ActionRunner {
 
     // We get the list of users that approved the PR
     const approvals = await this.prApi.listApprovedReviewsAuthors();
+    this.logger.info(`Found ${approvals.length} approvals.`);
 
     // This is the amount of reviews required. To succeed this should be 0 or lower
     let missingReviews = rule.min_approvals;
@@ -137,18 +157,22 @@ export class ActionRunner {
 
     // Now we verify if we have any remaining missing review.
     if (missingReviews > 0) {
+      const author = this.prApi.getAuthor();
+      this.logger.warn(`${missingReviews} reviews are missing.`);
       // If we have at least one missing review, we return an object with the list of missing reviewers, and
       // which users/teams we should request to review
       return [
         false,
         {
           missingReviews,
-          missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0),
+          // Remove all the users who approved the PR + the author (if he belongs to the group)
+          missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author),
           teamsToRequest: rule.teams ? rule.teams : undefined,
           usersToRequest: rule.users ? rule.users.filter((u) => approvals.indexOf(u)) : undefined,
         },
       ];
     } else {
+      this.logger.info("Rule requirements fulfilled");
       // If we don't have any missing reviews, we return the succesful case
       return [true];
     }

src/test/runner/basicRule.test.ts

@@ -87,4 +87,60 @@ describe("Basic rule parsing", () => {
         `);
     await expect(runner.getConfigFile("")).rejects.toThrowError('"value" must contain at least one of [users, teams]');
   });
+
+  test("should default min_approvals to 1", async () => {
+    api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: basic
+            users:
+              - user-example
+        `);
+    const config = await runner.getConfigFile("");
+    const [rule] = config.rules;
+    if (rule.type === "basic") {
+      expect(rule.min_approvals).toEqual(1);
+    } else {
+      throw new Error(`Rule type ${rule.type} is invalid`);
+    }
+  });
+
+  test("should fail with min_approvals in negative", async () => {
+    api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: basic
+            min_approvals: -99
+            users:
+              - user-example
+        `);
+    await expect(runner.getConfigFile("")).rejects.toThrowError('"min_approvals" must be greater than or equal to 1');
+  });
+
+  test("should fail with min_approvals in 0", async () => {
+    api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: basic
+            min_approvals: 0
+            users:
+              - user-example
+        `);
+    await expect(runner.getConfigFile("")).rejects.toThrowError('"min_approvals" must be greater than or equal to 1');
+  });
 });

src/test/runner/config.test.ts

@@ -40,6 +40,43 @@ describe("Config Parsing", () => {
     expect(api.getConfigFile).toHaveBeenCalledWith("example-location");
   });
 
+  describe("rule type", () => {
+    test("should fail with no rule type", async () => {
+      api.getConfigFile.mockResolvedValue(`
+          rules:
+            - name: Default review
+              condition:
+                include: 
+                  - '.*'
+                exclude: 
+                  - 'example'
+              teams:
+                - team-example
+          `);
+      await expect(runner.getConfigFile("")).rejects.toThrowError(
+        'Configuration file is invalid: "rules[0].type" is required',
+      );
+    });
+
+    test("should fail with no valid rule type", async () => {
+      api.getConfigFile.mockResolvedValue(`
+          rules:
+            - name: Default review
+              condition:
+                include: 
+                  - '.*'
+                exclude: 
+                  - 'example'
+              type: example-for-invalid
+              teams:
+                - team-example
+          `);
+      await expect(runner.getConfigFile("")).rejects.toThrowError(
+        'Configuration file is invalid: "rules[0].type" must be one of [basic, debug]',
+      );
+    });
+  });
+
   describe("regular expressions validator", () => {
     test("should fail with invalid regular expression", async () => {
       const invalidRegex = "(?(";