From a205154bf6e14d4c915cb01d332dadc2675abd20 Mon Sep 17 00:00:00 2001 From: John Karahalis Date: Thu, 21 Jan 2016 09:19:24 -0500 Subject: [PATCH 1/3] Fix #79: Never post the same comment twice --- migrations/20160121091841-create-comment.js | 39 ++++++++ models/comment.js | 17 ++++ routes/hook/index.js | 105 ++++++++++++++------ 3 files changed, 131 insertions(+), 30 deletions(-) create mode 100644 migrations/20160121091841-create-comment.js create mode 100644 models/comment.js diff --git a/migrations/20160121091841-create-comment.js b/migrations/20160121091841-create-comment.js new file mode 100644 index 0000000..6aacd2d --- /dev/null +++ b/migrations/20160121091841-create-comment.js @@ -0,0 +1,39 @@ +'use strict'; +module.exports = { + up: function(queryInterface, Sequelize) { + return queryInterface.createTable('Comments', { + id: { + allowNull: false, + autoIncrement: true, + primaryKey: true, + type: Sequelize.INTEGER + }, + repo: { + type: Sequelize.TEXT + }, + pr: { + type: Sequelize.INTEGER + }, + filename: { + type: Sequelize.TEXT + }, + line: { + type: Sequelize.INTEGER + }, + feature: { + type: Sequelize.TEXT + }, + createdAt: { + allowNull: false, + type: Sequelize.DATE + }, + updatedAt: { + allowNull: false, + type: Sequelize.DATE + } + }); + }, + down: function(queryInterface, Sequelize) { + return queryInterface.dropTable('Comments'); + } +}; diff --git a/models/comment.js b/models/comment.js new file mode 100644 index 0000000..726f0b9 --- /dev/null +++ b/models/comment.js @@ -0,0 +1,17 @@ +'use strict'; +module.exports = function(sequelize, DataTypes) { + var Comment = sequelize.define('Comment', { + repo: DataTypes.TEXT, + pr: DataTypes.INTEGER, + filename: DataTypes.TEXT, + line: DataTypes.INTEGER, + feature: DataTypes.TEXT + }, { + classMethods: { + associate: function(models) { + // associations can be defined here + } + } + }); + return Comment; +}; diff --git a/routes/hook/index.js b/routes/hook/index.js index a704c50..965f6e2 100644 --- a/routes/hook/index.js +++ b/routes/hook/index.js @@ -70,36 +70,51 @@ function processPullRequest(destinationRepo, originRepo, originBranch, prNumber, processor.process(githubClient, originRepo, originBranch, file, discordConfig, function(incompatibility) { // Callback for handling an incompatible line of code var line = diff.lineToIndex(file.patch, incompatibility.usage.source.start.line); - var comment = incompatibility.featureData.title + ' not supported by: ' + incompatibility.featureData.missing; - - var redisQueue = kue.createQueue({ - redis: config.redisURL - }); - - // Create a Redis job that will submit the comment - var commentJob = redisQueue.create('comment', { - commentURL: commentURL, - sha: currentCommit.sha, - filename: file.filename, - line: line, - comment: comment - }); - - // If the comment is rejected, re-attempt several times with - // exponentially longer waits between each attempt. - commentJob.attempts(config.commentAttempts); - commentJob.backoff({ - type: 'exponential' - }); - - // If the comment is rejected after several attempts, log an - // error. - commentJob.on('failed', function() { - logger.error('Error posting comment to line ' + line + ' of ' + file.filename + ' in ' + originRepo + ' pull request #' + prNumber); - }); - - commentJob.save(function(error) { - if (error) return logger.error(error); + var filename = file.filename; + var feature = incompatibility.featureData.title; + var comment = feature + ' not supported by: ' + incompatibility.featureData.missing; + + var alreadyCommented = commentExists(destinationRepo, prNumber, filename, line, feature); + alreadyCommented.then(function(alreadyCommented) { + if (!alreadyCommented) { + var redisQueue = kue.createQueue({ + redis: config.redisURL + }); + + // Create a Redis job that will submit the comment + var commentJob = redisQueue.create('comment', { + commentURL: commentURL, + sha: currentCommit.sha, + filename: filename, + line: line, + comment: comment + }); + + // If the comment is rejected, re-attempt several times with + // exponentially longer waits between each attempt. + commentJob.attempts(config.commentAttempts); + commentJob.backoff({ + type: 'exponential' + }); + + // If the comment is rejected after several attempts, log an + // error. + commentJob.on('failed', function() { + logger.error('Error posting comment to line ' + line + ' of ' + filename + ' in ' + originRepo + ' pull request #' + prNumber); + }); + + commentJob.save(function(error) { + if (error) return logger.error(error); + }); + + models.Comment.create({ + repo: destinationRepo, + pr: prNumber, + filename: filename, + line: line, + feature: feature + }); + } }); }); @@ -177,4 +192,34 @@ function getCommitDetail(repo, sha) { return deferred.promise; } +function commentExists(repo, prNumber, filename, line, feature) { + var deferred = Q.defer(); + + models.Comment.count({ + where: { + repo: repo, + pr: prNumber, + filename: filename, + line: line, + feature: feature + } + }).then( + // Success callback (a matching comment was found) + function(count) { + if (count > 0) { + deferred.resolve(true); + } else { + deferred.resolve(false); + } + }, + + // Failure callback (no matching comments were found) + function(error) { + deferred.reject(error); + } + ); + + return deferred.promise; +} + module.exports = router; From 61e97ebebe435c8cc86ea2e34306b9af8000eab3 Mon Sep 17 00:00:00 2001 From: groovecoder Date: Thu, 21 Jan 2016 10:02:22 -0600 Subject: [PATCH 2/3] move setupNock to test-utils --- tests/test-utils.js | 17 +++++++++++++++++ tests/tests.js | 33 +++++++++++---------------------- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/tests/test-utils.js b/tests/test-utils.js index 5ee3a89..c85cb99 100644 --- a/tests/test-utils.js +++ b/tests/test-utils.js @@ -3,6 +3,8 @@ var fs = require('fs'); var path = require('path'); +var nock = require('nock'); + var config = require('../lib/config'); module.exports = { @@ -32,4 +34,19 @@ module.exports = { return JSON.parse(fs.readFileSync(directory + '/' + fixture + '.json')); }, + // Utility to setup the nock and lock variables into place + setupNock: function(url, item, requestType, payload, manifest, done) { + var completedPosts = 0; + + nock(this.githubHost).persist()[requestType](url).reply(function() { + if (requestType === 'post') completedPosts++; + + if (completedPosts === manifest.posts) { + done(); + nock.cleanAll(); // Cleanup so there's no interfering with other tests + } + + return [200, payload]; + }); + } }; diff --git a/tests/tests.js b/tests/tests.js index 2de8d91..d229c09 100644 --- a/tests/tests.js +++ b/tests/tests.js @@ -7,14 +7,15 @@ var fs = require('fs'); var chai = require('chai'); var request = require('request'); -var nock = require('nock'); var assert = chai.assert; -var testUtils = require('./test-utils'); +var commenter = require('../lib/commenter'); var config = require('../lib/config'); var www = require('../bin/www'); var models = require('../models'); +var testUtils = require('./test-utils'); + var notFoundURL = testUtils.appHost + '/page-that-will-never-exist'; describe('Discord Tests', function() { @@ -100,34 +101,19 @@ describe('Discord Tests', function() { item = manifest.urls[url]; - setupNock( + testUtils.setupNock( url, item, item.method.toLowerCase(), testUtils.getFileContents(testUtils.recordedFixturesDir + plainIndex.toString(), item.file), - manifest + manifest, + done ); } } // Kick the test off sendHookPayload(testUtils.recordedFixturesDir + plainIndex.toString()); - - // Utility to setup the nock and lock variables into place - function setupNock(url, item, requestType, payload, manifest) { - var completedPosts = 0; - - nock(testUtils.githubHost).persist()[requestType](url).reply(function() { - if (requestType === 'post') completedPosts++; - - if (completedPosts === manifest.posts) { - done(); - nock.cleanAll(); // Cleanup so there's no interfering with other tests - } - - return [200, payload]; - }); - } }); }); @@ -150,6 +136,7 @@ describe('Discord Tests', function() { */ describe('Database Tests', function() { describe('Ping', function() { + it('Ping events are recorded', function(done) { models.Ping.count().then(function(countBeforePing) { assert.equal(countBeforePing, 0); @@ -195,11 +182,13 @@ describe('Discord Tests', function() { }); }); }); - }); - }); + + }); // End Ping tests + }); // End Database tests /** * We need to do cleanup so that the tests don't hang + * TODO: move test cleanup to a "tearDown" method? */ describe('Test Cleanup', function() { it('Server closes properly', function() { From 74339b8fd4770333f75c4a54274fa3f7e95089b0 Mon Sep 17 00:00:00 2001 From: groovecoder Date: Thu, 21 Jan 2016 10:32:28 -0600 Subject: [PATCH 3/3] new setupNocksForManifest in test-utils --- tests/test-utils.js | 25 +++++++++++++++++++++++-- tests/tests.js | 19 +------------------ 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/tests/test-utils.js b/tests/test-utils.js index c85cb99..3cceec3 100644 --- a/tests/test-utils.js +++ b/tests/test-utils.js @@ -35,18 +35,39 @@ module.exports = { }, // Utility to setup the nock and lock variables into place - setupNock: function(url, item, requestType, payload, manifest, done) { + setupNock: function(url, item, requestType, payload, posts, done) { var completedPosts = 0; nock(this.githubHost).persist()[requestType](url).reply(function() { if (requestType === 'post') completedPosts++; - if (completedPosts === manifest.posts) { + if (completedPosts === posts) { done(); nock.cleanAll(); // Cleanup so there's no interfering with other tests } return [200, payload]; }); + }, + + // Utility to setup all nocks for a manifest + setupNocksForManifest: function(manifest, index, done) { + var item; + for (var url in manifest.urls) { + if (manifest.urls.hasOwnProperty(url)) { + + item = manifest.urls[url]; + + this.setupNock( + url, + item, + item.method.toLowerCase(), + this.getFileContents(this.recordedFixturesDir + index.toString(), item.file), + manifest.posts, + done + ); + } + } + } }; diff --git a/tests/tests.js b/tests/tests.js index d229c09..a7e7c6f 100644 --- a/tests/tests.js +++ b/tests/tests.js @@ -9,7 +9,6 @@ var chai = require('chai'); var request = require('request'); var assert = chai.assert; -var commenter = require('../lib/commenter'); var config = require('../lib/config'); var www = require('../bin/www'); var models = require('../models'); @@ -94,23 +93,7 @@ describe('Discord Tests', function() { // Start the test definition it('Recorded test ' + plainIndex + ': ' + manifest.description, function(done) { - var item; - - for (var url in manifest.urls) { - if (manifest.urls.hasOwnProperty(url)) { - - item = manifest.urls[url]; - - testUtils.setupNock( - url, - item, - item.method.toLowerCase(), - testUtils.getFileContents(testUtils.recordedFixturesDir + plainIndex.toString(), item.file), - manifest, - done - ); - } - } + testUtils.setupNocksForManifest(manifest, plainIndex, done); // Kick the test off sendHookPayload(testUtils.recordedFixturesDir + plainIndex.toString());