pr_checker: Log a error is pr author has incorrect git config.

Fixes: nodejs/node-core-utils#150

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,24 +292,30 @@ 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;
     }
 
     // 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

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
             }

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,

test/fixtures/incorrect_git_config_commits.json

@@ -0,0 +1,38 @@
+[
+  {
+    "commit": {
+      "committedDate": "2017-10-26T12:35:26Z",
+      "author": {
+        "user": null,
+        "email": "[email protected]",
+        "name": "Their Github Account email"
+      },
+      "committer": {
+        "email": "[email protected]",
+        "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": "[email protected]",
+        "name": "Their git config user.email"
+      },
+      "committer": {
+        "email": "[email protected]",
+        "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
+    }
+  }
+]
+

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();