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; diff --git a/tests/test-utils.js b/tests/test-utils.js index 5ee3a89..3cceec3 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,40 @@ 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, posts, done) { + var completedPosts = 0; + + nock(this.githubHost).persist()[requestType](url).reply(function() { + if (requestType === 'post') completedPosts++; + + 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 2de8d91..a7e7c6f 100644 --- a/tests/tests.js +++ b/tests/tests.js @@ -7,14 +7,14 @@ 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 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() { @@ -93,41 +93,10 @@ 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]; - - setupNock( - url, - item, - item.method.toLowerCase(), - testUtils.getFileContents(testUtils.recordedFixturesDir + plainIndex.toString(), item.file), - manifest - ); - } - } + testUtils.setupNocksForManifest(manifest, plainIndex, 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 +119,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 +165,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() {