Created 'OR' rule (#48)

New feature: **The _OR_ rule**

# List of changes
- Created `OR` rule
  - This resolves #12
- The OR rule verifies that *one* of the conditions have been fulfilled.
- When a condition was fulfilled, it continues the loop to the next rule
- When the `OR` rule fails, it shows all the users it needs a review but
it only shows the amount of missing reviews using the sub condition with
the minimum amount of reviews needed.
    - Created lots of test.
- This are mainly a copy and modification of `AND` as they share logic
and structure.

---------

Co-authored-by: Przemek Rzad <roopert7@gmail.com>

.github/workflows/publish.yml

@@ -17,13 +17,13 @@ jobs:
         run: docker build . --file Dockerfile
   validate-action:
     runs-on: ubuntu-latest
+    if: github.event_name == 'pull_request'
     steps:
       - uses: actions/checkout@v3.3.0
       # This checks that .github/workflows/review-bot.yml is pointing towards the main branch
       # as, during development, we change this to use the code from the test branch and
       # we may forget to set it back to main
       - name: Validate that action points to main branch
-        if: github.event_name == 'pull_request'
         run: |
           BRANCH=$(yq '.jobs.review-approvals.steps[1].uses' $FILE_NAME | cut -d "@" -f2)
           # If the branch is not the main branch

src/rules/types.ts

@@ -2,9 +2,10 @@ export enum RuleTypes {
   Basic = "basic",
   Debug = "debug",
   And = "and",
+  Or = "or",
 }
 
-export type Reviewers = { users?: string[]; teams?: string[] };
+export type Reviewers = { users?: string[]; teams?: string[]; min_approvals: number };
 
 export interface Rule {
   name: string;
@@ -19,21 +20,23 @@ export interface DebugRule extends Rule {
 
 export interface BasicRule extends Rule, Reviewers {
   type: RuleTypes.Basic;
-  min_approvals: number;
 }
 
 export interface AndRule extends Rule {
   type: RuleTypes.And;
-  reviewers: ({
-    min_approvals: number;
-  } & Reviewers)[];
+  reviewers: Reviewers[];
+}
+
+export interface OrRule extends Rule {
+  type: RuleTypes.Or;
+  reviewers: Reviewers[];
 }
 
 export interface ConfigurationFile {
   /** Based on the `type` parameter, Typescript converts the object to the correct type
    * @see {@link Rules}
    */
-  rules: (BasicRule | DebugRule | AndRule)[];
+  rules: (BasicRule | DebugRule | AndRule | OrRule)[];
   preventReviewRequests?: {
     teams?: string[];
     users?: string[];

src/rules/validator.ts

@@ -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, RuleTypes.And).required(),
+  type: Joi.string().valid(RuleTypes.Basic, RuleTypes.Debug, RuleTypes.And, RuleTypes.Or).required(),
 });
 
 /** General Configuration schema.
@@ -41,7 +41,8 @@ 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({
+/** As, with the exception of basic, every other schema has the same structure, we can recycle this */
+export const otherRulesSchema = Joi.object<AndRule>().keys({
   reviewers: Joi.array<AndRule["reviewers"]>().items(basicRuleSchema).min(2).required(),
 });
 
@@ -66,8 +67,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 if (type === "and" || type === "or") {
+      validatedConfig.rules[i] = validate<AndRule>(rule, otherRulesSchema, { message });
     } else {
       // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
       throw new Error(`Rule ${name} has an invalid type: ${type}`);

src/runner.ts

@@ -59,7 +59,7 @@ export class ActionRunner {
    */
   async validatePullRequest({ rules }: ConfigurationFile): Promise<RuleReport[]> {
     const errorReports: RuleReport[] = [];
-    for (const rule of rules) {
+    ruleCheck: 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
@@ -87,16 +87,37 @@ export class ActionRunner {
             }
           }
           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,
-            };
+            const finalReport = unifyReport(reports, rule.name);
             this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`);
             errorReports.push(finalReport);
           }
+        } else if (rule.type === "or") {
+          const reports: ReviewReport[] = [];
+          for (const reviewer of rule.reviewers) {
+            const [result, missingData] = await this.evaluateCondition(reviewer);
+            if (result) {
+              // This is a OR condition, so with ONE positive result
+              // we can continue the loop to check the following rule
+              continue ruleCheck;
+            }
+            // But, until we get a positive case we add all the failed cases
+            reports.push(missingData);
+          }
+
+          // If the loop was not skipped it means that we have errors
+          if (reports.length > 0) {
+            // We get the lowest amount of reviews needed to fulfill one of the reviews
+            const lowerAmountOfReviewsNeeded = reports
+              .map((r) => r.missingReviews)
+              .reduce((a, b) => (a < b ? a : b), 999);
+            // We unify the reports
+            const finalReport = unifyReport(reports, rule.name);
+            // We set the value to the minimum neccesary
+            finalReport.missingReviews = lowerAmountOfReviewsNeeded;
+            this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`);
+            // We unify the reports and push them for handling
+            errorReports.push(finalReport);
+          }
         }
       } catch (error: unknown) {
         // We only throw if there was an unexpected error, not if the check fails
@@ -291,3 +312,13 @@ export class ActionRunner {
     return checkRunData;
   }
 }
+
+const unifyReport = (reports: ReviewReport[], name: string): RuleReport => {
+  return {
+    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,
+  };
+};

src/test/rules/config.test.ts

@@ -5,6 +5,7 @@ import { mock, MockProxy } from "jest-mock-extended";
 
 import { PullRequestApi } from "../../github/pullRequest";
 import { TeamApi } from "../../github/teams";
+import { RuleTypes } from "../../rules/types";
 import { ActionRunner } from "../../runner";
 import { TestLogger } from "../logger";
 
@@ -71,8 +72,10 @@ describe("Config Parsing", () => {
               teams:
                 - team-example
           `);
+      // prettifies string to [basic, debug, and, or...]
+      const types = JSON.stringify(Object.values(RuleTypes)).replace(/"/g, "").replace(/,/g, ", ");
       await expect(runner.getConfigFile("")).rejects.toThrowError(
-        'Configuration file is invalid: "rules[0].type" must be one of [basic, debug, and]',
+        'Configuration file is invalid: "rules[0].type" must be one of ' + types,
       );
     });
 

src/test/rules/or.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 { OrRule } from "../../rules/types";
+import { ActionRunner } from "../../runner";
+import { TestLogger } from "../logger";
+
+describe("'Or' 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: or
+            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("or");
+  });
+
+  describe("reviewers", () => {
+    test("should require teams", async () => {
+      api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: or
+            reviewers:
+              - teams:
+                - team-example
+              - teams:
+                - team-abc
+        `);
+      const config = await runner.getConfigFile("");
+      const rule = config.rules[0] as OrRule;
+      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: or
+            reviewers:
+              - users:
+                - user-example
+              - users:
+                - user-special
+        `);
+      const config = await runner.getConfigFile("");
+      const rule = config.rules[0] as OrRule;
+      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: or
+        `);
+      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: or
+            reviewers:
+              - teams:
+                - team-example
+              - min_approvals: 2
+                users:
+                  - abc
+                teams:
+                  - xyz
+        `);
+      const config = await runner.getConfigFile("");
+      const rule = config.rules[0] as OrRule;
+      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: or
+            reviewers:
+              - users:
+                - user-example
+              - teams:
+                - team-example
+        `);
+    const config = await runner.getConfigFile("");
+    const [rule] = config.rules;
+    if (rule.type === "or") {
+      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: or
+            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: or
+            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',
+    );
+  });
+});