Allow to respond to text in attachments #177 #180

mdudzinski · 2018-02-28T10:52:25Z

Closes #177

Added a new DSL attachment for scanning text in attachments. This simple approach just iterates over pretext, text and title fields of each attachment and test each field against the match. It returns the first match so if there is more than one attachment in the array then there is no easy way to tell which attachment record has been matched.

I generated a new .rubocop_todo.yml file as the new offenses require a general refactoring of SlackRubyBot::Commands::Base class.

~~I'll update the documentation and the changelog once this approach is accepted.~~

mdudzinski · 2018-02-28T11:25:29Z

lib/slack-ruby-bot/commands/base.rb

@@ -122,6 +131,23 @@ def finalize_routes!
          command command_name_from_class
        end

+        def match_attachments(data, route)
+          fields_to_scan = %i[pretext text title]
+          data.attachments_.each do |attachment|


Unnecessary usage of under-bang from Hashie::Mash as there is a validation against missing data (data_missing?). Will take it out.

mdudzinski · 2018-02-28T13:01:05Z

I also thought a bit about a possibility to know which attachment record has been actually matched. Adding a new param to the block seems to be overcomplicated to me but perhaps I'm wrong.
Anyway, I think it should be available via the match param. It could be achieved by either:

dynamically defining a new method on the match instance that would return the matched attachment or its index in the array
the bot_matcher approach - add the attachment record index value to each field value before testing it and extending the route[match] with a matcher for the index value. Then the matched attachment's index in the array could be accessing by match[:__attachment_index or something like that. Double underscore to avoid eventual conflicts.

dblock

Great work!

I think we need to make the attachment available inside the block as well as being able to tell what matched in the attachment (title, etc.). Eventually we can have more deliberate matching (match only title).

What do you think?

Some detailed comments below.

dblock · 2018-02-28T14:37:20Z

README.md

+
+```ruby
+class Attachment < SlackRubyBot::Bot
+  attachment 'Slack API Documentation' do |client, data, stocks|


What's stocks here? I think what we want is to pass in the matching attachment into this block.

Copy-paste issue 😃will fix this, thanks

dblock · 2018-02-28T14:43:20Z

lib/slack-ruby-bot/commands/base.rb

+
+        def data_missing?(match_method, expression, data)
+          return true if match_method != :attachment && expression.nil?
+          return true if match_method == :attachment && (data.attachments.nil? || data.attachments.empty?)


I feel like this muddles two somewhat separate things and isn't reused elsewhere. I would actually roll this method into two next statements above without extracting into a method or write this here as a case statement. Either way return false explicitly (I think currently it's nil).

I'll take a look on this, thanks.

mdudzinski · 2018-02-28T15:08:36Z

@dblock

I think we need to make the attachment available inside the block

So how about returning a Hashie::Mash that contains MatchData as well as the matched attachment and the matched field. Or extend the MatchData class itself and include information about matched attachment.

Eventually we can have more deliberate matching (match only title).

The DSL can be extended so it takes the attachment object fields to match against. If no fields are passed, the default one will be used.

mdudzinski · 2018-02-28T19:19:36Z

lib/slack-ruby-bot/commands/base.rb

+              )
+              match.singleton_class.send(
+                :define_method, :attachment_field, -> { field }
+              )


@dblock The above code adds the access to matched attachment information to the match object instance.

Feels way to complicated. You should just subclass the match object.

I see but subclassing a core class without a constructor doesn't seem to be less complicate. Unless there is some easy way I missed.
I think we can use

a new class that uses delegation instead of inheritance

a module and make the match object extend it on the fly

I am good with a new class that uses delegation.

mdudzinski · 2018-02-28T19:21:40Z

lib/slack-ruby-bot/commands/base.rb

+            return false unless expression.nil?
+          when :attachment
+            return false unless data.attachments.nil? || data.attachments.empty?
+          end


I prefer extracting logic into methods even if they aren't re-used. Just to increase readability and reduce the cyclomatic complexity. So I decided to re-organize the method to use a case statement according to your suggestion.

Something doesn't smell right here as we do the case twice, once in invoke and once in data_missing. Haven't tried to rewrite it, but usually this means both can be combined avoiding having to case twice.

I think this is simpler :)

case match_method when ... expression.nil? when :attachment data.attachments.nil? || data.attachments.empty else true end

We can just put the checks directly in the case in invoke but this will increase the cyclomatic complexity.
It would look like

when :match next unless expression # ... when :scan next unless expression # ... when :attachment next unless data.attachments && !data.attachments.empty? # ...

I am not a code purist re: cyclomatic complexity :)

Okay I'll add the checks to the invoke method then :)

mdudzinski · 2018-02-28T19:22:38Z

@dblock I implemented some of the improvements you had suggested. I pointed them as inline comments. The rest will be added soon. Let me know what you think so far.

dblock · 2018-03-01T13:44:25Z

lib/slack-ruby-bot/commands/base.rb

@@ -81,6 +85,11 @@ def scan(match, &block)
          self.routes[match] = { match_method: :scan, block: block }
        end

+        def attachment(match, &block)
+          self.routes ||= ActiveSupport::OrderedHash.new


Sounds like we need to make self.routes a method that does @routes ||= or something like that? We repeat this a few times. NBD

dblock · 2018-03-01T13:45:46Z

lib/slack-ruby-bot/commands/base.rb

@@ -122,6 +131,36 @@ def finalize_routes!
          command command_name_from_class
        end

+        def match_attachments(data, route)
+          fields_to_scan = %i[pretext text title]


Maybe this should be the third optional fields parameter here defaulted to these. Just thinking about the future as we pass this in dynamically from the attachments options.

Will think about this.

mdudzinski · 2018-03-06T15:19:37Z

@dblock Everything is fixed. I think. Please take a look.

dblock

Great. I am merging this.

In next steps - we already have a support folder with things like CommandsHelper, I think Commands::Help::Match should just be SlackRubyBot::Commands::Match or maybe SlackRubyBot::CommandMatch and moved into the support folder? Maybe something else? What do you think? Maybe try a few options, potentially renaming our current CommandsHelper class too?

dblock · 2018-03-09T21:03:29Z

@mdudzinski interested in joining the (co)maintainers of this gem? Maybe cutting the next release? If so lmk what your rubygems email is.

mdudzinski · 2018-03-12T13:59:04Z

@dblock Sure I'm interested -> michal[at]dudzin[dot]ski (https://rubygems.org/profiles/dudek)

Yeah I don't like Commands::Help::Match too and it was meant to be a temp solution.
I'll think about this but here go some thoughts off the top of my head:
SlackRubyBot::CommandMatch could work and would be consistent with CommandHelper. But since CommandHelper is used only inside of the commands directory then perhaps it would make more sense to move CommandHelper to the help directory.
Then the dir could be renamed from help to support and/or an additional namespace (Help:: or Support::) could be omitted. So we end up with SlackRubyBot::Commands::Helper, SlackRubyBot::Commands::Match etc.
But won't it be too confusing? SlackRubyBot::Commands::Match sounds like a command implementation.

dblock · 2018-03-12T15:26:49Z

Added you, feel free to cut a release whenever you see fit. Please follow https://github.com/slack-ruby/slack-ruby-bot/blob/master/RELEASING.md.

I think everything you say makes sense - play with it - try to make a PR that you can stand behind!

mdudzinski · 2018-03-15T13:38:04Z

@dblock Thanks! I'll do.

mdudzinski · 2018-03-21T11:24:32Z

@dblock Lets move the discussion to #182. Cheers!

mdudzinski mentioned this pull request Feb 28, 2018

Bot responds to text in attachments. #177

Closed

mdudzinski commented Feb 28, 2018

View reviewed changes

mdudzinski force-pushed the #177 branch from faa108a to 529f895 Compare February 28, 2018 12:52

dblock requested changes Feb 28, 2018

View reviewed changes

mdudzinski commented Feb 28, 2018

View reviewed changes

dblock reviewed Mar 1, 2018

View reviewed changes

Michal Dudzinski added 6 commits March 2, 2018 21:55

Allow to respond to text in attachments

37fc92d

Add access to attachment data in the 'match' param

bf3610c

Add optional 'fields_to_scan' param

e86967b

Allow a single attachment hash in custom matchers

076cd5c

Lazy 'routes' initialization

c326620

Update CHANGELOG.md and README.md

3be83f4

mdudzinski force-pushed the #177 branch from 56d8771 to 3be83f4 Compare March 6, 2018 15:11

dblock approved these changes Mar 9, 2018

View reviewed changes

dblock merged commit 9e7d0c9 into slack-ruby:master Mar 9, 2018

mdudzinski mentioned this pull request Mar 21, 2018

CommandsHelper class and Help module refactoring #182

Merged

problem-child mentioned this pull request Apr 3, 2018

Suitable for interactive dialogs with users? #184

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to respond to text in attachments #177 #180

Allow to respond to text in attachments #177 #180

mdudzinski commented Feb 28, 2018 •

edited

mdudzinski Feb 28, 2018

mdudzinski commented Feb 28, 2018

dblock left a comment

dblock Feb 28, 2018

mdudzinski Feb 28, 2018

dblock Feb 28, 2018

mdudzinski Feb 28, 2018

mdudzinski commented Feb 28, 2018 •

edited

mdudzinski Feb 28, 2018

dblock Mar 1, 2018

mdudzinski Mar 1, 2018

dblock Mar 1, 2018

mdudzinski Feb 28, 2018 •

edited

dblock Mar 1, 2018

dblock Mar 1, 2018 •

edited

mdudzinski Mar 2, 2018

dblock Mar 2, 2018

mdudzinski Mar 2, 2018

mdudzinski commented Feb 28, 2018 •

edited

dblock Mar 1, 2018 •

edited

mdudzinski Mar 2, 2018

dblock Mar 1, 2018

mdudzinski Mar 2, 2018

mdudzinski commented Mar 6, 2018

dblock left a comment

dblock commented Mar 9, 2018

mdudzinski commented Mar 12, 2018

dblock commented Mar 12, 2018

mdudzinski commented Mar 15, 2018

mdudzinski commented Mar 21, 2018

Allow to respond to text in attachments #177 #180

Allow to respond to text in attachments #177 #180

Conversation

mdudzinski commented Feb 28, 2018 • edited

Choose a reason for hiding this comment

mdudzinski commented Feb 28, 2018

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdudzinski commented Feb 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdudzinski Feb 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdudzinski commented Feb 28, 2018 • edited

dblock Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdudzinski commented Mar 6, 2018

dblock left a comment

Choose a reason for hiding this comment

dblock commented Mar 9, 2018

mdudzinski commented Mar 12, 2018

dblock commented Mar 12, 2018

mdudzinski commented Mar 15, 2018

mdudzinski commented Mar 21, 2018

mdudzinski commented Feb 28, 2018 •

edited

mdudzinski commented Feb 28, 2018 •

edited

mdudzinski Feb 28, 2018 •

edited

dblock Mar 1, 2018 •

edited

mdudzinski commented Feb 28, 2018 •

edited

dblock Mar 1, 2018 •

edited