diff --git a/index.js b/index.js index ab7c986..7c545d9 100644 --- a/index.js +++ b/index.js @@ -1,11 +1,12 @@ const debug = require("debug")("probot:pr-triage"); -const Raven = require("raven"); +const Sentry = require("@sentry/node"); const PRTriage = require("./lib/pr-triage"); -Raven.config( - process.env.NODE_ENV === "production" && - "https://dce36edab6334112b02122e07b2bc549@sentry.io/1222067" -).install(); +if (process.env.NODE_ENV === "production") { + Sentry.init({ + dsn: "https://dce36edab6334112b02122e07b2bc549@sentry.io/1222067" + }); +} function probotPlugin(robot) { const events = [ @@ -26,16 +27,14 @@ async function triage(context) { const prTriage = forRepository(context); const pullRequest = getPullRequest(context); - Raven.context(() => { - Raven.setContext({ - extra: { - owner: context.repo()["owner"], - repo: context.repo()["repo"], - number: pullRequest.number - } + try { + Sentry.configureScope(scope => { + scope.setExtra("pull_request_url", pullRequest.url); }); prTriage.triage(pullRequest); - }); + } catch (e) { + Sentry.captureMessage(e); + } } function forRepository(context) { 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 8fbfecd..91000e9 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" }); @@ -31,29 +32,18 @@ class PRTriage { async triage(pullRequest) { Object.assign(this.pullRequest, pullRequest); - const { owner, repo } = this.config; - const number = this.pullRequest.number; await this._ensurePRTriageLabelExists(); const state = await this._getState(); - switch (state) { 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: this._updateLabel(state); - this.logger( - "%s/%s#%s is labeld as %s", - owner, - repo, - number, - Object.keys(PRTriage.STATE).find(k => { - return PRTriage.STATE[k] === state; - }) - ); break; default: throw new Error("Undefined state"); @@ -70,6 +60,9 @@ class PRTriage { } const reviews = await this._getUniqueReviews(); + const requiredNumberOfReviews = await this._getRequiredNumberOfReviews(); + const numRequestedReviewsRemaining = await this._getRequestedNumberOfReviews(); + if (reviews.length === 0) { return PRTriage.STATE.UNREVIED; } else { @@ -82,6 +75,15 @@ class PRTriage { if (changeRequestedReviews.length > 0) { return PRTriage.STATE.CHANGES_REQUESTED; + } else if ( + approvedReviews.length < requiredNumberOfReviews || + 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; } @@ -89,14 +91,8 @@ class PRTriage { } async _getUniqueReviews() { - const { owner, repo } = this.config; - const pull_number = this.pullRequest.number; + const reviews = await this._getReviews(); const sha = this.pullRequest.head.sha; - - const reviews = - (await this.github.pulls.listReviews({ owner, repo, pull_number })) - .data || []; - const uniqueReviews = reviews .filter(review => review.commit_id === sha) .filter( @@ -131,9 +127,82 @@ class PRTriage { return Object.values(uniqueReviews); } + /** + * Get the number of required number of reviews according to branch protections + * @return {Promise} The number of required approving reviews, or 1 if Administration Permission is not granted or Branch Protection is not set up + * @private + */ + async _getRequiredNumberOfReviews() { + const { owner, repo } = this.config; + const branch = this.pullRequest.base.ref; + return ( + this.github.repos + // See: https://developer.github.com/v3/previews/#require-multiple-approving-reviews + .getBranchProtection({ + owner, + repo, + branch, + mediaType: { + previews: ["luke-cage"] + } + }) + .then(res => { + return res.data.required_pull_request_reviews + .required_approving_review_count; + }) + .catch(err => { + // Return the minium number of reviews if it's 403 or 403 because Administration Permission is not granted (403) or Branch Protection is not set up(404) + if (err.status === 404 || err.status === 403) { + return 1; + } + throw err; + }) + ); + } + + /** + * 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; + return this.github.pulls + .listReviewRequests({ owner, repo, pull_number }) + .then(res => res.data.teams.length + res.data.users.length); + } + + async _getReviews() { + const { owner, repo } = this.config; + // Ignore inconsitent variable name conversation + // because of https://octokit.github.io/rest.js/v17#pulls-list-reviews + const pull_number = this.pullRequest.number; + return this.github.pulls + .listReviews({ owner, repo, pull_number }) + .then(res => res.data || []); + } + async _ensurePRTriageLabelExists() { - for (const labelObj in this._getFilteredConfigObjByRegex(/label_*/)) { - await this._createLabel(labelObj); + const labelKeys = Object.keys(this._getFilteredConfigObjByRegex(/label_*/)); + for (let i = 0; i < labelKeys.length; i++) { + await this._createLabel(labelKeys[i]); + } + } + + async _updateLabel(labelKey) { + const currentLabelKey = await this._getCurrentLabelKey(); + if (currentLabelKey) { + if (labelKey === PRTriage.STATE.WIP) { + this._removeLabel(currentLabelKey); + } else if (currentLabelKey !== labelKey) { + this._removeLabel(currentLabelKey); + this._addLabel(labelKey); + } + } else { + if (labelKey !== PRTriage.STATE.WIP) { + this._addLabel(labelKey); + } } } @@ -141,6 +210,7 @@ class PRTriage { const { owner, repo } = this.config; const labelObj = this._getConfigObj(key); + // Create a label to repository if the label is not created. return this.github.issues .getLabel({ owner, repo, name: labelObj.name }) .catch(() => { @@ -155,15 +225,15 @@ class PRTriage { async _addLabel(key) { const { owner, repo } = this.config; - const number = this.pullRequest.number; + const issue_number = this.pullRequest.number; const labelObj = this._getConfigObj(key); - // Check if a label does not exist. If it does, it addes the label. + // Add a label to issue if it does not exist. return this._getLabel(key).catch(() => { return this.github.issues.addLabels({ owner, repo, - number, + issue_number, labels: [labelObj.name] }); }); @@ -174,14 +244,14 @@ class PRTriage { const issue_number = this.pullRequest.number; const labelObj = this._getConfigObj(key); - // Check if a label exists. If it does, it removes the label. + // Remove the label from a issue if it exists. return this._getLabel(key).then( labelObj => { return this.github.issues .removeLabel({ owner, repo, issue_number, name: labelObj.name }) .catch(err => { // Ignore if it's a 404 because then the label was already removed - if (err.code !== 404) { + if (err.status !== 404) { throw err; } }); @@ -190,22 +260,6 @@ class PRTriage { ); // Do nothing for error handling. } - async _updateLabel(labelKey) { - const currentLabelKey = await this._getCurrentLabelKey(); - if (currentLabelKey) { - if (labelKey === PRTriage.STATE.WIP) { - this._removeLabel(currentLabelKey); - } else if (currentLabelKey !== labelKey) { - this._removeLabel(currentLabelKey); - this._addLabel(labelKey); - } - } else { - if (labelKey !== PRTriage.STATE.WIP) { - this._addLabel(labelKey); - } - } - } - _getLabel(key) { return new Promise((resolve, reject) => { for (const label of this.pullRequest.labels) { diff --git a/package.json b/package.json index 3027cac..23016d2 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", @@ -32,6 +32,7 @@ "author": "Sam Yamashita", "license": "Apache-2.0", "dependencies": { + "@sentry/node": "^5.14.2", "debug": "^4.1.1", "dotenv": "^8.0.0", "probot": "^9.2.10", @@ -48,7 +49,6 @@ "nodemon": "^2.0.1", "prettier": "1.19.1", "probot-config": "^1.0.0", - "raven": "^2.6.3", "semantic-release": "^17.0.2", "smee-client": "^1.1.0", "validate-commit-msg": "^2.14.0" diff --git a/test/fixtures/payload.json b/test/fixtures/payload.json index 554ca8c..3f12660 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,168 @@ "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 + } + } + }, + "not_defined": { + "status": 404 + }, + "not_allowed": { + "status": 403 } } } diff --git a/test/pr-triage.test.js b/test/pr-triage.test.js index ec8aab9..26d65c8 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 }); @@ -82,7 +83,19 @@ describe("PRTriage", () => { describe("when there are NO reviews", () => { const github = { pulls: { - listReviews: jest.fn().mockReturnValue(Promise.resolve({})) + listReviews: jest.fn().mockReturnValue(Promise.resolve({})), + listReviewRequests: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["review_requests"]["none"]) + ) + }, + repos: { + getBranchProtection: jest + .fn() + .mockReturnValue( + Promise.reject(payload["branch_protection"]["not_defined"]) + ) } }; const klass = new PRTriage(github, { owner, repo }); @@ -105,6 +118,18 @@ describe("PRTriage", () => { Promise.resolve( payload["reviews"]["should_be"]["chnages_requested"] ) + ), + listReviewRequests: jest + .fn() + .mockReturnValue( + Promise.resolve(payload["review_requests"]["none"]) + ) + }, + repos: { + getBranchProtection: jest + .fn() + .mockReturnValue( + Promise.reject(payload["branch_protection"]["not_defined"]) ) } }; @@ -126,6 +151,18 @@ describe("PRTriage", () => { .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.reject(payload["branch_protection"]["not_defined"]) ) } }; @@ -139,8 +176,204 @@ describe("PRTriage", () => { 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.reject(payload["branch_protection"]["not_defined"]) + ) + } + }; + 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); + }); + }); + + 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.reject(payload["branch_protection"]["not_defined"]) + ) + } + }; + 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 */ @@ -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();