New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle reaction_added messages #360

Merged
merged 3 commits into from Sep 20, 2016

Conversation

Projects
None yet
6 participants
@mbland
Contributor

mbland commented Sep 19, 2016

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've been mindful about doing atomic commits, adding documentation to my changes, not refactoring too much.
  • I've a descriptive title and added any useful information for the reviewer. Where appropriate, I've attached a screenshot and/or screencast (gif preferrably).
  • I've written tests to cover the new code and functionality included in this PR.
  • I've read, agree to, and signed the Contributor License Agreement (CLA).

PR Summary

Handle reaction_added messages

Converts reaction_added messages to TextMessages before passing them to Hubot. Used to implement behavior needed by 18F/hubot-slack-github-issues and depends on behavior from slackapi/node-slack-sdk#96, which may be out-of-date by now. I'll take a look at the latest version and report back when I get a chance tonight or tomorrow, unless someone else already has context and beats me to it.

Related Issues

Supercedes and closes #271. Also pertains to #251.

Test strategy

Unit tests to validate the new SlackBot.reactionAdded method as part of the existing Handling incoming messages suite.

cc: @johnagan @mtlewis @patcon @afeld @ccostino @skehlet @MidnightLightning @stevecat @DEGoodmanWilson

Handle reaction_added messages
Supercedes and closes #271.

Converts reaction_added messages to TextMessages before passing them to
Hubot. Used to implement behavior needed by
18F/hubot-slack-github-issues and depends on behavior from
slackapi/node-slack-sdk#96. Also pertains to #251.
@coveralls

This comment has been minimized.

coveralls commented Sep 19, 2016

Coverage Status

Changes Unknown when pulling 560bf51 on mbland:reaction-added-v3 into * on slackhq:master*.

@DEGoodmanWilson

This comment has been minimized.

Contributor

DEGoodmanWilson commented Sep 19, 2016

Hello @mbland ! This looks quite nice, thank you for taking the time to update your PR!

However, I have one complaint. Rather than emitting a TextMessage, the current paradigm is to emit a new kind of message, so that clients can select what kinds of events they respond to. Look at bot.coffee:137–ff. I would recommend creating a ReactionMessage that specifies what happened in a structured way (what reaction? what message? was it added or removed?) rather than overloading TextMessage to accomplish this.

Thoughts?

@mtlewis

This comment has been minimized.

mtlewis commented Sep 20, 2016

@mbland thanks so much for picking this back up!

@coveralls

This comment has been minimized.

coveralls commented Sep 20, 2016

Coverage Status

Changes Unknown when pulling 87c3926 on mbland:reaction-added-v3 into * on slackhq:master*.

@mbland

This comment has been minimized.

Contributor

mbland commented Sep 20, 2016

@DEGoodmanWilson I was secretly hoping you'd suggest something like that. Done.

The reason I held back originally (other than trying to keep parity with the existing #271 while I re-familiarized myself with the code) was because I noticed all the existing message types are imported from the hubot npm. Would the long-term plan, then, be to add this message type upstream? I'm happy to send out another PR to github/hubot to do that if you think that makes sense.

@coveralls

This comment has been minimized.

coveralls commented Sep 20, 2016

Coverage Status

Changes Unknown when pulling 533682b on mbland:reaction-added-v3 into * on slackhq:master*.

@mbland

This comment has been minimized.

Contributor

mbland commented Sep 20, 2016

Also, looking back at slackapi/node-slack-sdk#96 and https://github.com/slackhq/node-slack-sdk/blob/3.4.0/lib/clients/rtm/client.js, it looks like the reaction_added and reaction_removed events are emitted by default, as with every other event type. Consequently, everything should "just work", modulo pushing the ReactionMessage class upstream.

@johnagan

This comment has been minimized.

johnagan commented Sep 20, 2016

LGTM 👍

great work!

@DEGoodmanWilson

This comment has been minimized.

Contributor

DEGoodmanWilson commented Sep 20, 2016

Yeah, this is great. I would advise against adding these types upstream in Hubot. Not all messaging services provide this kind of functionality, and to my mind at least, platform-specific features should remain with the adapter. Because, let's be honest, a Hubot script or plugin that makes use of this message type is simply not going to work on another service.

Merging this in, will release…soon. I promise.

@DEGoodmanWilson DEGoodmanWilson merged commit 5f4592c into slackapi:master Sep 20, 2016

@mbland mbland deleted the mbland:reaction-added-v3 branch Sep 20, 2016

@mbland

This comment has been minimized.

Contributor

mbland commented Sep 20, 2016

Cool, thanks! (And I was tilting towards the same conclusion re: pushing ReactionMessage upstream.)

@mbland

This comment has been minimized.

Contributor

mbland commented Sep 21, 2016

Looks like this just got pushed in v4.1.0. Thanks, @DEGoodmanWilson and @johnagan!

@neufeldtech

This comment has been minimized.

neufeldtech commented Oct 3, 2016

Hi there @mbland,
I've scanned the code in your PR, but I'm still unsure on how to actually implement reaction handling in a hubot script. Would you care to provide a simple "hello world" example on how to get hubot to listen for reaction_added messages? Thanks!

@mbland

This comment has been minimized.

Contributor

mbland commented Oct 3, 2016

@neufeldtech I'll try to get to it in the next day or two. Feel free to ping me by Friday if I haven't posted back.

FWIW, the original hack was to hook into the receiveMiddleware mechanism, and when a message came through, check the message type (reaction_added) and item type. I need to update that plugin as well, so I'll try to both knock it out and the "Hello, World!" example at the same time.

@mbland

This comment has been minimized.

Contributor

mbland commented Oct 5, 2016

@neufeldtech

This comment has been minimized.

neufeldtech commented Oct 5, 2016

@mbland Thanks! I'll be sure to take a look at this.

@mbland

This comment has been minimized.

Contributor

mbland commented Oct 6, 2016

Not to drag this thread out too long, but...I've already updated the example script to be even smaller and easier to follow.

And @DEGoodmanWilson: Do you think it'd be worth patching on a robot.react during SlackBot.run, so that clients won't have to require the ReactionMessage type or define the (message) -> message instanceof ReactionMessage matcher at all? (Happy to supply the PR myself, of course.)

@DEGoodmanWilson

This comment has been minimized.

Contributor

DEGoodmanWilson commented Oct 6, 2016

@mbland That might be a good idea, actually!

mbland added a commit to mbland/hubot-slack that referenced this pull request Oct 6, 2016

Bump Hubot development dependency to ^2.19
Per slackapi#360, this is the first step towards implementing a `robot.react`
method based on:

https://github.com/mbland/hubot-slack-reaction-example/blob/v1.1.0/scripts/handle-reaction.coffee

The previous Hubot development dependency version was ~2.11, but the
`robot.listen` method didn't appear until v2.16.0:

  hubotio/hubot#986
  hubotio/hubot#1035

mbland added a commit to mbland/hubot-slack that referenced this pull request Oct 6, 2016

Add hubot.Robot.react
Per slackapi#360, this implements `robot.react` as a replacement for the
approach from:

https://github.com/mbland/hubot-slack-reaction-example/blob/v1.1.0/scripts/handle-reaction.coffee

Specifically, the `ReactionMessage` definition will no longer be
required in client code, and this:

  robot.listen(
    (message) -> message instanceof ReactionMessage
    handleReaction
  )

can now become:

  robot.react handleReaction

`robot.react` can also take optional  `matcher` and `options` arguments
just like the underlying `robot.listen` method:

  robot.react(
    (message) -> message.type == 'added' && message.reaction == '+1'
    {id: 'my-reaction-matcher'}
    handleReaction
  )

@mbland mbland referenced this pull request Oct 6, 2016

Merged

Add hubot.Robot.react #363

6 of 6 tasks complete
@mbland

This comment has been minimized.

Contributor

mbland commented Oct 6, 2016

OK, just filed #363. Anyone who's still interested can subscribe to that PR.

mbland added a commit to mbland/hubot-slack-github-issues that referenced this pull request Oct 7, 2016

Update to use hubot-slack 4.10.0
Thanks to the new @slack/client npm, slackapi/hubot-slack#360, and
slackapi/hubot-slack#363, the `SlackBot` will now emit a
`ReactionMessage` object for `reaction_added` and `reaction_removed`
events.

A future commit will repackage the logic so that it is no longer
middleware, but called as part of a `robot.listen` or `robot.react`
callback.
@mbland

This comment has been minimized.

Contributor

mbland commented Oct 14, 2016

FYI @neufeldtech (and others), with the merge of #363 and release of v4.2.0, robot.react is now live. I've updated my hubot-slack-reaction-example to use it (as well as trimmed down the instructions), so you may want to look at it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment