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.
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
Make WebhookClient (sync/async) #send method accept link unfurl params #1047
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1047 +/- ##
==========================================
- Coverage 84.33% 84.33% -0.01%
==========================================
Files 95 95
Lines 8938 8937 -1
==========================================
- Hits 7538 7537 -1
Misses 1400 1400
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you have some tests that just run the code in this scenario? Having those helps us debug / detect future regressions.
slack_sdk/web/internal_utils.py
Outdated
@@ -229,7 +229,7 @@ def _next_cursor_is_present(data) -> bool: | |||
|
|||
def _to_0_or_1_if_bool(v: Any) -> Union[Any, str]: | |||
if isinstance(v, bool): | |||
return "1" if v else "0" | |||
return 1 if v else 0 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
Hmm, this test basically takes much longer than others (it verifies many patters of data chunk size). You can see the progress in |
self.assertEqual(200, response.status_code) | ||
self.assertEqual("ok", response.body) | ||
|
||
def __get_channel_id(self): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@seratch I've put some questions in alongside my latest changes (which add to the sync webhook's integration tests). Integration tests feel more appropriate for the behavior described than unit tests, a we cannot determine whether an unfurl options was applied correctly without also knowing what the Slack API is doing. Let me know what you think about that, and whether there's behavior at the unit level that you think would make sense to test in this case. And based on your feedback to my questions on the changes, I will go ahead an implement for async as well. Thanks in advance for taking a look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srajiang Thanks for the updates. Really nice!
Can you add some unit tests as well?
- https://github.com/slackapi/python-slack-sdk/blob/main/tests/slack_sdk/webhook/test_webhook.py
- https://github.com/slackapi/python-slack-sdk/blob/main/tests/slack_sdk_async/webhook/test_async_webhook.py
As you may be aware, only the integration tests can verify the actual behaviors in this PR but they are not part of the CI builds at the moment. We want to have something that may detect very basic level regression for safety and for slightly better code coverage in the CI builds.
The unit tests do not need to have unfurling-specific assertions. Verifying the method calls with the new arguments does not cause any errors with the mock HTTP server and any runtime errors is enough.
self.assertEqual(200, response.status_code) | ||
self.assertEqual("ok", response.body) | ||
|
||
def __get_channel_id(self): |
There was a problem hiding this comment.
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
@seratch Yes, the level of expected behavior for unit-level tests you describe makes sense to me, and I have added them alongside the integration tests. It also makes more sense to me to add the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Resolves #1045, and addresses a bug where bool values being converted to "0" and "1" were not correctly being interpreted by Slack API.
Changes:
WebhookClient#send
andAsyncWebhookClient#send
to acceptunfurl_links
andunfurl_media
as optional arguments)_to_0_or_1_if_bool()
method to return integer instead, the unfurl options provided are correctly applied.To recreate the bug:
Using the #send_dict method only, you can recreate this with
In this case, outgoing http body looks like:
Per docs, example, this link should be prevented from unfurling, but it does unfurl.
With the changes, the outgoing http body looks like:
And the link is properly prevented from unfurling.
Category (place an
x
in each of the[ ]
)/docs-src
(Documents, have you run./docs.sh
?)/docs-src-v2
(Documents, have you run./docs-v2.sh
?)/tutorial
(PythOnBoardingBot tutorial)tests
/integration_tests
(Automated tests for this library)Requirements (place an
x
in each[ ]
)python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh
after making the changes. <---- Ran up untiltest_interactions_websocket_client.py
and then hung again