Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add score grouping to fellows reviews #110

Closed
Tracked by #101
Bullrich opened this issue Jan 22, 2024 · 0 comments · Fixed by #113
Closed
Tracked by #101

Add score grouping to fellows reviews #110

Bullrich opened this issue Jan 22, 2024 · 0 comments · Fixed by #113
Assignees
Labels
Fellows Tickets related to the fellowship important

Comments

@Bullrich
Copy link
Contributor

Per proposed in polkadot-fellows/runtimes#93 we need to have a slight change in the review-bot type fellows.

The new solution will keep the old behaviour, with a slight modification:

It will still request a minimum amount of reviews of a given rank, but it will now have a minimum score that it must be achieved also to be approved, allowing more flexible reviews from lower ranks.

For this, a new field named score will be introduced, this will contain a list of all the available dans and their score.

When individuals approve a PR, their dan's score will count towards the total required score.

For example:

score:
  dan1: 1
  dan2: 2
  dan3: 5
  dan4: 9
  dan5: 10
  dan6: 15
rules:
  - name: CI Files
    condition:
      include:
        - ^\.github/.*
    type: fellows
    minRank: 3
    minApprovals: 1
    minScore: 18

This would work in the following way:

CI Files would require at least one approval from someone who is dan 3 or greater. And the combined value of all the approvals, defined in minScore, must get to 18.

So, in this case, a vote of a dan 6, would count 15 points. And we would still need 3 more points. Could either be from:

  • 3 dan 1.
  • 1 dan2 and 1 dan1.
  • 2 dan2.
  • 1 dan3 or greater.
@Bullrich Bullrich self-assigned this Jan 22, 2024
@Bullrich Bullrich moved this from Draft to Open in Parity Roadmap Jan 22, 2024
@Bullrich Bullrich changed the title [EngAut] Add score grouping to fellows reviews Add score grouping to fellows reviews Jan 22, 2024
Bullrich added a commit that referenced this issue Jan 25, 2024
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.
@Bullrich Bullrich added the Fellows Tickets related to the fellowship label Jan 29, 2024
@Bullrich Bullrich linked a pull request Jan 29, 2024 that will close this issue
Bullrich added a commit that referenced this issue Jan 29, 2024
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`
fellowship-merge-bot bot pushed a commit to polkadot-fellows/runtimes that referenced this issue Feb 14, 2024
Updated Review Bot to version 2.4.0 which contains
paritytech/review-bot#110 (solved in paritytech/review-bot#113)

This resolves #93 

Updated review bot to latest version which brings new changes to the
fellow object:

Added new optional field to the config named `scores` that has the score
of each fellow per dan (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).

Find a complete explanation of the new configuration here: [Fellows
Rule](https://github.com/paritytech/review-bot/blob/v2.4.0/README.md#fellows-rule)

- [x] Does not require a CHANGELOG entry

I also added some scores to all the dans (feel free to modify them) and
updated the `Relay and system files` rule to have a minimum score of 6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fellows Tickets related to the fellowship important
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant