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

Add support for response.sendPrivate #442

Open
3 tasks done
alFReD-NSH opened this issue Sep 29, 2017 · 7 comments
Open
3 tasks done

Add support for response.sendPrivate #442

alFReD-NSH opened this issue Sep 29, 2017 · 7 comments
Labels
enhancement M-T: A feature request for new functionality good first issue

Comments

@alFReD-NSH
Copy link

alFReD-NSH commented Sep 29, 2017

  • 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.

Description

It'd be nice to have response.sendPrivate which basically sends a direct message to the given user that initiated that response. It would be a shortcut for this mainly:

robot.messageRoom msg.message.user.name, "Hello!"

This is supported by hubot-irc, hubot-test-helper and hubot-rocketchat. Probably it would be also easier for users to figure out sending private messages quick and easy.(Check #159 and #299)

I'd like to contribute this, but I'm not really sure how to write to test it with the stubs and whatever we have in place.

@aoberoi
Copy link
Contributor

aoberoi commented Dec 17, 2017

thanks for pointing this hole in functionality out @alFReD-NSH! looking through those older issues really underscores how this adapter is really under serving users who want to simply send a DM.

although the snippet you posted works, its really not preferred and may break in the future. the slack platform is moving away from any guarantees of uniqueness for user names, ever since the introduction of Shared Channels. you can see the details in this changelog entry. the future is about only operating on user IDs. this means that the internal implementation wouldn't just be calling Web API method chat.postMessage (which is what SlackClient.send() uses currently) with the username in the channel argument. the right way to do this would be to first open a new conversation using conversations.open passing the user ID in the users argument. this call would return a channel ID, which can subsequently be used in chat.postMessage.

i'd love a contribution! let's talk a little about how this should be done. first of all, we should make sure the implementation does not depend on the RTMClient DataStore (see #385) since it is being removed. as i previously mentioned, it should not depend on the user name either, only the ID. one idea (that might not be a good one, but worth considering) is we can keep the functionality decoupled from the Adapter code by implementing it as a response middleware and then registering it on adapter initialization. i'm happy to hear your thoughts on an implementation and even collaborate if you'd like.

@aoberoi aoberoi added the enhancement M-T: A feature request for new functionality label Dec 17, 2017
@drdamour
Copy link

it seems like robot.messageRoom msg.message.user.name, "Hello!" is already broken i have to use robot.messageRoom msg.message.user.id, "Hello!" for things to work.

this unfortunately breaks the reply in private of hubot-help

@drdamour
Copy link

what's weird is robot.messageRoom msg.message.user.name works in the old hubot-slack, v 3.4.2, but in v4 it does not, why is that?

@aoberoi
Copy link
Contributor

aoberoi commented Dec 22, 2017

@drdamour the difference is because the old hubot-slack (v3.4.2) purely uses the RTM API to send messages (which also means it cannot handle attachments and other nice things). in the new version, those messages are sent over the Web API (using chat.postMessage). these two techniques have different behavior.

i still think this would be a good feature to have, so adding the good first issue label for anyone that feels enthusiastic.

@drdamour
Copy link

ok that makes sense...but it is a shame because a fair amount of hubot scripts rely on sendMessage room: user.name and this pass through nature straight to the service layer that can no longer handle that makes them ineffective. trying to conceive if there's a way in the slack adapter to build up a map as messages come in of username -> userId and to intercept messages targeting a user.name room and switch it to id.

@aoberoi
Copy link
Contributor

aoberoi commented Dec 22, 2017

@drdamour heh, that map is what the data store that we just deprecated contains (amongst other things).

i won't go into all the problems with the data store (see: slackapi/node-slack-sdk#330), but since the platform stopped guaranteeing the uniqueness of usernames its design hasn't worked as well. we could probably do better for just the username -> user ID map situation! we could build a cache where if the lookup failed, the web API would be used to find the correct mapping. the tricky part is cache invalidation. i'd have to think more about that.

open to any ideas and thoughts!

@drdamour
Copy link

yeah makes sense to kill it in core slack...but want to do it in the hubot-slack layer to "polyfill" the hubot contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality good first issue
Projects
None yet
Development

No branches or pull requests

3 participants