search

mozilla / testdaybot

Mozilla QA Test Day IRC Bot
9 stars 12 forks source link

updates per issue 24 and associated bug fixes #26

Closed galgeek closed 9 years ago

galgeek commented 9 years ago

Hi @whimboo I've re-packaged most of my earlier updates and more for issue 24 in this branch, in what I hope to be an easier to review package. It still includes a lot, but the only part that's not really issue 24 is fixing the existing stats, which otherwise crash the bot. This version of the bot is currently running on #testdaybotTest, /msg _TestDayBot :adminhelp for admin commands. It should be fine to close my earlier pull request unmerged. Thanks for your patience!

whimboo commented 9 years ago

Barbara, I will have a look soon. For now, you can close pull requests on your own.

whimboo commented 9 years ago

Still a huge PR. Lets make smaller increments in the future. Otherwise please check my comments and get them fixed. I still would like to get it landed tomorrow. Thanks.

galgeek commented 9 years ago

hi @whimboo! I've pushed today's updates, which should address your comments here.

whimboo commented 9 years ago

With the remaining comments implemented we are good to get it landed.

galgeek commented 9 years ago

hi @whimboo !

Thank you for your feedback!

I've updated as best I can. I've sadly discovered that :addAdmin with current verification check crashes the bot when given a nick that is not currently in use, and I haven't yet figured out how to fix that.

Other notes: I set up a config file, similar to fennecbot's, stopped expecting/accepting etherpad on command line + updated README, removed resetTime() after tweaking :stop command, set up admin checks on each admin command so that other pm's to bot don't result in "you're not an admin" message.

whimboo commented 9 years ago

Barbara, thanks for the update. I will have a look now. But one heads-up... For any remaining issue on this PR please really stop implementing new features like the config file. As I have said earlier I want to get this PR merged, but new features always defer the landing due to other introduced issues. Please keep code changes as small as possible. I don't want a massive PR which fixes all the P1 issues. :)

galgeek commented 9 years ago

Here's the error that's output currently when you :addAdmin a nick who's not present on IRC, and that crashes the bot:

{ prefix: 'belew.mozilla.org', server: 'belew.mozilla.org', command: 'err_nosuchnick', rawCommand: '401', commandType: 'error', args: [ '_TestDayBot', 'AutomatedTester', 'No such nick/channel' ] }

/Users/bara/Dev/testdaybot/node_modules/irc/lib/irc.js:520 throw err; ^ TypeError: Cannot read property 'nick' of undefined at Client.callbackWrapper (/Users/bara/Dev/testdaybot/node_modules/irc/lib/irc.js:666:22) at Client.emit (events.js:95:17) at Client. (/Users/bara/Dev/testdaybot/node_modules/irc/lib/irc.js:253:22) at Client.emit (events.js:95:17) at /Users/bara/Dev/testdaybot/node_modules/irc/lib/irc.js:517:22 at Array.forEach (native) at Socket. (/Users/bara/Dev/testdaybot/node_modules/irc/lib/irc.js:514:15) at Socket.emit (events.js:95:17) at Socket. (_stream_readable.js:764:14) at Socket.emit (events.js:92:17)

whimboo commented 9 years ago

@galgeek please file the error as separate issue as I have pointed out earlier, and reference this PR by adding PR #26 in its initial comment. So we have the cross-link.

whimboo commented 9 years ago

I squashed all the commits into a single one and updated the commit message to better reflect what those changes are for. The PR has been merged as https://github.com/mozilla/testdaybot/commit/81b076a6a67dcffd2bf4bdc42ae66c4d0d281bb4. Thanks Barbara!