HipChat Support #1012

kevsmith · 2016-10-05T18:07:20Z

This PR reinstates HipChat support on top of the new chat provider API.

Supports DMs, rooms, and private rooms
Supports both @ mentions and spoken commands
Full support for Greenbar templates
Replaced AdapterCase and provider-specific helper modules with ProviderCase and provider-specific stateful testing client to simplify integration testing. Tests will wait for an appropriate reply from Cog instead of polling chat history.

Fixes #968

pjmorr · 2016-10-05T18:09:26Z

+1

kevsmith · 2016-10-05T18:29:00Z

Thanks to @pjmorr for testing HipChat support w/his private HipChat server

* Moved chat adapter code into dedicated supervisor * Added the romeo XMPP library as a dep * Made startup of slack and romeo dependent on whether or not Cog.Chat.Slack.Provider and/or Cog.Chat.HipChat.Provider are enabled. * Support for: - Presence events - Accepting room invitations

- DMs detected and handled appropriately - Public MUC room invites work - @-mentioning Cog in a public MUC room works

- User and room caches extracted into separate modules - Basic Greenbar directives processor - Posting room and user messages via HipChat's V2 API

* Added support for Greenbar tables * Removed quick_lookup functions as they could race with HipChat connector initialization and return false negatives.

mpeck · 2016-10-05T18:27:05Z

lib/cog/chat/adapter.ex

-    else
-      nil
+   else
+     updated = Regex.replace(~r/^#{Regex.escape(String.downcase(bot_name))}/, text, "")


Could you do this is the regex to save some duplication?

~r/(^#{Regex.escape(bot_name)})|(^#{Regex.escape(String.downcase(bot_name))})/

mpeck · 2016-10-05T18:33:47Z

lib/cog/chat/hipchat/provider.ex

+      GenServer.call(__MODULE__, {:call_connector, {:lookup_user_jid, handle}}, :infinity)
+    else
+      GenServer.call(__MODULE__, {:call_connector, {:lookup_user_handle, handle}}, :infinity)
+    end


Would it behoove us to change the api for lookup_user to work like lookup_room? Maybe worth adding a issue?

Sure. Add a ticket and tag it with the 0.16 milestone.

Suite is patterned after the Slack tests.

Look up bot user in the XMPP roster and use the corresponding name for room joins.

vanstee · 2016-10-05T18:17:28Z

config/dev.exs

+  api_token: System.get_env("HIPCHAT_API_TOKEN"),
+  nickname: System.get_env("HIPCHAT_NICKNAME"),
+  jabber_id: System.get_env("HIPCHAT_JABBER_ID"),
+  jabber_password: System.get_env("HIPCHAT_JABBER_PASSWORD")


Looks like this is the same as the config changes in config/test.exs. Is this something we could put in the base config/config.exs instead?

vanstee · 2016-10-05T18:18:42Z

config/dev.exs

@@ -13,9 +13,19 @@ config :cog,
  :template_cache_ttl, {1, :sec}

 config :cog, Cog.Chat.Adapter,
-  providers: [slack: Cog.Chat.Slack.Provider],
+  providers: [#slack: Cog.Chat.Slack.Provider,
+              hipchat: Cog.Chat.HipChat.Provider],


I'm guessing we'll fix this up before merging with an env var or something. Just a friendly reminder.

Ah yeah. Joys of poking at $NEW_FEATURE locally.....

Will fix.

vanstee · 2016-10-05T18:22:23Z

lib/cog/chat/adapter.ex

-    else
-      nil
+   else
+     updated = Regex.replace(~r/^#{Regex.escape(String.downcase(bot_name))}/, text, "")


If we can assume that case doesn't matter here a case insensitive regex (/i suffix) might allow you to collapse these two code branches nicely.

vanstee · 2016-10-05T19:11:09Z

lib/cog/chat/hipchat/connector.ex

+    url = Enum.join([state.api_root, "room", room.secondary_id, "notification"], "/") <> "?token=#{state.api_token}"
+    response = HTTPotion.post(url, headers: ["Content-Type": "application/json",
+                                             "Accepts": "application/json",
+                                             "Authorization": "Bearer #{state.api_token}"],


Do we need to set the token as a query param and a header? That's kinda weird.

vanstee · 2016-10-05T19:11:48Z

lib/cog/chat/hipchat/connector.ex

+    response = HTTPotion.post(url, headers: ["Content-Type": "application/json",
+                                             "Accepts": "application/json",
+                                             "Authorization": "Bearer #{state.api_token}"],
+      body: body)


Might also be nice to wrap up this request stuff into a helper function. Totally not necessary though.

vanstee · 2016-10-05T19:33:26Z

lib/cog/repository/chat_providers.ex

+  def get_provider_state(name, default \\ %{}) when is_binary(name) do
+    case Repo.get_by!(ChatProvider, name: name) do
+      nil ->
+        nil


IIRC Repo.get_by! can't ever return nil.

- Extracted Slack and HipChat config into dedicated files - Removed existing HipChat and Slack config from `(dev,test,prod).exs`

Added randomized waits around all Slack API calls to reduce the likelihood of Slack's API throttling kicking in.

test/commands/*.exs are also skipped until we can suss out what is making ExVCR so grumpy.

- Restarts only the Cog.Chat.Adapter process instead of the entire Cog application tree - Added randomized pauses around all Slack API calls to avoid throttling

- Moved integration and unit tests into separate make targets

kevsmith added the review label Oct 5, 2016

Kevin Smith added 14 commits October 5, 2016 15:27

Skip CI if NO_CI file is present

a61bb9d

More HipChat progress

936ae71

- DMs detected and handled appropriately - Public MUC room invites work - @-mentioning Cog in a public MUC room works

More progress

ffed5b2

- User and room caches extracted into separate modules - Basic Greenbar directives processor - Posting room and user messages via HipChat's V2 API

Added persistent state for chat providers

728a0ad

Parameterize all 3 HipChat hosts

a2436a3

Table rendering;User/room lookup tweaks

c59aac6

* Added support for Greenbar tables * Removed quick_lookup functions as they could race with HipChat connector initialization and return false negatives.

Use secondary_id to send room notifications

860287d

Redirect support

bdb3b92

Render attachments

4f63a34

Ported Slack template tests to HipChat

34aab15

Integration test prep

17446cd

Test fixups after rebase

6208011

Stop hardcoding api.hipchat.com

1aa0fba

mpeck reviewed Oct 5, 2016

View reviewed changes

Kevin Smith added 3 commits October 5, 2016 15:29

De-tangling logic around provider-specific integ. tests

1337f32

HipChat integration tests

e1057f2

Suite is patterned after the Slack tests.

Find bot's XMPP name via roster

0b8a3af

Look up bot user in the XMPP roster and use the corresponding name for room joins.

kevsmith force-pushed the kevsmith/hipchat-WIP branch from 1e1b77b to 0b8a3af Compare October 5, 2016 19:31

vanstee approved these changes Oct 5, 2016

View reviewed changes

Kevin Smith added 5 commits October 5, 2016 15:47

Use non-throwing version of Repo.get_by/2

1aab1d4

Moved REST API calls to helper function

62c5773

DRY-ing up provider config

58019d3

- Extracted Slack and HipChat config into dedicated files - Removed existing HipChat and Slack config from `(dev,test,prod).exs`

Replaced ~r with ~i

afcd6ef

Updated test excludes to reflect current state

cda66f4

kevsmith force-pushed the kevsmith/hipchat-WIP branch from adce384 to 9e98970 Compare October 5, 2016 21:05

Put :chat config key back for now

5773f26

kevsmith force-pushed the kevsmith/hipchat-WIP branch from 9e98970 to 5773f26 Compare October 5, 2016 21:26

Kevin Smith added 8 commits October 6, 2016 13:54

Reduce likelihood of Slack throttling

b57a30b

Added randomized waits around all Slack API calls to reduce the likelihood of Slack's API throttling kicking in.

Hoist "integration" tests into unit tests

ce297aa

test/commands/*.exs are also skipped until we can suss out what is making ExVCR so grumpy.

Streamline restarts;Prevent Slack API throttling

8a78bcd

- Restarts only the Cog.Chat.Adapter process instead of the entire Cog application tree - Added randomized pauses around all Slack API calls to avoid throttling

Removed dead test

5be0cc2

Refactored config to more test-friendly form

8584571

Bumped timeout to allow for slow HipChat responses

b1b693e

Tagged w/integration test

13497d1

Separated integration and unit tests

c2dbc11

- Moved integration and unit tests into separate make targets

kevsmith merged commit f81973d into master Oct 6, 2016

kevsmith removed the review label Oct 6, 2016

kevsmith deleted the kevsmith/hipchat-WIP branch October 6, 2016 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HipChat Support #1012

HipChat Support #1012

kevsmith commented Oct 5, 2016

pjmorr commented Oct 5, 2016

kevsmith commented Oct 5, 2016

mpeck Oct 5, 2016

mpeck Oct 5, 2016

kevsmith Oct 5, 2016

vanstee Oct 5, 2016

vanstee Oct 5, 2016

kevsmith Oct 5, 2016

vanstee Oct 5, 2016

vanstee Oct 5, 2016

vanstee Oct 5, 2016

vanstee Oct 5, 2016

HipChat Support #1012

HipChat Support #1012

Conversation

kevsmith commented Oct 5, 2016

pjmorr commented Oct 5, 2016

kevsmith commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment