Skip to content

Updates to slack-adapter #1559

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
Merged
merged 11 commits into from
Apr 13, 2021
Merged

Updates to slack-adapter #1559

merged 11 commits into from
Apr 13, 2021

Conversation

tracyboehrer
Copy link
Member

Fixes #1548

There have been some some updates to the DotNet slack-adapter, and this PR includes most of those changes.
@tracyboehrer tracyboehrer requested a review from axelsrz March 4, 2021 19:52
@omid-jf omid-jf mentioned this pull request Mar 29, 2021
Open
@omid-jf
Copy link

omid-jf commented Mar 29, 2021

I noticed in my tests that channel_id of a ConversationReference object (which is always "slack") is not the same as the channel ID that chat.delete API requires.
Therefore, I believe in line 137 of slack_adapter.py in this PR, channel=reference.channel_id should be replaced by channel=reference.conversation.id
Moreover, since the current code of slack_helper.py does not set the timestamp property of activities, exception on line 134 of slack_adapter.py in this PR is always raised. There is a fix for this in PR #1605 (a conversion from datetime to epoch is required if using that PR) or ts=context.activity.timestamp in line 137 of slack_adapter.py in this PR can be replaced by ts=reference.activity_id since we already set activity_id to be the timestamp.
@tracyboehrer
Copy link
Member Author

@omid-jf Thanks! I'll take a look and apply the fixes.
axelsrz
axelsrz reviewed Apr 12, 2021
View reviewed changes
libraries/botbuilder-adapters-slack/botbuilder/adapters/slack/slack_adapter.py
super().__init__(on_turn_error)
self.slack_client = client
self.slack_logged_in = False
self.options = options if options else SlackAdapterOptions()
Copy link
Member

@axelsrz axelsrz Apr 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is nitpicking but this could be changed to:
self.options = options or SlackAdapterOptions
axelsrz
axelsrz approved these changes Apr 12, 2021
View reviewed changes
Copy link
Member

@axelsrz axelsrz left a comment

Choose a reason for hiding this comment

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

Approved with comments
@tracyboehrer tracyboehrer merged commit 2a84dc5 into main Apr 13, 2021
@tracyboehrer tracyboehrer deleted the trboehre/slackadapter-update branch April 13, 2021 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@tracyboehrer @omid-jf @axelsrz