search

mozilla / testdaybot

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

Advertise Test Day #38

Closed galgeek closed 9 years ago

galgeek commented 9 years ago

hi @whimboo !

thanks for your feedback on #35 — this patch replaces it, addressing issue #22

whimboo commented 9 years ago

Not sure why you have created a new PR for this patch, but lets close PR #35 then.

galgeek commented 9 years ago

Hi @whimboo ! Thank you for your feedback. I've updated the PR, the last of the old year. I've done a bit of testing, too. I suspect that fennecbot has some permissions set on Mozilla's irc server that testdaybot may not yet, or perhaps I've missed the place in the fennecbot code where the bot joins all the channels to which it sends Tablet Tuesday messages. I tried adding client.join code here but haven't succeeded in getting anything running satisfactorily. Thanks again!

whimboo commented 9 years ago

Beside the code comments please try to get the feedback from Anthony.

ghost commented 9 years ago

Not sure where the event organizer will be listed. Means if he/she will be an admin or a helper/moderator.

I think I'd like the "organizer" to be a separate actor in this case. It would be good at some point to be able to know who is organizing events and not just administrating or helping with an event.

@ashughes1 might be able to tell us who should be able to send those advertisements.

Leaving that responsibility to the event organizer should be sufficient for v1.

We always need fallbacks in case that the organizer is not around regarding time zones.

We're going to be trying out a new model this year where events won't go on for longer than a a couple hours. The timezone issue you speak of should be moot in that model.

galgeek commented 9 years ago

hi @ashughes1 and @whimboo

At least for v1, does the "organizer" require different testdaybot privileges from "admins"? (It does make sense to me that "organizer" should be responsible for advertising, but I wonder if this needs to be restricted in the code.)

Where do we want to specify the "organizer"? In the "next" command to schedule the Test Day, or "addOrganizer," or ...?

ghost commented 9 years ago

I don't think an "organizer" needs to have different permissions than an "admin". I would say that an "admin" should be the highest level and an organizer is just a different type of admin.

whimboo commented 9 years ago

@ashughes1 given the tight timeline for Barbara would it be ok to first let helpers allow to run the advertising command? Those are set by the admin and are under control for each testday. We could have a follow-up issue to actually implement a real organizer.

ghost commented 9 years ago

Why not just limit it to event admin then for now?

whimboo commented 9 years ago

Admins exist to administrate the bot but not to drive a testday in any given way. Because of that helpers seem to be a way better fit for me as long as we do not have the organizer user level. Also helpers should be trusted. The admin has to set them each time before a testday, so the list is not getting that large.

Also it would be limited to yourself if the admin way is used, given that we most likely don't want to make more people admins beside us two, maybe a fallback for each time zone but that should be all.

ghost commented 9 years ago

Thanks for clarifying the distinction. I'm fine with Helpers having the power to trigger advertising via the bot.

galgeek commented 9 years ago

Hello @whimboo and @ashughes1

I've updated the :advertise code to make it a helper command, and otherwise updated the code per Henrik's line notes.

The code is a pretty direct copy from fennecbot, but it doesn't manage to send messages to channels the bot hasn't joined in my testing, while I have the impression that fennecbot does. Is that because the fennecbot code that's in use differs significantly from the github repo, or it that because fennecbot has more privileges than my +opped test bot running in #testdaybotTest? Or am I wrong that fennecbot advertises on multiple channels?

Another issue is what kind of user verification to use when a helper issues the command, or when a new helper is added to the list for a test day.

whimboo commented 9 years ago

The code is a pretty direct copy from fennecbot, but it doesn't manage to send messages to channels the bot hasn't joined in my testing, while I have the impression that fennecbot does. Is that

Channels can prevent users from sending content unless they have actually joined the channel. So you will indeed make sure the bot quickly joins, sends the message, and leaves the channel. Similar to the github bot in the #automation channel.

galgeek commented 9 years ago

Hi @whimboo!

The code in this updated PR runs fine with node-irc v0.3.7 in my quick tests. I don't know whether you might want to have a look before the other PRs are landed.

I don't see any similar code in the github repo for the automation bot.

Maybe we should add subcommands to update the message or channels?

Thank you for all your help!

galgeek commented 9 years ago

Hi @whimboo!

I've left this code running, using node-irc v0.3.7, in #testdaybotTest.

qa and #automation are the advertisement channels, and the message is a short test message, "please pardon this test of an update to testdaybot!"

whimboo commented 9 years ago

This looks fine now. Given that it is blocked on version 0.3.7 I will defer its merge to default until the IRC plugin has been upgraded.

whimboo commented 9 years ago

I squashed all the commits, fixed the initial commit message, and merged the PR as https://github.com/mozilla/testdaybot/commit/f8e779cc6078e0d2d2b58642c6a64e3c356db5cf.