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 callback support to bot::{reply,send} #440

Closed
wants to merge 2 commits into from

Conversation

faridnsh
Copy link

  • 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 been mindful about doing atomic commits, adding documentation to my changes, not refactoring too much.
  • I've a descriptive title and added any useful information for the reviewer. Where appropriate, I've attached a screenshot and/or screencast (gif preferrably).
  • I've written tests to cover the new code and functionality included in this PR.
  • I've read, agree to, and signed the Contributor License Agreement (CLA).

PR Summary

This let's SlackBot::reply and SlackBot::send take an optional callback. While this is not documented, if a callback is passed to Hubot's Response::send and Response::reply, Hubot will pass it around..

My use case is that, I want to get the ts of the message that is sent and then update it later.

Related Issues

N/A

Test strategy

Passes the mocha callback to send and reply, so the test will only pass if the callback is called, also checking output to make sure the callback is not related, so not breaking compatibility in any way.

@CLAassistant
Copy link

CLAassistant commented Sep 26, 2017

CLA assistant check
All committers have signed the CLA.

@faridnsh
Copy link
Author

This CI is failing since one of the Slack node client dependencies, have published a version that uses ES6 const which is I think available only since node 4. I have tested this using node version 8 and everything passes.

sent_messages = []
for message in messages
if message isnt ''
sent_messages.push @client.send(envelope, message)
Promise.all(messages).then(callback.bind(null, null), callback)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this supposed to be Promise.all(sent_messages)...?

sent_messages = []
for message in messages
if message isnt ''
message = "<@#{envelope.user.id}>: #{message}" unless envelope.room[0] is 'D'
@robot.logger.debug "Sending to #{envelope.room}: #{message}"
sent_messages.push @client.send(envelope, message)
Promise.all(messages).then(callback.bind(null, null), callback)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto to above

@aoberoi
Copy link
Contributor

aoberoi commented Oct 17, 2017

@alFReD-NSH thanks for the contribution! i think its a good idea and i'd like to see it merged.

i think something strange is going on with github's "update branch" capability, because i don't see the merge commit being added to this PR after i click that button. the travis build is running against SHA 2428a1190468cdd5ac2ab7c3a342efa0bcdcf20b, which is a 404 in the repo, so the coverage data is not being reported correctly. can you give maintainers permission to update this PR so i can see if i can trigger the right reporting?

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

aoberoi commented Mar 30, 2018

i still think the idea of this PR is valid, but the code that it touches has changed and is being refactored heavily in #465. i'm happy to land this if you can implement the changes on top of that.

@aoberoi aoberoi added the needs info An issue that is claimed to be a bug and hasn't been reproduced, or otherwise needs more info label Mar 30, 2018
@aoberoi
Copy link
Contributor

aoberoi commented Apr 13, 2018

tracking this feature in #473

rtlechow added a commit to rtlechow/hubot-slack that referenced this pull request Oct 15, 2018
Allows `SlackBot::{send,reply}` to receive an optional callback. While
this is not documented, if a callback is passed to Hubot's
`Response::{send,reply}`, Hubot will pass it around.

Shamelessly plagiarized from slackapi#440 because I'm in need of this
functionality. Fixed the tests to work with @stubs.
@seratch
Copy link
Member

seratch commented Apr 24, 2020

Let me close this PR as #540 has been merged into master branch 🎉

@seratch seratch closed this Apr 24, 2020
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 needs info An issue that is claimed to be a bug and hasn't been reproduced, or otherwise needs more info
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants