Author as reviewer (paritytech#58)

Resolves paritytech#34

Created the option to count the author of a PR as an approval.

In the case of the "normal" rules it doesn't change the logic.

In the case of the "and-distinct" rule we have to decide if we filter the author from the PR on evaluation time.

.github/review-bot.yml

@@ -12,10 +12,11 @@ rules:
     condition:
       include:
         - 'review-bot.yml'
-    type: and
+    type: and-distinct
     reviewers:
       - teams:
         - opstooling
+        countAuthor: true
       - users:
         - mordamax
         - mutantcornholio

README.md

@@ -207,6 +207,7 @@ rules:
     users:
       - user-1
       - user-2
+    countAuthor: true
 ```
 It has the same parameters as a normal rule:
 -  **name**: Name of the rule. This value must be unique per rule.
@@ -223,6 +224,9 @@ It has the same parameters as a normal rule:
 	- *Optional if **users** is defined*.
 - **users**: An array of the GitHub usernames of the users who need to review this file. 
 	- *Optional if **teams** is defined*.
+- **countAuthor**: If the pull request author should be considered as an approval.
+	- If the author belongs to the list of approved users (either by team or by users) his approval will be counted (requiring one less approvals in total).
+	- ** Optional**: Defaults to `false`
 #### Other rules
 The other three rules (**or**, **and** and **and-distinct**) have the same configuration, so let’s summarize that here and then move into how they work.
 ```yaml
@@ -233,6 +237,7 @@ rules:
         - '.*'
       exclude: 
         - 'example'
+    countAuthor: true
     type: or | and | and-distinct
     reviewers:
       - teams:
@@ -246,6 +251,9 @@ rules:
 ```
 - The **name** and **conditions** fields have the same requirements that the `basic` rule has.
 - **type**: Must be `or`, `and` or `and-distinct`.
+- **countAuthor**: If the pull request author should be considered as an approval.
+	- If the author belongs to the list of approved users (either by team or by users) his approval will be counted (requiring one less approvals in total).
+	- ** Optional**: Defaults to `false`
 - **reviewers**: This is an array that contains all the available options for review.
 	- Each of these options works independently.
 	- Must have at least two options.
@@ -259,7 +267,6 @@ rules:
 			- *Optional if **users** is defined*.
 		- **users**: An array of the GitHub usernames of the users who need to review this file. 
 			- *Optional if **teams** is defined*.
-
 ##### Or rule logic
 This is a rule that has at least two available options of reviewers and needs **at least one group to approve**.
 

src/github/pullRequest.ts

@@ -53,7 +53,7 @@ export class PullRequestApi {
   }
 
   /** List all the approved reviews in a PR */
-  async listApprovedReviewsAuthors(): Promise<string[]> {
+  async listApprovedReviewsAuthors(countAuthor: boolean): Promise<string[]> {
     if (!this.usersThatApprovedThePr) {
       const request = await this.api.rest.pulls.listReviews({ ...this.repoInfo, pull_number: this.number });
       const reviews = request.data as PullRequestReview[];
@@ -97,6 +97,12 @@ export class PullRequestApi {
       this.usersThatApprovedThePr = approvals.map((approval) => approval.user.login);
     }
     this.logger.debug(`PR approvals are ${JSON.stringify(this.usersThatApprovedThePr)}`);
+
+    if (countAuthor) {
+      // If this value is true, we add the author to the list of approvals
+      return [...this.usersThatApprovedThePr, this.pr.user.login];
+    }
+
     return this.usersThatApprovedThePr;
   }
 

src/rules/types.ts

@@ -11,6 +11,7 @@ export type Reviewers = { users?: string[]; teams?: string[]; min_approvals: num
 export interface Rule {
   name: string;
   condition: { include: string[]; exclude?: string[] };
+  countAuthor?: boolean;
 }
 
 // TODO: Delete this once we add a second type of rule

src/rules/validator.ts

@@ -2,14 +2,15 @@ import { validate } from "@eng-automation/js";
 import Joi from "joi";
 
 import { ActionLogger } from "../github/types";
-import { AndRule, BasicRule, ConfigurationFile, DebugRule, Rule, RuleTypes } from "./types";
+import { AndRule, BasicRule, ConfigurationFile, DebugRule, Reviewers, 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
  */
 const reviewersObj = {
   users: Joi.array().items(Joi.string()).optional().empty(null),
   teams: Joi.array().items(Joi.string()).optional().empty(null),
+  min_approvals: Joi.number().min(1).default(1),
 };
 
 /** Base rule condition.
@@ -40,12 +41,16 @@ 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().min(1).default(1), ...reviewersObj })
+  .keys({ ...reviewersObj, countAuthor: Joi.boolean().default(false) })
   .or("users", "teams");
 
 /** 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(),
+  reviewers: Joi.array<AndRule["reviewers"]>()
+    .items(Joi.object<Reviewers>().keys(reviewersObj).or("users", "teams"))
+    .min(2)
+    .required(),
+  countAuthor: Joi.boolean().default(false),
 });
 
 /**
@@ -70,6 +75,8 @@ export const validateConfig = (config: ConfigurationFile): ConfigurationFile | n
     } else if (type === "debug") {
       validatedConfig.rules[i] = validate<DebugRule>(rule, ruleSchema, { message });
     } else if (type === "and" || type === "or" || type === "and-distinct") {
+      // Aside from the type, every other field in this rules is identical so we can
+      // use any of these rules to validate the other fields.
       validatedConfig.rules[i] = validate<AndRule>(rule, otherRulesSchema, { message });
     } else {
       // eslint-disable-next-line @typescript-eslint/restrict-template-expressions

src/runner.ts

@@ -79,7 +79,7 @@ export class ActionRunner {
           continue;
         }
         if (rule.type === "basic") {
-          const [result, missingData] = await this.evaluateCondition(rule);
+          const [result, missingData] = await this.evaluateCondition(rule, rule.countAuthor);
           if (!result) {
             this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`);
             errorReports.push({ ...missingData, name: rule.name });
@@ -88,7 +88,7 @@ export class ActionRunner {
           const reports: ReviewReport[] = [];
           // We evaluate every individual condition
           for (const reviewer of rule.reviewers) {
-            const [result, missingData] = await this.evaluateCondition(reviewer);
+            const [result, missingData] = await this.evaluateCondition(reviewer, rule.countAuthor);
             if (!result) {
               // If one of the conditions failed, we add it to a report
               reports.push(missingData);
@@ -102,7 +102,7 @@ export class ActionRunner {
         } else if (rule.type === "or") {
           const reports: ReviewReport[] = [];
           for (const reviewer of rule.reviewers) {
-            const [result, missingData] = await this.evaluateCondition(reviewer);
+            const [result, missingData] = await this.evaluateCondition(reviewer, rule.countAuthor);
             if (result) {
               // This is a OR condition, so with ONE positive result
               // we can continue the loop to check the following rule
@@ -231,7 +231,7 @@ export class ActionRunner {
     // We count how many reviews are needed in total
     const requiredAmountOfReviews = rule.reviewers.map((r) => r.min_approvals).reduce((a, b) => a + b, 0);
     // We get the list of users that approved the PR
-    const approvals = await this.prApi.listApprovedReviewsAuthors();
+    const approvals = await this.prApi.listApprovedReviewsAuthors(rule.countAuthor ?? false);
 
     // Utility method used to generate error
     const generateErrorReport = (): ReviewReport => {
@@ -266,6 +266,7 @@ export class ActionRunner {
     // Now we see, from all the approvals, which approvals could match each rule
     for (const { users, requiredApprovals } of requirements) {
       const ruleApprovals = approvals.filter((ap) => users.indexOf(ap) !== -1);
+
       conditionApprovals.push({ matchingUsers: ruleApprovals, requiredUsers: users, requiredApprovals });
     }
     this.logger.debug(`Matching approvals: ${JSON.stringify(conditionApprovals)}`);
@@ -334,7 +335,10 @@ export class ActionRunner {
    * @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> {
+  async evaluateCondition(
+    rule: { min_approvals: number } & Reviewers,
+    countAuthor: boolean = false,
+  ): Promise<ReviewState> {
     this.logger.debug(JSON.stringify(rule));
 
     // This is a list of all the users that need to approve a PR
@@ -368,7 +372,7 @@ export class ActionRunner {
     }
 
     // We get the list of users that approved the PR
-    const approvals = await this.prApi.listApprovedReviewsAuthors();
+    const approvals = await this.prApi.listApprovedReviewsAuthors(countAuthor ?? false);
     this.logger.info(`Found ${approvals.length} approvals.`);
 
     // This is the amount of reviews required. To succeed this should be 0 or lower

src/test/github.test.ts

@@ -0,0 +1,79 @@
+import { PullRequest, PullRequestReview } from "@octokit/webhooks-types";
+import { DeepMockProxy, mock, mockDeep, MockProxy } from "jest-mock-extended";
+
+import { PullRequestApi } from "../github/pullRequest";
+import { ActionLogger, GitHubClient } from "../github/types";
+
+describe("Pull Request API Tests", () => {
+  let api: PullRequestApi;
+  let logger: MockProxy<ActionLogger>;
+  let client: DeepMockProxy<GitHubClient>;
+  let pr: DeepMockProxy<PullRequest>;
+  beforeEach(() => {
+    logger = mock<ActionLogger>();
+    client = mockDeep<GitHubClient>();
+    pr = mockDeep<PullRequest>();
+    pr.number = 99;
+    api = new PullRequestApi(client, pr, logger, { owner: "org", repo: "repo" }, "");
+  });
+
+  describe("Approvals", () => {
+    const random = () => Math.floor(Math.random() * 1000);
+
+    let reviews: PullRequestReview[];
+    beforeEach(() => {
+      reviews = [];
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore because the official type and the library type do not match
+      client.rest.pulls.listReviews.mockResolvedValue({ data: reviews as unknown });
+    });
+
+    test("Should return approval", async () => {
+      const mockReviews: PullRequestReview[] = [
+        { state: "approved", user: { login: "yes-user", id: random() }, id: random() },
+      ] as PullRequestReview[];
+      reviews.push(...mockReviews);
+
+      const approvals = await api.listApprovedReviewsAuthors(false);
+      expect(approvals).toEqual(["yes-user"]);
+    });
+
+    test("Should return approvals and ignore other reviews", async () => {
+      const mockReviews: PullRequestReview[] = [
+        { state: "changes_requested", user: { login: "no-user", id: random() }, id: random() },
+        { state: "approved", user: { login: "yes-user", id: random() }, id: random() },
+        { state: "commented", user: { login: "other-user", id: random() }, id: random() },
+      ] as PullRequestReview[];
+      reviews.push(...mockReviews);
+
+      const approvals = await api.listApprovedReviewsAuthors(false);
+      expect(approvals).toEqual(["yes-user"]);
+    });
+
+    test("Should consider only oldest reviews per user", async () => {
+      const mockReviews: PullRequestReview[] = [
+        { state: "changes_requested", user: { login: "user-1", id: 1 }, id: 1000 },
+        { state: "approved", user: { login: "user-2", id: 2 }, id: 1200 },
+        { state: "approved", user: { login: "user-1", id: 1 }, id: 1500 },
+        { state: "changes_requested", user: { login: "user-2", id: 2 }, id: 1600 },
+      ] as PullRequestReview[];
+      reviews.push(...mockReviews);
+
+      const approvals = await api.listApprovedReviewsAuthors(false);
+      expect(approvals).toEqual(["user-1"]);
+    });
+
+    test("Should return approvals and the author", async () => {
+      pr.user.login = "abc";
+      const mockReviews: PullRequestReview[] = [
+        { state: "changes_requested", user: { login: "no-user", id: random() }, id: random() },
+        { state: "approved", user: { login: "yes-user", id: random() }, id: random() },
+        { state: "commented", user: { login: "other-user", id: random() }, id: random() },
+      ] as PullRequestReview[];
+      reviews.push(...mockReviews);
+
+      const approvals = await api.listApprovedReviewsAuthors(true);
+      expect(approvals).toEqual(["yes-user", "abc"]);
+    });
+  });
+});

src/test/rules/basicRule.test.ts

@@ -143,4 +143,66 @@ describe("Basic rule parsing", () => {
         `);
     await expect(runner.getConfigFile("")).rejects.toThrowError('"min_approvals" must be greater than or equal to 1');
   });
+
+  test("should default countAuthor to false", 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.countAuthor).toBeFalsy();
+    } else {
+      throw new Error(`Rule type ${rule.type} is invalid`);
+    }
+  });
+
+  test("should fail if countAuthor is not a boolean", async () => {
+    api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: basic
+            users:
+              - user-example
+            countAuthor: bla
+        `);
+    await expect(runner.getConfigFile("")).rejects.toThrowError('"countAuthor" must be a boolean');
+  });
+
+  test("should set countAuthor to true", async () => {
+    api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Test review
+            condition:
+              include: 
+                - '.*'
+              exclude: 
+                - 'example'
+            type: basic
+            users:
+              - user-example
+            countAuthor: true
+        `);
+    const config = await runner.getConfigFile("");
+    const [rule] = config.rules;
+    if (rule.type === "basic") {
+      expect(rule.countAuthor).toBeTruthy();
+    } else {
+      throw new Error(`Rule type ${rule.type} is invalid`);
+    }
+  });
 });