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.

Sign up for GitHub

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

Jump to bottom

[BREAKING CHANGE] Replace rtm.start with rtm.connect #184

Merged
merged 14 commits into from Feb 23, 2020
Merged

[BREAKING CHANGE] Replace rtm.start with rtm.connect #184

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0545ba6
WIP fix rtm connect issues
acconrad Mar 18, 2019
bdf2f1b
Fix tests and add deprecation
acconrad Mar 30, 2019
8efc5ad
Formatting
acconrad Mar 30, 2019
139b8f5
resolve conflict
acconrad Apr 12, 2019
933bca0
Bump to major version to properly remove rtm.start
acconrad Apr 16, 2019
afce9c2
Merge branch 'master' into acc-fix-rtm-connect
acconrad Apr 16, 2019
6435955
Bump to major version to properly remove rtm.start
acconrad Apr 16, 2019
1c2d347
Merge branch 'acc-fix-rtm-connect' of github.com:acconrad/Elixir-Slac…
acconrad Apr 16, 2019
edd81ca
fix conflicts
acconrad Apr 23, 2019
26a435a
merge conflicts
acconrad Nov 26, 2019
1fd95af
Update README.md
acconrad Nov 27, 2019
33a01e7
merge conflict
acconrad Dec 29, 2019
7956cc6
merge downstream
acconrad Dec 29, 2019
311bbd7
fix readme
acconrad Feb 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions lib/slack/bot.ex
View file
Open in desktop
Expand Up @@ -87,11 +87,11 @@ defmodule Slack.Bot do
token: token,
me: rtm.self,
team: rtm.team,
bots: rtm_list_to_map(rtm.bots),
channels: rtm_list_to_map(rtm.channels),
groups: rtm_list_to_map(rtm.groups),
users: rtm_list_to_map(rtm.users),
ims: rtm_list_to_map(rtm.ims)
bots: rtm_list_to_map(Slack.Web.Bots.info(%{token: token}) |> Map.get("bot")),
Copy link
Owner

Choose a reason for hiding this comment

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

This would kick off 5 different requests at once which I don't think is ideal. Is there no equivalent API to rtm.start to bootstrap the client?

Copy link
Owner

Choose a reason for hiding this comment

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

I can't link to it directly, but https://api.slack.com/rtm has rtm.connect vs rtm.start. It looks like both are still valid.

I'd be okay with reverting the change to use rtm.connect meaning the library will continue to only support rtm.start until someone takes the time to add rtm.connect support (assuming anyone needs it over rtm.start).

That being said, it does look like this approach of making multiple requests is how Slack recommends you use rtm.connect.

Copy link
Collaborator Author

@acconrad acconrad Mar 18, 2019 •
edited

Choose a reason for hiding this comment

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

That being said, it does look like this approach of making multiple requests is how Slack recommends you use rtm.connect.

Correct, it will fire off 5 more requests and this is the recommended way to do this. That, and anyone using the Enterprise Grid with rtm.start will not work as expected if we stick with the old solution.

The other option would be to not populate the slack state object you have in the first place. Save that for when people optimistically decide to pull channels, users, whatever (if ever). What do you think about that?

Copy link
Owner

Choose a reason for hiding this comment

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

That is true. I'm not super concerned about supporting it but if we want to move forward with this and we get support at the same time I'm all for it 👍

channels: rtm_list_to_map(Slack.Web.Channels.list(%{token: token}) |> Map.get("channels")),
groups: rtm_list_to_map(Slack.Web.Groups.list(%{token: token}) |> Map.get("groups")),
users: rtm_list_to_map(Slack.Web.Users.list(%{token: token}) |> Map.get("members")),
ims: rtm_list_to_map(Slack.Web.Im.list(%{token: token}) |> Map.get("ims"))
}

{:reconnect, %{slack: slack, bot_handler: bot_handler, process_state: initial_state}}
Expand Down
15 changes: 15 additions & 0 deletions lib/slack/web/docs/bots.info.json
View file
Open in desktop
@@ -0,0 +1,15 @@
{
"desc": "Gets information about a bot user.",
"args": {
"token": {
Copy link
Contributor

Choose a reason for hiding this comment

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

No other doc file lists token. Listing it results in the following error:

    (hackney) .../deps/hackney/src/hackney_bstr.erl:35: :hackney_bstr.to_binary/1
    (hackney) .../deps/hackney/src/hackney_url.erl:313: :hackney_url.urlencode/2
    (hackney) .../deps/hackney/src/hackney_url.erl:373: :hackney_url.qs/3
    (hackney) .../deps/hackney/src/hackney_request.erl:312: :hackney_request.encode_form/1
    (hackney) .../deps/hackney/src/hackney_request.erl:320: :hackney_request.handle_body/4
    (hackney) .../deps/hackney/src/hackney_request.erl:83: :hackney_request.perform/2
    (hackney) .../deps/hackney/src/hackney.erl:373: :hackney.send_request/2
    (httpoison) lib/httpoison/base.ex:787: HTTPoison.Base.request/6
    (httpoison) lib/httpoison.ex:128: HTTPoison.request!/5
    (slack) lib/slack/web/default_client.ex:23: Slack.Web.DefaultClient.post!/2

"example" : "xxxx-xxxxxxxxx-xxxx",
"required": true,
"desc" : "Authentication token bearing required scopes."
},
"bot": {
"example" : "B12345678",
"required": false,
"desc" : "Bot user to get info on"
}
}
}
19 changes: 8 additions & 11 deletions test/bot_test.exs
View file
Open in desktop
Expand Up @@ -8,12 +8,7 @@ defmodule Slack.BotTest do
@rtm %{
Copy link
Contributor

Choose a reason for hiding this comment

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

It's best to use a fake test web server here and instead of mocks later on. It's relatively safe to assume that unless Slack API misbehaves, the data will arrive as a well formed JSON. The tests will then catch issues with assuming keys to atoms conversion and others.

Mocking results of processing outside data leads to green tests that are likely to lie.

url: "http://example.com",
self: %{name: "fake"},
team: %{name: "Foo"},
bots: [%{id: "123"}],
channels: [%{id: "123"}],
groups: [%{id: "123"}],
users: [%{id: "123"}],
ims: [%{id: "123"}]
team: %{name: "Foo"}
}

test "init formats rtm results properly" do
Expand All @@ -29,11 +24,13 @@ defmodule Slack.BotTest do
assert bot_handler == Bot
assert slack.me.name == "fake"
assert slack.team.name == "Foo"
assert slack.bots == %{"123" => %{id: "123"}}
assert slack.channels == %{"123" => %{id: "123"}}
assert slack.groups == %{"123" => %{id: "123"}}
assert slack.users == %{"123" => %{id: "123"}}
assert slack.ims == %{"123" => %{id: "123"}}

# TODO: how do we test these now that they are coming from the web API?
# assert slack.bots == %{"123" => %{id: "123"}}
# assert slack.channels == %{"123" => %{id: "123"}}
# assert slack.groups == %{"123" => %{id: "123"}}
# assert slack.users == %{"123" => %{id: "123"}}
# assert slack.ims == %{"123" => %{id: "123"}}
end

defmodule Stubs.Slack.Rtm do
Expand Down