diff --git a/README.md b/README.md index b8ff2b5..e8002fe 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,7 @@ This is a GitHub Action created for complex pull request approval scenarios whic - [Rules syntax](#rules-syntax) - [Basic Rule syntax](#basic-rule-syntax) - [AND Rule syntax](#and-rule-syntax) + - [AND DISTINCT Rule syntax](#and-distinct-rule-syntax) - [OR Rule syntax](#or-rule-syntax) - [Workflow configuration](#workflow-configuration) - [GitHub repository configuration](#github-repository-configuration) @@ -137,7 +138,9 @@ rules: #### AND Rule syntax AND Rules will only match if **all** subconditions listed in `all` are -fulfilled. +fulfilled. Note that each subcondition is treated independently and therefore a +single approval can count towards multiple subconditions; for an alternative, +check [AND DISTINCT rules](#and-distinct-rule-syntax). ```yaml rules: @@ -158,6 +161,45 @@ rules: Visit [Basic Rule syntax](#basic-rule-syntax) for the full explanation of each field. +#### AND DISTINCT Rule syntax + +AND DISTINCT Rules work like [AND Rules](#and-rule-syntax) in the sense that all +subconditions have to be fulfilled, except that each approval contributes at +most **once** for a single subcondition, i.e. all approvals throughout all +subconditions have to come from different users (hence the name DISTINCT). + +```yaml +rules: + - name: Rule name + condition: .* + check_type: diff + all_distinct: + - min_approvals: 1 + teams: + - Team 1 + - min_approvals: 1 + teams: + - Team 2 +``` + +The common use-case for this rule is requiring that approvals come from +different teams even if one of the teams is a subset of another. Suppose the +following structure: + +``` +── Team1 (one approval required) + └── User1 +── Team2 (one approval required) + ├── User1 + └── User2 +``` + +For AND Rules, if User1 would approve the pull request, since they belong to +*both* Team1 and Team2, *both* subconditions would be fulfilled. By comparison, +for AND DISTINCT Rules the second subcondition would not be fulfilled with +User1's approval alone because his approval already counted towards the first +subcondition. + #### OR Rule syntax OR Rules will match if **any** subconditions listed in `any` are fulfilled. diff --git a/package-lock.json b/package-lock.json index 31269e3..cbce4e6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6298,9 +6298,9 @@ } }, "node_modules/minimist": { - "version": "1.2.5", - "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.5.tgz", - "integrity": "sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw==", + "version": "1.2.6", + "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.6.tgz", + "integrity": "sha512-Jsjnk4bw3YJqYzbdyBiNsPWHPfO++UGG749Cxs6peCu5Xg4nrena6OVxOYxrQTqww0Jmwt+Ref8rggumkTLz9Q==", "dev": true }, "node_modules/ms": { @@ -12665,9 +12665,9 @@ } }, "minimist": { - "version": "1.2.5", - "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.5.tgz", - "integrity": "sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw==", + "version": "1.2.6", + "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.6.tgz", + "integrity": "sha512-Jsjnk4bw3YJqYzbdyBiNsPWHPfO++UGG749Cxs6peCu5Xg4nrena6OVxOYxrQTqww0Jmwt+Ref8rggumkTLz9Q==", "dev": true }, "ms": { diff --git a/src/constants.ts b/src/constants.ts index cfe66b1..f492d0e 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -14,17 +14,22 @@ export const rulesConfigurations: RulesConfigurations = { basic: { kind: "BasicRule", uniqueFields: ["min_approvals", "teams", "users"], - invalidFields: ["any", "all"], + invalidFields: ["any", "all", "all_distinct"], }, and: { kind: "AndRule", uniqueFields: ["all"], - invalidFields: ["min_approvals", "teams", "users", "any"], + invalidFields: ["min_approvals", "teams", "users", "any", "all_distinct"], }, or: { kind: "OrRule", uniqueFields: ["any"], - invalidFields: ["min_approvals", "teams", "users", "all"], + invalidFields: ["min_approvals", "teams", "users", "all", "all_distinct"], + }, + and_distinct: { + kind: "AndDistinctRule", + uniqueFields: ["all_distinct"], + invalidFields: ["min_approvals", "teams", "users", "all", "any"], }, } diff --git a/src/core.ts b/src/core.ts index 3b82915..b48e476 100644 --- a/src/core.ts +++ b/src/core.ts @@ -9,7 +9,6 @@ import { configFilePath, maxGithubApiFilesPerPage, maxGithubApiTeamMembersPerPage, - rulesConfigurations, variableNameToActionInputName, } from "./constants" import { LoggerInterface } from "./logger" @@ -27,8 +26,8 @@ import { import { configurationSchema } from "./validation" type TeamsCache = Map< - /* Team slug */ string, - /* Usernames of team members */ string[] + string /* Team slug */, + string[] /* Usernames of team members */ > const combineUsers = async function ( pr: PR, @@ -41,12 +40,13 @@ const combineUsers = async function ( for (const user of presetUsers) { if (pr.user.login != user) { - users.set(user, { teams: null }) + users.set(user, { ...users.get(user), teams: null }) } } for (const team of teams) { let teamMembers = teamsCache.get(team) + if (teamMembers === undefined) { teamMembers = await octokit.paginate( octokit.rest.teams.listMembersInOrg, @@ -65,18 +65,22 @@ const combineUsers = async function ( } for (const teamMember of teamMembers) { - const userInfo = users.get(teamMember) + let userInfo = users.get(teamMember) + if (userInfo === undefined) { + userInfo = { teams: new Set([team]), teamsHistory: new Set([team]) } + users.set(teamMember, userInfo) + } else if (userInfo.teamsHistory === undefined) { + userInfo.teamsHistory = new Set([team]) + } else { + userInfo.teamsHistory.add(team) + } if ( pr.user.login != teamMember && // We do not want to register a team for this user if their approval is // supposed to be requested individually - userInfo?.teams !== null + userInfo.teams !== null ) { - if (userInfo === undefined) { - users.set(teamMember, { teams: new Set([team]) }) - } else { - userInfo.teams.add(team) - } + userInfo.teams.add(team) } } } @@ -146,16 +150,35 @@ export const runChecks = async function ( const lockExpression = /🔒[^\n]*\n[+|-]|(^|\n)[+|-][^\n]*🔒/ if (lockExpression.test(diff)) { logger.log("Diff has changes to 🔒 lines or lines following 🔒") - for (const team of [locksReviewTeam, teamLeadsTeam]) { - const users = await combineUsers(pr, octokit, [], [team], teamsCache) - matchedRules.push({ - name: `LOCKS TOUCHED (team: ${team})`, + const users = await combineUsers( + pr, + octokit, + [], + [locksReviewTeam, teamLeadsTeam], + teamsCache, + ) + const subConditions = [ + { min_approvals: 1, - kind: "AndRule", - users, - id: ++nextMatchedRuleId, - }) - } + teams: [locksReviewTeam], + name: `Locks Reviewers Approvals (team ${locksReviewTeam})`, + }, + { + min_approvals: 1, + teams: [teamLeadsTeam], + name: `Team Leads Approvals (team ${teamLeadsTeam})`, + }, + ] + matchedRules.push({ + name: "Locks touched", + kind: "AndDistinctRule", + users, + id: ++nextMatchedRuleId, + min_approvals: subConditions + .map(({ min_approvals }) => min_approvals) + .reduce((acc, val) => acc + val, 0), + subConditions, + }) } const changedFiles = new Set( @@ -252,44 +275,59 @@ export const runChecks = async function ( kind: RuleKind, subConditions: RuleCriteria[], ) { - let conditionIndex = -1 - for (const subCondition of subConditions) { - const users = await combineUsers( - pr, - octokit, - subCondition.users ?? [], - subCondition.teams ?? [], - teamsCache, - ) - matchedRules.push({ - name: `${name}[${++conditionIndex}]`, - min_approvals: subCondition.min_approvals, - users, - kind, - id, - }) - } - } - - for (const rule of config.rules) { - // Validate that rules which are matched to a "kind" do not have fields of other "kinds" - for (const { kind, uniqueFields, invalidFields } of Object.values( - rulesConfigurations, - )) { - for (const field of uniqueFields) { - if (field in rule) { - for (const invalidField of invalidFields) { - if (invalidField in rule) { - logger.failure( - `Rule "${rule.name}" was expected to be of kind "${kind}" because it had the field "${field}", but it also has the field "${invalidField}", which belongs to another kind. Mixing fields from different kinds of rules is not allowed.`, - ) - return commitStateFailure - } - } + switch (kind) { + case "AndDistinctRule": { + const users = await combineUsers( + pr, + octokit, + subConditions.map(({ users }) => users ?? []).flat(), + subConditions.map(({ teams }) => teams ?? []).flat(), + teamsCache, + ) + matchedRules.push({ + name: name, + users, + kind, + id, + subConditions, + min_approvals: subConditions + .map(({ min_approvals }) => min_approvals) + .reduce((acc, val) => acc + val, 0), + }) + break + } + case "BasicRule": + case "OrRule": + case "AndRule": { + let conditionIndex = -1 + for (const subCondition of subConditions) { + const users = await combineUsers( + pr, + octokit, + subCondition.users ?? [], + subCondition.teams ?? [], + teamsCache, + ) + matchedRules.push({ + name: `${name}[${++conditionIndex}]`, + min_approvals: subCondition.min_approvals, + users, + kind, + id, + }) } + break + } + default: { + const exhaustivenessCheck: never = kind + const failureMessage = `Rule kind is not handled: ${exhaustivenessCheck}` + logger.failure(failureMessage) + throw new Error(failureMessage) } } + } + for (const rule of config.rules) { const includeCondition = (function () { switch (typeof rule.condition) { case "string": { @@ -397,6 +435,13 @@ export const runChecks = async function ( "OrRule", rule.any, ) + } else if (/* AndDistinctRule */ "all_distinct" in rule) { + await processComplexRule( + ++nextMatchedRuleId, + rule.name, + "AndDistinctRule", + rule.all_distinct, + ) } else { const unmatchedRule = rule as BaseRule throw new Error( @@ -461,7 +506,72 @@ export const runChecks = async function ( } } - if (approvedBy.size < rule.min_approvals) { + if (rule.kind === "AndDistinctRule") { + const ruleApprovedBy: Set = new Set() + + const failedSubconditions: Array = [] + toNextSubcondition: for ( + let i = 0; + i < rule.subConditions.length; + i++ + ) { + const subCondition = rule.subConditions[i] + let approvalCount = 0 + for (const user of subCondition.users ?? []) { + if (approvedBy.has(user) && !ruleApprovedBy.has(user)) { + ruleApprovedBy.add(user) + approvalCount++ + if (approvalCount === subCondition.min_approvals) { + continue toNextSubcondition + } + } + } + for (const team of subCondition.teams ?? []) { + for (const [user, userInfo] of rule.users) { + if ( + approvedBy.has(user) && + !ruleApprovedBy.has(user) && + userInfo?.teamsHistory?.has(team) + ) { + ruleApprovedBy.add(user) + approvalCount++ + if (approvalCount === subCondition.min_approvals) { + continue toNextSubcondition + } + } + } + } + failedSubconditions.push( + typeof subCondition.name === "string" + ? `"${subCondition.name}"` + : `at index ${i}`, + ) + } + + if (failedSubconditions.length) { + const usersToAskForReview: Map = new Map( + Array.from(rule.users.entries()).filter(function ([username]) { + return !approvedBy.has(username) + }), + ) + const problem = `Rule "${rule.name}" needs in total ${ + rule.min_approvals + } DISTINCT approvals, meaning users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subcondition${ + failedSubconditions.length > 1 ? "s" : "" + } ${failedSubconditions.join( + " and ", + )} failed. The following users have not approved yet: ${Array.from( + usersToAskForReview.entries(), + ) + .map(function ([username, { teams }]) { + return `${username}${teams ? ` (team${teams.size === 1 ? "" : "s"}: ${Array.from(teams).join(",")})` : ""}` + }) + .join(", ")}.` + outcomes.push(new RuleFailure(rule, problem, usersToAskForReview)) + } else { + outcomes.push(new RuleSuccess(rule)) + } + } else if (approvedBy.size < rule.min_approvals) { const usersToAskForReview: Map = new Map( Array.from(rule.users.entries()).filter(function ([username]) { return !approvedBy.has(username) @@ -512,11 +622,11 @@ export const runChecks = async function ( const prevUser = pendingUsersToAskForReview.get(username) if ( prevUser === undefined || - // If the team is null, this user was not asked as part of a - // team, but individually. They should always be registered with - // "team: null" that case to be sure the review will be - // requested individually, even if they were previously - // registered as part of a team. + // If the team is null, this user was not asked as part of a team, + // but individually. They should always be registered with "team: + // null" that case to be sure the review will be requested + // individually, even if they were previously registered as part + // of a team. userInfo.teams === null ) { pendingUsersToAskForReview.set(username, { @@ -543,11 +653,10 @@ export const runChecks = async function ( const prevUser = usersToAskForReview.get(username) if ( prevUser === undefined || - // If the team is null, this user was not asked as part of a - // team, but individually. They should always be registered with - // "team: null" that case to be sure the review will be - // requested individually, even if they were previously - // registered as part of a team. + // If the team is null, this user was not asked as part of a team, but + // individually. They should always be registered with "team: null" in + // that case to be sure the review will be requested individually, + // even if they were previously registered as part of a team. userInfo.teams === null ) { usersToAskForReview.set(username, { teams: userInfo.teams }) diff --git a/src/types.ts b/src/types.ts index 4d2ab0b..207d689 100644 --- a/src/types.ts +++ b/src/types.ts @@ -37,6 +37,7 @@ export type BaseRule = { export type RuleCriteria = { min_approvals: number + name?: string users?: Array | null teams?: Array | null } @@ -51,24 +52,33 @@ export type AndRule = BaseRule & { all: RuleCriteria[] } -export type RuleKind = "BasicRule" | "OrRule" | "AndRule" -export type Rule = BasicRule | OrRule | AndRule +export type AndDistinctRule = BaseRule & { + all_distinct: RuleCriteria[] +} + +export type RuleKind = "BasicRule" | "OrRule" | "AndRule" | "AndDistinctRule" +export type Rule = BasicRule | OrRule | AndRule | AndDistinctRule export type RulesConfigurations = { basic: { kind: "BasicRule" uniqueFields: ["min_approvals", "teams", "users"] - invalidFields: ["any", "all"] + invalidFields: ["any", "all", "all_distinct"] } and: { kind: "AndRule" uniqueFields: ["all"] - invalidFields: ["min_approvals", "teams", "users", "any"] + invalidFields: ["min_approvals", "teams", "users", "any", "all_distinct"] } or: { kind: "OrRule" uniqueFields: ["any"] - invalidFields: ["min_approvals", "teams", "users", "all"] + invalidFields: ["min_approvals", "teams", "users", "all", "all_distinct"] + } + and_distinct: { + kind: "AndDistinctRule" + uniqueFields: ["all_distinct"] + invalidFields: ["min_approvals", "teams", "users", "all", "any"] } } @@ -76,15 +86,26 @@ export type Configuration = { rules: Rule[] } -export type RuleUserInfo = { teams: Set | null } +export type RuleUserInfo = { + teams: Set | null + teamsHistory?: Set +} -export type MatchedRule = { +type MatchedRuleBase = { name: string - min_approvals: number users: Map - kind: RuleKind id: number + kind: RuleKind + min_approvals: number } +export type MatchedRule = + | (MatchedRuleBase & { + kind: "AndRule" | "OrRule" | "BasicRule" + }) + | (MatchedRuleBase & { + kind: "AndDistinctRule" + subConditions: RuleCriteria[] + }) export class RuleSuccess { constructor(public rule: MatchedRule) {} diff --git a/src/validation.ts b/src/validation.ts index 0e792aa..c838d1e 100644 --- a/src/validation.ts +++ b/src/validation.ts @@ -1,26 +1,49 @@ import Joi from "joi" -import { Configuration, Rule, RuleCriteria } from "./types" +import { + AndDistinctRule, + AndRule, + BasicRule, + Configuration, + OrRule, + RuleCriteria, +} from "./types" -const ruleCriteria = function ({ - isMinApprovalsOptional, +const ruleCriterion = function ({ + isMinApprovalsAllowed, + isNameOptional, }: { - isMinApprovalsOptional: boolean + isMinApprovalsAllowed: boolean + isNameOptional: boolean }) { - let minApprovals = Joi.number().min(1) - if (isMinApprovalsOptional) { - minApprovals = minApprovals.optional().allow(null) + let name = Joi.string() + if (isNameOptional) { + name = name.optional().allow(null) + } else { + name = name.required() } + return { - min_approvals: minApprovals, + name, users: Joi.array().items(Joi.string()).optional().allow(null), teams: Joi.array().items(Joi.string()).optional().allow(null), + ...(isMinApprovalsAllowed + ? { min_approvals: Joi.number().min(1).required() } + : {}), } } +const ruleCriterionArraySchema = Joi.array() + .items( + Joi.object().keys( + ruleCriterion({ isMinApprovalsAllowed: true, isNameOptional: true }), + ), + ) + .required() + const includeConditionSchema = Joi.string().required() const excludeConditionSchema = Joi.string().required() -const ruleSchema = Joi.object().keys({ +const commonRuleSchema = { name: Joi.string().required(), condition: Joi.alternatives([ includeConditionSchema, @@ -32,24 +55,26 @@ const ruleSchema = Joi.object().keys({ }), ]).required(), check_type: Joi.string().valid("diff", "changed_files").required(), - ...ruleCriteria({ isMinApprovalsOptional: true }), - all: Joi.array() - .items( - Joi.object().keys( - ruleCriteria({ isMinApprovalsOptional: false }), - ), - ) - .optional() - .allow(null), - any: Joi.array() - .items( - Joi.object().keys( - ruleCriteria({ isMinApprovalsOptional: false }), - ), - ) - .optional() - .allow(null), -}) +} + +const ruleSchema = Joi.alternatives([ + Joi.object().keys({ + ...commonRuleSchema, + ...ruleCriterion({ isMinApprovalsAllowed: true, isNameOptional: false }), + }), + Joi.object().keys({ + ...commonRuleSchema, + all: ruleCriterionArraySchema, + }), + Joi.object().keys({ + ...commonRuleSchema, + any: ruleCriterionArraySchema, + }), + Joi.object().keys({ + ...commonRuleSchema, + all_distinct: ruleCriterionArraySchema, + }), +]) export const configurationSchema = Joi.object().keys({ rules: Joi.array().items(ruleSchema).required(), diff --git a/test/batch/configuration.ts b/test/batch/configuration.ts index 72d6a50..4ecbddb 100644 --- a/test/batch/configuration.ts +++ b/test/batch/configuration.ts @@ -41,11 +41,11 @@ describe("Configuration", function () { .reply(200, [{ filename: condition }]) }) - for (const [missingField, value] of [ - ["name", condition], - ["condition", condition], - ["check_type", "diff"], - ["min_approvals", 1], + for (const missingField of [ + "name", + "condition", + "check_type", + "min_approvals", ]) { it(`Configuration is invalid if ${missingField} is missing`, async function () { nock(githubApi) @@ -54,14 +54,10 @@ describe("Configuration", function () { content: Buffer.from( ` rules: - - ${missingField === "name" ? "" : `name: ${value}`} - ${missingField === "condition" ? "" : `condition: ${value}`} - ${missingField === "check_type" ? "" : `check_type: ${value}`} - ${ - missingField === "min_approvals" - ? "" - : `min_approvals: ${value}` - } + - ${missingField === "name" ? "" : `name: condition`} + ${missingField === "condition" ? "" : `condition: condition`} + ${missingField === "check_type" ? "" : `check_type: diff`} + ${missingField === "min_approvals" ? "" : `min_approvals: 1`} `, ).toString("base64"), }) @@ -126,6 +122,7 @@ describe("Configuration", function () { const invalidFieldValidValue = (function () { switch (invalidField) { case "all": + case "all_distinct": case "teams": case "users": case "any": { @@ -187,6 +184,8 @@ describe("Configuration", function () { ? { all: [{ min_approvals: invalidValue }] } : "any" in exampleRule ? { any: [{ min_approvals: invalidValue }] } + : "all_distinct" in exampleRule + ? { all_distinct: [{ min_approvals: invalidValue }] } : {}), }, ], diff --git a/test/batch/configuration.ts.snap b/test/batch/configuration.ts.snap index 8a2b3b9..4286434 100644 --- a/test/batch/configuration.ts.snap +++ b/test/batch/configuration.ts.snap @@ -10,13 +10,13 @@ exports[`Configuration Configuration is invalid if check_type is missing 1`] = ` Array [ "Changed files Set(1) { 'condition' }", "ERROR: Configuration file is invalid", - "[Error [ValidationError]: \\"rules[0].check_type\\" is required] { + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { _original: { rules: [ [Object] ] }, details: [ { - message: '\\"rules[0].check_type\\" is required', + message: '\\"rules[0]\\" does not match any of the allowed types', path: [Array], - type: 'any.required', + type: 'alternatives.match', context: [Object] } ] @@ -28,13 +28,13 @@ exports[`Configuration Configuration is invalid if condition is missing 1`] = ` Array [ "Changed files Set(1) { 'condition' }", "ERROR: Configuration file is invalid", - "[Error [ValidationError]: \\"rules[0].condition\\" is required] { + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { _original: { rules: [ [Object] ] }, details: [ { - message: '\\"rules[0].condition\\" is required', + message: '\\"rules[0]\\" does not match any of the allowed types', path: [Array], - type: 'any.required', + type: 'alternatives.match', context: [Object] } ] @@ -52,13 +52,13 @@ exports[`Configuration Configuration is invalid if min_approvals is less than 1 Array [ "Changed files Set(1) { 'condition' }", "ERROR: Configuration file is invalid", - "[Error [ValidationError]: \\"rules[0].min_approvals\\" must be greater than or equal to 1] { + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { _original: { rules: [ [Object] ] }, details: [ { - message: '\\"rules[0].min_approvals\\" must be greater than or equal to 1', + message: '\\"rules[0]\\" does not match any of the allowed types', path: [Array], - type: 'number.min', + type: 'alternatives.match', context: [Object] } ] @@ -70,13 +70,13 @@ exports[`Configuration Configuration is invalid if min_approvals is missing 1`] Array [ "Changed files Set(1) { 'condition' }", "ERROR: Configuration file is invalid", - "[Error [ValidationError]: \\"rules[0].name\\" must be a string] { + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { _original: { rules: [ [Object] ] }, details: [ { - message: '\\"rules[0].name\\" must be a string', + message: '\\"rules[0]\\" does not match any of the allowed types', path: [Array], - type: 'string.base', + type: 'alternatives.match', context: [Object] } ] @@ -88,13 +88,13 @@ exports[`Configuration Configuration is invalid if name is missing 1`] = ` Array [ "Changed files Set(1) { 'condition' }", "ERROR: Configuration file is invalid", - "[Error [ValidationError]: \\"rules[0].name\\" is required] { + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { _original: { rules: [ [Object] ] }, details: [ { - message: '\\"rules[0].name\\" is required', + message: '\\"rules[0]\\" does not match any of the allowed types', path: [Array], - type: 'any.required', + type: 'alternatives.match', context: [Object] } ] @@ -108,73 +108,363 @@ Array [ ] `; +exports[`Configuration Rule kind AndDistinctRule does not allow invalid field all 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", +] +`; + +exports[`Configuration Rule kind AndDistinctRule does not allow invalid field any 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", +] +`; + +exports[`Configuration Rule kind AndDistinctRule does not allow invalid field min_approvals 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", +] +`; + +exports[`Configuration Rule kind AndDistinctRule does not allow invalid field teams 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", +] +`; + +exports[`Configuration Rule kind AndDistinctRule does not allow invalid field users 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", +] +`; + +exports[`Configuration Rule kind AndRule does not allow invalid field all_distinct 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", +] +`; + exports[`Configuration Rule kind AndRule does not allow invalid field any 1`] = ` Array [ "Changed files Set(1) { 'condition' }", - "ERROR: Rule \\"condition\\" was expected to be of kind \\"AndRule\\" because it had the field \\"all\\", but it also has the field \\"any\\", which belongs to another kind. Mixing fields from different kinds of rules is not allowed.", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", ] `; exports[`Configuration Rule kind AndRule does not allow invalid field min_approvals 1`] = ` Array [ "Changed files Set(1) { 'condition' }", - "ERROR: Rule \\"condition\\" was expected to be of kind \\"BasicRule\\" because it had the field \\"min_approvals\\", but it also has the field \\"all\\", which belongs to another kind. Mixing fields from different kinds of rules is not allowed.", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", ] `; exports[`Configuration Rule kind AndRule does not allow invalid field teams 1`] = ` Array [ "Changed files Set(1) { 'condition' }", - "ERROR: Rule \\"condition\\" was expected to be of kind \\"BasicRule\\" because it had the field \\"teams\\", but it also has the field \\"all\\", which belongs to another kind. Mixing fields from different kinds of rules is not allowed.", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", ] `; exports[`Configuration Rule kind AndRule does not allow invalid field users 1`] = ` Array [ "Changed files Set(1) { 'condition' }", - "ERROR: Rule \\"condition\\" was expected to be of kind \\"BasicRule\\" because it had the field \\"users\\", but it also has the field \\"all\\", which belongs to another kind. Mixing fields from different kinds of rules is not allowed.", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", ] `; exports[`Configuration Rule kind BasicRule does not allow invalid field all 1`] = ` Array [ "Changed files Set(1) { 'condition' }", - "ERROR: Rule \\"condition\\" was expected to be of kind \\"BasicRule\\" because it had the field \\"min_approvals\\", but it also has the field \\"all\\", which belongs to another kind. Mixing fields from different kinds of rules is not allowed.", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", +] +`; + +exports[`Configuration Rule kind BasicRule does not allow invalid field all_distinct 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", ] `; exports[`Configuration Rule kind BasicRule does not allow invalid field any 1`] = ` Array [ "Changed files Set(1) { 'condition' }", - "ERROR: Rule \\"condition\\" was expected to be of kind \\"BasicRule\\" because it had the field \\"min_approvals\\", but it also has the field \\"any\\", which belongs to another kind. Mixing fields from different kinds of rules is not allowed.", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", ] `; exports[`Configuration Rule kind OrRule does not allow invalid field all 1`] = ` Array [ "Changed files Set(1) { 'condition' }", - "ERROR: Rule \\"condition\\" was expected to be of kind \\"AndRule\\" because it had the field \\"all\\", but it also has the field \\"any\\", which belongs to another kind. Mixing fields from different kinds of rules is not allowed.", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", +] +`; + +exports[`Configuration Rule kind OrRule does not allow invalid field all_distinct 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", ] `; exports[`Configuration Rule kind OrRule does not allow invalid field min_approvals 1`] = ` Array [ "Changed files Set(1) { 'condition' }", - "ERROR: Rule \\"condition\\" was expected to be of kind \\"BasicRule\\" because it had the field \\"min_approvals\\", but it also has the field \\"any\\", which belongs to another kind. Mixing fields from different kinds of rules is not allowed.", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", ] `; exports[`Configuration Rule kind OrRule does not allow invalid field teams 1`] = ` Array [ "Changed files Set(1) { 'condition' }", - "ERROR: Rule \\"condition\\" was expected to be of kind \\"BasicRule\\" because it had the field \\"teams\\", but it also has the field \\"any\\", which belongs to another kind. Mixing fields from different kinds of rules is not allowed.", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", ] `; exports[`Configuration Rule kind OrRule does not allow invalid field users 1`] = ` Array [ "Changed files Set(1) { 'condition' }", - "ERROR: Rule \\"condition\\" was expected to be of kind \\"BasicRule\\" because it had the field \\"users\\", but it also has the field \\"any\\", which belongs to another kind. Mixing fields from different kinds of rules is not allowed.", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", +] +`; + +exports[`Configuration min_approvals is invalid for AndDistinctRule if it is less than 1 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", +] +`; + +exports[`Configuration min_approvals is invalid for AndDistinctRule if it is null 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] +}", ] `; @@ -182,13 +472,13 @@ exports[`Configuration min_approvals is invalid for AndRule if it is less than 1 Array [ "Changed files Set(1) { 'condition' }", "ERROR: Configuration file is invalid", - "[Error [ValidationError]: \\"rules[0].all[0].min_approvals\\" must be greater than or equal to 1] { + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { _original: { rules: [ [Object] ] }, details: [ { - message: '\\"rules[0].all[0].min_approvals\\" must be greater than or equal to 1', + message: '\\"rules[0]\\" does not match any of the allowed types', path: [Array], - type: 'number.min', + type: 'alternatives.match', context: [Object] } ] @@ -200,13 +490,13 @@ exports[`Configuration min_approvals is invalid for AndRule if it is null 1`] = Array [ "Changed files Set(1) { 'condition' }", "ERROR: Configuration file is invalid", - "[Error [ValidationError]: \\"rules[0].all[0].min_approvals\\" must be a number] { + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { _original: { rules: [ [Object] ] }, details: [ { - message: '\\"rules[0].all[0].min_approvals\\" must be a number', + message: '\\"rules[0]\\" does not match any of the allowed types', path: [Array], - type: 'number.base', + type: 'alternatives.match', context: [Object] } ] @@ -218,13 +508,13 @@ exports[`Configuration min_approvals is invalid for BasicRule if it is less than Array [ "Changed files Set(1) { 'condition' }", "ERROR: Configuration file is invalid", - "[Error [ValidationError]: \\"rules[0].min_approvals\\" must be greater than or equal to 1] { + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { _original: { rules: [ [Object] ] }, details: [ { - message: '\\"rules[0].min_approvals\\" must be greater than or equal to 1', + message: '\\"rules[0]\\" does not match any of the allowed types', path: [Array], - type: 'number.min', + type: 'alternatives.match', context: [Object] } ] @@ -235,13 +525,17 @@ Array [ exports[`Configuration min_approvals is invalid for BasicRule if it is null 1`] = ` Array [ "Changed files Set(1) { 'condition' }", - "Matched expression \\"condition\\" of rule \\"condition\\" on diff", - "ERROR: Rule \\"condition\\" has invalid min_approvals", - "{ - name: 'condition', - condition: 'condition', - check_type: 'diff', - min_approvals: null + "ERROR: Configuration file is invalid", + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { + _original: { rules: [ [Object] ] }, + details: [ + { + message: '\\"rules[0]\\" does not match any of the allowed types', + path: [Array], + type: 'alternatives.match', + context: [Object] + } + ] }", ] `; @@ -250,13 +544,13 @@ exports[`Configuration min_approvals is invalid for OrRule if it is less than 1 Array [ "Changed files Set(1) { 'condition' }", "ERROR: Configuration file is invalid", - "[Error [ValidationError]: \\"rules[0].any[0].min_approvals\\" must be greater than or equal to 1] { + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { _original: { rules: [ [Object] ] }, details: [ { - message: '\\"rules[0].any[0].min_approvals\\" must be greater than or equal to 1', + message: '\\"rules[0]\\" does not match any of the allowed types', path: [Array], - type: 'number.min', + type: 'alternatives.match', context: [Object] } ] @@ -268,13 +562,13 @@ exports[`Configuration min_approvals is invalid for OrRule if it is null 1`] = ` Array [ "Changed files Set(1) { 'condition' }", "ERROR: Configuration file is invalid", - "[Error [ValidationError]: \\"rules[0].any[0].min_approvals\\" must be a number] { + "[Error [ValidationError]: \\"rules[0]\\" does not match any of the allowed types] { _original: { rules: [ [Object] ] }, details: [ { - message: '\\"rules[0].any[0].min_approvals\\" must be a number', + message: '\\"rules[0]\\" does not match any of the allowed types', path: [Array], - type: 'number.base', + type: 'alternatives.match', context: [Object] } ] diff --git a/test/batch/rules.ts b/test/batch/rules.ts index 215e269..2cba1fb 100644 --- a/test/batch/rules.ts +++ b/test/batch/rules.ts @@ -25,7 +25,7 @@ import { maxGithubApiTeamMembersPerPage, } from "src/constants" import { runChecks } from "src/core" -import { BasicRule } from "src/types" +import { Rule } from "src/types" describe("Rules", function () { let logger: Logger @@ -56,7 +56,7 @@ describe("Rules", function () { users?: string[] diff?: string teams?: { name: string; members: string[] }[] - rules?: BasicRule[] + rules?: Rule[] changedFiles?: string[] } = {}) { users ??= coworkers @@ -759,6 +759,53 @@ describe("Rules", function () { expect(logHistory).toMatchSnapshot() }) } + + it(`${scenario} for AndDistinctRule when user belongs to multiple teams`, async function () { + setup({ + rules: [ + { + name: condition, + condition: condition, + check_type: "diff", + all_distinct: [ + { min_approvals: 1, teams: [team] }, + { min_approvals: 1, teams: [team2] }, + ], + }, + ], + teams: [ + { name: team, members: [coworkers[0]] }, + { name: team2, members: [coworkers[0], coworkers[1]] }, + ], + }) + + switch (scenario) { + case "Has no approval": + case "Is missing approval": { + nock(githubApi) + .post(requestedReviewersApiPath, function (body) { + expect(body).toMatchObject({ + reviewers: [], + team_reviewers: + scenario === "Has no approval" ? [team, team2] : [team2], + }) + return true + }) + .reply(201) + break + } + } + + expect( + await runChecks(basePR, octokit, logger, { + locksReviewTeam: team, + teamLeadsTeam: team2, + actionReviewTeam: team3, + }), + ).toBe(scenario === "Approved" ? "success" : "failure") + + expect(logHistory).toMatchSnapshot() + }) } afterEach(function () { diff --git a/test/batch/rules.ts.snap b/test/batch/rules.ts.snap index 9884bd5..3ec979f 100644 --- a/test/batch/rules.ts.snap +++ b/test/batch/rules.ts.snap @@ -1,5 +1,16 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`Rules Approved for AndDistinctRule when user belongs to multiple teams 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "Matched expression \\"condition\\" of rule \\"condition\\" on diff", + "latestReviews [Map Iterator] { + { id: 0, user: 'userCoworker', isApproval: true }, + { id: 1, user: 'userCoworker2', isApproval: true } +}", +] +`; + exports[`Rules Approved on rule including both teams and users for changed_files 1`] = ` Array [ "Changed files Set(1) { 'condition' }", @@ -198,6 +209,21 @@ Array [ ] `; +exports[`Rules Has no approval for AndDistinctRule when user belongs to multiple teams 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "Matched expression \\"condition\\" of rule \\"condition\\" on diff", + "latestReviews [Map Iterator] { }", + "usersToAskForReview Map(2) { + 'userCoworker' => { teams: Set(2) { 'team', 'team2' } }, + 'userCoworker2' => { teams: Set(1) { 'team2' } } +}", + "ERROR: The following problems were found:", + "Rule \\"condition\\" needs in total 2 DISTINCT approvals, meaning users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subconditions at index 0 and at index 1 failed. The following users have not approved yet: userCoworker (teams: team,team2), userCoworker2 (team: team2).", + "", +] +`; + exports[`Rules Has no approval on rule including both teams and users for changed_files 1`] = ` Array [ "Changed files Set(1) { 'condition' }", @@ -342,8 +368,7 @@ Array [ 'userCoworker2' => { teams: Set(1) { 'team2' } } }", "ERROR: The following problems were found:", - "Rule \\"LOCKS TOUCHED (team: team)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker (team: team).", - "Rule \\"LOCKS TOUCHED (team: team2)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker2 (team: team2).", + "Rule \\"Locks touched\\" needs in total 2 DISTINCT approvals, meaning users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subconditions \\"Locks Reviewers Approvals (team team)\\" and \\"Team Leads Approvals (team team2)\\" failed. The following users have not approved yet: userCoworker (team: team), userCoworker2 (team: team2).", "", ] `; @@ -358,8 +383,7 @@ Array [ 'userCoworker2' => { teams: Set(1) { 'team2' } } }", "ERROR: The following problems were found:", - "Rule \\"LOCKS TOUCHED (team: team)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker (team: team).", - "Rule \\"LOCKS TOUCHED (team: team2)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker2 (team: team2).", + "Rule \\"Locks touched\\" needs in total 2 DISTINCT approvals, meaning users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subconditions \\"Locks Reviewers Approvals (team team)\\" and \\"Team Leads Approvals (team team2)\\" failed. The following users have not approved yet: userCoworker (team: team), userCoworker2 (team: team2).", "", ] `; @@ -374,8 +398,7 @@ Array [ 'userCoworker2' => { teams: Set(1) { 'team2' } } }", "ERROR: The following problems were found:", - "Rule \\"LOCKS TOUCHED (team: team)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker (team: team).", - "Rule \\"LOCKS TOUCHED (team: team2)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker2 (team: team2).", + "Rule \\"Locks touched\\" needs in total 2 DISTINCT approvals, meaning users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subconditions \\"Locks Reviewers Approvals (team team)\\" and \\"Team Leads Approvals (team team2)\\" failed. The following users have not approved yet: userCoworker (team: team), userCoworker2 (team: team2).", "", ] `; @@ -390,8 +413,7 @@ Array [ 'userCoworker2' => { teams: Set(1) { 'team2' } } }", "ERROR: The following problems were found:", - "Rule \\"LOCKS TOUCHED (team: team)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker (team: team).", - "Rule \\"LOCKS TOUCHED (team: team2)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker2 (team: team2).", + "Rule \\"Locks touched\\" needs in total 2 DISTINCT approvals, meaning users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subconditions \\"Locks Reviewers Approvals (team team)\\" and \\"Team Leads Approvals (team team2)\\" failed. The following users have not approved yet: userCoworker (team: team), userCoworker2 (team: team2).", "", ] `; @@ -442,6 +464,18 @@ Array [ ] `; +exports[`Rules Is missing approval for AndDistinctRule when user belongs to multiple teams 1`] = ` +Array [ + "Changed files Set(1) { 'condition' }", + "Matched expression \\"condition\\" of rule \\"condition\\" on diff", + "latestReviews [Map Iterator] { { id: 1, user: 'userCoworker', isApproval: true } }", + "usersToAskForReview Map(1) { 'userCoworker2' => { teams: Set(1) { 'team2' } } }", + "ERROR: The following problems were found:", + "Rule \\"condition\\" needs in total 2 DISTINCT approvals, meaning users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subcondition at index 1 failed. The following users have not approved yet: userCoworker2 (team: team2).", + "", +] +`; + exports[`Rules Is missing approval on rule including both teams and users for changed_files 1`] = ` Array [ "Changed files Set(1) { 'condition' }", @@ -571,7 +605,7 @@ Array [ "latestReviews [Map Iterator] { { id: 1, user: 'userCoworker', isApproval: true } }", "usersToAskForReview Map(1) { 'userCoworker2' => { teams: Set(1) { 'team2' } } }", "ERROR: The following problems were found:", - "Rule \\"LOCKS TOUCHED (team: team2)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker2 (team: team2).", + "Rule \\"Locks touched\\" needs in total 2 DISTINCT approvals, meaning users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subcondition \\"Team Leads Approvals (team team2)\\" failed. The following users have not approved yet: userCoworker2 (team: team2).", "", ] `; @@ -583,7 +617,7 @@ Array [ "latestReviews [Map Iterator] { { id: 1, user: 'userCoworker', isApproval: true } }", "usersToAskForReview Map(1) { 'userCoworker2' => { teams: Set(1) { 'team2' } } }", "ERROR: The following problems were found:", - "Rule \\"LOCKS TOUCHED (team: team2)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker2 (team: team2).", + "Rule \\"Locks touched\\" needs in total 2 DISTINCT approvals, meaning users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subcondition \\"Team Leads Approvals (team team2)\\" failed. The following users have not approved yet: userCoworker2 (team: team2).", "", ] `; @@ -595,7 +629,7 @@ Array [ "latestReviews [Map Iterator] { { id: 1, user: 'userCoworker', isApproval: true } }", "usersToAskForReview Map(1) { 'userCoworker2' => { teams: Set(1) { 'team2' } } }", "ERROR: The following problems were found:", - "Rule \\"LOCKS TOUCHED (team: team2)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker2 (team: team2).", + "Rule \\"Locks touched\\" needs in total 2 DISTINCT approvals, meaning users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subcondition \\"Team Leads Approvals (team team2)\\" failed. The following users have not approved yet: userCoworker2 (team: team2).", "", ] `; @@ -607,7 +641,7 @@ Array [ "latestReviews [Map Iterator] { { id: 1, user: 'userCoworker', isApproval: true } }", "usersToAskForReview Map(1) { 'userCoworker2' => { teams: Set(1) { 'team2' } } }", "ERROR: The following problems were found:", - "Rule \\"LOCKS TOUCHED (team: team2)\\" needs at least 1 approvals, but 0 were matched. The following users have not approved yet: userCoworker2 (team: team2).", + "Rule \\"Locks touched\\" needs in total 2 DISTINCT approvals, meaning users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subcondition \\"Team Leads Approvals (team team2)\\" failed. The following users have not approved yet: userCoworker2 (team: team2).", "", ] `; diff --git a/test/constants.ts b/test/constants.ts index d0168b7..00235a4 100644 --- a/test/constants.ts +++ b/test/constants.ts @@ -1,5 +1,5 @@ import { configFilePath, maxGithubApiFilesPerPage } from "src/constants" -import { AndRule, BasicRule, OrRule, PR } from "src/types" +import { AndDistinctRule, AndRule, BasicRule, OrRule, PR } from "src/types" export const org = "org" export const repo = "repo" @@ -38,6 +38,7 @@ export const rulesExamples: { BasicRule: BasicRule AndRule: AndRule OrRule: OrRule + AndDistinctRule: AndDistinctRule } = { BasicRule: { name: condition, @@ -57,4 +58,10 @@ export const rulesExamples: { check_type: "diff", any: [], }, + AndDistinctRule: { + name: condition, + condition: condition, + check_type: "diff", + all_distinct: [], + }, }