Skip to content

Commit

Permalink
Added more details in the rule output (#105)
Browse files Browse the repository at this point in the history 
Added detailed explanation in the rules output, informing how each rule
works and a list of approvals that counts towards incomplete rules (so
we can see who's review count where).

Resolves #103
  • Loading branch information
@Bullrich
Bullrich committed Nov 29, 2023
1 parent f4e4c61 commit 5c058f0
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 12 deletions.
79 changes: 69 additions & 10 deletions src/runner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@ 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;
export type RuleReport = { name: string; type: RuleTypes } & ReviewReport;

type ReviewState = [true] | [false, ReviewReport];

Expand Down Expand Up @@ -96,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;
Expand All @@ -112,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);
}
Expand All @@ -139,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)}`);
Expand All @@ -152,15 +154,15 @@ 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;
}
case RuleTypes.Fellows: {
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;
}
Expand All @@ -177,21 +179,27 @@ export class ActionRunner {
return { files: modifiedFiles, reports: errorReports };
}

/** WIP - Class that will assign the requests for review */
async requestReviewers(
reports: RuleReport[],
preventReviewRequests: ConfigurationFile["preventReviewRequests"],
): Promise<void> {
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)}`);
Expand Down Expand Up @@ -243,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)
.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
.addHeading("Missing users", 3)
Expand All @@ -263,6 +298,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";
}
Expand All @@ -289,6 +331,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[] =>
Expand All @@ -301,6 +345,7 @@ export class ActionRunner {
missingUsers: filterMissingUsers(requirements),
teamsToRequest: rule.reviewers.flatMap((r) => r.teams ?? []),
usersToRequest: filterMissingUsers(rule.reviewers),
countingReviews,
};
};

Expand All @@ -327,6 +372,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");
Expand Down Expand Up @@ -421,12 +468,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);
}
}

Expand All @@ -444,6 +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,
},
];
} else {
Expand Down Expand Up @@ -472,12 +524,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);
}
}

Expand All @@ -494,6 +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,
},
];
} else {
Expand Down Expand Up @@ -589,7 +646,7 @@ const getRequiredRanks = (reports: Pick<ReviewReport, "missingRank">[]): 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;

Expand All @@ -599,6 +656,8 @@ 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))],
};
};
6 changes: 4 additions & 2 deletions src/test/runner/runner.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<PullRequestApi>;
Expand Down Expand Up @@ -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 () => {
Expand Down

0 comments on commit 5c058f0

Please sign in to comment.