xmppjs / hubot-xmpp

XMPP adapter for Hubot
181 stars 101 forks source link

Private messages require user to use bot's name #85

Closed juhokuu closed 9 years ago

juhokuu commented 9 years ago

I tried searching for this in closed issues and, surprisingly, did not find anything. I get a sense that no one is bothered by this, but here goes.

I find it odd that you need to use the <name> <command> syntax when addressing Hubot via private message. It should be simply <command>. Is there an easy way to achieve this?

markstory commented 9 years ago

This might be outside of the adapter code. However, if you're interested in trying to implement this, the following code is where the adapter handles incoming messages and passes them off to hubot's command/listen matcher.

juhokuu commented 9 years ago

Looking at that bit, I'm thinking of just checking the beginning of a message for the bot's name and inserting it when it's missing. I know it's dirty but I could create an env variable for it, turn it off by default, and explain in the readme that turning it on will cause the bot to ignore /^command/i.

I don't know if this is something you'd want to merge because hubot-scripts almost certainly has commands that only match ^. Thoughts?

markstory commented 9 years ago

Would you only be adding the bot name in private messages?

juhokuu commented 9 years ago

Yes. Right now, I've got it to work with this:

readMessage: (stanza) =>
    # ignore non-messages
    return if stanza.attrs.type not in ['groupchat', 'direct', 'chat']
    return if stanza.attrs.from is undefined

    # ignore empty bodies (i.e., topic changes -- maybe watch these someday)
    body = stanza.getChild 'body'
    return unless body

    from = stanza.attrs.from
    message = body.getText()

    # Allow calling without using bot's name in private messages
    # by prepending the message with bot's name (comes with caveats)
    if @options.pmUsername and stanza.attrs.type == 'chat' and
    message.slice(0, @robot.name.length) != @robot.name
      message = "#{@robot.name} #{message}"

    if stanza.attrs.type == 'groupchat'
      ...

But I've got very little experience with JavaScript and none with CoffeeScript so I'm still looking if I'm missing something.

EDIT: Seems to make more sense to have that in the else clause in if stanza.attrs.type == 'groupchat'.

markstory commented 9 years ago

What are the caveats you're finding with prepending the bot name?

juhokuu commented 9 years ago

At the moment, the /^command/ thing I already mentioned is the only one I'm actually running into, but it's a major one. Many of the various scripts available only trigger when the message starts with something specific and not the bot's name. This is the one I was going to address in the readme.

Then there's robot.respond vs robot.hear. At the moment robot.hear triggers even when the bot is addressed by name and this makes no sense to me personally, so in my mind there's a chance this will change in the future, and then none of the robot.hear commands would work with the env var set. Very, very, very unlikely that this will happen though.

I can't think or foresee anything else atm, but I don't feel confident enough about it to say for certain that there's nothing else.

markstory commented 9 years ago

I would have thought that scripts binding to /^command would only be triggered when the bot is addressed directly.

juhokuu commented 9 years ago

/^command/ is definitely meant to trigger only when not addressing the bot directly.

I just noticed that Hubot has a [-a|--alias] argument, which allows you to replace the bot's name with a "!", for example. So hubot, pug me and !pug me become equivalent. In light of this, my gripe with having to address the bot in private messages becomes less relevant but I'd still like to implement this, and I'd hate to maintain a fork just for this.

I need to add a test for the presence of the alias arg though. Still, by default, it wont cost anything but the first test (if envar is set).

markstory commented 9 years ago

I am fine with supporting a more direct way to invole commands in private chats. I wouldn't want to add tedious to debug edge cases though.

anupdhml commented 9 years ago

I needed this functionality, but I am also using HUBOT_ALIAS. I got it all working by adding the following code in the else block, for if stanza.attrs.type == 'groupchat'

  if message.slice(0, @robot.name.length).toLowerCase() != @robot.name.toLowerCase() and
      message.slice(0, process.env.HUBOT_ALIAS?.length).toLowerCase() != process.env.HUBOT_ALIAS?.toLowerCase() 
    message = "#{@robot.name} #{message}"

Hacky, but it works.

hubot-irc adapter follows a similar approach and they don't seem concerned about supporting /^command/.

https://github.com/nandub/hubot-irc/blob/master/src/irc.coffee#L292-L294

If we do want to support /^command/, maybe we can get a list of all such commands hubot is listening for, and if the input matches one of them, not add the robot name. Probably not worth it though.

markstory commented 9 years ago

@anupdhml Would you be able to put a pull request together with the changes you had and those that @juhokuu had worked on? I'm good with going forward on this.

anupdhml commented 9 years ago

@markstory ok I can do that. Should I add tests for it too?

markstory commented 9 years ago

@anupdhml That would be awesome.

anupdhml commented 9 years ago

@markstory pull request is up. Wasn't sure whether the option to turn it on/off was needed (haven't seen /^command really) so I leave it up to you to decide whether to include that feature. I have separated that out as the last commit.

@juhokuu thanks for opening the issue and working on it! Coming from hubot-irc, we were used to this feature and wanted the same for our xmpp chat.

markstory commented 9 years ago

Closing as the pull request looks good.

juhokuu commented 9 years ago

@anupdhml @markstory Thank you!