Extract event and pull request filtering and attempt to make robust

This is not plain refactoring, as the `state == "closed"` condition
replaces the check on `merged`. It also ensures that we perform the
same checks both on the event payload and on the response from
https://developer.github.com/v3/pulls/#get-a-single-pull-request.

Fixes #38.
Fixes #69.

index.js

@@ -10,6 +10,7 @@ var express = require("express"),
     github = require('./lib/github'),
     checkRequest = require('./lib/check-request'),
     epochs = require('./lib/epochs'),
+    filter = require('./lib/filter'),
     q = require('q');
 
 function promise(value) {
@@ -70,22 +71,17 @@ app.post('/github-hook', function (req, res, next) {
             } catch(e) {
                 return;
             }
-            if (!body) {
-                logArgs("Ignoring event: empty body");
-                return;
-            }
-            if (body.sender && body.sender.login == "wpt-pr-bot") {
-                logArgs("Ignoring event: sender is wpt-pr-bot");
+            if (!filter.event(body, logArgs)) {
                 return;
             }
             if (!body.pull_request && (body.issue && !body.issue.pull_request)) {
                 logArgs("Ignoring event: not a pull request");
                 return;
             }
-            if (body.pull_request && body.pull_request.draft) {
-                logArgs("Ignoring event: pull request is a draft");
+            if (!filter.pullRequest(pull_request, logArgs)) {
                 return;
             }
+            // Note: `filter.pullRequest` is checked again after a timeout.
             // END FILTERNG //
 
             var action = body.action;
@@ -95,7 +91,6 @@ app.post('/github-hook', function (req, res, next) {
             var u = (issue.user && issue.user.login) || null;
             var content = issue.body || "";
             if (!isComment && action == "edited") {
-                if (body.pull_request.merged) return; // we know .pullrequest is available because it's not a comment.
                 logArgs("#" + n, "pull request edited");
                 metadata(n, u, content).then(function(metadata) {
                     return removeReviewableBanner(n, metadata);
@@ -113,7 +108,9 @@ app.post('/github-hook', function (req, res, next) {
                 waitFor(5 * 1000).then(function() { // Avoid race condition
                     return getPullRequest(n, body);
                 }).then(function(pull_request) {
-                    if (pull_request.merged) return;
+                    if (!filter.pullRequest(pull_request, logArgs)) {
+                        return;
+                    }
                     return metadata(n, u, content).then(function(metadata) {
                         logArgs(metadata);
                         return labelModel.post(n, metadata.labels).then(

lib/filter.js

@@ -0,0 +1,32 @@
+"use strict";
+
+// Filter functions for GitHub API responses. Returns true if the entity passes
+// the filter, like a `Array.prototype.filter` callback. The `log` callback is
+// called with any console messages that should be logged.
+
+function filterEvent(body, log) {
+    if (!body) {
+        log("Ignoring event: empty body");
+        return false;
+    }
+    if (body.sender && body.sender.login == "wpt-pr-bot") {
+        log("Ignoring event: sender is wpt-pr-bot");
+        return false;
+    }
+    return true;
+}
+
+function filterPullRequest(pull_request, log) {
+    if (pull_request.state == "closed") {
+        log(`Ignoring #${pull_request.number}: pull request is closed`);
+        return false;
+    }
+    if (pull_request.draft) {
+        log(`Ignoring #${pull_request.number}: pull request is a draft`);
+        return false;
+    }
+    return true;
+}
+
+exports.event = filterEvent;
+exports.pullRequest = filterPullRequest;

test/filter.js

@@ -0,0 +1,55 @@
+"use strict";
+
+const assert = require('assert'),
+      filter = require('../lib/filter');
+
+function nullLog() {
+    // Do nothing. Log messages could be verified but aren't.
+}
+
+suite('Event filtering', function() {
+    test('null body', function() {
+        assert.strictEqual(filter.event(null, nullLog), false);
+    });
+    test('empty body', function() {
+        assert.strictEqual(filter.event({}, nullLog), true);
+    });
+    test('wpt-pr-bot sender', function() {
+        assert.strictEqual(filter.event({
+            sender: {
+                login: 'wpt-pr-bot'
+            }
+        }, nullLog), false);
+    });
+    test('other sender', function() {
+        assert.strictEqual(filter.event({
+            sender: {
+                login: 'some-other-user'
+            }
+        }, nullLog), true);
+    });
+
+});
+
+suite('Pull request filtering', function() {
+    test('null payload', function() {
+        assert.throws(function() {
+            filter.pullRequest(null, nullLog);
+        });
+    });
+    test('empty payload', function() {
+        assert.strictEqual(filter.pullRequest({}, nullLog), true);
+    });
+    test('closed pull request', function() {
+        assert.strictEqual(filter.pullRequest({state: "closed"}, nullLog), false);
+    });
+    test('open pull request', function() {
+        assert.strictEqual(filter.pullRequest({state: "open"}, nullLog), true);
+    });
+    test('draft pull request', function() {
+        assert.strictEqual(filter.pullRequest({draft: true}, nullLog), false);
+    });
+    test('explicitly non-draft pull request', function() {
+        assert.strictEqual(filter.pullRequest({draft: false}, nullLog), true);
+    });
+});