Fix the bot commenting on edits more than once #13294
Conversation
Note: this does the *bare minimum* to make tests pass. This change has no real test coverage, but this bug is causing real pain and I _did_ test it manually in production so it'll have to do for now. Fixes EFForg#12855
There was a problem hiding this comment.
This seems reasonable as a quick patch, provided you have tested it appropriately. What would be preferable when you revisit this issue, is for the bot to edit it's original comment with any additional error messages, and for it to delete it's comment when all errors are resolved.
|
|
||
| if (commentedBefore) return; | ||
|
|
||
| const params = context.issue({body: 'Thanks! Your edit helped me out. I\'ll take it from here now. <!-- HELPED_COMMENT_POSTED -->'}); |
There was a problem hiding this comment.
const params = context.issue({ body: "Thanks! Your edit helped me out. I'll take it from here now. <!-- HELPED_COMMENT_POSTED -->" });
There was a problem hiding this comment.
@koops76 what's the difference, besides the quotes? I've consistently been using single quotes.
There was a problem hiding this comment.
It's ok to use double quotes to avoid escaping
| return { | ||
| data: [] | ||
| }; | ||
| }, | ||
| addLabels: sinon.spy(), | ||
| removeLabel: sinon.spy(() => new Promise(noop)) |
There was a problem hiding this comment.
removeLabel: sinon.spy(() => Promise.resolve())?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
new Promise(noop) never resolves since resolve is never called.
| })); | ||
| const allComments = _allComments.data; | ||
|
|
||
| allComments.forEach(comment => { |
| // | ||
| // 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({ |
There was a problem hiding this comment.
const allComments = (await context.github.issues.getComments(context.issue({
per_page: 100, // eslint-disable-line camelcase
since: '2017-09-25'
}))).data;
There was a problem hiding this comment.
I did this originally but decided the other way made what was happening clearer. Your version is super dense :)
There was a problem hiding this comment.
At least call it allCommentsData instead of the same name but without an underscore.
|
Thanks, this looks good to me. Merging and deploying fix now. |
Note: this does the bare minimum to make tests pass. This change has no real test coverage, but this bug is causing real pain and I did test it manually in production so it'll have to do for now. At some point I'll send a followup PR adding the missing coverage.
Fixes #12855