Skip to content
This repository has been archived by the owner on Nov 6, 2023. It is now read-only.

Fix the bot commenting on edits more than once #13294

Merged
merged 1 commit into from
Oct 25, 2017
Merged

Fix the bot commenting on edits more than once #13294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion utils/issue-format-bot/lib/issueedit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,34 @@ module.exports = function(robot, alexa) {

if (problems.length === 0) {
// User submission is OK
const params = context.issue({body: 'Thanks! Your edit helped me out. I\'ll take it from here now.'});

let commentedBefore = false;

// This won't work if the bot comments success past 100 comments
// but since this will probably never happen, who cares. Also,
// the bot was originally put into production late September, so
// only ask for comments after then.
//
// XXX handle the user screwing up, then fixing it (and getting
// the success comment), then screwing up again
const _allComments = await context.github.issues.getComments(context.issue({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

const allComments = (await context.github.issues.getComments(context.issue({
  per_page: 100, // eslint-disable-line camelcase
  since: '2017-09-25'
}))).data;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this originally but decided the other way made what was happening clearer. Your version is super dense :)

Copy link

@ghost ghost Oct 25, 2017 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least call it allCommentsData instead of the same name but without an underscore.

per_page: 100, // eslint-disable-line camelcase
since: '2017-09-25'
}));
const allComments = _allComments.data;

allComments.forEach(comment => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for (const comment of allComments) {

// 'Your edit helped me out' is here to match legacy comments
// before this conditional was put in place
if (comment.body.includes('HELPED_COMMENT_POSTED')
|| comment.body.includes('Your edit helped me out')) {
commentedBefore = true;
}
});

if (commentedBefore) return;

const params = context.issue({body: 'Thanks! Your edit helped me out. I\'ll take it from here now. <!-- HELPED_COMMENT_POSTED -->'});
Copy link

@ghost ghost Oct 24, 2017 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const params = context.issue({ body: "Thanks! Your edit helped me out. I'll take it from here now. <!-- HELPED_COMMENT_POSTED -->" });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koops76 what's the difference, besides the quotes? I've consistently been using single quotes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok to use double quotes to avoid escaping

await context.github.issues.createComment(params);

return labeler(context, data, alexa);
Expand Down
2 changes: 2 additions & 0 deletions utils/issue-format-bot/test/issueedit-handler-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
const vows = require('perjury'),
handlerutil = require('./lib/handlerutil');

// TODO make this test that the bot doesn't post >1 "you fixed it" comments

vows.describe('issue edit handler').addBatch(
handlerutil.setup('../../lib/issueedit', {
'and we pass it the context of an issue edit with a null body': handlerutil.nullBody('don\'t see any text'),
Expand Down
6 changes: 6 additions & 0 deletions utils/issue-format-bot/test/mocks/context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ module.exports = {
github: {
issues: {
createComment: sinon.spy(),
// TODO actually make this keep track of data
getComments: function() {
return {
data: []
};
},
addLabels: sinon.spy(),
removeLabel: sinon.spy(() => new Promise(noop))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removeLabel: sinon.spy(() => Promise.resolve())?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think new Promise(noop) makes it clearer what's going on - we're creating a new Promise that's a noop. Whereas with your example you have to know offhand what Promise.resolve() does.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new Promise(noop) never resolves since resolve is never called.

}
Expand Down