From 0c64ff261f54c972eb2ef8f2f65945fa3ad7d63c Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 29 Nov 2023 13:23:07 +0100 Subject: [PATCH 1/4] added a list of approvals This way we can know what approvals counted towards a rule --- src/runner.ts | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index 06c1b90..fd7a17e 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -20,6 +20,8 @@ type ReviewReport = { usersToRequest?: string[]; /** If applicable, the missing minimum fellows rank required to review */ missingRank?: number; + /** If applicable, reviews that count towards this rule */ + countingReviews: string[] }; export type RuleReport = { name: string } & ReviewReport; @@ -41,7 +43,7 @@ export class ActionRunner { private readonly polkadotApi: TeamApi, private readonly checks: GitHubChecksApi, private readonly logger: ActionLogger, - ) {} + ) { } /** * Fetches the configuration file, parses it and validates it. @@ -177,7 +179,6 @@ export class ActionRunner { return { files: modifiedFiles, reports: errorReports }; } - /** WIP - Class that will assign the requests for review */ async requestReviewers( reports: RuleReport[], preventReviewRequests: ConfigurationFile["preventReviewRequests"], @@ -185,13 +186,14 @@ export class ActionRunner { if (reports.length === 0) { return; } - const finalReport: ReviewReport = { missingReviews: 0, missingUsers: [], teamsToRequest: [], usersToRequest: [] }; + const finalReport: ReviewReport = { missingReviews: 0, missingUsers: [], teamsToRequest: [], usersToRequest: [], countingReviews: [] }; for (const report of reports) { finalReport.missingReviews += report.missingReviews; finalReport.missingUsers = concatArraysUniquely(finalReport.missingUsers, report.missingUsers); finalReport.teamsToRequest = concatArraysUniquely(finalReport.teamsToRequest, report.teamsToRequest); finalReport.usersToRequest = concatArraysUniquely(finalReport.usersToRequest, report.usersToRequest); + finalReport.countingReviews = concatArraysUniquely(finalReport.countingReviews, report.countingReviews); } this.logger.debug(`Request data: ${JSON.stringify(finalReport)}`); @@ -263,6 +265,13 @@ export class ActionRunner { .addRaw(`Missing reviews from rank \`${report.missingRank}\` or above`) .addEOL(); } + if (report.countingReviews.length > 0) { + text = text + .addHeading("Users approvals that counted towards this rule", 3) + .addEOL() + .addList(report.countingReviews) + .addEOL(); + } check.output.text += text.stringify() + "\n"; } @@ -289,6 +298,8 @@ export class ActionRunner { // We get the list of users that approved the PR const approvals = await this.prApi.listApprovedReviewsAuthors(rule.countAuthor ?? false); + let countingReviews: string[] = []; + // Utility method used to generate error const generateErrorReport = (): ReviewReport => { const filterMissingUsers = (reviewData: { users?: string[] }[]): string[] => @@ -301,6 +312,7 @@ export class ActionRunner { missingUsers: filterMissingUsers(requirements), teamsToRequest: rule.reviewers.flatMap((r) => r.teams ?? []), usersToRequest: filterMissingUsers(rule.reviewers), + countingReviews }; }; @@ -327,6 +339,8 @@ export class ActionRunner { } this.logger.debug(`Matching approvals: ${JSON.stringify(conditionApprovals)}`); + countingReviews = [...new Set(conditionApprovals.flatMap(({ matchingUsers }) => matchingUsers))]; + // If one of the rules doesn't have the required approval we fail the evaluation if (conditionApprovals.some((cond) => cond.matchingUsers.length === 0)) { this.logger.warn("One of the groups does not have any approvals"); @@ -421,12 +435,16 @@ export class ActionRunner { const approvals = await this.prApi.listApprovedReviewsAuthors(countAuthor ?? false); this.logger.info(`Found ${approvals.length} approvals.`); + // List of user reviews which fulfill this rule + const countingReviews: string[] = []; + // This is the amount of reviews required. To succeed this should be 0 or lower let missingReviews = rule.minApprovals; for (const requiredUser of requiredUsers) { // We check for the approvals, if it is a required reviewer we lower the amount of missing reviews if (approvals.indexOf(requiredUser) > -1) { missingReviews--; + countingReviews.push(requiredUser); } } @@ -444,6 +462,7 @@ export class ActionRunner { missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author), teamsToRequest: rule.teams ? rule.teams : undefined, usersToRequest: rule.users ? rule.users.filter((u) => approvals.indexOf(u)) : undefined, + countingReviews }, ]; } else { @@ -472,12 +491,16 @@ export class ActionRunner { const approvals = await this.prApi.listApprovedReviewsAuthors(rule.countAuthor ?? false); this.logger.info(`Found ${approvals.length} approvals.`); + // List of user reviews which fulfill this rule + const countingReviews: string[] = []; + // This is the amount of reviews required. To succeed this should be 0 or lower let missingReviews = rule.minApprovals; for (const requiredUser of requiredUsers) { // We check for the approvals, if it is a required reviewer we lower the amount of missing reviews if (approvals.indexOf(requiredUser) > -1) { missingReviews--; + countingReviews.push(requiredUser); } } @@ -494,6 +517,7 @@ export class ActionRunner { // Remove all the users who approved the PR + the author (if he belongs to the group) missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author), missingRank: rule.minRank, + countingReviews }, ]; } else { @@ -600,5 +624,6 @@ const unifyReport = (reports: ReviewReport[], name: string): RuleReport => { usersToRequest: [...new Set(reports.flatMap((r) => r.usersToRequest ?? []))], name, missingRank, + countingReviews: [...new Set(reports.flatMap((r) => r.countingReviews))], }; }; From 6c53b5fc60ec5b56a1003386f7e047613bfb5ff8 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 29 Nov 2023 14:40:37 +0100 Subject: [PATCH 2/4] added explanation on how rules work --- src/runner.ts | 62 +++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 48 insertions(+), 14 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index fd7a17e..ee00551 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -21,10 +21,10 @@ type ReviewReport = { /** If applicable, the missing minimum fellows rank required to review */ missingRank?: number; /** If applicable, reviews that count towards this rule */ - countingReviews: string[] + countingReviews: string[]; }; -export type RuleReport = { name: string } & ReviewReport; +export type RuleReport = { name: string; type: RuleTypes } & ReviewReport; type ReviewState = [true] | [false, ReviewReport]; @@ -43,7 +43,7 @@ export class ActionRunner { private readonly polkadotApi: TeamApi, private readonly checks: GitHubChecksApi, private readonly logger: ActionLogger, - ) { } + ) {} /** * Fetches the configuration file, parses it and validates it. @@ -98,7 +98,7 @@ export class ActionRunner { const [result, missingData] = await this.evaluateCondition(rule, rule.countAuthor); if (!result) { this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); - errorReports.push({ ...missingData, name: rule.name }); + errorReports.push({ ...missingData, name: rule.name, type: rule.type }); } break; @@ -114,7 +114,7 @@ export class ActionRunner { } } if (reports.length > 0) { - const finalReport = unifyReport(reports, rule.name); + const finalReport = unifyReport(reports, rule.name, rule.type); this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`); errorReports.push(finalReport); } @@ -141,7 +141,7 @@ export class ActionRunner { .reduce((a, b) => (a < b ? a : b), 999); // We get the lowest rank required // We unify the reports - const finalReport = unifyReport(reports, rule.name); + const finalReport = unifyReport(reports, rule.name, rule.type); // We set the value to the minimum neccesary finalReport.missingReviews = lowerAmountOfReviewsNeeded; this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`); @@ -154,7 +154,7 @@ export class ActionRunner { const [result, missingData] = await this.andDistinctEvaluation(rule); if (!result) { this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); - errorReports.push({ ...missingData, name: rule.name }); + errorReports.push({ ...missingData, name: rule.name, type: rule.type }); } break; } @@ -162,7 +162,7 @@ export class ActionRunner { const [result, missingData] = await this.fellowsEvaluation(rule); if (!result) { this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); - errorReports.push({ ...missingData, name: rule.name }); + errorReports.push({ ...missingData, name: rule.name, type: rule.type }); } break; } @@ -186,7 +186,13 @@ export class ActionRunner { if (reports.length === 0) { return; } - const finalReport: ReviewReport = { missingReviews: 0, missingUsers: [], teamsToRequest: [], usersToRequest: [], countingReviews: [] }; + const finalReport: ReviewReport = { + missingReviews: 0, + missingUsers: [], + teamsToRequest: [], + usersToRequest: [], + countingReviews: [], + }; for (const report of reports) { finalReport.missingReviews += report.missingReviews; @@ -245,11 +251,38 @@ export class ActionRunner { } for (const report of reports) { + const ruleExplanation = (type: RuleTypes) => { + switch (type) { + case RuleTypes.Basic: + return "Rule 'Basic' requires a given amount of reviews from users/teams"; + case RuleTypes.And: + return "Rule 'And' has many required reviewers/teams and requires all of them to be fulfilled."; + case RuleTypes.Or: + return "Rule 'Or' has many required reviewers/teams and requires at least one of them to be fulfilled."; + case RuleTypes.AndDistinct: + return ( + "Rule 'And Distinct' has many required reviewers/teams and requires all of them to be fulfilled **by different users**.\n\n" + + "The approval of one user that belongs to _two teams_ will count only towards one team." + ); + case RuleTypes.Fellows: + return "Rule 'Fellows' requires a given amount of reviews from users whose Fellowship ranking is the required rank or great."; + default: + console.error("Out of range for rule type", type); + return "Unhandled rule"; + } + }; + check.output.summary += `- **${report.name}**\n`; let text = summary .emptyBuffer() .addHeading(report.name, 2) - .addHeading(`Missing ${report.missingReviews} review${report.missingReviews > 1 ? "s" : ""}`, 4); + .addHeading(`Missing ${report.missingReviews} review${report.missingReviews > 1 ? "s" : ""}`, 4) + .addHeading("Rule explanation", 5) + .addRaw(ruleExplanation(report.type)) + .addEOL() + .addRaw( + "For more info found out how the rules work in [Review-bot types](/~https://github.com/paritytech/review-bot#types)", + ); if (report.usersToRequest && report.usersToRequest.length > 0) { text = text .addHeading("Missing users", 3) @@ -312,7 +345,7 @@ export class ActionRunner { missingUsers: filterMissingUsers(requirements), teamsToRequest: rule.reviewers.flatMap((r) => r.teams ?? []), usersToRequest: filterMissingUsers(rule.reviewers), - countingReviews + countingReviews, }; }; @@ -462,7 +495,7 @@ export class ActionRunner { missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author), teamsToRequest: rule.teams ? rule.teams : undefined, usersToRequest: rule.users ? rule.users.filter((u) => approvals.indexOf(u)) : undefined, - countingReviews + countingReviews, }, ]; } else { @@ -517,7 +550,7 @@ export class ActionRunner { // Remove all the users who approved the PR + the author (if he belongs to the group) missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author), missingRank: rule.minRank, - countingReviews + countingReviews, }, ]; } else { @@ -613,7 +646,7 @@ const getRequiredRanks = (reports: Pick[]): number[ } }; -const unifyReport = (reports: ReviewReport[], name: string): RuleReport => { +const unifyReport = (reports: ReviewReport[], name: string, type: RuleTypes): RuleReport => { const ranks = getRequiredRanks(reports); const missingRank = ranks ? Math.max(...(ranks as number[])) : undefined; @@ -623,6 +656,7 @@ const unifyReport = (reports: ReviewReport[], name: string): RuleReport => { teamsToRequest: [...new Set(reports.flatMap((r) => r.teamsToRequest ?? []))], usersToRequest: [...new Set(reports.flatMap((r) => r.usersToRequest ?? []))], name, + type, missingRank, countingReviews: [...new Set(reports.flatMap((r) => r.countingReviews))], }; From 61c42a8f9b9fa8eb2b270e7f42b8f190bdf996be Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 29 Nov 2023 14:52:40 +0100 Subject: [PATCH 3/4] fixed tests --- src/test/runner/runner.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index 66dc029..8dfd5e7 100644 --- a/src/test/runner/runner.test.ts +++ b/src/test/runner/runner.test.ts @@ -5,7 +5,7 @@ import { GitHubChecksApi } from "../../github/check"; import { PullRequestApi } from "../../github/pullRequest"; import { ActionLogger, TeamApi } from "../../github/types"; import { ConfigurationFile, Rule, RuleTypes } from "../../rules/types"; -import { ActionRunner } from "../../runner"; +import { ActionRunner, RuleReport } from "../../runner"; describe("Shared validations", () => { let api: MockProxy; @@ -94,12 +94,14 @@ describe("Shared validations", () => { }); describe("Validation in requestReviewers", () => { - const exampleReport = { + const exampleReport: RuleReport = { name: "Example", missingUsers: ["user-1", "user-2", "user-3"], missingReviews: 2, teamsToRequest: ["team-1"], usersToRequest: ["user-1"], + type: RuleTypes.Basic, + countingReviews: [], }; test("should request reviewers if object is not defined", async () => { From 9348348aaab56080ca5c8e8b35e10e15197f0b1f Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 29 Nov 2023 14:53:02 +0100 Subject: [PATCH 4/4] converted rule explanation into a detail This way it is better shown for people who care about the rule logic --- src/runner.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index ee00551..9dd0c9d 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -277,11 +277,11 @@ export class ActionRunner { .emptyBuffer() .addHeading(report.name, 2) .addHeading(`Missing ${report.missingReviews} review${report.missingReviews > 1 ? "s" : ""}`, 4) - .addHeading("Rule explanation", 5) - .addRaw(ruleExplanation(report.type)) - .addEOL() - .addRaw( - "For more info found out how the rules work in [Review-bot types](/~https://github.com/paritytech/review-bot#types)", + .addDetails( + "Rule explanation", + `${ruleExplanation( + report.type, + )}\n\nFor more info found out how the rules work in [Review-bot types](/~https://github.com/paritytech/review-bot#types)`, ); if (report.usersToRequest && report.usersToRequest.length > 0) { text = text