Skip to content
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

ReactionMessage item_user property should be marked as optional (Incoming Webhooks) #468

Closed
4 of 9 tasks
y-i opened this issue Mar 27, 2018 · 5 comments · Fixed by #511
Closed
4 of 9 tasks

ReactionMessage item_user property should be marked as optional (Incoming Webhooks) #468

y-i opened this issue Mar 27, 2018 · 5 comments · Fixed by #511
Labels
docs M-T: Documentation work only

Comments

@y-i
Copy link

y-i commented Mar 27, 2018

Description

hubot-slack can't react to reaction_added events if the message which reaction was added to is incoming webhook.

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • testing related
  • discussion

Requirements (place an x in each of the [ ])

  • 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 searched for any related issues and avoided creating a duplicate issue.

Bug Report

Filling out the following details about bugs will help us solve your issue sooner.

Reproducible in:

hubot-slack version: 4.4.0

node version: v8.9.4

OS version(s): macOS Sierra (10.12.6)

Steps to reproduce:

  1. Join the bot which uses robot.react() to a channel.
  2. Send a incoming webhook message to the channel.
  3. Add reaction to the message.

Expected result:

robot.react() will be triggered.

Actual result:

robot.react() will not be triggered.

Attachments:

At this point, item_user is undefined in message if the message was posted by incoming webhook.
I can react to messages which were posted by incoming hook by changing return unless user && item_user to return unless user.

@aoberoi
Copy link
Contributor

aoberoi commented Mar 30, 2018

thanks for the report! this might be fixed in #465, because the code path changes significantly.

@y-i by any chance, is the user reacting to the incoming webhook message not a member of the local workspace? specifically, is this occurring in a shared channel?

@shanedewael do we have any tests that will cover this scenario? i don't think its specific to Incoming Webhook, but we should try.

@aoberoi aoberoi added the bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented label Mar 30, 2018
@y-i
Copy link
Author

y-i commented Apr 2, 2018

@aoberoi I didn't use a shared channel. So the user is a member of the local workspace.

@aoberoi
Copy link
Contributor

aoberoi commented Apr 3, 2018

@y-i thanks, i was able to reproduce this (both on RTM and on Events API)

it turns out that this is expected (but not well documented) behavior. when the message is generated from an Incoming Webhook, there actually is no "user" responsible for sending it, so there's no item_user value at all.

we'll be updating the following documentation page to more clearly describe this: https://api.slack.com/events/reaction_added

in the meantime, i think we can more clearly mark the property as optional here:

# item_user - A String indicating the user that posted the item.

@aoberoi aoberoi added docs M-T: Documentation work only and removed bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented labels Apr 3, 2018
@aoberoi aoberoi changed the title Robot.react cannot respond to incoming webhook messages ReactionMessage item_user property should be marked as optional (Incoming Webhooks) Apr 3, 2018
@y-i
Copy link
Author

y-i commented Apr 17, 2018

@aoberoi thank you,robot.react() seems to be fixed in #465.

but #465 looks like broken.

  • robot.react() doesn't respond to the reaction message.
  • robot.hear() throws exception when the incoming webhook message is posted.

@shaydewael shaydewael mentioned this issue Apr 17, 2018
2 tasks
@shaydewael
Copy link
Contributor

@y-i Sorry about that (and thanks for pointing it out). That should be fixed with #475.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs M-T: Documentation work only
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants