diff --git a/lib/pr_checker.js b/lib/pr_checker.js index e48bf2d5..5f223d3e 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -30,6 +30,15 @@ const FAST_TRACK_RE = /^Fast-track has been requested by @(.+?)\. Please 👍 to const FAST_TRACK_MIN_APPROVALS = 2; const GIT_CONFIG_GUIDE_URL = 'https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork'; +// eslint-disable-next-line no-extend-native +Array.prototype.findLastIndex ??= function findLastIndex(fn) { + const reversedIndex = Reflect.apply( + Array.prototype.findIndex, + this.slice().reverse(), + arguments); + return reversedIndex === -1 ? -1 : this.length - reversedIndex - 1; +}; + export default class PRChecker { /** * @param {{}} cli @@ -520,11 +529,14 @@ export default class PRChecker { } = this; const { maxCommits } = argv; - if (reviews.length < 1) { + const reviewIndex = reviews.findLastIndex( + review => review.authorCanPushToRepository && review.state === 'APPROVED' + ); + + if (reviewIndex === -1) { return false; } - const reviewIndex = reviews.length - 1; const reviewDate = reviews[reviewIndex].publishedAt; const afterCommits = []; @@ -537,7 +549,7 @@ export default class PRChecker { const totalCommits = afterCommits.length; if (totalCommits > 0) { - cli.warn('Commits were pushed since the last review:'); + cli.warn('Commits were pushed since the last approving review:'); const sliceLength = maxCommits === 0 ? totalCommits : -maxCommits; afterCommits.slice(sliceLength) .forEach(commit => { diff --git a/lib/queries/Reviews.gql b/lib/queries/Reviews.gql index cb9374f9..4761fdcb 100644 --- a/lib/queries/Reviews.gql +++ b/lib/queries/Reviews.gql @@ -13,6 +13,7 @@ query Reviews($prid: Int!, $owner: String!, $repo: String!, $after: String) { author { login } + authorCanPushToRepository url publishedAt } diff --git a/test/fixtures/more_than_three_commits_after_review_reviews.json b/test/fixtures/more_than_three_commits_after_review_reviews.json index 842cb67d..039b51d3 100644 --- a/test/fixtures/more_than_three_commits_after_review_reviews.json +++ b/test/fixtures/more_than_three_commits_after_review_reviews.json @@ -4,6 +4,7 @@ "author": { "login": "foo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-89923489", "publishedAt": "2017-07-23T11:19:25Z" }] diff --git a/test/fixtures/multiple_commits_after_review_reviews.json b/test/fixtures/multiple_commits_after_review_reviews.json index 7b5ae2d8..1307b1eb 100644 --- a/test/fixtures/multiple_commits_after_review_reviews.json +++ b/test/fixtures/multiple_commits_after_review_reviews.json @@ -4,6 +4,7 @@ "author": { "login": "foo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-89923489", "publishedAt": "2017-09-23T11:19:25Z" }] diff --git a/test/fixtures/reviews_approved.json b/test/fixtures/reviews_approved.json index 17952164..4024c5ad 100644 --- a/test/fixtures/reviews_approved.json +++ b/test/fixtures/reviews_approved.json @@ -4,6 +4,7 @@ "author": { "login": "foo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624", "publishedAt": "2017-10-24T11:19:00Z" }, @@ -13,6 +14,7 @@ "author": { "login": "Baz" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488392", "publishedAt": "2017-10-24T11:50:52Z" }, @@ -22,6 +24,7 @@ "author": { "login": "Baz" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-714882992", "publishedAt": "2017-10-24T12:30:52Z" }, @@ -31,6 +34,7 @@ "author": { "login": "Quux" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236", "publishedAt": "2017-10-24T14:49:01Z" }, @@ -40,6 +44,7 @@ "author": { "login": "Baz" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236", "publishedAt": "2017-10-24T14:49:02Z" }, @@ -49,6 +54,7 @@ "author": { "login": "Quo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236", "publishedAt": "2017-10-24T19:09:52Z" }, @@ -58,6 +64,7 @@ "author": { "login": "bot" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71839232", "publishedAt": "2017-10-28T19:21:52Z" }] diff --git a/test/fixtures/reviews_requesting_changes.json b/test/fixtures/reviews_requesting_changes.json index 18a5417c..7684e4ec 100644 --- a/test/fixtures/reviews_requesting_changes.json +++ b/test/fixtures/reviews_requesting_changes.json @@ -5,6 +5,7 @@ "author": { "login": "foo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624", "publishedAt": "2017-10-24T11:19:25Z" }, @@ -14,6 +15,7 @@ "author": { "login": "Baz" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488392", "publishedAt": "2017-10-24T11:50:52Z" }, @@ -23,6 +25,7 @@ "author": { "login": "Baz" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-714882992", "publishedAt": "2017-10-24T12:30:52Z" }, @@ -32,6 +35,7 @@ "author": { "login": "Quo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236", "publishedAt": "2017-10-24T19:09:52Z" }, @@ -41,6 +45,7 @@ "author": { "login": "bot" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71839232", "publishedAt": "2017-10-24T19:21:52Z" }, @@ -50,6 +55,7 @@ "author": { "login": "bar" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71482624", "publishedAt": "2017-10-24T11:27:02Z" }] diff --git a/test/fixtures/single_commit_after_review_reviews.json b/test/fixtures/single_commit_after_review_reviews.json index 7fdae6e2..c37c880c 100644 --- a/test/fixtures/single_commit_after_review_reviews.json +++ b/test/fixtures/single_commit_after_review_reviews.json @@ -4,6 +4,25 @@ "author": { "login": "foo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624", "publishedAt": "2017-10-24T11:19:25Z" +},{ + "bodyText": "Not sure about that last change", + "state": "COMMENTED", + "author": { + "login": "foo" + }, + "authorCanPushToRepository": true, + "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480625", + "publishedAt": "2017-10-26T11:19:25Z" +},{ + "bodyText": "Good idea", + "state": "APPROVED", + "author": { + "login": "bar" + }, + "authorCanPushToRepository": false, + "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480626", + "publishedAt": "2017-10-26T11:19:25Z" }] diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 441200e5..74a6e92e 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -2059,7 +2059,7 @@ describe('PRChecker', () => { const expectedLogs = { warn: [ - ['Commits were pushed since the last review:'], + ['Commits were pushed since the last approving review:'], ['- src: fix issue with es-modules'] ], info: [], @@ -2091,7 +2091,7 @@ describe('PRChecker', () => { const expectedLogs = { warn: [ - ['Commits were pushed since the last review:'], + ['Commits were pushed since the last approving review:'], ['- src: add requested feature'], ['- nit: edit mistakes'] ], @@ -2122,7 +2122,7 @@ describe('PRChecker', () => { const { commits, reviews } = moreThanThreeCommitsAfterReview; const expectedLogs = { warn: [ - ['Commits were pushed since the last review:'], + ['Commits were pushed since the last approving review:'], ['- src: add requested feature'], ['- nit: edit mistakes'], ['- final: we should be good to go'], @@ -2180,20 +2180,18 @@ describe('PRChecker', () => { commits: simpleCommits, collaborators, authorIsNew: () => true, - getThread() { - return PRData.prototype.getThread.call(this); - } + getThread: PRData.prototype.getThread }, {}, argv); const status = checker.checkCommitsAfterReview(); - assert.deepStrictEqual(status, true); + assert.strictEqual(status, true); }); it('should log as expected if passed 1 as flag', () => { const { commits, reviews } = moreThanThreeCommitsAfterReview; const expectedLogs = { warn: [ - ['Commits were pushed since the last review:'], + ['Commits were pushed since the last approving review:'], ['- final: we should be good to go'], ['...(use `--max-commits 4` to see the full list of commits)'] ], @@ -2224,7 +2222,7 @@ describe('PRChecker', () => { const { commits, reviews } = moreThanThreeCommitsAfterReview; const expectedLogs = { warn: [ - ['Commits were pushed since the last review:'], + ['Commits were pushed since the last approving review:'], ['...(use `--max-commits 4` to see the full list of commits)'] ], info: [],