Skip to content

Commit

Permalink
Refactor of failed reviews objects (#112)
Browse files Browse the repository at this point in the history
This creates a new type of object: `ReviewFailure`. This object is an
abstract class that contains all the information of the errors. By
having each kind of error implementing it, we can customize the reports.

This was done because reporting problems in the PR is basically the most
exposed part (and the clearer it is, the less that people will ask for
help).

Each class has it's own summary to generate and returns the reviewers
that should be requested. It allows customization per class. It can
solve a lot of problems in the future.

Two error classes were created: `CommonRuleFailure` and
`FellowMissingRankFailure`

### CommonRuleFailure

Wraps all the errors for all the non fellows rules and list what users
are required (and which reviews counted towards fulfilling the rule).

### FellowMissingRankFailure

Quite similar to the `CommonRuleFailure` error, but also handles the
rank required.

In the future I'll add one for `Fellow Score`, which will be a
requirement for detailing the information in #110.

# Other changes

- Renamed structs to contain more clear names.
- Fixed summary not being written for action when check was already
existing
- Simplified `generateCheckRunData` 
- now it is only a couple of lines and the `if` checks are done inside
the errors
- Changed `evaluateCondition` and `andDistinctEvaluation` return type.
- Before they were returning an `[true] | [false, ErrorStruct]` tuple.
After thinking about it, it is simpler to return a `ErrorStruct | null`
type and simply checking if it's null.
- If I would be returning a different object with the `true` condition I
could have keep it, but for this case it was over engineered.
- Changed returned type for `fellowsEvaluation`
    - It now returns the error object directly when applicable.
  • Loading branch information
Bullrich authored Jan 25, 2024
1 parent 5f58f48 commit 5371807
Show file tree
Hide file tree
Showing 15 changed files with 330 additions and 223 deletions.
2 changes: 1 addition & 1 deletion action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,4 @@ outputs:

runs:
using: 'docker'
image: 'docker://ghcr.io/paritytech/review-bot/action:2.3.1'
image: 'Dockerfile'
48 changes: 48 additions & 0 deletions src/failures/commonRules.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { summary } from "@actions/core";

import { toHandle } from "../util";
import { RequiredReviewersData, ReviewFailure, RuleFailedSummary } from "./types";

export class CommonRuleFailure extends ReviewFailure {
public readonly usersToRequest: string[];
public readonly teamsToRequest: string[];

constructor(report: RuleFailedSummary & RequiredReviewersData) {
super(report);
this.usersToRequest = report.usersToRequest ?? [];
this.teamsToRequest = report.teamsToRequest ?? [];
}

generateSummary(): typeof summary {
let text = super.generateSummary();

if (this.usersToRequest.length > 0) {
text = text.addHeading("Missing users", 3).addList(this.usersToRequest);
}
if (this.teamsToRequest.length > 0) {
text = text.addHeading("Missing reviews from teams", 3).addList(this.teamsToRequest);
}

if (this.countingReviews.length > 0) {
text = text
.addHeading("Users approvals that counted towards this rule", 3)
.addEOL()
.addList(this.countingReviews.map(toHandle))
.addEOL();
}

if (this.missingUsers.length > 0)
text = text.addDetails(
"GitHub users whose approval counts",
`This is a list of all the GitHub users whose approval would count towards fulfilling this rule:\n\n - ${this.missingUsers
.map(toHandle)
.join("\n - ")}`,
);

return text;
}

getRequestLogins(): { users: string[]; teams: string[] } {
return { users: this.usersToRequest, teams: this.teamsToRequest };
}
}
44 changes: 44 additions & 0 deletions src/failures/fellowsRules.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { summary } from "@actions/core";

import { toHandle } from "../util";
import { ReviewFailure, RuleFailedSummary } from "./types";

export class FellowMissingRankFailure extends ReviewFailure {
constructor(
ruleInfo: RuleFailedSummary,
public readonly missingRank: number,
) {
super(ruleInfo);
}

generateSummary(): typeof summary {
let text = super.generateSummary();

text = text
.addHeading("Missing reviews from Fellows", 3)
.addEOL()
.addRaw(`Missing reviews from rank \`${this.missingRank}\` or above`)
.addEOL();
if (this.missingUsers.length > 0)
text = text.addDetails(
"GitHub users whose approval counts",
`This is a list of all the GitHub users who are rank ${this.missingRank} or above:\n\n - ${this.missingUsers
.map(toHandle)
.join("\n - ")}`,
);

if (this.countingReviews.length > 0) {
text = text
.addHeading("Users approvals that counted towards this rule", 3)
.addEOL()
.addList(this.countingReviews.map(toHandle))
.addEOL();
}

return text;
}

getRequestLogins(): { users: string[]; teams: string[] } {
return { users: [], teams: [] };
}
}
3 changes: 3 additions & 0 deletions src/failures/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export * from "./types";
export * from "./commonRules";
export * from "./fellowsRules";
86 changes: 86 additions & 0 deletions src/failures/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { summary } from "@actions/core";

import { RuleTypes } from "../rules/types";

/** Object containing the report on why a rule failed */
export type RuleFailedReport = {
/** The amount of missing reviews to fulfill the requirements */
missingReviews: number;
/** The users who would qualify to complete those reviews */
missingUsers: string[];
/** If applicable, reviews that count towards this rule */
countingReviews: string[];
};

export type RuleFailedSummary = {
type: RuleTypes;
name: string;
} & RuleFailedReport;

export type RequiredReviewersData = {
/** If applicable, the teams that should be requested to review */
teamsToRequest?: string[];
/** If applicable, the users that should be requested to review */
usersToRequest?: string[];
};

/** Class which contains the reports of a failed rule
* Here you can find details on why a rule failed and what requirements it has
*/
export abstract class ReviewFailure {
public readonly name: string;
public readonly type: RuleTypes;
/** The amount of missing reviews */
public readonly missingReviews: number;

/** Approvals that counted towards this rule */
public readonly countingReviews: string[];

/** List of users who would classify to approve this rule */
public readonly missingUsers: string[];

constructor(ruleInfo: RuleFailedSummary) {
this.name = ruleInfo.name;
this.type = ruleInfo.type;
this.missingReviews = ruleInfo.missingReviews;
this.countingReviews = ruleInfo.countingReviews;
this.missingUsers = ruleInfo.missingUsers;
}

ruleExplanation(type: RuleTypes): string {
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);
throw new Error("Unhandled rule");
}
}

generateSummary(): typeof summary {
return summary
.emptyBuffer()
.addHeading(this.name, 2)
.addHeading(`Missing ${this.missingReviews} review${this.missingReviews > 1 ? "s" : ""}`, 4)
.addDetails(
"Rule explanation",
this.ruleExplanation(this.type) +
"\n\n" +
"For more info found out how the rules work in [Review-bot types](https://github.com/paritytech/review-bot#types)",
);
}

/** Get the users/teams whose review should be requested */
abstract getRequestLogins(): { users: string[]; teams: string[] };
}
11 changes: 8 additions & 3 deletions src/github/check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export class GitHubChecksApi {
this.logger.debug(`Found match: ${JSON.stringify(check)}`);
await this.api.rest.checks.update({ ...checkData, check_run_id: check.id });
this.logger.debug("Updated check data");
await this.writeSummary(checkData, check.html_url ?? "");
return;
}
}
Expand All @@ -57,16 +58,20 @@ export class GitHubChecksApi {

const check = await this.api.rest.checks.create(checkData);

await this.writeSummary(checkData, check.data.html_url ?? "");

this.logger.debug(JSON.stringify(check.data));
}

private async writeSummary(checkResult: CheckData, resultUrl: string) {
// We publish it in the action summary
await summary
.emptyBuffer()
.addHeading(checkResult.output.title)
// We redirect to the check as it can changed if it is triggered again
.addLink("Find the result here", check.data.html_url ?? "")
.addLink("Find the result here", resultUrl)
.addBreak()
.addRaw(checkResult.output.text)
.write();

this.logger.debug(JSON.stringify(check.data));
}
}
Loading

0 comments on commit 5371807

Please sign in to comment.