Skip to content

Commit

Permalink
Fix groovecoder#79: Never post the same comment twice
Browse files Browse the repository at this point in the history
  • Loading branch information
openjck committed Jan 26, 2016
1 parent c3459bb commit 78f18d5
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 24 deletions.
92 changes: 70 additions & 22 deletions lib/commenter.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,39 +5,87 @@
* create_pull_request_comment.
*/

var Q = require('q');
var request = require('request');

var config = require('./config');
var models = require('../models');

/**
* Post a line comment on a particular pull request. commentURL should be an
* instance of review_comments_url.
* https://developer.github.com/v3/activity/events/types/#pullrequestevent
*/
function postPullRequestComment(commentURL, comment, filename, commitSHA, line, done) {

request({
url: commentURL,
method: 'POST',
headers: {
'User-Agent': config.brand,
'Authorization': 'token ' + config.token
},
body: JSON.stringify({
body: comment,
path: filename,
commit_id: commitSHA,
position: line
})
}, function(error, response) {
// If the comment could not be submitted, notify Redis so that the job
// can be re-attempted later. Otherwise, mark the job as done.
if (error || response.statusCode === 403) {
return done(new Error('Comment could not be submitted'));
} else {
done();
function postPullRequestComment(commentURL, commitSHA, repo, pr, filename, line, comment, incompatibleFeature, done) {
var alreadyCommented = commentExists(repo, pr, filename, line, incompatibleFeature);
alreadyCommented.then(function(alreadyCommented) {
if (!alreadyCommented) {
request({
url: commentURL,
method: 'POST',
headers: {
'User-Agent': config.brand,
'Authorization': 'token ' + config.token
},
body: JSON.stringify({
body: comment,
path: filename,
commit_id: commitSHA,
position: line
})
}, function(error, response) {
// If the comment could not be submitted, notify Redis so that the job
// can be re-attempted later. Otherwise, mark the job as done.
if (error || response.statusCode === 403) {
return done(new Error('Comment could not be submitted'));
} else {
models.Comment.create({
repo: repo,
pr: pr,
filename: filename,
line: line,
feature: incompatibleFeature
}).then(function() {
done();
});
}
});

}
});
}

/**
* Return true if a comment has already been posted about this incompatibility.
*/
function commentExists(repo, pr, filename, line, incompatibleFeature) {
var deferred = Q.defer();

models.Comment.count({
where: {
repo: repo,
pr: pr,
filename: filename,
line: line,
feature: incompatibleFeature
}
}).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;
}

exports.postPullRequestComment = postPullRequestComment;
39 changes: 39 additions & 0 deletions migrations/20160121091841-create-comment.js
Original file line number Diff line number Diff line change
@@ -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');
}
};
17 changes: 17 additions & 0 deletions models/comment.js
Original file line number Diff line number Diff line change
@@ -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;
};
5 changes: 4 additions & 1 deletion routes/hook/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,12 @@ function processPullRequest(destinationRepo, originRepo, originBranch, prNumber,
var commentJob = redisQueue.create('comment', {
commentURL: commentURL,
sha: currentCommit.sha,
repo: destinationRepo,
pr: prNumber,
filename: file.filename,
line: line,
comment: comment
comment: comment,
incompatibleFeature: incompatibility.featureData.title
});

// If the comment is rejected, re-attempt several times with
Expand Down
2 changes: 1 addition & 1 deletion worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ redisQueue.process('comment', function(job, done) {
}

setTimeout(function() {
commenter.postPullRequestComment(job.data.commentURL, job.data.comment, job.data.filename, job.data.sha, job.data.line, done);
commenter.postPullRequestComment(job.data.commentURL, job.data.sha, job.data.repo, job.data.pr, job.data.filename, job.data.line, job.data.comment, job.data.incompatibleFeature, done);
lastCommentTimestamp = Date.now(); // Use a new, fresh timestamp
}, waitRemaining);
});

0 comments on commit 78f18d5

Please sign in to comment.