Make time errors more explicit when setting the ":next" Testday #77

xabolcs · 2015-02-18T22:59:39Z

Fix for #74.

…next" Testday

xabolcs · 2015-02-18T23:01:29Z

Example talk with my testdaybot named xabolcs_tdb:

[23:46:38]  xabolcs :next
[23:46:39]  xabolcs_tdb Need some help? :next <start as YYYY-MM-DDThh:mmZ> <end as YYYY-MM-DDThh:mmZ> <etherpad> <topic>: set next Test Day
[23:49:41]  xabolcs :next 2014-12-13T23:30Z 2013-12-13T23:45Z etherpad topic
[23:49:41]  xabolcs_tdb Please use valid dates.
[23:49:41]  xabolcs_tdb Start time was set after End time.
[23:50:09]  xabolcs :next 2014-12-13T23:30Z 2014-12-13T23:45Z etherpad topic
[23:50:10]  xabolcs_tdb Please use valid dates.
[23:50:10]  xabolcs_tdb Start time is set in the past.
[23:50:29]  xabolcs :next 2015-12-13T23:30Z 2014-12-13T23:45Z etherpad topic
[23:50:30]  xabolcs_tdb Please use valid dates.
[23:50:30]  xabolcs_tdb Start time was set after End time.
[23:50:35]  xabolcs :next 2015-12-13T23:30Z 2015-12-13T23:45Z etherpad topic
[23:50:36]  xabolcs_tdb Next Test Day's start is Sun, 13 Dec 2015 23:30:00 GMT
[23:50:36]  xabolcs_tdb Next Test Day's end is Sun, 13 Dec 2015 23:45:00 GMT
[23:50:36]  xabolcs_tdb Next Test Day's etherpad is etherpad
[23:50:36]  xabolcs_tdb Next Test Day's topic is topic

xabolcs · 2015-02-18T23:03:25Z

Interestingly there was a message in the channel after setting up a test day. Was it expected?

[23:50:36]  xabolcs_tdb The topic testday starts now. For details, see etherpad

xabolcs · 2015-02-20T08:01:42Z

bot.js

@@ -418,6 +418,11 @@ client.addListener('pm', function(from, message) { // private messages to bot
              client.say(from, "Next Test Day's topic is " + testDay.topic);
            } else {
              client.say(from, "Please use valid dates.");
+              if (testDay.end < testDay.start) {
+                client.say(from, "Start time was set after End time.");
+              } else {


Other option here is using if to print all errors at once.

@whimboo, should I go that way? Currently only one error is printed.

An advanced solution would be to count all the errors and print a it like "Your specified a time interval which has the following 2 errors: the start time is after end time, the start time is in the past."

Thoughts?

You could use a string variable for the client.say call in line 420, and then just combine the text for the individual error message like "Please use valid dates: %error%". That way we also only send a single message to the user.

I don't think the extended logic is that more helpful right now. Personally I would move those sanity checks for times up before line 410, so we do not have to check for the dates twice but return early.

Personally I would move those sanity checks for times up before line 410, so we do not have to check for the dates twice but return early.

I happily do this move in this PR, should I?

That would be perfect.

xabolcs · 2015-02-22T23:41:53Z

~~This is ready to review!~~

…e error messages

xabolcs · 2015-02-28T13:07:17Z

Pushed commits to PR's branch including 45f4fc9.
Ready to review.

Please see the following conversation!


URL     ircs://moznet/xabolcs_tdb,isnick    Connected via   fripp.mozilla.org
Conversation with xabolcs_tdb <nodebot@moz-fci6nk.catv.broadband.hu>
nodeJS IRC client
xabolcs_tdb
[13:54:06]  [INFO]  Messages Cleared.
[13:54:11]  xabolcs :next
[13:54:12]  xabolcs_tdb Need some help? :next <start as YYYY-MM-DDThh:mmZ> <end as YYYY-MM-DDThh:mmZ> <etherpad> <topic>: set next Test Day
[13:55:46]  xabolcs :next 2014-03-04T11:00Z 2014-03-04T11:30Z https://etherpad.location.url TestDayBot Test Day
[13:55:46]  xabolcs_tdb Please use valid dates: start time is set in the past.
[13:56:01]  xabolcs :next 2014-03-04T11:00Z 2014-03-04T10:30Z https://etherpad.location.url TestDayBot Test Day
[13:56:02]  xabolcs_tdb Please use valid dates: start time is set after End time, start time is set in the past.
[13:56:24]  xabolcs :next 2015-03-04T11:00Z 2014-03-04T10:30Z https://etherpad.location.url TestDayBot Test Day
[13:56:24]  xabolcs_tdb Please use valid dates: start time is set after End time.
[13:56:43]  xabolcs :next 2015-03-04T11:00Z 2015-03-04T11:30Z https://etherpad.location.url TestDayBot Test Day
[13:56:43]  xabolcs_tdb Next Test Day's start is Wed, 04 Mar 2015 11:00:00 GMT
[13:56:43]  xabolcs_tdb Next Test Day's end is Wed, 04 Mar 2015 11:30:00 GMT
[13:56:43]  xabolcs_tdb Next Test Day's etherpad is https://etherpad.location.url
[13:56:43]  xabolcs_tdb Next Test Day's topic is TestDayBot Test Day

xabolcs · 2015-02-28T19:04:44Z

bot.js

              if (timerID !== 0) {
                clearTimeout(timerID);
              }
+              testDay.start = new Date(command[1]);
+              testDay.end = new Date(command[2]);


Ahh, these should be startTime and endTime.

xabolcs · 2015-03-02T23:09:13Z

This is ready to review!

whimboo · 2015-03-04T10:57:05Z

bot.js

-            testDay.topic = command.slice(4, cmdLen).join(" ");
-            // if the start and end dates appear valid, set the test date
-            if ((testDay.end > testDay.start) && (testDay.start > Date.now())) {
+            var startTime, endTime, dateErrors = [];


Do not declare multiple variables in a single line. So move the declaration for startTime and endTime to the lines 406 and 407.

xabolcs · 2015-03-10T00:31:00Z

This is ready to review up to commit b14a8a0.

whimboo · 2015-03-10T08:40:23Z

This looks fine! I got it merged as 5a749f9. Thanks for your contribution.

Issue mozilla#74 - Make time errors more explicit when setting the ":…

e317084

…next" Testday

xabolcs reviewed Feb 20, 2015
View reviewed changes

whimboo assigned xabolcs Feb 26, 2015

Issue mozilla#74 - Move date sanity checks before line 410 and combin…

45f4fc9

…e error messages

xabolcs reviewed Feb 28, 2015
View reviewed changes

Issue mozilla#74 - Don't parse again the dates

0f55f87

whimboo reviewed Mar 4, 2015
View reviewed changes

Issue mozilla#74 - multi line define, newline separator

b14a8a0

whimboo closed this Mar 10, 2015

whimboo mentioned this pull request Mar 10, 2015

Improve time handling in :next command #74

Closed

xabolcs deleted the branch-issue-74-next-more-explicit-errors branch March 10, 2015 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make time errors more explicit when setting the ":next" Testday #77

Make time errors more explicit when setting the ":next" Testday #77

xabolcs commented Feb 18, 2015

xabolcs commented Feb 18, 2015

xabolcs commented Feb 18, 2015

xabolcs Feb 20, 2015

whimboo Feb 26, 2015

xabolcs Feb 26, 2015

whimboo Feb 26, 2015

xabolcs commented Feb 22, 2015

xabolcs commented Feb 28, 2015

xabolcs Feb 28, 2015

xabolcs commented Mar 2, 2015

whimboo Mar 4, 2015

xabolcs commented Mar 10, 2015

whimboo commented Mar 10, 2015

Make time errors more explicit when setting the ":next" Testday #77

Make time errors more explicit when setting the ":next" Testday #77

Conversation

xabolcs commented Feb 18, 2015

xabolcs commented Feb 18, 2015

xabolcs commented Feb 18, 2015

xabolcs Feb 20, 2015

Choose a reason for hiding this comment

whimboo Feb 26, 2015

Choose a reason for hiding this comment

xabolcs Feb 26, 2015

Choose a reason for hiding this comment

whimboo Feb 26, 2015

Choose a reason for hiding this comment

xabolcs commented Feb 22, 2015

xabolcs commented Feb 28, 2015

xabolcs Feb 28, 2015

Choose a reason for hiding this comment

xabolcs commented Mar 2, 2015

whimboo Mar 4, 2015

Choose a reason for hiding this comment

xabolcs commented Mar 10, 2015

whimboo commented Mar 10, 2015