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
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
744359d
updated logic and tests to support v3 of node-slack-sdk
Jun 16, 2016
e5dd2f8
clean up unused functions
Jun 16, 2016
c244c3d
reverted clean up of unused functions
Jun 17, 2016
cd61c60
added some coveralls tests and ignores
Jun 17, 2016
62062c7
No function getBody()
Jun 20, 2016
710454c
Repair the missing getBody method, not updated in testing
Jun 20, 2016
7441fad
This should handle a number of the errors we've seen
Jun 21, 2016
d5b5cd7
corrected package.json
Jun 21, 2016
2a66fb4
fixed channel test
Jun 21, 2016
7679d26
Initial attempt at fixing #114 to correct formatting urls
Jun 27, 2016
900402e
an attempt at rewriting to have better support for complex messages a…
Jul 1, 2016
93d01dc
adjusted method names
Jul 1, 2016
0224285
adjusted method names
Jul 1, 2016
09514bf
Merge pull request #314 from slackhq/hubot-v4-proto
Jul 1, 2016
9153bf5
Fix errors handling message with un-handled subtypes that may or may …
Jul 1, 2016
4e078ac
Getting at least some of the tests to pass. This is terrible.
Jul 1, 2016
aef4233
Tests are running...and passing!
Jul 5, 2016
12bcff4
Look at that, so many green lights.
Jul 5, 2016
705ffd2
All original passing tests are now in and passing.
Jul 5, 2016
ad68cdd
Fixing Silliness
Jul 5, 2016
3bfdc18
Don't need that! Doesn't work in the stub anyway.
Jul 5, 2016
341e54d
This is an experiment
Jul 5, 2016
8a8d19d
Make this node 0.10 compatible
Jul 5, 2016
09d4dd6
Merge pull request #315 from slackhq/v3-sdk-updates-writing-tests
Jul 5, 2016
306142b
More tests on SlackFormatter, and a bug fix!
Jul 5, 2016
d3e7f53
Clean up beforeEach triggers
Jul 5, 2016
48de700
Adding tests for client
gtr32x Jul 8, 2016
881b5a9
Filling in the remainder of the tests for client
gtr32x Jul 11, 2016
2aab972
Updating package.json to reflect future reality
Jul 12, 2016
d93be77
added robot logger coverage
Jul 14, 2016
3e6ae9e
Making the tests a little more specific.
Jul 14, 2016
43d3c65
Tests around replying...and found a bug
Jul 14, 2016
179b424
Small victories
Jul 14, 2016
7249c89
Adding tests on receiving messages
Jul 14, 2016
e85b67d
One more test, commiting to set it aside.
Jul 14, 2016
1def6e8
Fix that broken test.
Jul 14, 2016
25a4b6c
Message handling tests
Jul 14, 2016
b5683c3
Fixing some errors, writing some tests
Jul 14, 2016
4f3f430
There is no bot_message anymore
Jul 14, 2016
8a21aa3
CHeck error event handling
Jul 14, 2016
e2fbdd6
Merge pull request #321 from slackhq/v3-coverage
Jul 14, 2016
4639968
Updating the docs
Jul 15, 2016
2d3160e
Minor tweaks to README
Jul 15, 2016
e71cf7c
Merge pull request #322 from slackhq/v3-docs
Jul 15, 2016
e375beb
Because Hubot adapters have very strict naming restrictions
Jul 15, 2016
09ba54d
Merge branch 'master' into v3-sdk-updates
Jul 15, 2016
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gruntfile.coffee
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
### istanbul ignore next ###
module.exports = (grunt) ->
grunt.loadNpmTasks 'grunt-release'
grunt.loadNpmTasks 'grunt-contrib-watch'
Expand Down
12 changes: 10 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "hubot-slack",
"version": "3.4.2",
"version": "3.5.0",
"description": "A Slack adapter for hubot",
"main": "./index",
"scripts": {
Expand All @@ -24,7 +24,7 @@
"url": "https://github.com/slackhq/hubot-slack/issues"
},
"dependencies": {
"slack-client": "~1.5"
"@slack/client": "^3.4.0"
},
"devDependencies": {
"coffee-coverage": "^1.0.1",
Expand Down Expand Up @@ -65,6 +65,14 @@
{
"name": "Eric Lindvall",
"email": "eric@papertrailapp.com"
},
{
"email": "don@slack-corp.com",
"name": "D.E. Goodman-Wilson"
},
{
"email": "jagan@slack-corp.com",
"name": "John Agan"
}
]
}
112 changes: 47 additions & 65 deletions src/slack.coffee
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
{Robot, Adapter, EnterMessage, LeaveMessage, TopicMessage} = require 'hubot'
{SlackTextMessage, SlackRawMessage, SlackBotMessage} = require './message'
{SlackRawListener, SlackBotListener} = require './listener'

SlackClient = require 'slack-client'
Util = require 'util'
# {SlackRawListener, SlackBotListener} = require './listener'
{RtmClient, MemoryDataStore} = require '@slack/client'

class SlackBot extends Adapter
@MAX_MESSAGE_LENGTH: 4000
Expand All @@ -17,33 +15,31 @@ class SlackBot extends Adapter
exitProcessOnDisconnect = !!process.env.HUBOT_SLACK_EXIT_ON_DISCONNECT

# Take our options from the environment, and set otherwise suitable defaults
options =
@options =
autoMark: true
logLevel: 'error'
dataStore: new MemoryDataStore()
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.

return @robot.logger.error "No services token provided to Hubot" unless options.token
return @robot.logger.error "v2 services token provided, please follow the upgrade instructions" unless (options.token.substring(0, 5) in ['xoxb-', 'xoxp-'])

@options = options
return @robot.logger.error "No services token provided to Hubot" unless @options.token
return @robot.logger.error "v2 services token provided, please follow the upgrade instructions" unless (@options.token.substring(0, 5) in ['xoxb-', 'xoxp-'])

# Create our slack client object
@client = new SlackClient options.token, options.autoReconnect, options.autoMark, options.proxyUrl
@client = new RtmClient @options.token, @options

# Setup event handlers
# TODO: Handle eventual events at (re-)connection time for unreads and provide a config for whether we want to process them
@client.on 'error', @.error
@client.on 'loggedIn', @.loggedIn
@client.on 'open', @.open
@client.on 'close', @.clientClose
@client.on 'message', @.message
@client.on 'userChange', @.userChange
@robot.brain.on 'loaded', @.brainLoaded
@client.on 'error', @error
@client.on 'loggedIn', @loggedIn
@client.on 'open', @open
@client.on 'close', @clientClose
@client.on 'message', @message
@client.on 'userChange', @userChange
@robot.brain.on 'loaded', @brainLoaded

@robot.on 'slack-attachment', @.customMessage
@robot.on 'slack.attachment', @.customMessage
@robot.on 'slack-attachment', @customMessage
@robot.on 'slack.attachment', @customMessage

# Start logging in
@client.login()
Expand All @@ -62,12 +58,12 @@ class SlackBot extends Adapter
# Provide our name to Hubot
@robot.name = self.name

for id, user of @client.users
for id, user of @client.dataStore.users
@userChange user

brainLoaded: =>
# once the brain has loaded, reload all the users from the client
for id, user of @client.users
for id, user of @client.dataStore.users
@userChange user

# also wipe out any broken users stored under usernames instead of ids
Expand All @@ -82,9 +78,9 @@ class SlackBot extends Adapter
email_address: user.profile.email
slack: {}
for key, value of user
# don't store the SlackClient, because it'd cause a circular reference
# don't store the RtmClient, because it'd cause a circular reference
# (it contains users and channels), and because it has sensitive information like the token
continue if value instanceof SlackClient
continue if value instanceof RtmClient
newUser.slack[key] = value

if user.id of @robot.brain.data.users
Expand All @@ -103,23 +99,28 @@ class SlackBot extends Adapter
clientClose: =>
if @options.exitOnDisconnect
@robot.logger.info 'Slack client connection was closed, exiting hubot process'
@client.removeListener 'error', @.error
@client.removeListener 'loggedIn', @.loggedIn
@client.removeListener 'open', @.open
@client.removeListener 'close', @.clientClose
@client.removeListener 'message', @.message
@client.removeListener 'userChange', @.userChange
@client.removeListener 'error', @error
@client.removeListener 'loggedIn', @loggedIn
@client.removeListener 'open', @open
@client.removeListener 'close', @clientClose
@client.removeListener 'message', @message
@client.removeListener 'userChange', @userChange
process.exit 1
else
@robot.logger.info 'Slack client closed, waiting for reconnect'

message: (msg) =>
# Ignore our own messages
return if msg.user == @self.id
return if msg.user and msg.user == @id

channel = @client.dataStore.getChannelGroupOrDMById msg.channel if msg.channel
user = @client.dataStore.getUserById msg.user if msg.user
user.room ?= msg.channel if channel and user

channel = @client.getChannelGroupOrDMByID msg.channel if msg.channel
rawText = msg.getBody()

Choose a reason for hiding this comment

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

Getting ERROR TypeError: msg.getBody is not a function here.

text = @removeFormatting rawText

if msg.hidden or (not msg.text and not msg.attachments) or msg.subtype is 'bot_message' or not msg.user or not channel
if msg.hidden or (not rawText and not msg.attachments) or msg.subtype is 'bot_message' or not msg.user or not channel
# use a raw message, so scripts that care can still see these things

if msg.user
Expand All @@ -131,8 +132,6 @@ class SlackBot extends Adapter
user.name = msg.username if msg.username?
user.room = channel.name if channel

rawText = msg.getBody()
text = @removeFormatting rawText

if msg.subtype is 'bot_message'
@robot.logger.debug "Received bot message: '#{text}' in channel: #{channel?.name}, from: #{user?.name}"
Expand All @@ -142,10 +141,6 @@ class SlackBot extends Adapter
@receive new SlackRawMessage user, text, rawText, msg
return

# Process the user into a full hubot user
user = @robot.brain.userForId msg.user
user.room = channel.name

# Test for enter/leave messages
if msg.subtype is 'channel_join' or msg.subtype is 'group_join'
@robot.logger.debug "#{user.name} has joined #{channel.name}"
Expand All @@ -160,14 +155,10 @@ class SlackBot extends Adapter
@receive new TopicMessage user, msg.topic, msg.ts

else
# Build message text to respond to, including all attachments
rawText = msg.getBody()
text = @removeFormatting rawText

@robot.logger.debug "Received message: '#{text}' in channel: #{channel.name}, from: #{user.name}"
@robot.logger.debug "Received message: '#{text}' in channel: #{user.room}, from: #{user.name}"

# If this is a DM, pretend it was addressed to us
if msg.getChannelType() == 'DM'
if msg.channel[0] == 'D'
text = "#{@robot.name} #{text}"

@receive new SlackTextMessage user, text, rawText, msg
Expand All @@ -188,13 +179,13 @@ class SlackBot extends Adapter

when '@'
if label then return label
user = @client.getUserByID link
user = @client.dataStore.getUserById link
if user
return "@#{user.name}"

when '#'
if label then return label
channel = @client.getChannelByID link
channel = @client.dataStore.getChannelById link
if channel
return "\##{channel.name}"

Expand All @@ -214,21 +205,12 @@ class SlackBot extends Adapter
text

send: (envelope, messages...) ->
channel = @client.getChannelGroupOrDMByName envelope.room
channel = @client.getChannelGroupOrDMByID(envelope.room) unless channel

if not channel and @client.getUserByName(envelope.room)
user_id = @client.getUserByName(envelope.room).id
@client.openDM user_id, =>
this.send envelope, messages...
return

for msg in messages
continue if msg.length < SlackBot.MIN_MESSAGE_LENGTH

# Replace @username with <@UXXXXX> for mentioning users and channels
msg = msg.replace /(?:^| )@([\w]+)/gm, (match, p1) =>
user = @client.getUserByName p1
user = @client.dataStore.getUserByName p1
if user
match = match.replace /@[\w]+/, "<@#{user.id}>"
else if p1 in SlackBot.RESERVED_KEYWORDS
Expand All @@ -237,9 +219,9 @@ class SlackBot extends Adapter
match = match

@robot.logger.debug "Sending to #{envelope.room}: #{msg}"

if msg.length <= SlackBot.MAX_MESSAGE_LENGTH
channel.send msg
@client.sendMessage msg, envelope.room

Choose a reason for hiding this comment

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

Why did this logic get cut? Did I do that?

Copy link
Author

@johnagan johnagan Jun 20, 2016

Choose a reason for hiding this comment

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

Nope. I cut it because we're send from the client now, rather than the channel object. I didn't see where else we were using channel here and figured it could go.

Very possible I missed something though.

Choose a reason for hiding this comment

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

It looks like if it doesn't find a valid channel in the envelope, it opens a DM with the sending user. The reason and purpose of this logic isn't totally clear to me, but it is a a bit of functionality we probably shouldn't elide.

# If message is greater than MAX_MESSAGE_LENGTH, split it into multiple messages
else
Expand Down Expand Up @@ -271,7 +253,7 @@ class SlackBot extends Adapter

msg = msg.substring(breakIndex, msg.length)

channel.send m for m in submessages
@client.sendMessage(m, envelope.room) for m in submessages

reply: (envelope, messages...) ->
@robot.logger.debug "Sending reply"
Expand All @@ -281,7 +263,7 @@ class SlackBot extends Adapter
@send envelope, "<@#{envelope.user.id}>: #{msg}"

topic: (envelope, strings...) ->
channel = @client.getChannelGroupOrDMByName envelope.room
channel = @client.dataStore.getChannelGroupOrDMByName envelope.room
channel.setTopic strings.join "\n"

customMessage: (data) =>
Expand All @@ -292,8 +274,8 @@ class SlackBot extends Adapter
data.message.envelope.room
else data.message.room

channel = @client.getChannelGroupOrDMByName channelName
channel = @client.getChannelGroupOrDMByID(channelName) unless channel
channel = @client.dataStore.getChannelGroupOrDMByName channelName
channel = @client.dataStore.getChannelGroupOrDMById(channelName) unless channel
return unless channel

msg = {}
Expand Down
29 changes: 29 additions & 0 deletions test/listener.coffee
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{SlackRawMessage, SlackBotMessage, SlackTextMessage} = require '../src/message'
{SlackBotListener, SlackRawListener} = require '../src/listener'
should = require 'should'


describe 'Listeners', ->

it 'Should fail on incorrect message type', ->
callback = (response) ->
matcher = (message) -> message.text.match /Hello/

listener = new SlackRawListener(@stubs.robot, matcher, callback)
listener.call().should.be.false


it 'Should call a SlackRawListener', ->
callback = (response) ->
matcher = (message) -> message.text.match /Hello/

message = new SlackRawMessage(@stubs.user.id, 'Hello world', 'Hello world', {ts: 1234})
listener = new SlackRawListener(@stubs.robot, matcher, callback)
listener.call(message).should.be.true


it 'Should call a SlackBotListener', ->
callback = (response) ->
message = new SlackBotMessage(@stubs.user.id, 'Hello world', 'Hello world', {ts: 1234})
listener = new SlackBotListener(@stubs.robot, /Hello/, callback)
listener.call(message).should.be.true
1 change: 1 addition & 0 deletions test/slack.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

should = require 'should'


describe 'Adapter', ->
it 'Should initialize with a robot', ->
@slackbot.robot.should.eql @stubs.robot
Expand Down
Loading