-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Ignore edits to issues predating the bot #13460
Conversation
Should fix the scenario described in #10843 (comment). |
/cc @Hainish @koops76 |
@@ -20,6 +20,13 @@ module.exports = function(robot, alexa) { | |||
return; | |||
} | |||
|
|||
const createdAt = new Date(context.payload.issue.created_at); | |||
// September 25th, 2017 | |||
if (createdAt.getFullYear() <= 2017 && createdAt.getMonth() <= 8 && createdAt.getDate() <= 25) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conditional will fail for an issue created in September, 2016.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleanest way to do this without a long conditional is probably formatted string comparison. Munge createdAt
into YYYY-MM-DD
format, then compare it to "2017-08-25"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we compare Date
s directly, or at least by conversion to timestamps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hainish Better way is to compare dates directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hainish nice catch. Will push a fix based on #13460 (comment)
@@ -20,6 +20,13 @@ module.exports = function(robot, alexa) { | |||
return; | |||
} | |||
|
|||
const createdAt = new Date(context.payload.issue.created_at); | |||
// September 25th, 2017 | |||
if (createdAt.getFullYear() <= 2017 && createdAt.getMonth() <= 8 && createdAt.getDate() <= 25) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const botCreatedDate = new Date(2017, 10, 25)
if (createdAt <= botCreatedDate) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks! I had no idea you could compare Date
s like this; I was gonna compare Unix timestamps.
b6232e9
to
1fb5445
Compare
Fixed the date comparison and rebased on master. |
Also pushed an unrelated test suite improvement that didn't seem worth sending a separate PR for. |
No description provided.