Prevent review request

README.md

@@ -169,20 +169,30 @@ interface Report {
 ## Rule configuration file
 This is the file where all the available rules are written. 
 
-**This file is only read from the main branch.** So if you modify the file the changes won’t happen until it is merged into the main branch. 
-This is done to stop users from modifying the rules in their own PRs.
+**This file is only read from the main branch.** So if you modify the file, the changes won’t happen until it is merged into the main branch. 
+This is done to stop users from modifying the rules in their PRs.
 
-It contains an object called `rules` which has an array of rules. Every rule has a same base structure:
+It contains an object called `rules` which has an array of rules. Every rule has a same base structure. There is also a second optional field called `preventReviewRequests`.
 ```yaml
+rules:
   - name: Rule name
     condition:
       include:
         - '.*'
       exclude:
         - 'README.md'
     type: the type of the rule
+
+preventReviewRequests:
+  users:
+    - user-a
+    - user-b
+  teams:
+    - team-a
+    - team-b
 ```
 
+#### Rules fields
 - **name**: Name of the rule. This value must be unique per rule.
 - **condition**: This is an object that contains two values:
 	- **include**: An array of regex expressions of the files that match this rule.
@@ -197,8 +207,16 @@ It contains an object called `rules` which has an array of rules. Every rule has
 			- **or**: Has many review options, requires at least *one option* to be fulfilled.
 			- **and**: Has many review options, requires *all the options* to be fulfilled.
 			-  **and-distinct**: Has many review options, requires *all the options* to be fulfilled *by different people*.
+
+#### preventReviewRequests
+This is a special field that applies to all the rules.
+
+This field is **optional** and currently not used. Pending on https://github.com/paritytech/review-bot/issues/53
+
+
 ### Types
 Every type has a *slightly* different configuration and works for different scenarios, so let’s analyze all of them.
+
 #### Basic rule
 As the name implies, this type is elementary. All the files that fall under the rule evaluation must receive a given number of approvals by the listed users and/or team members.
 

src/rules/validator.ts

@@ -10,9 +10,10 @@ import { AndRule, BasicRule, ConfigurationFile, DebugRule, Reviewers, Rule, Rule
 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),
 };
 
+const reviewerConditionObj = { ...reviewersObj, min_approvals: Joi.number().min(1).default(1) };
+
 /** Base rule condition.
  * This are the minimum requirements that all the rules must have.
  * After we evaluated this, we can run a custom evaluation per rule
@@ -34,20 +35,20 @@ const ruleSchema = Joi.object<Rule & { type: string }>().keys({
  */
 export const generalSchema = Joi.object<ConfigurationFile>().keys({
   rules: Joi.array<ConfigurationFile["rules"]>().items(ruleSchema).unique("name").required(),
-  preventReviewRequests: Joi.object().keys(reviewersObj).optional().xor("users", "teams"),
+  preventReviewRequests: Joi.object().keys(reviewersObj).optional().or("users", "teams"),
 });
 
 /** Basic rule schema
  * This rule is quite simple as it only has the min_approvals field and the required reviewers
  */
 export const basicRuleSchema = Joi.object<BasicRule>()
-  .keys({ ...reviewersObj, countAuthor: Joi.boolean().default(false) })
+  .keys({ ...reviewerConditionObj, 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(Joi.object<Reviewers>().keys(reviewersObj).or("users", "teams"))
+    .items(Joi.object<Reviewers>().keys(reviewerConditionObj).or("users", "teams"))
     .min(2)
     .required(),
   countAuthor: Joi.boolean().default(false),

src/runner.ts

@@ -67,8 +67,10 @@ export class ActionRunner {
    * @returns an array of error reports for each failed rule. An empty array means no errors
    */
   async validatePullRequest({ rules }: ConfigurationFile): Promise<PullRequestReport> {
-    const errorReports: RuleReport[] = [];
     const modifiedFiles = await this.prApi.listModifiedFiles();
+
+    const errorReports: RuleReport[] = [];
+
     ruleCheck: for (const rule of rules) {
       try {
         this.logger.info(`Validating rule '${rule.name}'`);
@@ -146,7 +148,7 @@ export class ActionRunner {
   }
 
   /** WIP - Class that will assign the requests for review */
-  requestReviewers(reports: RuleReport[]): void {
+  requestReviewers(reports: RuleReport[], preventReviewRequests: ConfigurationFile["preventReviewRequests"]): void {
     if (reports.length === 0) {
       return;
     }
@@ -159,7 +161,29 @@ export class ActionRunner {
       finalReport.usersToRequest = concatArraysUniquely(finalReport.usersToRequest, report.usersToRequest);
     }
 
-    const { teamsToRequest, usersToRequest } = finalReport;
+    let { teamsToRequest, usersToRequest } = finalReport;
+
+    /**
+     * Evaluates if the user belongs to the special rule of preventReviewRequests
+     * and if the request for a review should be skipped
+     */
+    if (preventReviewRequests) {
+      if (
+        preventReviewRequests.teams &&
+        teamsToRequest?.some((team) => preventReviewRequests.teams?.indexOf(team) !== -1)
+      ) {
+        this.logger.info("Filtering teams to request a review from.");
+        teamsToRequest = teamsToRequest?.filter((team) => preventReviewRequests.teams?.indexOf(team) === -1);
+      }
+      if (
+        preventReviewRequests.users &&
+        usersToRequest?.some((user) => preventReviewRequests.users?.indexOf(user) !== -1)
+      ) {
+        this.logger.info("Filtering users to request a review from.");
+        usersToRequest = usersToRequest?.filter((user) => preventReviewRequests.users?.indexOf(user) === -1);
+      }
+    }
+
     const validArray = (array: string[] | undefined): boolean => !!array && array.length > 0;
     const reviewersLog = [
       validArray(teamsToRequest) ? `Teams: ${JSON.stringify(teamsToRequest)}` : "",
@@ -448,7 +472,7 @@ export class ActionRunner {
     const checkRunData = this.generateCheckRunData(reports);
     await this.checks.generateCheckRun(checkRunData);
 
-    this.requestReviewers(reports);
+    this.requestReviewers(reports, config.preventReviewRequests);
 
     setOutput("report", JSON.stringify(prValidation));
 

src/test/rules/config.test.ts

@@ -169,7 +169,7 @@ describe("Config Parsing", () => {
       expect(config.preventReviewRequests?.users).toEqual(["user-a", "user-b"]);
     });
 
-    test("should fail with both users and teams", async () => {
+    test("should get both users and teams", async () => {
       api.getConfigFile.mockResolvedValue(`
       rules:
         - name: Default review
@@ -187,12 +187,12 @@ describe("Config Parsing", () => {
           - user-a
           - user-b
         teams:
-              - team-a
-              - team-b
+          - team-a
+          - team-b
         `);
-      await expect(runner.getConfigFile("")).rejects.toThrowError(
-        '"preventReviewRequests" contains a conflict between exclusive peers [users, teams]',
-      );
+      const config = await runner.getConfigFile("");
+      expect(config.preventReviewRequests?.users).toEqual(["user-a", "user-b"]);
+      expect(config.preventReviewRequests?.teams).toEqual(["team-a", "team-b"]);
     });
 
     test("should pass if preventReviewRequests is not assigned", async () => {

src/test/runner/runner.test.ts

@@ -1,3 +1,4 @@
+/* eslint-disable @typescript-eslint/unbound-method */
 import { mock, MockProxy } from "jest-mock-extended";
 
 import { GitHubChecksApi } from "../../github/check";
@@ -10,10 +11,13 @@ import { ActionRunner } from "../../runner";
 describe("Shared validations", () => {
   let api: MockProxy<PullRequestApi>;
   let teamsApi: MockProxy<TeamApi>;
+  let logger: MockProxy<ActionLogger>;
   let runner: ActionRunner;
   beforeEach(() => {
     api = mock<PullRequestApi>();
-    runner = new ActionRunner(api, teamsApi, mock<GitHubChecksApi>(), mock<ActionLogger>());
+    logger = mock<ActionLogger>();
+    teamsApi = mock<TeamApi>();
+    runner = new ActionRunner(api, teamsApi, mock<GitHubChecksApi>(), logger);
   });
 
   test("validatePullRequest should return true if no rule matches any files", async () => {
@@ -59,4 +63,40 @@ describe("Shared validations", () => {
       expect(result).not.toContain(".github/workflows/review-bot.yml");
     });
   });
+
+  describe("Validation in requestReviewers", () => {
+    const exampleReport = {
+      name: "Example",
+      missingUsers: ["user-1", "user-2", "user-3"],
+      missingReviews: 2,
+      teamsToRequest: ["team-1"],
+      usersToRequest: ["user-1"],
+    };
+
+    test("should request reviewers if object is not defined", () => {
+      runner.requestReviewers([exampleReport], undefined);
+      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
+      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
+    });
+
+    test("should not request user if he is defined", () => {
+      runner.requestReviewers([exampleReport], { users: ["user-1"] });
+      expect(logger.info).toHaveBeenCalledWith("Filtering users to request a review from.");
+      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
+      expect(logger.info).not.toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
+    });
+
+    test("should not request team if it is defined", () => {
+      runner.requestReviewers([exampleReport], { teams: ["team-1"] });
+      expect(logger.info).toHaveBeenCalledWith("Filtering teams to request a review from.");
+      expect(logger.info).not.toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
+      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
+    });
+
+    test("should request reviewers if the team and user are not the same", () => {
+      runner.requestReviewers([exampleReport], { users: ["user-pi"], teams: ["team-alpha"] });
+      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
+      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
+    });
+  });
 });