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

Updated Logic and Tests for Slack SDK v3 support #309

Merged
merged 46 commits into from
Jul 15, 2016

Conversation

johnagan
Copy link

This is a first pass at adding support for v3 of the Slack SDK 💥

The core logic has been updated to use dataStore and the test stubs have been modified to support the relocation of data calls.

@coveralls
Copy link

coveralls commented Jun 16, 2016

Coverage Status

Coverage decreased (-1.7%) to 58.974% when pulling e5dd2f8 on v3-sdk-updates into c0775f4 on master.

@coveralls
Copy link

coveralls commented Jun 17, 2016

Coverage Status

Coverage decreased (-1.7%) to 58.974% when pulling c244c3d on v3-sdk-updates into c0775f4 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+4.8%) to 65.464% when pulling cd61c60 on v3-sdk-updates into c0775f4 on master.

token: process.env.HUBOT_SLACK_TOKEN
autoReconnect: !exitProcessOnDisconnect
autoMark: true
exitOnDisconnect: exitProcessOnDisconnect
proxyUrl: process.env.https_proxy

Choose a reason for hiding this comment

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

I think the change in the options means this qualifies as a breaking change, and hence will require us to bump the major version number.

Choose a reason for hiding this comment

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

I also want to verify that the reconnection logic is working.

@DEGoodmanWilson
Copy link

DEGoodmanWilson commented Jun 20, 2016

There's another big issue: If you send a message to the bot in a public channel, it responds in a DM—always. The original responded in the same channel.

Correction. It always sends its responses to the first channel that it receives a message in since starting.

@coveralls
Copy link

Coverage Status

Coverage increased (+6.6%) to 67.317% when pulling 710454c on v3-sdk-updates into c0775f4 on master.

@johnagan
Copy link
Author

@DEGoodmanWilson I had to roll back your package.json commit. Looks like you overwrote it with the version from your node_modules folder 😎

@coveralls
Copy link

coveralls commented Jun 21, 2016

Coverage Status

Coverage increased (+7.0%) to 67.633% when pulling 2a66fb4 on v3-sdk-updates into c0775f4 on master.

@coveralls
Copy link

coveralls commented Jul 14, 2016

Coverage Status

Coverage increased (+24.8%) to 85.507% when pulling e2fbdd6 on v3-sdk-updates into c0775f4 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+24.8%) to 85.507% when pulling 09ba54d on v3-sdk-updates into 10fb8b7 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+24.8%) to 85.507% when pulling 09ba54d on v3-sdk-updates into 10fb8b7 on master.

@DEGoodmanWilson DEGoodmanWilson merged commit bed7406 into master Jul 15, 2016
@dnrce
Copy link

dnrce commented Jul 15, 2016

🎉 👍

opadron added a commit to osumo/sumobot that referenced this pull request Aug 26, 2016
 - Should help with sporadic disconnect issues.
   - See slackapi/hubot-slack#309
* Messages from bots are no longer filtered out, which is both cool and potentially terrifying, but we should never have silenced the robots in the first place.
* Remember how if you tried to hack on this adapter and used `npm link` to plug that into a live bot? And how that didn't work? Yeah? Well now it does. Stupid `instanceof`.
* Total refactoring of the functionality, exposing a slightly different interface. So watch out for that.
* You can now access the underlying Slack client directly, for when you really need low-level functionality therein.
Copy link

Choose a reason for hiding this comment

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

Is there documentation available on how to do just that ?

Copy link
Author

Choose a reason for hiding this comment

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

.rtm or .web will return the clients.

.send is just a shortcut to select the best method.

Copy link

Choose a reason for hiding this comment

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

Thanks!

@ianfixes
Copy link

ianfixes commented Jan 20, 2017

If there is documentation on how to upload files or send attachments in hubot slack messages, I'm having a hard time locating it. Here's where I looked:

Can someone point me to a brief example of hubot uploading a file to a slack channel?

@Treyone
Copy link

Treyone commented Feb 7, 2017

@ifreecarve did you find out how to upload files ? Same situation here. Thanks !

@ianfixes
Copy link

ianfixes commented Feb 7, 2017

Elsewhere in this thread, it was noted that

.web or .rtm will return the clients.

So I assume that, worst case, you get the native client and access that method directly. Haven't tried it yet.

@Treyone
Copy link

Treyone commented Feb 8, 2017

Thanks, I'll give it a try !

@ianfixes
Copy link

I'm using robot.adapter.client.web.files.upload to access the function described here: https://api.slack.com/methods/files.upload

@Treyone
Copy link

Treyone commented Feb 10, 2017

Thanks ! the upload works. The binary files are however displayed as text and unreadable, but this seems to be related to another issue.

@ianfixes
Copy link

in contentOpts, use file: fs.createReadStream(filePath).

content: fs.readFileSync(filePath) doesn't work with binary

@aoberoi aoberoi deleted the v3-sdk-updates branch July 4, 2018 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants