slack-ruby / slack-ruby-bot

The easiest way to write a Slack bot in Ruby.
MIT License
1.12k stars 187 forks source link

Loops with ephemeral messages #226

Open sihu opened 5 years ago

sihu commented 5 years ago

I noticed that my Bot had problems with ephemeral messages, in particular with unfolding URLs.

How it can be reproduced

  1. bot posts an URL to a channel
  2. slackbot unfolds it
  3. bot posts the same URL to the channel
  4. slackbot sends message: "Pssst! I didn\u2019t unfurl <https:\/\/example.com> because it was already shared in this channel quite recently (within the last hour) and I didn\u2019t want to clutter things up."
  5. my bot responds with a Sorry @slackbot, I don't understand that command!
  6. now slackbot responds that he didn't understand
  7. ...there is a pingpong

Possible solution

The message hook should filter out those ephemeral messages. I think it will be something like:

lib/slack-ruby-bot/hooks/message.rb:

module SlackRubyBot
  module Hooks
    class Message
      def call(client, data)
        return if message_to_self_not_allowed? && message_to_self?(client, data)
        return if ephemeral_message?(data) # this would be new
        data.text = data.text.strip if data.text
        result = child_command_classes.detect { |d| d.invoke(client, data) }
        result ||= built_in_command_classes.detect { |d| d.invoke(client, data) }
        result ||= SlackRubyBot::Commands::Unknown.tap { |d| d.invoke(client, data) }
        result
      end

      ...

      private

      def  ephemeral_message?(data)
          data.is_ephemeral
          # and you could also check for the data.subtype == 'bot_message'
      end

I would be willing to contribute this fix if you like the solution.

dblock commented 5 years ago

That seems consistent with what I saw. Would love a PR with proper README/UPGRADING documentation and an option to override this behavior just like for bot messages.

sihu commented 5 years ago

@dblock that's fine! I have two open questions before i start:

  1. you said override this behavior just like for bot messages., did you mean, just like message_to_self?
  2. i asked myself what the check should include. Should it never respond to ephemeral messages or just not to ephemeral messages from bots? Or should it not reply to bots at all? This could bring some breaking changes. An alternative solution could also be to catch it in SlackRubyBot::Commands::Unknown ... so an unknown-message should never go to a bot.
dblock commented 5 years ago
  1. I think that yes, that's what I meant. But really I only care about being able to turn off the behavior you're introducing.
  2. Both? Either? Make it configurable and let people decide!