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

Make WebhookClient (sync/async) #send method accept link unfurl params #1047

Merged
merged 18 commits into from Jun 28, 2021
Merged

Make WebhookClient (sync/async) #send method accept link unfurl params #1047

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e65f9b6
Adds unfurl optional fields to webhook clients
srajiang Jun 23, 2021
da827cd
Add reference comments
srajiang Jun 23, 2021
dae98dd
Merge branch 'main' into sj_update_webhook_clients
srajiang Jun 23, 2021
75efbee
Integer 0|1 instead of string
srajiang Jun 24, 2021
4782359
Revert import for async module
srajiang Jun 24, 2021
5ad43b9
Removes test script + updates fxn return type
srajiang Jun 24, 2021
80f61c1
Add example integration test for comment
srajiang Jun 24, 2021
12f07c6
Remove repetetive logic
srajiang Jun 24, 2021
52c44f8
Remove boolean conversion and revert web util
srajiang Jun 24, 2021
326ca99
Revert type
srajiang Jun 24, 2021
9d32daa
Cleanup comments
srajiang Jun 28, 2021
ac4f8a4
Add setup
srajiang Jun 28, 2021
d8389f4
Add async webhook integration test
srajiang Jun 28, 2021
da13780
Use async test case class
srajiang Jun 28, 2021
693b27b
Add unit tests
srajiang Jun 28, 2021
784bee9
Remove dep
srajiang Jun 28, 2021
4d5d95b
Merge branch 'slackapi:main' into sj_update_webhook_clients
srajiang Jun 28, 2021
d410e8d
Remove async class
srajiang Jun 28, 2021
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
59 changes: 51 additions & 8 deletions integration_tests/webhook/test_webhook.py
View file
Open in desktop
@@ -1,5 +1,6 @@
import os
import unittest
import time

from integration_tests.env_variable_names import (
SLACK_SDK_TEST_INCOMING_WEBHOOK_URL,
Expand All @@ -21,13 +22,7 @@ def setUp(self):
def tearDown(self):
pass

def test_webhook(self):
url = os.environ[SLACK_SDK_TEST_INCOMING_WEBHOOK_URL]
webhook = WebhookClient(url)
response = webhook.send(text="Hello!")
self.assertEqual(200, response.status_code)
self.assertEqual("ok", response.body)

def __get_channel_id(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

For review: I needed the functionality of pulling channel ID in the two new tests. As this would be repeated code, I have pulled the logic into separate function and use in the test_webhook, test_with_unfurls_off and test_with_unfurls_on methods where needed.

Copy link
Member

Choose a reason for hiding this comment

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

You can do the same in the setUp method and use self.channel_id in test methods. If you use token / client in test methods, setting them to the instance fields would be also fine.

diff --git a/integration_tests/webhook/test_webhook.py b/integration_tests/webhook/test_webhook.py
index 12f60d0..82d0b6f 100644
--- a/integration_tests/webhook/test_webhook.py
+++ b/integration_tests/webhook/test_webhook.py
@@ -16,7 +16,20 @@ from slack_sdk.models.blocks.basic_components import MarkdownTextObject, PlainTe
 
 class TestWebhook(unittest.TestCase):
     def setUp(self):
-        pass
+        if not hasattr(self, "channel_id"):
+            token = os.environ[SLACK_SDK_TEST_BOT_TOKEN]
+            channel_name = os.environ[SLACK_SDK_TEST_INCOMING_WEBHOOK_CHANNEL_NAME].replace(
+                "#", ""
+            )
+            client = WebClient(token=token)
+            self.channel_id = None
+            for resp in client.conversations_list(limit=10):
+                for c in resp["channels"]:
+                    if c["name"] == channel_name:
+                        self.channel_id = c["id"]
+                        break
+                if self.channel_id is not None:
+                    break
 
     def tearDown(self):
         pass

token = os.environ[SLACK_SDK_TEST_BOT_TOKEN]
channel_name = os.environ[SLACK_SDK_TEST_INCOMING_WEBHOOK_CHANNEL_NAME].replace(
"#", ""
Expand All @@ -41,12 +36,60 @@ def test_webhook(self):
break
if channel_id is not None:
break
return channel_id

def test_webhook(self):
url = os.environ[SLACK_SDK_TEST_INCOMING_WEBHOOK_URL]
webhook = WebhookClient(url)
response = webhook.send(text="Hello!")
self.assertEqual(200, response.status_code)
self.assertEqual("ok", response.body)

history = client.conversations_history(channel=channel_id, limit=1)
token = os.environ[SLACK_SDK_TEST_BOT_TOKEN]
client = WebClient(token=token)
history = client.conversations_history(channel=self.__get_channel_id(), limit=1)
self.assertIsNotNone(history)
actual_text = history["messages"][0]["text"]
self.assertEqual("Hello!", actual_text)

def test_with_unfurls_off(self):
url = os.environ[SLACK_SDK_TEST_INCOMING_WEBHOOK_URL]
token = os.environ[SLACK_SDK_TEST_BOT_TOKEN]
webhook = WebhookClient(url)
client = WebClient(token=token)
# send message that does not unfurl
response = webhook.send(
text="<https://imgs.xkcd.com/comics/desert_golfing_2x.png|Desert Golfing>",
unfurl_links=False,
unfurl_media=False,
)
self.assertEqual(200, response.status_code)
self.assertEqual("ok", response.body)
# wait in order to allow Slack API to edit message with attachments
time.sleep(2)
srajiang marked this conversation as resolved.
Show resolved Hide resolved
history = client.conversations_history(channel=self.__get_channel_id(), limit=1)
self.assertIsNotNone(history)
self.assertTrue("attachments" not in history["messages"][0])

def test_with_unfurls_on(self):
srajiang marked this conversation as resolved.
Show resolved Hide resolved
url = os.environ[SLACK_SDK_TEST_INCOMING_WEBHOOK_URL]
token = os.environ[SLACK_SDK_TEST_BOT_TOKEN]
webhook = WebhookClient(url)
client = WebClient(token=token)
# send message that does unfurl
response = webhook.send(
text="<https://imgs.xkcd.com/comics/red_spiders_small.jpg|Spiders>",
unfurl_links=True,
unfurl_media=True,
)
self.assertEqual(200, response.status_code)
self.assertEqual("ok", response.body)
# wait in order to allow Slack API to edit message with attachments
time.sleep(2)
srajiang marked this conversation as resolved.
Show resolved Hide resolved
history = client.conversations_history(channel=self.__get_channel_id(), limit=1)
self.assertIsNotNone(history)
self.assertTrue("attachments" in history["messages"][0])
srajiang marked this conversation as resolved.
Show resolved Hide resolved

def test_with_blocks(self):
url = os.environ[SLACK_SDK_TEST_INCOMING_WEBHOOK_URL]
webhook = WebhookClient(url)
Expand Down
4 changes: 2 additions & 2 deletions slack_sdk/web/internal_utils.py
View file
Open in desktop
Expand Up @@ -227,9 +227,9 @@ def _next_cursor_is_present(data) -> bool:
return present


def _to_0_or_1_if_bool(v: Any) -> Union[Any, str]:
def _to_0_or_1_if_bool(v: Any) -> Union[Any, int]:
if isinstance(v, bool):
return "1" if v else "0"
return 1 if v else 0
Copy link
Member

Choose a reason for hiding this comment

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

Probably, this change is fine for WebClient etc. but I will check the behavior later.

Copy link
Member

Choose a reason for hiding this comment

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

As WebookClient sends content-type: application/json data, we don't necessarily convert boolean values to 1/0. Can you modify slack_sdk/webhook/internal_utils.py this way instead?

diff --git a/slack_sdk/webhook/internal_utils.py b/slack_sdk/webhook/internal_utils.py
index 37def28..091c06c 100644
--- a/slack_sdk/webhook/internal_utils.py
+++ b/slack_sdk/webhook/internal_utils.py
@@ -12,7 +12,6 @@ from .webhook_response import WebhookResponse
 def _build_body(original_body: Optional[Dict[str, Any]]) -> Optional[Dict[str, Any]]:
     if original_body:
         body = {k: v for k, v in original_body.items() if v is not None}
-        body = convert_bool_to_0_or_1(body)
         _parse_web_class_objects(body)
         return body
     return None

Copy link
Member Author

@srajiang srajiang Jun 24, 2021 •
edited

Choose a reason for hiding this comment

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

@seratch. Okay, I see that changing it this way reduces the number of possible affected surfaces. Thanks! I have also reverted the above line to return strings instead of integers. Feel free to resolve this, if these changes are what you had intended!

return v


Expand Down
6 changes: 6 additions & 0 deletions slack_sdk/webhook/async_client.py
View file
Open in desktop
Expand Up @@ -87,6 +87,8 @@ async def send(
response_type: Optional[str] = None,
replace_original: Optional[bool] = None,
delete_original: Optional[bool] = None,
unfurl_links: Optional[bool] = None,
unfurl_media: Optional[bool] = None,
headers: Optional[Dict[str, str]] = None,
) -> WebhookResponse:
"""Performs a Slack API request and returns the result.
Expand All @@ -98,6 +100,8 @@ async def send(
response_type: The type of message (either 'in_channel' or 'ephemeral')
replace_original: True if you use this option for response_url requests
delete_original: True if you use this option for response_url requests
unfurl_links: Option to indicate whether text url should unfurl
unfurl_media: Option to indicate whether media url should unfurl
headers: Request headers to append only for this request

Returns:
Expand All @@ -113,6 +117,8 @@ async def send(
"response_type": response_type,
"replace_original": replace_original,
"delete_original": delete_original,
"unfurl_links": unfurl_links,
"unfurl_media": unfurl_media,
},
headers=headers,
)
Expand Down
6 changes: 6 additions & 0 deletions slack_sdk/webhook/client.py
View file
Open in desktop
Expand Up @@ -77,6 +77,8 @@ def send(
response_type: Optional[str] = None,
replace_original: Optional[bool] = None,
delete_original: Optional[bool] = None,
unfurl_links: Optional[bool] = None,
unfurl_media: Optional[bool] = None,
headers: Optional[Dict[str, str]] = None,
) -> WebhookResponse:
"""Performs a Slack API request and returns the result.
Expand All @@ -89,6 +91,8 @@ def send(
response_type: The type of message (either 'in_channel' or 'ephemeral')
replace_original: True if you use this option for response_url requests
delete_original: True if you use this option for response_url requests
unfurl_links: Option to indicate whether text url should unfurl
unfurl_media: Option to indicate whether media url should unfurl
headers: Request headers to append only for this request

Returns:
Expand All @@ -104,6 +108,8 @@ def send(
"response_type": response_type,
"replace_original": replace_original,
"delete_original": delete_original,
"unfurl_links": unfurl_links,
"unfurl_media": unfurl_media,
},
headers=headers,
)
Expand Down