Trigger enter/ leave actions by presence change #364
Conversation
The current adapter implementation triggers robot.enter / robot.leave events when someone joins/ leaves a channel or group. However, most Hubot scripts use these callbacks registers with the much more frequent Campfire/ XMPP/ etc presence change events in mind (e.g. when someone logs on/ logs off). Since joining a slack channel is something you tend to do only once, or at most once in a while, the callbacks are not so very useful at present for the slack adapter. One downside to the new implementation is that because a presence change event is generic, not specific to a (Hubot) room/ (Slack) channel, there is no room in the envelope passed to your enter/ leave callback. I adapted our own scripts to use robot.messageRoom rather than envelope.reply, but am open to suggestions on how to improve this, perhaps by looking at the feasibility on sending a enter/ leave notice to each channel the user in question is in.
|
Changes Unknown when pulling 9f5d185 on kippr:master into * on slackhq:master*. |
|
Hi folks Is there any help you need from me on this? Did I miss anything? Thanks |
|
@kippr No, I am so sorry that it has taken us too long to get to this. I'm going to have a look at this now! |
There was a problem hiding this comment.
It looks like you removed the [channel|group]_[join|leave] event handlers entirely, which is no good unfortunately. I'd need you to restore those event handlers, preferably in a non-breaking way.
Finally, signing the CLA is a necessary condition for accepting any PR, I'm sorry to say. We'll need to get your electronic signature on that before we can move forward.
FWIW, I disagree with this sentiment. While it may be true for your team, on other Slack teams channel membership can be very fluid. On the other hand, presence in Slack can be very misleading—maybe someone pulled out their phone to read a DM in a different team? They get listed as available on all their teams on their phone. A better indicator is the Do Not Disturb system in place. That might prove more useful. That said, it might also be true that our adapter has mistaken channel membership with presence as Hubot generally understands them, and that is something to think about. I'd welcome a discussion of these points. |
|
Hi Don Thanks for your thoughtful comments. Unfortunately I don't think there is a completely clean mapping of Hubot presence to Slack, for the reasons you mention. And as there are also no comments in Hubot code or adapter documentation specifying what the enter/ leave events should map to, or what a script writer would use the callbacks for, there is also ambiguity to what Hubot developers actually meant these to map to in the first place.. From looking at sample code in the wild, what other adapters map these events to, reading the (limited) comments we have from others using the slack adapter (e.g. #296), and also my own team's use in scripts, I concluded (perhaps incorrectly) that the Hubot enter/ leave events map more cleanly to 'I am available'/ 'I have gone offline' real world events rather than the 'I'm interested in this channel each time I go online'/ 'I'm no longer interested in this channel' events they currently map to. But they kind of map to both tbh if you consider how campfire, jabber, irc implementations work: you log in and join a room/ channel (even if that's automatic). The slack model of 'persistent' joins is different from other Hubot-supporting tools, so its tricky. I take your point that the 'whip out your phone to reply' to a DM does not mean you want a bot giving you a status update on your production systems, for instance (which is the specific use case I was looking to address.) So, I like your idea of mapping to the do not disturb status (perhaps combined with the presence API?)... so e.g. you have your automatic do not disturb schedule set from 11pm to 9 am, and then open slack on your phone/ unlock your laptop at 9:30am, it would trigger the enter events... but if you login for an earlier start one day, it would not trigger until the do not disturb turns off automatically at 9am. Also if you are on the couch at home after hours and answer a DM, or you have explicitly set do not disturb on during work hours and whip out your phone in a meeting to answer someone, the enter events aren't fired... That would work very nicely for us as well.. I'm happy to take a look at implementing that (and signing the CLA) if you think that works? It's a breaking change so I appreciate you may not be keen.. At least, its breaking if we were to leave the the [channel|group]_[join|leave] event handlers removed.. I can re-add but having both the persistent channel join/leave and do not disturb + presence change events map to the enter/ leave events doesn't make as much sense to me, unless we find a way for the user of the callbacks to distinguish between the two? Thanks See also: |
|
Thanks for your feedback. Some more thoughts have come to mind reading over your message above. DnD is designed to work more or less transparently, that is, without having to know about it. So, messages that would normally trigger a notification will get queued up, and then presented all at once once DnD is unset. This way, you don't have to think about whether you should send the message or not. Likewise, being an async medium with mobile notifications, you probably shouldn't really care whether someone is online or offline. If you really need someone to see the message, you just @-tag them, and they can look at their own convenience, without having to coordinate the user being online with the bot sending the message. In that respect, I am now fairly convinced that the right mapping here is the user leaving and entering a channel. These are synchronous events for which an immediate response might be good. That said, the way you think about these events should be accounted for: Someone who leaves might not come back. In light of IRC and Campfire, I understand now why you see these as low-frequency events. At any rate, there isn't a strong notion of someone going away, and then needing to be caught up upon return, as there is in IRC. So, for now, I think that a) neither presence nor DnD map neatly to the concepts in Campfire, and that therefore the best course of action is to leave these events wired up as they are—and perhaps to add new events for presence and DnD. |
Current coverage is 83.52% (diff: 80.00%)@@ master #364 diff @@
==========================================
Files 4 4
Lines 164 170 +6
Methods 0 0
Messages 0 0
Branches 37 39 +2
==========================================
+ Hits 138 142 +4
- Misses 26 28 +2
Partials 0 0
|
|
OK Don, fair enough. Unfortunately I'm not sure how to handle the morning scripts that we used under jabber in slack: when you first appear after being away for 4 hours, Hubot checks systems status and let you know about any issues (e.g. nagios alerts, failed batch jobs, etc). Its been really useful over the years for non critical issues and figuring out the state of the world when you come online, but guess i'll need to find another way. Cheers |
|
I understand. FWIW, the Slack Way is just to go ahead and place these items in the channel as they happen, and rely on the fact that these will be marked "unread" when the user comes online. The other alternative that we suggest is creating a digest of events, as you describe, and posting them to the channel at a set time, for example, at 9AM. Again, relying on the fact that these will be marked unread on a per-user basis, so that people can catch themselves up as they feel the need. |
|
Thanks for the suggestions. I was considering something similar to the 2nd actually. The first is suboptimal for status updates because (like email) you see the 'transactions' rather than the 'balance'. E.g. if there was a failure, but someone sorted it out, I don't need to act, and if I've been away on a vacation I've got other things I want to catch up on first :) Staleness is one of the biggest drawbacks of using email (or Slack as you describe) for anything that changes state. The scheduled 'tell me current state' is better because it'll be more timely, but suffers from some of the same drawbacks (e.g. a bunch of expired status messages if I have a couple days off, when I'd rather be catching up what people have been up to/ discussing while I was out)... Also, in our team (and most these days I guess) people log in from all over the world at different times, so if I'm the first person in my team Europe to log in, I will take a look at anything outstanding. If a teammate gets there first, then great, they may have cleared any problems by the time I log in. So long story short, appreciate the suggestions! I may take a look at finding a way to hook Hubot up to slack-specific messages, so I can explore the presense + dnd status stuff you suggested after all. Don't think the hubot interface facilitates much in adapter specific callbacks, but we shall see... (On that note, I was interested in exploring exposing the richer formatting possibilities Slack affords too.) |
|
The other option is to trigger a digest at will with a slash-command. Not as cool as automatically receiving one when you join the channel, but given our persistence of channel membership, it might be a better option? |
|
i just want to say this was quite a productive and informative discussion. thanks! |
PR Summary
Use the more granular/ frequent RTM presence_change events to trigger enter/ leave callbacks, rather than the generally infrequent channel/ group join/ leave events.
Related Issues
I think this also addresses #296
Test strategy
Unit tests modified/ added.
Manually tested with our own morning scripts, which were not getting triggered when we moved our Hubot from Jabber to Slack.
The current adapter implementation triggers robot.enter / robot.leave events when someone joins/ leaves a channel or group. However, most Hubot scripts use these callbacks registers with the much more frequent Campfire/ XMPP/ etc presence change events in mind (e.g. when someone logs on/ logs off). Since joining a slack channel is something you tend to do only once, or at most once in a while, the callbacks are not so very useful at present for the slack adapter.
One downside to the new implementation is that because a presence change event is generic, not specific to a (Hubot) room/ (Slack) channel, there is no room in the envelope passed to your enter/ leave callback. I adapted our own scripts to use robot.messageRoom rather than envelope.reply, but am open to suggestions on how to improve this, perhaps by looking at the feasibility on sending a enter/ leave notice to each channel the user in question is in.