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

Improve flexibility of Slack connector #4812

Merged
merged 31 commits into from
Feb 26, 2020
Merged

Improve flexibility of Slack connector #4812

Changes from 9 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6eca764
fix to allow the bot to respond when mentioned or directly messaged
Nov 21, 2019
f5aa081
allow bot to communicate without mention on a configured channel
Nov 21, 2019
aec05aa
Resolve merge conflicts
Dec 17, 2019
3450447
add output channel to metadata
Dec 17, 2019
6a5e21b
Merge branch 'master' into master
kearnsw Dec 17, 2019
559b727
make message parsing methods private
Dec 17, 2019
c6cc6ac
Merge branch 'master' of https://github.com/kearnsw/rasa
Dec 17, 2019
995cace
update to create a new channel for each user interaction
Dec 17, 2019
097dc97
fix typing error
Jan 16, 2020
d5907f9
Apply suggestions from code review
kearnsw Jan 23, 2020
aecdd77
add test case and comment for extracting metadata from the slack even…
Jan 23, 2020
f11754f
remove print statements
Jan 23, 2020
8c710e5
Merge remote-tracking branch 'upstream/master'
Jan 23, 2020
3df6fac
Apply suggestions from code review
kearnsw Jan 30, 2020
b622f8d
add additional test for missing data and authed users and clarity fixes
Feb 4, 2020
d98c742
Merge branch 'master' into master
kearnsw Feb 4, 2020
1f6d347
update changelog
Feb 4, 2020
3ffd78d
updated docs to the new behavior of slack connector.
Feb 4, 2020
0a6fa56
remove extra blank space
Feb 4, 2020
a1ae358
Merge branch 'master' into master
kearnsw Feb 7, 2020
bedab05
add changelog, update documentation, fix mock request
Feb 11, 2020
c4f0f8f
Merge branch 'master' into master
kearnsw Feb 11, 2020
ea859a3
revert changelog and remove check on metadata
Feb 20, 2020
8a0cc26
Merge branch 'master' of https://github.com/kearnsw/rasa
Feb 20, 2020
cf27402
fixed typing issue and separated metadata and core message data
Feb 20, 2020
167c055
fix unit test after the removal of sender from metadata object
Feb 21, 2020
8f96d03
Update CHANGELOG.rst
kearnsw Feb 24, 2020
9bf680b
Merge branch 'master' into master
kearnsw Feb 24, 2020
78702ea
Merge branch 'master' into master
kearnsw Feb 24, 2020
e46f14b
Merge branch 'master' into master
kearnsw Feb 25, 2020
f2d501f
Merge branch 'master' into master
kearnsw Feb 26, 2020
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
69 changes: 56 additions & 13 deletions rasa/core/channels/slack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,20 @@ def __init__(
self.retry_reason_header = slack_retry_reason_header
self.retry_num_header = slack_retry_number_header

@staticmethod
def _is_app_mention(slack_event: Dict) -> bool:
try:
return slack_event["event"]["type"] == "app_mention"
except KeyError:
return False

@staticmethod
def _is_direct_message(slack_event: Dict) -> bool:
try:
return slack_event["event"]["channel_type"] == "im"
except KeyError:
return False

@staticmethod
def _is_user_message(slack_event: Dict) -> bool:
return (
Expand Down Expand Up @@ -294,11 +308,15 @@ async def process_message(

return response.text(None, status=201, headers={"X-Slack-No-Retry": 1})

if metadata is not None:
wochinge marked this conversation as resolved.
Show resolved Hide resolved
output_channel = metadata.get("out_channel")
else:
output_channel = None

try:
out_channel = self.get_output_channel()
user_msg = UserMessage(
text,
out_channel,
self.get_output_channel(output_channel),
sender_id,
input_channel=self.name(),
metadata=metadata,
Expand All @@ -311,6 +329,18 @@ async def process_message(

return response.text("")

def get_metadata(self, request: Request):
kearnsw marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please unit test this method?

slack_event = request.json
event = slack_event.get("event")
kearnsw marked this conversation as resolved.
Show resolved Hide resolved

metadata = {}
kearnsw marked this conversation as resolved.
Show resolved Hide resolved
metadata["out_channel"] = event.get("channel")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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'
}

metadata["text"] = event.get("text")
metadata["sender"] = event.get("user")
metadata["users"] = slack_event.get("authed_users")

return metadata

def blueprint(
self, on_new_message: Callable[[UserMessage], Awaitable[Any]]
) -> Blueprint:
Expand Down Expand Up @@ -348,19 +378,32 @@ async def webhook(request: Request) -> HTTPResponse:

elif self._is_user_message(output):
metadata = self.get_metadata(request)
return await self.process_message(
request,
on_new_message,
self._sanitize_user_message(
output["event"]["text"], output["authed_users"]
),
output.get("event").get("user"),
metadata,
)
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

what do we need this condition for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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"]}")

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

self._is_direct_message(output)
or self._is_app_mention(output)
or metadata["out_channel"] == self.slack_channel
):
return await self.process_message(
request,
on_new_message,
text=self._sanitize_user_message(
metadata["text"], metadata["users"]
),
sender_id=metadata["sender"],
metadata=metadata,
)
else:
return response.text("")

return response.text("Bot message delivered")

return slack_webhook

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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

kearnsw marked this conversation as resolved.
Show resolved Hide resolved
return SlackBot(self.slack_token, channel)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should still delete the lines after

else:
return SlackBot(self.slack_token, self.slack_channel)

def set_output_channel(self, channel) -> None:
kearnsw marked this conversation as resolved.
Show resolved Hide resolved
self.slack_channel = channel