diff --git a/src/file/validator.ts b/src/file/validator.ts index 59337d9..551b758 100644 --- a/src/file/validator.ts +++ b/src/file/validator.ts @@ -5,7 +5,7 @@ import { ActionLogger } from "../github/types"; import { BasicRule, ConfigurationFile, Rule } from "./types"; /** For the users or team schema. Will be recycled A LOT - * Remember to add `.xor("users", "teams")` to force one of the two to be picked up + * 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), @@ -39,7 +39,7 @@ export const generalSchema = Joi.object().keys({ */ export const basicRuleSchema = Joi.object() .keys({ min_approvals: Joi.number().empty(1), ...reviewersObj }) - .xor("users", "teams"); + .or("users", "teams"); /** * Evaluates a config thoroughly. If there is a problem with it, it will throw. diff --git a/src/runner.ts b/src/runner.ts index 276cb2e..9959e3e 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -6,6 +6,7 @@ import { validateConfig, validateRegularExpressions } from "./file/validator"; import { PullRequestApi } from "./github/pullRequest"; import { TeamApi } from "./github/teams"; import { ActionLogger } from "./github/types"; +import { concatArraysUniquely } from "./util"; type ReviewReport = { /** The amount of missing reviews to fulfill the requirements */ @@ -88,7 +89,7 @@ export class ActionRunner { */ async evaluateCondition(rule: { min_approvals: number } & Reviewers): Promise { // This is a list of all the users that need to approve a PR - const requiredUsers: string[] = []; + let requiredUsers: string[] = []; // If team is set, we fetch the members of such team if (rule.teams) { for (const team of rule.teams) { @@ -101,11 +102,12 @@ export class ActionRunner { } } // If, instead, users are set, we simply push them to the array as we don't need to scan a team - } else if (rule.users) { - requiredUsers.push(...rule.users); - } else { - // This should be captured before by the validation - throw new Error("Teams and Users field are not set for rule."); + } + if (rule.users) { + requiredUsers = concatArraysUniquely(requiredUsers, rule.users); + } + if (requiredUsers.length === 0) { + throw new Error("No users have been found in the required reviewers"); } if (requiredUsers.length < rule.min_approvals) { diff --git a/src/test/runner/conditions.test.ts b/src/test/runner/conditions.test.ts index 3a3b71b..a174882 100644 --- a/src/test/runner/conditions.test.ts +++ b/src/test/runner/conditions.test.ts @@ -19,7 +19,7 @@ describe("evaluateCondition tests", () => { test("should throw if no teams or users were set", async () => { await expect(runner.evaluateCondition({ min_approvals: 99 })).rejects.toThrowError( - "Teams and Users field are not set for rule.", + "No users have been found in the required reviewers", ); }); @@ -131,6 +131,23 @@ describe("evaluateCondition tests", () => { expect(report?.missingUsers).toEqual([...team1.users, team2.users[0]]); expect(report?.teamsToRequest).toEqual([team1.name, team2.name]); }); + + describe("teams and users combined", () => { + test("should not duplicate users if they belong to team and user list", async () => { + teamsApi.getTeamMembers.calledWith(team).mockResolvedValue(users); + api.listApprovedReviewsAuthors.mockResolvedValue([]); + const [result, report] = await runner.evaluateCondition({ + min_approvals: 1, + teams: [team], + users: [users[0]], + }); + expect(result).toBeFalsy(); + // Should not send required users more than once + expect(report?.missingUsers).toEqual(users); + expect(report?.teamsToRequest).toEqual([team]); + expect(report?.usersToRequest).toEqual([users[0]]); + }); + }); }); }); }); diff --git a/src/util.ts b/src/util.ts index 1197efc..95688d0 100644 --- a/src/util.ts +++ b/src/util.ts @@ -5,3 +5,11 @@ import { ActionLogger } from "./github/types"; export function generateCoreLogger(): ActionLogger { return { info, debug, warn: warning, error }; } + +/** Concats two arrays and remove the duplicates */ +export function concatArraysUniquely(arr1?: T[], arr2?: T[]): T[] { + // We concat the two arrays + const concatedArray = (arr1 ?? []).concat(arr2 ?? []); + // We remove the duplicated values and return the array + return concatedArray.filter((item, pos) => concatedArray.indexOf(item) === pos); +}