Skip to content
This repository has been archived by the owner on Feb 2, 2021. It is now read-only.

updates per issue 24 and associated bug fixes #26

Closed
wants to merge 8 commits into from

Conversation

galgeek
Copy link
Contributor

@galgeek galgeek commented Dec 18, 2014

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
Copy link
Contributor

whimboo commented Dec 18, 2014

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

@@ -5,7 +5,7 @@ var irc = require('irc')
var ircServer = 'irc.mozilla.org',
nick = '_TestDayBot',
options = {
channels: ['#testday'],
channels: ['#testdaybotTest'], // for testing; to be run in #qa?
Copy link
Contributor

Choose a reason for hiding this comment

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

For the landing I need the real channel used. This will be #qa, right.

@whimboo
Copy link
Contributor

whimboo commented Dec 18, 2014

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
Copy link
Contributor Author

galgeek commented Dec 18, 2014

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

@@ -18,135 +18,215 @@ var ircServer = 'irc.mozilla.org',
usersTalked: {},
hourUTC: {},
},
RUNNING_TIME = 1000 * 60 * 60 * 20;
testDay = false,
testDayAdmins = ["ashughes", "whimboo", "galgeek"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Those names are about to go away once we are done with most of the refactoring. All that information should then be stored in a file on disk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@whimboo – I understood from your comment here that you wanted to implement a config file like the one that fennecbot uses. It was an easy update.

@whimboo
Copy link
Contributor

whimboo commented Dec 19, 2014

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

@galgeek
Copy link
Contributor Author

galgeek commented Dec 19, 2014

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
Copy link
Contributor

whimboo commented Dec 22, 2014

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. :)

etherpad = "",
testDay = false,
testDayAdmins = config.testDayAdmins,
helpers = config.helpers,
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think that we should store the helpers in the config file. That setting needs to be reset for each testday to the known admins and can then be extended. For now lets keep but please file a new issue for that.

@galgeek
Copy link
Contributor Author

galgeek commented Dec 22, 2014

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
Copy link
Contributor

whimboo commented Dec 22, 2014

@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
Copy link
Contributor

whimboo commented Dec 22, 2014

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 81b076a. Thanks Barbara!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants