diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 53b0a39..d692013 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -54,7 +54,8 @@ class PRChecker { this.checkCI(), this.checkPRWait(new Date()), this.checkMergeableState(), - this.checkPRState() + this.checkPRState(), + this.checkGitConfig() ]; if (this.data.authorIsNew()) { @@ -277,7 +278,7 @@ class PRChecker { } isOddAuthor(commit) { - const { pr, collaboratorEmails } = this; + const { pr } = this; // They have turned on the private email feature, can't really check // anything, GitHub should know how to link that, see nodejs/node#15489 @@ -291,12 +292,6 @@ class PRChecker { return false; } - // The commit author is one of the collaborators, they should know - // what they are doing anyway - if (collaboratorEmails.has(commit.author.email)) { - return false; - } - if (commit.author.email === pr.author.email) { return false; } @@ -304,11 +299,23 @@ class PRChecker { // At this point, the commit: // 1. is not authored by the commiter i.e. author email is not in the // committer's Github account - // 2. is not authored by a collaborator // 3. is not authored by the people opening the PR return true; } + checkGitConfig() { + const { cli, commits } = this; + for (let { commit } of commits) { + if (commit.author.user === null) { + const msg = 'Author does not have correct git config!'; + cli.error(msg); + return false; + } + } + + return true; + } + checkCommitsAfterReview() { const { commits, reviews, cli, argv diff --git a/lib/queries/PRCommits.gql b/lib/queries/PRCommits.gql index bb54713..4eced12 100644 --- a/lib/queries/PRCommits.gql +++ b/lib/queries/PRCommits.gql @@ -11,6 +11,9 @@ query Commits($prid: Int!, $owner: String!, $repo: String!, $after: String) { commit { committedDate author { + user { + login + } email name } diff --git a/test/fixtures/data.js b/test/fixtures/data.js index 54d7c9f..153ae5c 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -27,6 +27,7 @@ const commentsWithCI = readJSON('comments_with_ci.json'); const commentsWithLGTM = readJSON('comments_with_lgtm.json'); const oddCommits = readJSON('odd_commits.json'); +const incorrectGitConfigCommits = readJSON('incorrect_git_config_commits.json'); const simpleCommits = readJSON('simple_commits.json'); const collabArr = readJSON('collaborators.json'); @@ -79,6 +80,7 @@ module.exports = { commentsWithCI, commentsWithLGTM, oddCommits, + incorrectGitConfigCommits, simpleCommits, singleCommitAfterReview, multipleCommitsAfterReview, diff --git a/test/fixtures/incorrect_git_config_commits.json b/test/fixtures/incorrect_git_config_commits.json new file mode 100644 index 0000000..5f617e8 --- /dev/null +++ b/test/fixtures/incorrect_git_config_commits.json @@ -0,0 +1,38 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:35:26Z", + "author": { + "user": null, + "email": "pr_author@example.com", + "name": "Their Github Account email" + }, + "committer": { + "email": "pr_author@example.com", + "name": "Their Github Account email" + }, + "oid": "ffdef335209c77f66d933bd873950747bfe42264", + "messageHeadline": "doc: some changes", + "message": "Normal commit, same email for everything", + "authoredByCommitter": true + } + }, + { + "commit": { + "committedDate": "2017-09-26T12:35:14Z", + "author": { + "email": "test@example.com", + "name": "Their git config user.email" + }, + "committer": { + "email": "pr_author@example.com", + "name": "Their Github Account email" + }, + "oid": "e3ad7c72e88c83b7184563e8fdb63df02e2fbacb", + "messageHeadline": "doc: some changes 2", + "message": "Either they have not configured git user.email, or have not add the email to their account", + "authoredByCommitter": false + } + } +] + diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 87cc13a..8ac0d79 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -18,6 +18,7 @@ const { multipleCommitsAfterReview, moreThanThreeCommitsAfterReview, oddCommits, + incorrectGitConfigCommits, simpleCommits, commitsAfterCi, mulipleCommitsAfterCi, @@ -53,6 +54,7 @@ describe('PRChecker', () => { let checkCommitsAfterReviewStub; let checkMergeableStateStub; let checkPRState; + let checkGitConfigStub; before(() => { checkReviewsStub = sinon.stub(checker, 'checkReviews'); @@ -63,6 +65,7 @@ describe('PRChecker', () => { sinon.stub(checker, 'checkCommitsAfterReview'); checkMergeableStateStub = sinon.stub(checker, 'checkMergeableState'); checkPRState = sinon.stub(checker, 'checkPRState'); + checkGitConfigStub = sinon.stub(checker, 'checkGitConfig'); }); after(() => { @@ -73,6 +76,7 @@ describe('PRChecker', () => { checkCommitsAfterReviewStub.restore(); checkMergeableStateStub.restore(); checkPRState.restore(); + checkGitConfigStub.restore(); }); it('should run necessary checks', () => { @@ -85,6 +89,7 @@ describe('PRChecker', () => { assert.strictEqual(checkCommitsAfterReviewStub.calledOnce, true); assert.strictEqual(checkMergeableStateStub.calledOnce, true); assert.strictEqual(checkPRState.calledOnce, true); + assert.strictEqual(checkGitConfigStub.calledOnce, true); }); }); @@ -590,6 +595,54 @@ describe('PRChecker', () => { }); }); + describe('checkGitConfig', () => { + it('should log an error is user has wrong git config', () => { + const cli = new TestCLI(); + const expectedLogs = { + error: [ + [ 'Author does not have correct git config!' ] + ] + }; + + const data = { + pr: firstTimerPrivatePR, + reviewers: allGreenReviewers, + comments: commentsWithLGTM, + reviews: approvingReviews, + commits: incorrectGitConfigCommits, + collaborators, + authorIsNew: () => true + }; + + const checker = new PRChecker(cli, data, argv); + const status = checker.checkGitConfig(); + + assert.deepStrictEqual(status, false); + cli.assertCalledWith(expectedLogs); + }); + + it('should return the status of true if user had correct config', () => { + const cli = new TestCLI(); + const expectedLogs = {}; + + const data = { + pr: firstTimerPrivatePR, + reviewers: allGreenReviewers, + comments: commentsWithLGTM, + reviews: approvingReviews, + commits: simpleCommits, + collaborators, + authorIsNew: () => true + }; + + const checker = new PRChecker(cli, data, argv); + const status = checker.checkGitConfig(); + + assert(status); + cli.assertCalledWith(expectedLogs); + }); + }); + describe('checkCommitsAfterReview', () => { let cli = new TestCLI();