Added required score grouping to fellows reviews (#113)

Added new optional field to the config named `FellowScores` that has the
score of each fellow per dan. It was done as an object (and not a
collection) because it will never change in the foreseeable future
(added up to dan IX).

When the rule of type `fellows` has the optional field of
`minTotalScore` it will check that the score of the fellows who approved
the PR sums up to that number. If it doesn't sum to that number it will
fail with a custom error listing the score of each fellow for reference
(and saying how many points are missing).

Added `FellowMissingScoreFailure` class which handles the formatting of
errors when the score of fellows is not high enough.

This resolves #110

Downgraded JOI as there was a version mismatch that made the test fails.

This also updated the version to `2.4.0`

Author: Bullrich

README.md

@@ -411,21 +411,37 @@ Meaning that if a user belongs to both `team-abc` and `team-example` their appro
 
 This rule is useful when you need to make sure that at leasts two sets of eyes of two different teams review a Pull Request.
 #### Fellows rule
-The fellows rule has a slight difference to all of the rules:
+The fellows rule requires an extra collection to be added to the configuration and distinguishes itself from the other rules with some new fields:
 ```yaml
-- name: Fellows review
-  condition:
-    include:
-      - '.*'
-    exclude:
-      - 'example'
-  type: fellows
-  minRank: 2
-  minApprovals: 2
+rules:
+  - name: Fellows review
+    condition:
+      include:
+        - '.*'
+      exclude:
+        - 'example'
+    type: fellows
+    minRank: 2
+    minApprovals: 2
+    minTotalScore: 4
+scores:
+  dan1: 1
+  dan2: 2
+  dan3: 4
+  dan4: 8
+  dan5: 12
+  dan6: 15
+  dan7: 20
+  dan8: 25
+  dan9: 30
 ```
-The biggest difference is that it doesn’t have a reviewers type (it doesn’t have a `teams` or `users` field); instead, it has a `minRank` field.
+The biggest difference in the rule configuration is that it doesn’t have a reviewers type (it doesn’t have a `teams` or `users` field); instead, it has a `minRank` field and `minTotalScore` field.
+
+The `minRank` field receives a number, which will be the lowest rank required to evaluate the PR, and then it fetches [all the fellows from the chain data](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/fellowship), filters only the one to belong to that rank or above and then [looks into their metadata for a field name `github` and the handle there](https://github.com/polkadot-fellows/runtimes/issues/7).
+
+The `minTotalScore` field relies on the `scores` collection. In this collection, each rank (dan) receives a score, so, in the example, a fellow dan 3 will have a score of 4. If the field `minTotalScore` is enabled, the conditions to approve a pull request, aside from requiring a given amount of reviews from a given rank, will be that _the total sum of all the scores from the fellows that have approved the pull request are equal or greater to the `minTotalScore` field_. This field is optional.
 
-This field receives a number, which will be the lowest rank required to evaluate the PR, and then it fetches [all the fellows from the chain data](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/fellowship), filters only the one to belong to that rank or above and then [looks into their metadata for a field name `github` and the handle there](https://github.com/polkadot-fellows/runtimes/issues/7).
+For *every dan that has not been attributed a score, their score will default to 0*.
 
 After this is done, the resulting handles will be treated like a normal list of required users.
 
@@ -439,6 +455,8 @@ It also has any other field from the [`basic rule`](#basic-rule) (with the excep
 	- **Optional**: Defaults to `false`.
 - **minRank**: Must be a number.
 	- **Required**
+- **minTotalScore**: Must be a number
+	- **Optional**: Defaults to 0.
 
 ##### Note
 The fellows rule will never request reviewers, even if `request-reviewers` rule is enabled. 

action.yml

@@ -32,4 +32,4 @@ outputs:
 
 runs:
   using: 'docker'
-  image: 'Dockerfile'
+  image: 'docker://ghcr.io/paritytech/review-bot/action:2.4.0'

package.json

@@ -1,6 +1,6 @@
 {
   "name": "review-bot",
-  "version": "2.3.1",
+  "version": "2.4.0",
   "description": "Have custom review rules for PRs with auto assignment",
   "main": "src/index.ts",
   "scripts": {
@@ -39,7 +39,7 @@
     "@actions/github": "^6.0.0",
     "@eng-automation/js": "^1.0.2",
     "@polkadot/api": "^10.11.2",
-    "joi": "^17.12.0",
+    "joi": "^17.6.4",
     "yaml": "^2.3.4"
   }
 }

src/failures/fellowsRules.ts

@@ -42,3 +42,64 @@ export class FellowMissingRankFailure extends ReviewFailure {
     return { users: [], teams: [] };
   }
 }
+
+export class FellowMissingScoreFailure extends ReviewFailure {
+  public readonly currentScore: number;
+  constructor(
+    ruleInfo: Omit<RuleFailedSummary, "missingUsers" | "countingReviews" | "missingReviews">,
+    public readonly requiredScore: number,
+    approvalsWithScores: [string, number][],
+    missingFellowsWithScore: [string, number][],
+  ) {
+    const unifyFellowWithScore = ([handle, score]: [string, number]) => `${handle} -> <b>${score}</b>`;
+    super({
+      ...ruleInfo,
+      countingReviews: approvalsWithScores.map(unifyFellowWithScore),
+      missingUsers: missingFellowsWithScore.map(unifyFellowWithScore),
+      missingReviews: 1,
+    });
+
+    this.currentScore = approvalsWithScores.reduce((n, [_, score]) => n + score, 0);
+  }
+
+  generateSummary(): typeof summary {
+    let text = summary
+      .emptyBuffer()
+      .addHeading(this.name, 2)
+      .addHeading("Missing minimum required score from Fellows", 4)
+      .addDetails(
+        "Rule explanation",
+        "Rule 'Fellows' gives every fellow a score based on their rank, and required that the sum of all the scores is greater than the required score." +
+          "\n\n" +
+          "For more info found out how the rules work in [Review-bot types](https://github.com/paritytech/review-bot#types)",
+      );
+
+    text = text
+      .addHeading(`Missing a score of ${this.requiredScore}`, 3)
+      .addEOL()
+      .addRaw(`Missing ${this.requiredScore - this.currentScore} in the required score.`)
+      .addEOL()
+      .addRaw(`Current score is ${this.currentScore}/${this.requiredScore}`);
+    if (this.missingUsers.length > 0)
+      text = text.addDetails(
+        "GitHub users whose approval counts",
+        `This is a list of all the Fellows that have not reviewed with their current scores:\n\n - ${this.missingUsers
+          .map(toHandle)
+          .join("\n - ")}`,
+      );
+
+    if (this.countingReviews.length > 0) {
+      text = text
+        .addHeading("Users approvals that counted towards this rule with their scores", 3)
+        .addEOL()
+        .addList(this.countingReviews.map(toHandle))
+        .addEOL();
+    }
+
+    return text;
+  }
+
+  getRequestLogins(): { users: string[]; teams: string[] } {
+    return { users: [], teams: [] };
+  }
+}

src/polkadot/fellows.ts

@@ -10,7 +10,7 @@ export class PolkadotFellows implements TeamApi {
 
   constructor(private readonly logger: ActionLogger) {}
 
-  async fetchAllFellows(): Promise<Map<string, number>> {
+  private async fetchAllFellows(): Promise<Map<string, number>> {
     let api: ApiPromise;
     this.logger.debug("Connecting to collective parachain");
     // we connect to the collective rpc node
@@ -91,6 +91,18 @@ export class PolkadotFellows implements TeamApi {
     }
   }
 
+  /** Returns all the fellows with their rankings */
+  async listFellows(): Promise<[string, number][]> {
+    this.logger.info("Fetching all fellows with their ranks");
+
+    if (this.fellowsCache.size < 1) {
+      this.logger.debug("Cache not found. Fetching fellows.");
+      this.fellowsCache = await this.fetchAllFellows();
+    }
+
+    return Array.from(this.fellowsCache.entries());
+  }
+
   async getTeamMembers(ranking: string): Promise<string[]> {
     const requiredRank = Number(ranking);
     this.logger.info(`Fetching members of rank '${requiredRank}' or higher`);

src/rules/types.ts

@@ -38,6 +38,19 @@ export interface FellowsRule extends Rule {
   type: RuleTypes.Fellows;
   minRank: number;
   minApprovals: number;
+  minTotalScore?: number;
+}
+
+export interface FellowsScore {
+  dan1: number;
+  dan2: number;
+  dan3: number;
+  dan4: number;
+  dan5: number;
+  dan6: number;
+  dan7: number;
+  dan8: number;
+  dan9: number;
 }
 
 export interface ConfigurationFile {
@@ -49,4 +62,5 @@ export interface ConfigurationFile {
     teams?: string[];
     users?: string[];
   };
+  score?: FellowsScore;
 }

src/rules/validator.ts

@@ -2,7 +2,7 @@ import { validate } from "@eng-automation/js";
 import Joi from "joi";
 
 import { ActionLogger } from "../github/types";
-import { AndRule, BasicRule, ConfigurationFile, FellowsRule, Reviewers, Rule, RuleTypes } from "./types";
+import { AndRule, BasicRule, ConfigurationFile, FellowsRule, FellowsScore, Reviewers, Rule, RuleTypes } from "./types";
 
 /** For the users or team schema. Will be recycled A LOT
  * Remember to add `.or("users", "teams")` to force at least one of the two to be defined
@@ -30,13 +30,27 @@ const ruleSchema = Joi.object<Rule & { type: string }>().keys({
     .required(),
 });
 
+/** Schema for ensuring that all the dan ranks are set properly */
+export const fellowScoreSchema = Joi.object<FellowsScore>().keys({
+  dan1: Joi.number().default(0),
+  dan2: Joi.number().default(0),
+  dan3: Joi.number().default(0),
+  dan4: Joi.number().default(0),
+  dan5: Joi.number().default(0),
+  dan6: Joi.number().default(0),
+  dan7: Joi.number().default(0),
+  dan8: Joi.number().default(0),
+  dan9: Joi.number().default(0),
+});
+
 /** General Configuration schema.
  * Evaluates all the upper level field plus the generic rules fields.
  * Remember to evaluate the rules with their custom rules
  */
 export const generalSchema = Joi.object<ConfigurationFile>().keys({
   rules: Joi.array<ConfigurationFile["rules"]>().items(ruleSchema).unique("name").required(),
   preventReviewRequests: Joi.object().keys(reviewersObj).optional().or("users", "teams"),
+  score: fellowScoreSchema,
 });
 
 /** Basic rule schema
@@ -59,6 +73,7 @@ export const fellowsRuleSchema = Joi.object<FellowsRule>().keys({
   countAuthor: Joi.boolean().default(false),
   minRank: Joi.number().required().min(1).empty(null),
   minApprovals: Joi.number().min(1).default(1),
+  minTotalScore: Joi.number().min(1).optional(),
 });
 
 /**

src/runner.ts

@@ -5,6 +5,7 @@ import { Inputs } from ".";
 import {
   CommonRuleFailure,
   FellowMissingRankFailure,
+  FellowMissingScoreFailure,
   RequiredReviewersData,
   ReviewFailure,
   RuleFailedReport,
@@ -13,9 +14,18 @@ import {
 import { GitHubChecksApi } from "./github/check";
 import { PullRequestApi } from "./github/pullRequest";
 import { ActionLogger, CheckData, TeamApi } from "./github/types";
-import { AndDistinctRule, ConfigurationFile, FellowsRule, Reviewers, Rule, RuleTypes } from "./rules/types";
+import { PolkadotFellows } from "./polkadot/fellows";
+import {
+  AndDistinctRule,
+  ConfigurationFile,
+  FellowsRule,
+  FellowsScore,
+  Reviewers,
+  Rule,
+  RuleTypes,
+} from "./rules/types";
 import { validateConfig, validateRegularExpressions } from "./rules/validator";
-import { concatArraysUniquely } from "./util";
+import { concatArraysUniquely, rankToScore } from "./util";
 
 type BaseRuleReport = RuleFailedReport & RequiredReviewersData;
 
@@ -31,7 +41,7 @@ export class ActionRunner {
   constructor(
     private readonly prApi: PullRequestApi,
     private readonly teamApi: TeamApi,
-    private readonly polkadotApi: TeamApi,
+    private readonly polkadotApi: PolkadotFellows,
     private readonly checks: GitHubChecksApi,
     private readonly logger: ActionLogger,
   ) {}
@@ -61,7 +71,7 @@ export class ActionRunner {
    * The action evaluates if the rules requirements are meet for a PR
    * @returns an array of error reports for each failed rule. An empty array means no errors
    */
-  async validatePullRequest({ rules }: ConfigurationFile): Promise<PullRequestReport> {
+  async validatePullRequest({ rules, score }: ConfigurationFile): Promise<PullRequestReport> {
     const modifiedFiles = await this.prApi.listModifiedFiles();
 
     const errorReports: ReviewFailure[] = [];
@@ -150,7 +160,7 @@ export class ActionRunner {
             break;
           }
           case RuleTypes.Fellows: {
-            const fellowReviewError = await this.fellowsEvaluation(rule);
+            const fellowReviewError = await this.fellowsEvaluation(rule, score);
             if (fellowReviewError) {
               this.logger.error(`Missing the reviews from ${JSON.stringify(fellowReviewError.missingReviews)}`);
               // errorReports.push({ ...missingData, name: rule.name, type: rule.type });
@@ -440,7 +450,7 @@ export class ActionRunner {
     }
   }
 
-  async fellowsEvaluation(rule: FellowsRule): Promise<ReviewFailure | null> {
+  async fellowsEvaluation(rule: FellowsRule, scores?: FellowsScore): Promise<ReviewFailure | null> {
     // This is a list of all the users that need to approve a PR
     const requiredUsers: string[] = await this.polkadotApi.getTeamMembers(rule.minRank.toString());
 
@@ -472,9 +482,10 @@ export class ActionRunner {
       }
     }
 
+    const author = this.prApi.getAuthor();
+
     // Now we verify if we have any remaining missing review.
     if (missingReviews > 0) {
-      const author = this.prApi.getAuthor();
       this.logger.warn(`${missingReviews} reviews are missing.`);
       // If we have at least one missing review, we return an object with the list of missing reviewers, and
       // which users/teams we should request to review
@@ -487,11 +498,54 @@ export class ActionRunner {
         },
         rule.minRank,
       );
-    } else {
-      this.logger.info("Rule requirements fulfilled");
-      // If we don't have any missing reviews, we return no error
-      return null;
+      // Then we verify if we need to have a minimum score
+    } else if (rule.minTotalScore && scores) {
+      this.logger.debug("Validating required minimum score");
+      // We get all the fellows with their ranks and convert them to their score
+      const fellows: [string, number][] = (await this.polkadotApi.listFellows()).map(([handle, rank]) => [
+        handle,
+        rankToScore(rank, scores),
+      ]);
+
+      const maximumScore = fellows.reduce((a, [_, score]) => a + score, 0);
+      if (rule.minTotalScore > maximumScore) {
+        throw new Error(
+          `Minimum score of ${rule.minTotalScore} is higher that the obtainable score of ${maximumScore}!`,
+        );
+      }
+
+      let score = 0;
+
+      const countingFellows: [string, number][] = [];
+
+      // We iterate over all the approvals and convert their rank to their score
+      for (const [handle, fellowScore] of fellows) {
+        // We filter fellows whose score is 0
+        if (approvals.indexOf(handle) > -1 && fellowScore > 0) {
+          score += fellowScore;
+          countingFellows.push([handle, fellowScore]);
+        }
+      }
+
+      this.logger.debug(`Current score is ${score} and the minimum required score is ${rule.minTotalScore}`);
+
+      if (rule.minTotalScore > score) {
+        const missingUsers = fellows
+          // Remove all the fellows who score is worth 0
+          .filter(([_, fellowScore]) => fellowScore > 0)
+          // Remove the author
+          .filter(([handle]) => handle != author)
+          // Remove the approvals
+          .filter(([handle]) => approvals.indexOf(handle) < 0);
+
+        this.logger.warn(`Missing score of ${rule.minTotalScore} by ${score - rule.minTotalScore}`);
+
+        return new FellowMissingScoreFailure(rule, rule.minTotalScore, countingFellows, missingUsers);
+      }
     }
+    this.logger.info("Rule requirements fulfilled");
+    // If we don't have any missing reviews, we return no error
+    return null;
   }
 
   /** Using the include and exclude condition, it returns a list of all the files in a PR that matches the criteria */

src/test/fellows.test.ts

@@ -4,7 +4,7 @@ import { mock, mockClear, MockProxy } from "jest-mock-extended";
 import { ActionLogger, TeamApi } from "../github/types";
 import { PolkadotFellows } from "../polkadot/fellows";
 
-const timeout = 15_000;
+const timeout = 25_000;
 
 describe("CAPI test", () => {
   let fellows: TeamApi;