Improve flexibility of Slack connector #4812

kearnsw · 2019-11-21T05:21:57Z

Proposed changes:
By default:

Bot responds when mentioned with @ symbol
Bot responds when directly messaged

Previously, this functionality was either impossible or difficult for a new user to implement. See full description in #4811

Status (please check what you already did):

made PR ready for code review
added some tests for the functionality
updated the documentation
updated the changelog
reformat files using black (please check Readme for instructions)

tmbo

thanks a lot for the slack connector improvement 🚀

I added a couple suggestions and I think we need to take a closer look at that channel name being stored in the input channel.

tmbo · 2019-11-22T14:57:22Z

rasa/core/channels/slack.py

+                        or is_app_mention(output)
+                        or channel == self.slack_channel
+                    ):
+                        self.set_output_channel(channel)


we can't change the channel of the slack input. it is shared between all the async incoming messages so you might change it while another request is also getting processed (this problem might not surface right now, if the asyncs & awaits are at the right )

I think we should remove that object state on the input channel and directly pass it to the output channel

@tmbo , I am using the metadata object to store this value as well as others related to a specific user request. I think we'll need to explore incorporating the new converation session feature from 1.6.0.

tmbo · 2019-11-22T14:58:20Z

rasa/core/channels/slack.py

@@ -109,6 +109,20 @@ def _get_text_from_slack_buttons(buttons):
        return super(SlackBot, self).api_call("chat.postMessage", **json_message)


+def is_app_mention(user_message) -> bool:


please add tests for these as well as type annotations for the parameters & a doc string

I've added the type annotations and standardized these with the other functions in the SlackInput channel. However, I'm not certain how to proceed with the test cases as the current channel tests for slack do not have a token. Is there a mock testing kit you'd prefer that I use for these?

erohmensing · 2019-12-16T18:56:26Z

@kearnsw are you still planning to move forward with this PR?

kearnsw · 2019-12-17T17:25:38Z

@erohmensing: Yes, I am not intimately familiar with the way channels work so I need to spend some time reviewing that part of the codebase as @tmbo suggests.

wochinge · 2020-01-10T15:05:52Z

@kearnsw Let us know if you have any questions or need feedback :-)

kearnsw · 2020-01-16T18:27:38Z

@kearnsw Let us know if you have any questions or need feedback :-)

There was a typing issue that slipped in when I made the prior updates. I've uploaded the latest after testing everything with pytype and formatted the code with black. @wochinge, can you provide feedback on the updates? I have taken @tmbo's suggestion and am no longer setting the output channel property, and instead using the metadata parameter to store request specific information. The current implementation will leave the output channel configured static, while allowing for async messages based on the request from the sender.

wochinge

@kearnsw Thanks for your great work! Left a bit of feedback, but already looks great 🎉

rasa/core/channels/slack.py

wochinge · 2020-01-22T09:02:39Z

rasa/core/channels/slack.py

+        event = slack_event.get("event")
+
+        metadata = {}
+        metadata["out_channel"] = event.get("channel")


I just checked https://api.slack.com/types/event and there the channel param is part of item? Would also be awesome to add the link to https://api.slack.com/types/event as a comment in the code

That's odd. This is the event object that I receive from the slack connector:

{ 'client_msg_id': 'XXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX, 'type': 'message', 'text': 'hi', 'user': 'XXXXXXXXX', 'ts': '1579802617.000800', 'team': 'XXXXXXXXX', 'blocks': [{'type': 'rich_text', 'block_id': 'XXXXX', 'elements': [{'type': 'rich_text_section', 'elements': [{'type': 'text', 'text': 'hi'}]}]}], 'channel': 'XXXXXXXXX', 'event_ts': '1579802617.000800', 'channel_type': 'im' }

wochinge · 2020-01-22T09:04:01Z

rasa/core/channels/slack.py

-                        output.get("event").get("user"),
-                        metadata,
-                    )
+                    if (


what do we need this condition for?

This is a holdover from the original codebase. I added the checks for direct message and app mention events. I think the response text should probably be set to "Received message on unsupported channel" or something to that effect instead of a blank response.

Ok 👍 But then let's move it up to the condition in line 386 right? you could also write a helper function is_supported_channel(output, metadata) and then have a condition like that:

elif self_is_user_message(output) and self._is_supported_channel(output): ... else: logger.warning(f"Received message on unsupported channel: {metadata["out_channel"]}")

I would rather log the "unsupport channel" case instead of returning it as part response to the HTTP request, because Slack is just throwing away this message, while logger.warning(...) will show it to the user.

wochinge · 2020-01-22T09:04:23Z

rasa/core/channels/slack.py

@@ -311,6 +329,18 @@ def _get_interactive_response(action: Dict) -> Optional[Text]:

        return response.text("")

+    def get_metadata(self, request: Request):


Can we please unit test this method?

kearnsw · 2020-01-23T13:49:37Z

@wochinge , can you advise on how to unit test the slack connector, since this requires API credentials. I do not see a precedent in the test suite currently. Thank you!

wochinge · 2020-01-23T16:45:44Z

@kearnsw Which methods do you want to unit test? I suggest targeting the methods individually so you don't have to deal with any actual API request (e.g. is_app_mention can be tested with a dictionary which you pass in). Does that make sense?

kearnsw · 2020-01-23T17:13:30Z

@kearnsw Which methods do you want to unit test? I suggest targeting the methods individually so you don't have to deal with any actual API request (e.g. is_app_mention can be tested with a dictionary which you pass in). Does that make sense?

I can proceed that way. I wasn't sure if there would be interest in setting up the CI/CD pipeline with environment variables for testing the Slack interface, in case there are breaking changes to the Slack API. Thanks for the feedback, @wochinge!

Co-Authored-By: Tobias Wochinger <mail@tobias-wochinger.de>

…t API

outdated

wochinge

Thanks for your work and great documentation. Really appreciate it 🎉
Could you please also add a changelog? Thanks!

rasa/core/channels/slack.py

wochinge · 2020-01-30T09:47:05Z

rasa/core/channels/slack.py

+            request: a `Request` object that contains a slack API event in the body.
+
+        Returns:
+            A `dict` containing the output channel for the response, the text from the user, the sender's ID, and


Let's avoid repeating the type in the docstring. Makes it harder to maintain if we ever change the return types. Note that you probably have to reformat my suggestion cause it's too long for a single line.

Suggested change

A `dict` containing the output channel for the response, the text from the user, the sender's ID, and

Metadata extracted from the sent event payload. This includes the output channel for the response, the text from the user and users that have installed the bot.

@kearnsw Have you seen that one?

wochinge · 2020-01-30T09:52:11Z

rasa/core/channels/slack.py

-                        output.get("event").get("user"),
-                        metadata,
-                    )
+                    if (


Ok 👍 But then let's move it up to the condition in line 386 right? you could also write a helper function is_supported_channel(output, metadata) and then have a condition like that:

elif self_is_user_message(output) and self._is_supported_channel(output): ... else: logger.warning(f"Received message on unsupported channel: {metadata["out_channel"]}")

wochinge · 2020-01-30T09:58:39Z

rasa/core/channels/slack.py

-                        output.get("event").get("user"),
-                        metadata,
-                    )
+                    if (


I would rather log the "unsupport channel" case instead of returning it as part response to the HTTP request, because Slack is just throwing away this message, while logger.warning(...) will show it to the user.

wochinge · 2020-01-30T10:00:03Z

rasa/core/channels/slack.py

-    def get_output_channel(self) -> OutputChannel:
-        return SlackBot(self.slack_token, self.slack_channel)
+    def get_output_channel(self, channel: Optional[Text] = None) -> OutputChannel:
+        if channel is not None:


optional: I'd turn it around (if is None else ...) because it's a bit easier to read (you don't have to negate in your head), but that's really just a personal tase

tests/core/test_channels.py

wochinge · 2020-01-30T10:33:11Z

tests/core/test_channels.py

@@ -462,6 +462,50 @@ def test_botframework_attachments():
    assert ch.add_attachments_to_metadata(payload, metadata) == updated_metadata


+def test_slack_metadata():


Thanks for adding the test 🚀

rasa/core/channels/slack.py

Co-Authored-By: Tobias Wochinger <mail@tobias-wochinger.de>

wochinge · 2020-02-04T08:44:32Z

rasa/core/channels/slack.py

@@ -411,7 +411,8 @@ def blueprint(
        return slack_webhook

    def get_output_channel(self, channel: Optional[Text] = None) -> OutputChannel:
-        if channel is not None:
+        channel = channel or self.slack_channel
+        return SlackBot(self.slack_token, channel)
            return SlackBot(self.slack_token, channel)


We should still delete the lines after

wochinge · 2020-02-11T08:27:12Z

CHANGELOG.rst

@@ -54,6 +54,11 @@ Features
  ``intent_tokenization_flag``: Flag to check whether to split intents (default ``False``).
  ``intent_split_symbol``: Symbol on which intent should be split (default ``_``)

+- `#4811 <https://github.com/RasaHQ/rasa/issues/4811>`_: Support invoking a ``SlackBot`` by direct messaging or @ mentions.


@kearnsw Please don't edit the Changelog.rst manually. Instead, create a file 4812.improvement.rst in the changelog directory (the process is also described in here https://github.com/RasaHQ/rasa/blob/master/changelog/README.md )

wochinge · 2020-02-11T08:27:57Z

docs/user-guide/connectors/slack.rst

-  the argument to post DMs to the bot.
+- The ``slack_channel`` is a channel that the bot will listen to for messages
+  in addition to direct messages and app mentions, i.e. "@app_name".
+  This can be a channel or an individual person.


what do you mean by "This can be a channel or an individual person."?

@wochinge, this is from the original documentation. I added the part about "in addition to direct messages and app mentions, i.e. "@app_name". I'll try to clarify this sentence overall.

wochinge · 2020-02-11T08:32:51Z

tests/core/test_channels.py

+        slack_token="YOUR_SLACK_TOKEN", slack_channel="YOUR_SLACK_CHANNEL"
+    )
+
+    r = Request(None, None, None, None, None, app="test")


we recently discovered that this will lead to an error when we upgrade dependencies, let's rather do:

Suggested change

r = Request(None, None, None, None, None, app="test")

r = Mock()

r.json = direct_message_event

kearnsw · 2020-02-11T17:18:44Z

The build is failing on tests unrelated to the modified code. Can you advise @wochinge ?

wochinge · 2020-02-17T08:14:45Z

Hi @kearnsw ,

can you please rebase to / merge in master ? We switched to GitHub actions which are more reliable and also faster.

wochinge · 2020-02-17T08:15:09Z

CHANGELOG.rst

@@ -1,8 +1,8 @@
-:desc: Rasa Open Source Changelog
+:desc: Rasa OSS Changelog


can you please undo this change?

wochinge · 2020-02-17T08:15:12Z

CHANGELOG.rst



-Rasa Open Source Change Log
-===========================
+Rasa OSS Change Log


can you please undo this change?

wochinge · 2020-02-17T08:15:22Z

CHANGELOG.rst

@@ -17,89 +17,6 @@ This project adheres to `Semantic Versioning`_ starting with version 1.0.

 .. towncrier release notes start

-[1.7.0] - 2020-01-29


can you please undo this change?

wochinge · 2020-02-17T08:15:44Z

changelog/4811.improvement.rst

@@ -0,0 +1 @@
+Support invoking a ``SlackBot`` by direct messaging or @ mentions.


Suggested change

Support invoking a ``SlackBot`` by direct messaging or @ mentions.

Support invoking a ``SlackBot`` by direct messaging or ``@<app name>`` mentions.

rasa/core/channels/slack.py

wochinge · 2020-02-17T08:20:17Z

rasa/core/channels/slack.py

+        event = slack_event.get("event", {})
+        return {
+            "out_channel": event.get("channel"),
+            "text": event.get("text"),


it feels a bit weird that the metadata also returns sender and text. The metadata is meant to provide additional information around a message. text and sender are the message. So in my opinion it would be a bit cleaner to keep this separated, what you think?

kearnsw · 2020-02-21T00:20:42Z

@wochinge, looks like the build environment errored out.

CHANGELOG.rst

Co-Authored-By: Tobias Wochinger <mail@tobias-wochinger.de>

wochinge · 2020-02-24T18:02:30Z

@kearnsw Can you please update with the latest master again?

wochinge · 2020-02-26T10:08:01Z

@kearnsw I'm sorry, but you have to merge in the latest master state again 🙈

kearnsw · 2020-02-26T15:04:13Z

Hi @wochinge , I’m not understanding the errors for the past few merge builds. Are these backend issues or problems with the submitted code?

tmbo · 2020-02-26T23:32:37Z

Looks all good 👍

Will Kearns added 2 commits November 20, 2019 20:51

fix to allow the bot to respond when mentioned or directly messaged

6eca764

allow bot to communicate without mention on a configured channel

f5aa081

tmbo previously requested changes Nov 22, 2019

View reviewed changes

Will Kearns and others added 6 commits December 17, 2019 11:15

Resolve merge conflicts

aec05aa

add output channel to metadata

3450447

Merge branch 'master' into master

6a5e21b

make message parsing methods private

559b727

Merge branch 'master' of https://github.com/kearnsw/rasa

c6cc6ac

update to create a new channel for each user interaction

995cace

fix typing error

097dc97

wochinge reviewed Jan 22, 2020

View reviewed changes

kearnsw and others added 4 commits January 23, 2020 09:22

Apply suggestions from code review

d5907f9

Co-Authored-By: Tobias Wochinger <mail@tobias-wochinger.de>

add test case and comment for extracting metadata from the slack even…

aecdd77

…t API

remove print statements

f11754f

Merge remote-tracking branch 'upstream/master'

8c710e5

kearnsw requested a review from tmbo January 28, 2020 18:49

tmbo requested review from wochinge and removed request for tmbo January 28, 2020 19:18

wochinge reviewed Jan 30, 2020

View reviewed changes

Apply suggestions from code review

3df6fac

Co-Authored-By: Tobias Wochinger <mail@tobias-wochinger.de>

wochinge reviewed Feb 4, 2020

View reviewed changes

add additional test for missing data and authed users and clarity fixes

b622f8d

kearnsw and others added 4 commits February 4, 2020 11:49

Merge branch 'master' into master

d98c742

update changelog

1f6d347

updated docs to the new behavior of slack connector.

3ffd78d

remove extra blank space

0a6fa56

kearnsw requested a review from wochinge February 5, 2020 00:40

Merge branch 'master' into master

a1ae358

wochinge reviewed Feb 11, 2020

View reviewed changes

Will Kearns and others added 2 commits February 11, 2020 07:08

add changelog, update documentation, fix mock request

bedab05

Merge branch 'master' into master

c4f0f8f

wochinge reviewed Feb 17, 2020

View reviewed changes

Will Kearns added 4 commits February 20, 2020 13:41

revert changelog and remove check on metadata

ea859a3

Merge branch 'master' of https://github.com/kearnsw/rasa

8a0cc26

fixed typing issue and separated metadata and core message data

cf27402

fix unit test after the removal of sender from metadata object

167c055

wochinge reviewed Feb 24, 2020

View reviewed changes

CHANGELOG.rst Outdated Show resolved Hide resolved

kearnsw and others added 2 commits February 24, 2020 07:01

Update CHANGELOG.rst

8f96d03

Co-Authored-By: Tobias Wochinger <mail@tobias-wochinger.de>

Merge branch 'master' into master

9bf680b

kearnsw added 2 commits February 24, 2020 10:08

Merge branch 'master' into master

78702ea

Merge branch 'master' into master

e46f14b

Merge branch 'master' into master

f2d501f

tmbo merged commit f7a66e9 into RasaHQ:master Feb 26, 2020

kearnsw mentioned this pull request Mar 19, 2020

Slack Connector: support app mentions and direct messaging #4811

Closed

		@@ -109,6 +109,20 @@ def _get_text_from_slack_buttons(buttons):
		return super(SlackBot, self).api_call("chat.postMessage", **json_message)


		def is_app_mention(user_message) -> bool:

		@@ -311,6 +329,18 @@ def _get_interactive_response(action: Dict) -> Optional[Text]:

		return response.text("")

		def get_metadata(self, request: Request):

	A `dict` containing the output channel for the response, the text from the user, the sender's ID, and
	Metadata extracted from the sent event payload. This includes the output channel for the response, the text from the user and users that have installed the bot.

		@@ -462,6 +462,50 @@ def test_botframework_attachments():
		assert ch.add_attachments_to_metadata(payload, metadata) == updated_metadata


		def test_slack_metadata():

	r = Request(None, None, None, None, None, app="test")
	r = Mock()
	r.json = direct_message_event

		@@ -1,8 +1,8 @@
		:desc: Rasa Open Source Changelog
		:desc: Rasa OSS Changelog

		@@ -17,89 +17,6 @@ This project adheres to `Semantic Versioning`_ starting with version 1.0.

		.. towncrier release notes start

		[1.7.0] - 2020-01-29

		@@ -0,0 +1 @@
		Support invoking a ``SlackBot`` by direct messaging or @ mentions.

Improve flexibility of Slack connector #4812

Improve flexibility of Slack connector #4812

Conversation

kearnsw commented Nov 21, 2019 • edited Loading

tmbo left a 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

erohmensing commented Dec 16, 2019

kearnsw commented Dec 17, 2019

wochinge commented Jan 10, 2020

kearnsw commented Jan 16, 2020 • edited Loading

wochinge left a 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

kearnsw commented Jan 23, 2020

wochinge commented Jan 23, 2020

kearnsw commented Jan 23, 2020

wochinge left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kearnsw commented Feb 11, 2020

wochinge commented Feb 17, 2020

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

kearnsw commented Feb 21, 2020

wochinge commented Feb 24, 2020

wochinge commented Feb 26, 2020

kearnsw commented Feb 26, 2020

tmbo commented Feb 26, 2020

kearnsw commented Nov 21, 2019 •

edited

Loading

kearnsw commented Jan 16, 2020 •

edited

Loading