Created "and" rule (paritytech#44)

# Changes
- Created the `AND` rule with a configuration.
  - This resolves paritytech#10 
  - Created the type for it
  - Created the JOI validation
- It must have at least two individual conditions for it to be valid so
it is not used as a `basic` rule.
- Fixed bug where check summary duplicated content
  - This happened because it has a buffer and I had to empty it.
- Created `AND` rule evaluation for pull requests.
  - It combines its report into one.
- Fixed bug where author appeared as one of the missing reviews
- Created tests for `AND` rule.
- Renamed files and directories
- `file` directory now is `rule` directory (as it store the types and
validations for the clases)

.github/review-bot.yml

@@ -8,3 +8,16 @@ rules:
     type: basic
     teams:
       - opstooling
+  - name: Config file
+    condition:
+      include:
+        - 'review-bot.yml'
+    type: and
+    reviewers:
+      - teams:
+        - opstooling
+      - users:
+        - mordamax
+        - mutantcornholio
+        - rzadp
+        - bullrich

src/cli.ts

@@ -1,8 +1,8 @@
 import { readFileSync } from "fs";
 import { parse } from "yaml";
 
-import { ConfigurationFile } from "./file/types";
-import { validateConfig, validateRegularExpressions } from "./file/validator";
+import { ConfigurationFile } from "./rules/types";
+import { validateConfig, validateRegularExpressions } from "./rules/validator";
 
 const fileLocation = process.argv[2];
 

src/github/pullRequest.ts

@@ -145,6 +145,7 @@ export class PullRequestApi {
 
     // We publish it in the action summary
     await summary
+      .emptyBuffer()
       .addHeading(checkResult.output.title)
       // We redirect to the check as it can changed if it is triggered again
       .addLink("Find the result here", check.data.html_url ?? "")

src/file/types.ts → src/rules/types.ts

@@ -1,6 +1,7 @@
 export enum RuleTypes {
   Basic = "basic",
   Debug = "debug",
+  And = "and",
 }
 
 export type Reviewers = { users?: string[]; teams?: string[] };
@@ -21,11 +22,18 @@ export interface BasicRule extends Rule, Reviewers {
   min_approvals: number;
 }
 
+export interface AndRule extends Rule {
+  type: RuleTypes.And;
+  reviewers: ({
+    min_approvals: number;
+  } & Reviewers)[];
+}
+
 export interface ConfigurationFile {
   /** Based on the `type` parameter, Typescript converts the object to the correct type
    * @see {@link Rules}
    */
-  rules: (BasicRule | DebugRule)[];
+  rules: (BasicRule | DebugRule | AndRule)[];
   preventReviewRequests?: {
     teams?: string[];
     users?: string[];

src/file/validator.ts → src/rules/validator.ts

@@ -2,7 +2,7 @@ import { validate } from "@eng-automation/js";
 import Joi from "joi";
 
 import { ActionLogger } from "../github/types";
-import { BasicRule, ConfigurationFile, DebugRule, Rule, RuleTypes } from "./types";
+import { AndRule, 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().valid(RuleTypes.Basic, RuleTypes.Debug).required(),
+  type: Joi.string().valid(RuleTypes.Basic, RuleTypes.Debug, RuleTypes.And).required(),
 });
 
 /** General Configuration schema.
@@ -41,6 +41,10 @@ export const basicRuleSchema = Joi.object<BasicRule>()
   .keys({ min_approvals: Joi.number().min(1).default(1), ...reviewersObj })
   .or("users", "teams");
 
+export const andRuleSchema = Joi.object<AndRule>().keys({
+  reviewers: Joi.array<AndRule["reviewers"]>().items(basicRuleSchema).min(2).required(),
+});
+
 /**
  * Evaluates a config thoroughly. If there is a problem with it, it will throw.
  *
@@ -62,6 +66,8 @@ export const validateConfig = (config: ConfigurationFile): ConfigurationFile | n
       validatedConfig.rules[i] = validate<BasicRule>(rule, basicRuleSchema, { message });
     } else if (type === "debug") {
       validatedConfig.rules[i] = validate<DebugRule>(rule, ruleSchema, { message });
+    } else if (type === "and") {
+      validatedConfig.rules[i] = validate<AndRule>(rule, andRuleSchema, { message });
     } else {
       // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
       throw new Error(`Rule ${name} has an invalid type: ${type}`);

src/runner.ts

@@ -2,12 +2,12 @@ import { summary } from "@actions/core";
 import { parse } from "yaml";
 
 import { Inputs } from ".";
-import { ConfigurationFile, Reviewers, Rule } from "./file/types";
-import { validateConfig, validateRegularExpressions } from "./file/validator";
 import { PullRequestApi } from "./github/pullRequest";
 import { TeamApi } from "./github/teams";
 import { ActionLogger, CheckData } from "./github/types";
-import { concatArraysUniquely } from "./util";
+import { ConfigurationFile, Reviewers, Rule } from "./rules/types";
+import { validateConfig, validateRegularExpressions } from "./rules/validator";
+import { caseInsensitiveEqual, concatArraysUniquely } from "./util";
 
 type ReviewReport = {
   /** The amount of missing reviews to fulfill the requirements */
@@ -76,6 +76,27 @@ export class ActionRunner {
             this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`);
             errorReports.push({ ...missingData, name: rule.name });
           }
+        } else if (rule.type === "and") {
+          const reports: ReviewReport[] = [];
+          // We evaluate every individual condition
+          for (const reviewer of rule.reviewers) {
+            const [result, missingData] = await this.evaluateCondition(reviewer);
+            if (!result) {
+              // If one of the conditions failed, we add it to a report
+              reports.push(missingData);
+            }
+          }
+          if (reports.length > 0) {
+            const finalReport: RuleReport = {
+              missingReviews: reports.reduce((a, b) => a + b.missingReviews, 0),
+              missingUsers: [...new Set(reports.flatMap((r) => r.missingUsers))],
+              teamsToRequest: [...new Set(reports.flatMap((r) => r.teamsToRequest ?? []))],
+              usersToRequest: [...new Set(reports.flatMap((r) => r.usersToRequest ?? []))],
+              name: rule.name,
+            };
+            this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`);
+            errorReports.push(finalReport);
+          }
         }
       } catch (error: unknown) {
         // We only throw if there was an unexpected error, not if the check fails
@@ -111,7 +132,9 @@ export class ActionRunner {
     this.logger.info(`Need to request reviews from ${reviewersLog}`);
   }
 
-  /** Aggregates all the reports and generate a status report */
+  /** Aggregates all the reports and generate a status report
+   * This also filters the author of the PR if he belongs to the group of users
+   */
   generateCheckRunData(reports: RuleReport[]): CheckData {
     // Count how many reviews are missing
     const missingReviews = reports.reduce((a, b) => a + b.missingReviews, 0);
@@ -131,9 +154,14 @@ export class ActionRunner {
 
     for (const report of reports) {
       check.output.summary += `- **${report.name}**\n`;
-      let text = summary.addHeading(report.name, 2).addHeading(`Missing ${report.missingReviews} reviews`, 4);
+      let text = summary
+        .emptyBuffer()
+        .addHeading(report.name, 2)
+        .addHeading(`Missing ${report.missingReviews} review${report.missingReviews > 1 ? "s" : ""}`, 4);
       if (report.usersToRequest && report.usersToRequest.length > 0) {
-        text = text.addHeading("Missing users", 3).addList(report.usersToRequest);
+        text = text
+          .addHeading("Missing users", 3)
+          .addList(report.usersToRequest.filter((u) => !caseInsensitiveEqual(u, this.prApi.getAuthor())));
       }
       if (report.teamsToRequest && report.teamsToRequest.length > 0) {
         text = text.addHeading("Missing reviews from teams", 3).addList(report.teamsToRequest);

src/test/rules/andRule.test.ts

@@ -0,0 +1,200 @@
+/* eslint-disable @typescript-eslint/unbound-method */
+/* eslint-disable @typescript-eslint/no-unsafe-assignment */
+/* eslint-disable @typescript-eslint/no-unsafe-call */
+import { mock, MockProxy } from "jest-mock-extended";
+
+import { PullRequestApi } from "../../github/pullRequest";
+import { TeamApi } from "../../github/teams";
+import { AndRule } from "../../rules/types";
+import { ActionRunner } from "../../runner";
+import { TestLogger } from "../logger";
+
+describe("'And' rule parsing", () => {
+  let api: MockProxy<PullRequestApi>;
+  let runner: ActionRunner;
+  let teamsApi: MockProxy<TeamApi>;
+  let logger: TestLogger;
+  beforeEach(() => {
+    logger = new TestLogger();
+    api = mock<PullRequestApi>();
+    runner = new ActionRunner(api, teamsApi, logger);
+  });
+  test("should get minimal config", async () => {
+    api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: and
+            reviewers:
+              - teams:
+                - team-example
+              - teams:
+                - team-abc
+        `);
+    const config = await runner.getConfigFile("");
+    expect(config.rules[0].name).toEqual("Test review");
+    expect(config.rules[0].type).toEqual("and");
+  });
+
+  describe("reviewers", () => {
+    test("should require teams", async () => {
+      api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: and
+            reviewers:
+              - teams:
+                - team-example
+              - teams:
+                - team-abc
+        `);
+      const config = await runner.getConfigFile("");
+      const rule = config.rules[0] as AndRule;
+      expect(rule.reviewers).toHaveLength(2);
+      expect(rule.reviewers[0].teams).toContainEqual("team-example");
+      expect(rule.reviewers[0].users).toBeUndefined();
+      expect(rule.reviewers[1].teams).toContainEqual("team-abc");
+      expect(rule.reviewers[1].users).toBeUndefined();
+    });
+    test("should require users", async () => {
+      api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: and
+            reviewers:
+              - users:
+                - user-example
+              - users:
+                - user-special
+        `);
+      const config = await runner.getConfigFile("");
+      const rule = config.rules[0] as AndRule;
+      expect(rule.reviewers[0].users).toContainEqual("user-example");
+      expect(rule.reviewers[0].teams).toBeUndefined();
+      expect(rule.reviewers[1].users).toContainEqual("user-special");
+      expect(rule.reviewers[1].teams).toBeUndefined();
+    });
+
+    test("should fail without reviewers", async () => {
+      api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: and
+        `);
+      await expect(runner.getConfigFile("")).rejects.toThrowError('"reviewers" is required');
+    });
+
+    test("should fill the reviewers array", async () => {
+      api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: and
+            reviewers:
+              - teams:
+                - team-example
+              - min_approvals: 2
+                users:
+                  - abc
+                teams:
+                  - xyz
+        `);
+      const config = await runner.getConfigFile("");
+      const rule = config.rules[0] as AndRule;
+      expect(rule.reviewers).toHaveLength(2);
+      expect(rule.reviewers[0].teams).toContainEqual("team-example");
+      expect(rule.reviewers[0].users).toBeUndefined();
+      expect(rule.reviewers[1].min_approvals).toEqual(2);
+      expect(rule.reviewers[1].users).toContainEqual("abc");
+      expect(rule.reviewers[1].teams).toContainEqual("xyz");
+    });
+  });
+
+  test("should default min_approvals to 1", async () => {
+    api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: and
+            reviewers:
+              - users:
+                - user-example
+              - teams:
+                - team-example
+        `);
+    const config = await runner.getConfigFile("");
+    const [rule] = config.rules;
+    if (rule.type === "and") {
+      expect(rule.reviewers[0].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: and
+            reviewers:
+              - min_approvals: -99
+                users:
+                - user-example
+        `);
+    await expect(runner.getConfigFile("")).rejects.toThrowError(
+      '"reviewers[0].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: and
+            reviewers:
+              - min_approvals: 0
+                users:
+                  - user-example
+        `);
+    await expect(runner.getConfigFile("")).rejects.toThrowError(
+      '"reviewers[0].min_approvals" must be greater than or equal to 1',
+    );
+  });
+});

src/test/runner/basicRule.test.ts → src/test/rules/basicRule.test.ts

@@ -3,9 +3,9 @@
 /* eslint-disable @typescript-eslint/no-unsafe-call */
 import { mock, MockProxy } from "jest-mock-extended";
 
-import { BasicRule } from "../../file/types";
 import { PullRequestApi } from "../../github/pullRequest";
 import { TeamApi } from "../../github/teams";
+import { BasicRule } from "../../rules/types";
 import { ActionRunner } from "../../runner";
 import { TestLogger } from "../logger";
 

src/test/runner/config.test.ts → src/test/rules/config.test.ts

@@ -72,7 +72,7 @@ describe("Config Parsing", () => {
                 - team-example
           `);
       await expect(runner.getConfigFile("")).rejects.toThrowError(
-        'Configuration file is invalid: "rules[0].type" must be one of [basic, debug]',
+        'Configuration file is invalid: "rules[0].type" must be one of [basic, debug, and]',
       );
     });