Commands that delegate to controllers incorrectly raise NotImplementedError when called

[jasonwc]

Hey @dblock! Been loving this library for building some internal tooling at my company, but I've encountered an issue recently when trying to use the MVC architecture of SlackRubyBot.

My original use case was to create a controller to do CRUD on a server to facilitate adding and viewing "interestings" that people had learned at work throughout the week.

I structured it similar to a Rails MVC architecture:
interestings add
interestings list

These were defined in a controller called the InterestingsController that implemented the following methods:

def interestings_add
def interestings_list

It was important that it be scoped to interestings as I'm also intending to work on a retro helper allowing users to submit good, bad, and meh items in advance of their team retrospectives. That would likely have a similar retro add, retro list, etc... interface and so I needed to scope add, list commands accordingly.

I started having problems where if a user typed interestings followed by any non command text, the slack bot would crash and not recover. This would also happen if the user happened to just type interestings.

I ended up just defining an interestings method to return help about the controller methods. It will trigger on any message to the bot starting with interestings that is not an existing command (e.g. interestings list).

Another case that I encountered was where the controller was named to encompass its actions (i.e. group all the commands related to viewing information about, announcing deploys, etc.. for our various production applications under an AppsController). In that case, I don't really want to define an apps command as the interface isn't @bot apps do something, I would much rather just have it send back the standard Sorry @user, I don't understand that command!.

Anyway, I wanted to report this to see if I was misunderstanding or setting up my controllers/views/models incorrectly.

I created a sample repo to reproduce this issue here. It implements a greetings controller with the same issue.

References:

Logs

[dblock]

Can you try to turn this into a failing spec and make a PR? We (you) should be able to fix it from there.

[jasonwc]

Sure thing!

[jasonwc]

@dblock I was able to reproduce the issue in spec. It looks like in a normal command class (not using MVC) where the name matches, if you haven't put any command blocks in it, it would correctly raise NotImplementedError.

However if you have a command class that delegates to MVC controller that does have methods in it, if one of the methods doesn't match the command class name, then it incorrectly raises NotImplementedError.

In a normal command, if I had named the class Normal, and gave a command block named foo, it would simply default to the Unknown behavior if the user did @bot normal.

[jasonwc]

I'm still getting familiar with the code base, so I'm not sure what the solution is yet but if you have any ideas on how to proceed I'd love to take a stab at the implementation.

[chuckremes]

I originally wrote the MVC support. Let me take a quick peek at this and hopefully I can guide you on the best way to solve it. I'll ping here tomorrow.

[jasonwc]

Awesome! Thanks @chuckremes!

[chuckremes]

My laptop is on the fritz. Need a genius bar visit before I can really look at this. Sorry for delay.

[Startouf]

I am getting the same error after upgrading 0.14 -> 0.15 on my controllers

E, [2018-01-28T20:03:03.940744 #16991] ERROR -- : thread crashed
NotImplementedError: my_method <mailto:[email protected]|[email protected]>
/Users/Cyril/.rvm/gems/ruby-2.4.1/gems/slack-ruby-bot-0.10.5/lib/slack-ruby-bot/commands/base.rb:76:in `block in invoke'
/Users/Cyril/.rvm/gems/ruby-2.4.1/gems/slack-ruby-bot-0.10.5/lib/slack-ruby-bot/commands/base.rb:57:in `each_pair'
/Users/Cyril/.rvm/gems/ruby-2.4.1/gems/slack-ruby-bot-0.10.5/lib/slack-ruby-bot/commands/base.rb:57:in `invoke'
/Users/Cyril/.rvm/gems/ruby-2.4.1/gems/slack-ruby-bot-0.10.5/lib/slack-ruby-bot/hooks/message.rb:7:in `block in call'
/Users/Cyril/.rvm/gems/ruby-2.4.1/gems/slack-ruby-bot-0.10.5/lib/slack-ruby-bot/hooks/message.rb:7:in `each'
/Users/Cyril/.rvm/gems/ruby-2.4.1/gems/slack-ruby-bot-0.10.5/lib/slack-ruby-bot/hooks/message.rb:7:in `detect'
/Users/Cyril/.rvm/gems/ruby-2.4.1/gems/slack-ruby-bot-0.10.5/lib/slack-ruby-bot/hooks/message.rb:7:in `call'
/Users/Cyril/.rvm/gems/ruby-2.4.1/gems/slack-ruby-bot-0.10.5/lib/slack-ruby-bot/hooks/set.rb:33:in `block (2 levels) in register_callback'
/Users/Cyril/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/set.rb:324:in `each_key'
/Users/Cyril/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/set.rb:324:in `each'

[chuckremes]

@jasonwc There are probably a few ways of solving this problem. Here's one suggestion.

Note this line in the code:
https://github.com/slack-ruby/slack-ruby-bot/blob/master/lib/slack-ruby-bot/mvc/controller/base.rb#L160

Instead of blindly sending the method name to the instance object, we could first check to see if it respond_to? to the name. If it doesn't, then issue the "I don't understand that command" reply.

That's probably the easiest approach.

If you want a little more magic, you could add a method_missing to the Controller base class. Any commands that the object doesn't understand would call method_missing where you could issue the same reply. This is a cleaner approach than the first one above but it does assume you are comfortable with some metaprogramming magic.

If you want some help, ping me here.

[Startouf]

I realized my occurence of the bug was because I had not totally migrated from pure command to MVC and that a command() call was left in my SlackRubyBot::Commands::Base which caused the code to look for the call() method instead of using the MVC code. Apparently this was not crashing in 0.14. So not really related to this bug actually...

[jasonwc]

Thanks for looking into it @chuckremes! Been busy but I'll try to make some progress on this issue this weekend.

[chuckremes]

@jasonwc Sounds fine. Again, if you want to bounce some ideas around or need a hand with figuring out the right approach, ping me here.