From bb2109d07db705f743e782979b4dd30877d3e6a5 Mon Sep 17 00:00:00 2001 From: James Hollowell Date: Thu, 20 Feb 2020 15:39:16 -0500 Subject: [PATCH 1/3] feat: support multiple PR reviews Provide support for requiring multiple reviews. Add a "partially-approved" state, which occurs when the number of approvals is less than the required number (from branch protections) or the number of reviewers who have been requested but have not submitted an approval fixes #194 --- lib/defaults.js | 4 + lib/pr-triage.js | 58 ++++++++- test/pr-triage.test.js | 271 ++++++++++++++++++++++++++++++++++++++--- 3 files changed, 313 insertions(+), 20 deletions(-) diff --git a/lib/defaults.js b/lib/defaults.js index ec5088b..db22b24 100644 --- a/lib/defaults.js +++ b/lib/defaults.js @@ -19,5 +19,9 @@ module.exports = { labelDraft: { name: "PR: draft", color: "6a737d" + }, + labelPartiallyApproved: { + name: "PR: partially-approved", + color: "7E9C82" } }; diff --git a/lib/pr-triage.js b/lib/pr-triage.js index ea1ce88..5e5c371 100644 --- a/lib/pr-triage.js +++ b/lib/pr-triage.js @@ -15,6 +15,7 @@ class PRTriage { UNREVIED: "labelUnreviewed", APPROVED: "labelApproved", CHANGES_REQUESTED: "labelChangesRequested", + PARTIALLY_APPROVED: "labelPartiallyApproved", MERGED: "labelMerged", DRAFT: "labelDraft" }); @@ -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: @@ -70,6 +72,9 @@ class PRTriage { } const reviews = await this._getUniqueReviews(); + const numNeededReviews = await this._getRequiredNumberOfReviews(); + const numRequestedReviews = await this._getRequestedNumberOfReviews(); + if (reviews.length === 0) { return PRTriage.STATE.UNREVIED; } else { @@ -82,19 +87,70 @@ class PRTriage { if (changeRequestedReviews.length > 0) { return PRTriage.STATE.CHANGES_REQUESTED; + } else if ( + approvedReviews.length < numNeededReviews || + approvedReviews.length < numRequestedReviews + ) { + // Mark if partially approved if: + // 1) Branch protections require more approvals + // - or - + // 2) not everyone requested has approved + // (2) might need to be configurable in the future + 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} 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 + ); + } + + /** + * Get the number of users and teams that have been requested to review the PR + * @return {Promise} + * @private + */ + async _getRequestedNumberOfReviews() { + const { owner, repo } = this.config; + const pull_number = this.pullRequest.number; + 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; const sha = this.pullRequest.head.sha; const reviews = - (await this.github.pullRequests.listReviews({ owner, repo, pull_number })) + (await this.github.pulls.listReviews({ owner, repo, pull_number })) .data || []; const uniqueReviews = reviews diff --git a/test/pr-triage.test.js b/test/pr-triage.test.js index cf4faea..93e0226 100644 --- a/test/pr-triage.test.js +++ b/test/pr-triage.test.js @@ -14,6 +14,7 @@ describe("PRTriage", () => { UNREVIED: expect.any(String), APPROVED: expect.any(String), CHANGES_REQUESTED: expect.any(String), + PARTIALLY_APPROVED: expect.any(String), MERGED: expect.any(String), DRAFT: expect.any(String) }); @@ -38,10 +39,9 @@ describe("PRTriage", () => { const subject = () => klass._getState(); test("should be STATE.DRAFT", async () => { - klass.pullRequest = - payload["pull_request"]["with"]["draft"]["data"]; - const result = await subject(); - expect(result).toEqual(PRTriage.STATE.DRAFT) + klass.pullRequest = payload["pull_request"]["with"]["draft"]["data"]; + const result = await subject(); + expect(result).toEqual(PRTriage.STATE.DRAFT); }); }); @@ -50,11 +50,12 @@ describe("PRTriage", () => { const subject = () => klass._getState(); test("should be STATE.DRAFT", async () => { - klass.pullRequest = payload["pull_request"]["with"]["draft_and_wip_title"]["data"]; + klass.pullRequest = + payload["pull_request"]["with"]["draft_and_wip_title"]["data"]; const result = await subject(); - expect(result).toEqual(PRTriage.STATE.DRAFT) - }) - }) + expect(result).toEqual(PRTriage.STATE.DRAFT); + }); + }); describe("when pull request title include WIP regex", () => { const klass = new PRTriage({}, { owner, repo }); @@ -81,8 +82,20 @@ describe("PRTriage", () => { describe("when there are NO reviews", () => { const github = { - pullRequests: { - listReviews: jest.fn().mockReturnValue(Promise.resolve({})) + pulls: { + listReviews: jest.fn().mockReturnValue(Promise.resolve({})), + listReviewRequests: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["review_requests"]["none"]) + ) + }, + repos: { + getBranchProtection: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["branch_protection"]["none"]) + ) } }; const klass = new PRTriage(github, { owner, repo }); @@ -98,13 +111,25 @@ describe("PRTriage", () => { describe("when number of CHANGES_REQUESTED is more than 0", () => { const github = { - pullRequests: { + pulls: { listReviews: jest .fn() .mockReturnValue( Promise.resolve( payload["reviews"]["should_be"]["chnages_requested"] ) + ), + listReviewRequests: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["review_requests"]["none"]) + ) + }, + repos: { + getBranchProtection: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["branch_protection"]["none"]) ) } }; @@ -121,26 +146,234 @@ describe("PRTriage", () => { describe("when number of reviews and approved reviews are same", () => { const github = { - pullRequests: { + pulls: { + listReviews: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["reviews"]["should_be"]["approve"]) + ), + listReviewRequests: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["review_requests"]["none"]) + ) + }, + repos: { + getBranchProtection: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["branch_protection"]["none"]) + ) + } + }; + const klass = new PRTriage(github, { owner, repo }); + const subject = () => klass._getState(); + + test("should be STATE.APPROVE", async () => { + klass.pullRequest = + payload["pull_request"]["with"]["unreviewed_label"]["data"]; + const result = await subject(); + expect(result).toEqual(PRTriage.STATE.APPROVED); + }); + }); + + describe("when number of requested reviews is more than 0", () => { + const github = { + pulls: { listReviews: jest .fn() .mockReturnValue( Promise.resolve(payload["reviews"]["should_be"]["approve"]) + ), + listReviewRequests: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["review_requests"]["one_user"]) + ) + }, + repos: { + getBranchProtection: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["branch_protection"]["none"]) ) } }; const klass = new PRTriage(github, { owner, repo }); const subject = () => klass._getState(); - test("should be STATE.APPROVE", async () => { + test("should be STATE.PARTIALLY_APPROVED", async () => { klass.pullRequest = payload["pull_request"]["with"]["unreviewed_label"]["data"]; const result = await subject(); - expect(result).toEqual(PRTriage.STATE.APPROVED); + expect(result).toEqual(PRTriage.STATE.PARTIALLY_APPROVED); + }); + }); + + describe("when number of approved reviews is less than the number of required reviews", () => { + const github = { + pulls: { + listReviews: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["reviews"]["should_be"]["approve"]) + ), + listReviewRequests: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["review_requests"]["none"]) + ) + }, + repos: { + getBranchProtection: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["branch_protection"]["required_three"]) + ) + } + }; + const klass = new PRTriage(github, { owner, repo }); + const subject = () => klass._getState(); + + test("should be STATE.PARTIALLY_APPROVED", async () => { + klass.pullRequest = + payload["pull_request"]["with"]["unreviewed_label"]["data"]; + const result = await subject(); + expect(result).toEqual(PRTriage.STATE.PARTIALLY_APPROVED); }); }); }); // _getState + describe("_getRequiredNumberOfReviews", () => { + describe("when there is no branch protections", () => { + const github = { + repos: { + getBranchProtection: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["branch_protection"]["none"]) + ) + } + }; + const klass = new PRTriage(github, { owner, repo }); + const subject = () => klass._getRequiredNumberOfReviews(); + + test("should be return a default of 1", async () => { + klass.pullRequest = + payload["pull_request"]["with"]["unreviewed_label"]["data"]; + const result = await subject(); + expect(result).toEqual(1); + }); + }); + + describe("when there is a branch protection for the branch", () => { + const github = { + repos: { + getBranchProtection: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["branch_protection"]["required_three"]) + ) + } + }; + const klass = new PRTriage(github, { owner, repo }); + const subject = () => klass._getRequiredNumberOfReviews(); + + test("should return the `required_approving_review_count`", async () => { + klass.pullRequest = + payload["pull_request"]["with"]["unreviewed_label"]["data"]; + const result = await subject(); + expect(result).toEqual(3); + }); + }); + }); + + describe("_getRequestedNumberOfReviews", () => { + describe("when there is no remaining review requests", () => { + const github = { + pulls: { + listReviewRequests: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["review_requests"]["none"]) + ) + } + }; + const klass = new PRTriage(github, { owner, repo }); + const subject = () => klass._getRequestedNumberOfReviews(); + + test("should be return 0", async () => { + klass.pullRequest = + payload["pull_request"]["with"]["unreviewed_label"]["data"]; + const result = await subject(); + expect(result).toEqual(0); + }); + }); + + describe("when there is 1 remaining review TEAM requests", () => { + const github = { + pulls: { + listReviewRequests: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["review_requests"]["one_team"]) + ) + } + }; + const klass = new PRTriage(github, { owner, repo }); + const subject = () => klass._getRequestedNumberOfReviews(); + + test("should be return 0", async () => { + klass.pullRequest = + payload["pull_request"]["with"]["unreviewed_label"]["data"]; + const result = await subject(); + expect(result).toEqual(1); + }); + }); + + describe("when there is 1 remaining review USER requests", () => { + const github = { + pulls: { + listReviewRequests: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["review_requests"]["one_user"]) + ) + } + }; + const klass = new PRTriage(github, { owner, repo }); + const subject = () => klass._getRequestedNumberOfReviews(); + + test("should be return 0", async () => { + klass.pullRequest = + payload["pull_request"]["with"]["unreviewed_label"]["data"]; + const result = await subject(); + expect(result).toEqual(1); + }); + }); + + describe("when there is 1 remaining review TEAM and USER requests", () => { + const github = { + pulls: { + listReviewRequests: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["review_requests"]["one_of_each"]) + ) + } + }; + const klass = new PRTriage(github, { owner, repo }); + const subject = () => klass._getRequestedNumberOfReviews(); + + test("should be return 0", async () => { + klass.pullRequest = + payload["pull_request"]["with"]["unreviewed_label"]["data"]; + const result = await subject(); + expect(result).toEqual(2); + }); + }); + }); + /** * _getUniqueReviews */ @@ -148,7 +381,7 @@ describe("PRTriage", () => { describe("when there are reviews", () => { describe("and filtered by sha", () => { const github = { - pullRequests: { + pulls: { listReviews: jest .fn() .mockReturnValue( @@ -173,7 +406,7 @@ describe("PRTriage", () => { describe("and filtered by state", () => { const github = { - pullRequests: { + pulls: { listReviews: jest .fn() .mockReturnValue( @@ -197,7 +430,7 @@ describe("PRTriage", () => { describe("and filtered by date", () => { const github = { - pullRequests: { + pulls: { listReviews: jest .fn() .mockReturnValue( @@ -222,7 +455,7 @@ describe("PRTriage", () => { describe("when there are NO reviews", () => { const github = { - pullRequests: { + pulls: { listReviews: jest.fn().mockReturnValue(Promise.resolve({})) } }; @@ -244,7 +477,7 @@ describe("PRTriage", () => { describe("_ensurePRTriageLabelExists", () => { const klass = new PRTriage({}, { owner, repo }); klass._createLabel = jest.fn().mockReturnValue(Promise.resolve({})); - const n = 5; + const n = 6; test(`createLabel() should be called ${n} times`, async () => { await klass._ensurePRTriageLabelExists(); From 7f458d6dd75f6eb51aa50f0a4ed3632ac0f3aba4 Mon Sep 17 00:00:00 2001 From: James Hollowell Date: Tue, 25 Feb 2020 14:59:44 -0500 Subject: [PATCH 2/3] fix: resolve incorrectly using number of requested reviews Resolve incorrectly using the requested number of reviews. The value is not the total number of requested reviews, but the number of reviews remaining. Also, fix a commit issue where the `payload.json` fixture was not committed along with the tests --- lib/pr-triage.js | 7 +- package.json | 2 +- test/fixtures/payload.json | 228 ++++++++++++++++++++++++------------- 3 files changed, 155 insertions(+), 82 deletions(-) diff --git a/lib/pr-triage.js b/lib/pr-triage.js index 5e5c371..6e6cd08 100644 --- a/lib/pr-triage.js +++ b/lib/pr-triage.js @@ -73,7 +73,7 @@ class PRTriage { const reviews = await this._getUniqueReviews(); const numNeededReviews = await this._getRequiredNumberOfReviews(); - const numRequestedReviews = await this._getRequestedNumberOfReviews(); + const numRequestedReviewsRemaining = await this._getRequestedNumberOfReviews(); if (reviews.length === 0) { return PRTriage.STATE.UNREVIED; @@ -89,13 +89,12 @@ class PRTriage { return PRTriage.STATE.CHANGES_REQUESTED; } else if ( approvedReviews.length < numNeededReviews || - approvedReviews.length < numRequestedReviews + numRequestedReviewsRemaining > 0 ) { // Mark if partially approved if: // 1) Branch protections require more approvals // - or - - // 2) not everyone requested has approved - // (2) might need to be configurable in the future + // 2) not everyone requested has approved (requested remaining > 0) return PRTriage.STATE.PARTIALLY_APPROVED; } else if (reviews.length === approvedReviews.length) { return PRTriage.STATE.APPROVED; diff --git a/package.json b/package.json index 93579e3..e7f21e6 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/test/fixtures/payload.json b/test/fixtures/payload.json index 554ca8c..1fb4469 100644 --- a/test/fixtures/payload.json +++ b/test/fixtures/payload.json @@ -36,6 +36,9 @@ "state": "open", "head": { "sha": "head" + }, + "base": { + "ref": "master" } } }, @@ -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": {} + } } } From 47c0e904c95922e05e07488bf3e0c9d2fdbdb9e2 Mon Sep 17 00:00:00 2001 From: James Hollowell Date: Sun, 8 Mar 2020 14:13:48 -0400 Subject: [PATCH 3/3] fix: use camelCase for the PR number instead of snake_case --- lib/pr-triage.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/pr-triage.js b/lib/pr-triage.js index 6e6cd08..06f6c5c 100644 --- a/lib/pr-triage.js +++ b/lib/pr-triage.js @@ -130,11 +130,11 @@ class PRTriage { */ async _getRequestedNumberOfReviews() { const { owner, repo } = this.config; - const pull_number = this.pullRequest.number; + const pullNumber = this.pullRequest.number; const requestedReviewsObj = await this.github.pulls.listReviewRequests({ owner, repo, - pull_number + pullNumber }); return ( @@ -145,11 +145,11 @@ class PRTriage { async _getUniqueReviews() { const { owner, repo } = this.config; - const pull_number = this.pullRequest.number; + const pullNumber = this.pullRequest.number; const sha = this.pullRequest.head.sha; const reviews = - (await this.github.pulls.listReviews({ owner, repo, pull_number })) + (await this.github.pulls.listReviews({ owner, repo, pullNumber })) .data || []; const uniqueReviews = reviews