From 3be1c526a9805bdd53d384b2dadeb713e70ea767 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 29 Aug 2023 16:01:31 +0200 Subject: [PATCH 01/10] added method for preventReviewRequests --- src/rules/validator.ts | 2 +- src/runner.ts | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/rules/validator.ts b/src/rules/validator.ts index 3a32c8e..a98d9d7 100644 --- a/src/rules/validator.ts +++ b/src/rules/validator.ts @@ -34,7 +34,7 @@ const ruleSchema = Joi.object().keys({ */ export const generalSchema = Joi.object().keys({ rules: Joi.array().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 diff --git a/src/runner.ts b/src/runner.ts index 6dbc81a..d541037 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -332,6 +332,35 @@ export class ActionRunner { return [false, generateErrorReport()]; } + async preventReviewEvaluation({ + preventReviewRequests, + }: Pick): Promise { + if (!preventReviewRequests) { + this.logger.debug("No preventReviewRequests. Skipping check"); + return false; + } + + const author = this.prApi.getAuthor(); + + if (preventReviewRequests.users && preventReviewRequests.users.indexOf(author)) { + this.logger.info("User does belongs to list of users to prevent the review request."); + return true; + } + + if (preventReviewRequests.teams) { + for (const team of preventReviewRequests.teams) { + const members = await this.teamApi.getTeamMembers(team); + if (members.indexOf(author) > -1) { + this.logger.info(`User belong to the team ${team} which is part of the preventReviewRequests.`); + return true; + } + } + } + + this.logger.debug("User does not belong to any of the preventReviewRequests requirements"); + return false; + } + /** 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 From 32d53c071b534f4773778d1623fc12d3f84b9b32 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 29 Aug 2023 17:07:18 +0200 Subject: [PATCH 02/10] implemented the method --- src/runner.ts | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index d541037..a2156b6 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -66,9 +66,17 @@ export class ActionRunner { * The action evaluates if the rules requirements are meet for a PR * @returns an array of error reports for each failed rule. An empty array means no errors */ - async validatePullRequest({ rules }: ConfigurationFile): Promise { - const errorReports: RuleReport[] = []; + async validatePullRequest({ rules, preventReviewRequests }: ConfigurationFile): Promise { const modifiedFiles = await this.prApi.listModifiedFiles(); + + // verify if we should skip the validation + if (await this.preventReviewEvaluation({ preventReviewRequests })) { + this.logger.info("Skipping validation. User belongs to 'preventReviewRequests'"); + return { files: modifiedFiles, reports: [] }; + } + + const errorReports: RuleReport[] = []; + ruleCheck: for (const rule of rules) { try { this.logger.info(`Validating rule '${rule.name}'`); @@ -332,6 +340,11 @@ export class ActionRunner { return [false, generateErrorReport()]; } + /** + * Evaluates if the user belongs to the special rule of preventReviewRequests + * and if the check should be skipped. + * @returns a boolean. True if the check should be skipped. + */ async preventReviewEvaluation({ preventReviewRequests, }: Pick): Promise { From 8072b0a8f9fc39b573827294cdcbbac15ab3885e Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 29 Aug 2023 17:10:27 +0200 Subject: [PATCH 03/10] fixed failing test --- src/rules/validator.ts | 7 ++++--- src/test/rules/config.test.ts | 26 -------------------------- 2 files changed, 4 insertions(+), 29 deletions(-) diff --git a/src/rules/validator.ts b/src/rules/validator.ts index a98d9d7..24c307f 100644 --- a/src/rules/validator.ts +++ b/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 @@ -41,13 +42,13 @@ export const generalSchema = Joi.object().keys({ * This rule is quite simple as it only has the min_approvals field and the required reviewers */ export const basicRuleSchema = Joi.object() - .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().keys({ reviewers: Joi.array() - .items(Joi.object().keys(reviewersObj).or("users", "teams")) + .items(Joi.object().keys(reviewerConditionObj).or("users", "teams")) .min(2) .required(), countAuthor: Joi.boolean().default(false), diff --git a/src/test/rules/config.test.ts b/src/test/rules/config.test.ts index d3e50fd..40b76f6 100644 --- a/src/test/rules/config.test.ts +++ b/src/test/rules/config.test.ts @@ -169,32 +169,6 @@ describe("Config Parsing", () => { expect(config.preventReviewRequests?.users).toEqual(["user-a", "user-b"]); }); - test("should fail with both users and teams", async () => { - api.getConfigFile.mockResolvedValue(` - rules: - - name: Default review - condition: - include: - - '.*' - exclude: - - 'example' - type: basic - teams: - - team-example - - preventReviewRequests: - users: - - user-a - - user-b - teams: - - team-a - - team-b - `); - await expect(runner.getConfigFile("")).rejects.toThrowError( - '"preventReviewRequests" contains a conflict between exclusive peers [users, teams]', - ); - }); - test("should pass if preventReviewRequests is not assigned", async () => { api.getConfigFile.mockResolvedValue(` rules: From c6c8e26135e9e7a6a476f81fde3e17a820ee1bd7 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 29 Aug 2023 17:29:56 +0200 Subject: [PATCH 04/10] added tests --- src/runner.ts | 4 +-- src/test/rules/config.test.ts | 26 ++++++++++++++++++ src/test/runner/runner.test.ts | 48 +++++++++++++++++++++++++++++++++- 3 files changed, 75 insertions(+), 3 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index a2156b6..bcec9d6 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -355,7 +355,7 @@ export class ActionRunner { const author = this.prApi.getAuthor(); - if (preventReviewRequests.users && preventReviewRequests.users.indexOf(author)) { + if (preventReviewRequests.users && preventReviewRequests.users.indexOf(author) > -1) { this.logger.info("User does belongs to list of users to prevent the review request."); return true; } @@ -364,7 +364,7 @@ export class ActionRunner { for (const team of preventReviewRequests.teams) { const members = await this.teamApi.getTeamMembers(team); if (members.indexOf(author) > -1) { - this.logger.info(`User belong to the team ${team} which is part of the preventReviewRequests.`); + this.logger.info(`User belong to the team '${team}' which is part of the preventReviewRequests.`); return true; } } diff --git a/src/test/rules/config.test.ts b/src/test/rules/config.test.ts index 40b76f6..ee13322 100644 --- a/src/test/rules/config.test.ts +++ b/src/test/rules/config.test.ts @@ -169,6 +169,32 @@ describe("Config Parsing", () => { expect(config.preventReviewRequests?.users).toEqual(["user-a", "user-b"]); }); + test("should get both users and teams", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Default review + condition: + include: + - '.*' + exclude: + - 'example' + type: basic + teams: + - team-example + + preventReviewRequests: + users: + - user-a + - user-b + teams: + - team-a + - team-b + `); + 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 () => { api.getConfigFile.mockResolvedValue(` rules: diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index b846f20..21c91a7 100644 --- a/src/test/runner/runner.test.ts +++ b/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,11 @@ import { ActionRunner } from "../../runner"; describe("Shared validations", () => { let api: MockProxy; let teamsApi: MockProxy; + let logger: MockProxy; let runner: ActionRunner; beforeEach(() => { api = mock(); - runner = new ActionRunner(api, teamsApi, mock(), mock()); + runner = new ActionRunner(api, teamsApi, logger); }); test("validatePullRequest should return true if no rule matches any files", async () => { @@ -59,4 +61,48 @@ describe("Shared validations", () => { expect(result).not.toContain(".github/workflows/review-bot.yml"); }); }); + + describe("Validation in reviewerConditionObj", () => { + const authorName = "my-great-author"; + beforeEach(() => { + api.getAuthor.mockReturnValue(authorName); + }); + test("should return false if the object is not defined", async () => { + const config: ConfigurationFile = { rules: [] }; + const result = await runner.preventReviewEvaluation(config); + expect(result).toBeFalsy(); + }); + + test("should return true if the user is in the users", async () => { + const config: ConfigurationFile = { rules: [], preventReviewRequests: { users: [authorName] } }; + const result = await runner.preventReviewEvaluation(config); + expect(result).toBeTruthy(); + expect(logger.info).toHaveBeenCalledWith("User does belongs to list of users to prevent the review request."); + }); + + test("should return true if the user is a team member", async () => { + const config: ConfigurationFile = { rules: [], preventReviewRequests: { teams: ["team-a", "team-b"] } }; + teamsApi.getTeamMembers.calledWith("team-a").mockResolvedValue(["abc", "def", "ghi"]); + teamsApi.getTeamMembers.calledWith("team-b").mockResolvedValue(["zyx", "wvt", authorName]); + const result = await runner.preventReviewEvaluation(config); + expect(result).toBeTruthy(); + expect(logger.info).toHaveBeenCalledWith( + "User belong to the team 'team-b' which is part of the preventReviewRequests.", + ); + }); + + test("should return false if the user is not in the team and users", async () => { + const config: ConfigurationFile = { + rules: [], + preventReviewRequests: { teams: ["team-a", "team-b"], users: ["qwerty", "dvorak"] }, + }; + teamsApi.getTeamMembers.calledWith("team-a").mockResolvedValue(["abc", "def", "ghi"]); + teamsApi.getTeamMembers.calledWith("team-b").mockResolvedValue(["zyx", "wvu", "tsr"]); + const result = await runner.preventReviewEvaluation(config); + expect(result).toBeFalsy(); + expect(logger.debug).toHaveBeenCalledWith( + "User does not belong to any of the preventReviewRequests requirements", + ); + }); + }); }); From 0f8e6ea0087baa5a281d33aa8779f67b3a410412 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 29 Aug 2023 17:36:23 +0200 Subject: [PATCH 05/10] updated readme --- README.md | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 94f71f2..92083c5 100644 --- a/README.md +++ b/README.md @@ -169,11 +169,12 @@ 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: @@ -181,8 +182,17 @@ It contains an object called `rules` which has an array of rules. Every rule has 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,19 @@ 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**. + +It supports two fields: `users` and `teams`. If *the user is inside the users array, or they belong to one of the teams listed there, the validation will automatically be skipped, and a success status check will be reported*. + +It is intended to stop the custom rules being required when a user is already trusted and doesn’t need such a thorough review process. + ### 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. From 5cc9f25885711be8e177e8e8dece2ad25df6ccfd Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 29 Aug 2023 17:38:59 +0200 Subject: [PATCH 06/10] fixed readme technicality --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 92083c5..c5edd17 100644 --- a/README.md +++ b/README.md @@ -213,7 +213,7 @@ This is a special field that applies to all the rules. This field is **optional**. -It supports two fields: `users` and `teams`. If *the user is inside the users array, or they belong to one of the teams listed there, the validation will automatically be skipped, and a success status check will be reported*. +It supports two fields: `users` and `teams`. If *the author of the Pull Request is inside the users array, or they belong to one of the teams listed there, the validation will automatically be skipped, and a success status check will be reported*. It is intended to stop the custom rules being required when a user is already trusted and doesn’t need such a thorough review process. From 6ee24d2dc5b2457e14e118903827b943f4d65b24 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 30 Aug 2023 10:54:00 +0200 Subject: [PATCH 07/10] fixed merge conflicts --- src/test/runner/runner.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index 21c91a7..d629ef0 100644 --- a/src/test/runner/runner.test.ts +++ b/src/test/runner/runner.test.ts @@ -15,7 +15,9 @@ describe("Shared validations", () => { let runner: ActionRunner; beforeEach(() => { api = mock(); - runner = new ActionRunner(api, teamsApi, logger); + logger = mock(); + teamsApi = mock(); + runner = new ActionRunner(api, teamsApi, mock(), logger); }); test("validatePullRequest should return true if no rule matches any files", async () => { From 0b1e8c5a9c2db1190ec06da3b7d344fa4f2a5beb Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 30 Aug 2023 12:13:01 +0200 Subject: [PATCH 08/10] moved evaluation to be inside the request for review --- src/runner.ts | 62 +++++++++++++++++++++------------------------------ 1 file changed, 25 insertions(+), 37 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index bcec9d6..932ff63 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -154,7 +154,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; } @@ -167,7 +167,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)}` : "", @@ -340,40 +362,6 @@ export class ActionRunner { return [false, generateErrorReport()]; } - /** - * Evaluates if the user belongs to the special rule of preventReviewRequests - * and if the check should be skipped. - * @returns a boolean. True if the check should be skipped. - */ - async preventReviewEvaluation({ - preventReviewRequests, - }: Pick): Promise { - if (!preventReviewRequests) { - this.logger.debug("No preventReviewRequests. Skipping check"); - return false; - } - - const author = this.prApi.getAuthor(); - - if (preventReviewRequests.users && preventReviewRequests.users.indexOf(author) > -1) { - this.logger.info("User does belongs to list of users to prevent the review request."); - return true; - } - - if (preventReviewRequests.teams) { - for (const team of preventReviewRequests.teams) { - const members = await this.teamApi.getTeamMembers(team); - if (members.indexOf(author) > -1) { - this.logger.info(`User belong to the team '${team}' which is part of the preventReviewRequests.`); - return true; - } - } - } - - this.logger.debug("User does not belong to any of the preventReviewRequests requirements"); - return false; - } - /** 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 @@ -490,7 +478,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)); From d1653309bf43bf879a1e352c60e3a87848677fd6 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 30 Aug 2023 16:54:03 +0200 Subject: [PATCH 09/10] updated tests --- src/runner.ts | 12 ++----- src/test/runner/runner.test.ts | 62 +++++++++++++++------------------- 2 files changed, 30 insertions(+), 44 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index 932ff63..800bd52 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -66,15 +66,9 @@ export class ActionRunner { * The action evaluates if the rules requirements are meet for a PR * @returns an array of error reports for each failed rule. An empty array means no errors */ - async validatePullRequest({ rules, preventReviewRequests }: ConfigurationFile): Promise { + async validatePullRequest({ rules }: ConfigurationFile): Promise { const modifiedFiles = await this.prApi.listModifiedFiles(); - // verify if we should skip the validation - if (await this.preventReviewEvaluation({ preventReviewRequests })) { - this.logger.info("Skipping validation. User belongs to 'preventReviewRequests'"); - return { files: modifiedFiles, reports: [] }; - } - const errorReports: RuleReport[] = []; ruleCheck: for (const rule of rules) { @@ -176,14 +170,14 @@ export class ActionRunner { if (preventReviewRequests) { if ( preventReviewRequests.teams && - teamsToRequest?.some((team) => preventReviewRequests.teams?.indexOf(team) === -1) + 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) + 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); diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index d629ef0..35619e2 100644 --- a/src/test/runner/runner.test.ts +++ b/src/test/runner/runner.test.ts @@ -64,47 +64,39 @@ describe("Shared validations", () => { }); }); - describe("Validation in reviewerConditionObj", () => { - const authorName = "my-great-author"; - beforeEach(() => { - api.getAuthor.mockReturnValue(authorName); - }); - test("should return false if the object is not defined", async () => { - const config: ConfigurationFile = { rules: [] }; - const result = await runner.preventReviewEvaluation(config); - expect(result).toBeFalsy(); + 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 return true if the user is in the users", async () => { - const config: ConfigurationFile = { rules: [], preventReviewRequests: { users: [authorName] } }; - const result = await runner.preventReviewEvaluation(config); - expect(result).toBeTruthy(); - expect(logger.info).toHaveBeenCalledWith("User does belongs to list of users to prevent the review request."); + 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 return true if the user is a team member", async () => { - const config: ConfigurationFile = { rules: [], preventReviewRequests: { teams: ["team-a", "team-b"] } }; - teamsApi.getTeamMembers.calledWith("team-a").mockResolvedValue(["abc", "def", "ghi"]); - teamsApi.getTeamMembers.calledWith("team-b").mockResolvedValue(["zyx", "wvt", authorName]); - const result = await runner.preventReviewEvaluation(config); - expect(result).toBeTruthy(); - expect(logger.info).toHaveBeenCalledWith( - "User belong to the team 'team-b' which is part of the preventReviewRequests.", - ); + 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 return false if the user is not in the team and users", async () => { - const config: ConfigurationFile = { - rules: [], - preventReviewRequests: { teams: ["team-a", "team-b"], users: ["qwerty", "dvorak"] }, - }; - teamsApi.getTeamMembers.calledWith("team-a").mockResolvedValue(["abc", "def", "ghi"]); - teamsApi.getTeamMembers.calledWith("team-b").mockResolvedValue(["zyx", "wvu", "tsr"]); - const result = await runner.preventReviewEvaluation(config); - expect(result).toBeFalsy(); - expect(logger.debug).toHaveBeenCalledWith( - "User does not belong to any of the preventReviewRequests requirements", - ); + 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"]))); }); }); }); From 3dd80577c3ea42ab1e3a0f6036bf924fc2f4d330 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 30 Aug 2023 16:56:30 +0200 Subject: [PATCH 10/10] updated readme --- README.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/README.md b/README.md index c5edd17..a286686 100644 --- a/README.md +++ b/README.md @@ -211,11 +211,8 @@ preventReviewRequests: #### preventReviewRequests This is a special field that applies to all the rules. -This field is **optional**. +This field is **optional** and currently not used. Pending on https://github.com/paritytech/review-bot/issues/53 -It supports two fields: `users` and `teams`. If *the author of the Pull Request is inside the users array, or they belong to one of the teams listed there, the validation will automatically be skipped, and a success status check will be reported*. - -It is intended to stop the custom rules being required when a user is already trusted and doesn’t need such a thorough review process. ### Types Every type has a *slightly* different configuration and works for different scenarios, so let’s analyze all of them.