Skip to content
This repository has been archived by the owner on Jul 21, 2024. It is now read-only.

feat: support multiple PR reviews #223

Merged
merged 6 commits into from
Mar 11, 2020
Merged

feat: support multiple PR reviews #223

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bb2109d
feat: support multiple PR reviews
allout58 Feb 20, 2020
7f458d6
fix: resolve incorrectly using number of requested reviews
allout58 Feb 25, 2020
5d7c2a6
Merge branch 'master' into feat.194.partial-approval
sotayamashita Mar 3, 2020
7eecb4e
Merge branch 'master' into feat.194.partial-approval
sotayamashita Mar 8, 2020
f15e9b7
Merge branch 'master' into feat.194.partial-approval
sotayamashita Mar 8, 2020
47c0e90
fix: use camelCase for the PR number instead of snake_case
allout58 Mar 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,9 @@ module.exports = {
labelDraft: {
name: "PR: draft",
color: "6a737d"
},
labelPartiallyApproved: {
name: "PR: partially-approved",
color: "7E9C82"
sotayamashita marked this conversation as resolved.
Show resolved Hide resolved
}
};
55 changes: 55 additions & 0 deletions lib/pr-triage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class PRTriage {
UNREVIED: "labelUnreviewed",
APPROVED: "labelApproved",
CHANGES_REQUESTED: "labelChangesRequested",
PARTIALLY_APPROVED: "labelPartiallyApproved",
MERGED: "labelMerged",
DRAFT: "labelDraft"
});
Expand All @@ -41,6 +42,7 @@ class PRTriage {
case PRTriage.STATE.WIP:
case PRTriage.STATE.UNREVIED:
case PRTriage.STATE.CHANGES_REQUESTED:
case PRTriage.STATE.PARTIALLY_APPROVED:
case PRTriage.STATE.APPROVED:
case PRTriage.STATE.DRAFT:
case PRTriage.STATE.MERGED:
Expand Down Expand Up @@ -70,6 +72,9 @@ class PRTriage {
}

const reviews = await this._getUniqueReviews();
const numNeededReviews = await this._getRequiredNumberOfReviews();
const numRequestedReviewsRemaining = await this._getRequestedNumberOfReviews();

if (reviews.length === 0) {
return PRTriage.STATE.UNREVIED;
} else {
Expand All @@ -82,12 +87,62 @@ class PRTriage {

if (changeRequestedReviews.length > 0) {
return PRTriage.STATE.CHANGES_REQUESTED;
} else if (
approvedReviews.length < numNeededReviews ||
numRequestedReviewsRemaining > 0
) {
// Mark if partially approved if:
// 1) Branch protections require more approvals
// - or -
// 2) not everyone requested has approved (requested remaining > 0)
return PRTriage.STATE.PARTIALLY_APPROVED;
} else if (reviews.length === approvedReviews.length) {
return PRTriage.STATE.APPROVED;
}
}
}

/**
* Get the number of required number of reviews according to branch protections
* @return {Promise<number>} The number of required approving reviews, or 1 if branch protections are not set up.
* @private
*/
async _getRequiredNumberOfReviews() {
const { owner, repo } = this.config;
const branch = this.pullRequest.base.ref;
const branchProtections = await this.github.repos.getBranchProtection({
owner,
repo,
branch
});

const requiredReviews =
branchProtections.data.required_pull_request_reviews;
return (
(requiredReviews && requiredReviews.required_approving_review_count) || 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it return 1 even if there is no required number of reviews? I would appreciate it if you could tell me the reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a result of this comment on the original issue. Should be easy enough to change to zero, or in the future have some way of configuring the default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allout58 I understand!

);
}

/**
* Get the number of users and teams that have been requested to review the PR
* @return {Promise<number>}
* @private
*/
async _getRequestedNumberOfReviews() {
const { owner, repo } = this.config;
const pull_number = this.pullRequest.number;
sotayamashita marked this conversation as resolved.
Show resolved Hide resolved
const requestedReviewsObj = await this.github.pulls.listReviewRequests({
owner,
repo,
pull_number
});

return (
requestedReviewsObj.data.teams.length +
requestedReviewsObj.data.users.length
);
}

async _getUniqueReviews() {
const { owner, repo } = this.config;
const pull_number = this.pullRequest.number;
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"description": "GitHub App built with Probot that support pull request workflow",
"main": "index.js",
"scripts": {
"start": "DEBUG=probot* probot run ./index.js",
"start": "probot run ./index.js",
"start:dev": "nodemon --exec \"npm start\"",
"commit": "git-cz",
"lint": "prettier --write {index,lib/**/*,test/pr-triage.test}.js",
Expand Down
228 changes: 151 additions & 77 deletions test/fixtures/payload.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
"state": "open",
"head": {
"sha": "head"
},
"base": {
"ref": "master"
}
}
},
Expand Down Expand Up @@ -64,94 +67,165 @@
"reviews": {
"should_be": {
"chnages_requested": {
"data": [{
"id": 1,
"user": { "id": 1 },
"state": "CHANGES_REQUESTED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
}, {
"id": 1,
"user": { "id": 2 },
"state": "CHANGES_REQUESTED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
}, {
"id": 2,
"user": { "id": 3 },
"state": "APPROVED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "behind"
}]
"data": [
{
"id": 1,
"user": { "id": 1 },
"state": "CHANGES_REQUESTED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
},
{
"id": 1,
"user": { "id": 2 },
"state": "CHANGES_REQUESTED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
},
{
"id": 2,
"user": { "id": 3 },
"state": "APPROVED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "behind"
}
]
},
"approve": {
"data": [{
"id": 1,
"user": { "id": 1 },
"state": "APPROVED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
}, {
"id": 1,
"user": { "id": 2 },
"state": "APPROVED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
}]
"data": [
{
"id": 1,
"user": { "id": 1 },
"state": "APPROVED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
},
{
"id": 1,
"user": { "id": 2 },
"state": "APPROVED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
}
]
}
},
"filtered_by": {
"sha": {
"data": [{
"id": 1,
"user": { "id": 1 },
"state": "CHANGES_REQUESTED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
}, {
"id": 1,
"user": { "id": 2 },
"state": "CHANGES_REQUESTED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
}, {
"id": 2,
"user": { "id": 3 },
"state": "APPROVED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "behind"
}]
"data": [
{
"id": 1,
"user": { "id": 1 },
"state": "CHANGES_REQUESTED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
},
{
"id": 1,
"user": { "id": 2 },
"state": "CHANGES_REQUESTED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
},
{
"id": 2,
"user": { "id": 3 },
"state": "APPROVED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "behind"
}
]
},
"state": {
"data": [{
"id": 1,
"user": { "id": 1 },
"state": "COMMENTED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
}, {
"id": 2,
"user": { "id": 2 },
"state": "APPROVED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
}]
"data": [
{
"id": 1,
"user": { "id": 1 },
"state": "COMMENTED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
},
{
"id": 2,
"user": { "id": 2 },
"state": "APPROVED",
"submitted_at": "2018-01-06T08:28:10Z",
"commit_id": "head"
}
]
},
"date": {
"data": [{
"id": 1,
"user": { "id": 1 },
"state": "CHANGES_REQUESTED",
"submitted_at": "1970-01-01T00:00:00Z",
"commit_id": "head"
}, {
"id": 1,
"user": { "id": 1 },
"state": "APPROVED",
"submitted_at": "2020-07-24T00:00:00Z",
"commit_id": "head"
}]
"data": [
{
"id": 1,
"user": { "id": 1 },
"state": "CHANGES_REQUESTED",
"submitted_at": "1970-01-01T00:00:00Z",
"commit_id": "head"
},
{
"id": 1,
"user": { "id": 1 },
"state": "APPROVED",
"submitted_at": "2020-07-24T00:00:00Z",
"commit_id": "head"
}
]
}
}
},
"review_requests": {
"one_of_each": {
"data": {
"teams": [
{
"id": 1
}
],
"users": [
{
"id": 2
}
]
}
},
"one_user": {
"data": {
"teams": [],
"users": [
{
"id": 2
}
]
}
},
"one_team": {
"data": {
"teams": [
{
"id": 1
}
],
"users": []
}
},
"none": {
"data": {
"teams": [],
"users": []
}
}
},
"branch_protection": {
"required_three": {
"data": {
"required_pull_request_reviews": {
"required_approving_review_count": 3
}
}
},
"none": {
"data": {}
}
}
}
Loading