Make WebhookClient (sync/async) #send method accept link unfurl params by srajiang · Pull Request #1047 · slackapi/python-slack-sdk

srajiang · 2021-06-24T01:11:20Z

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:

Modifies WebhookClient#send and AsyncWebhookClient#send to accept unfurl_links and unfurl_media as optional arguments)
Previously booleans False and True were being converted to "0" and "1" in the outgoing http request body, but the options were not correctly being applied. See "recreate bug" section below. I believe this is due to the value being returned as a string. With a small modification to _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

dict = {
   "text": ""<https://imgs.xkcd.com/comics/new_sports_system_2x.png>", "unfurl_links": False, "unfurl_media": False
}

response = webhook.send_dict(dict)
assert response.status_code == 200
assert response.body == "ok"

In this case, outgoing http body looks like:

{"text": "hello webhook world\n<https://imgs.xkcd.com/comics/new_sports_system_2x.png>", "unfurl_links": "0", "unfurl_media": "0"}

Per docs, example, this link should be prevented from unfurling, but it does unfurl.

With the changes, the outgoing http body looks like:

{"text": "hello webhook world\n<https://imgs.xkcd.com/comics/new_sports_system_2x.png>", "unfurl_links": 0, "unfurl_media": 0}

And the link is properly prevented from unfurling.

Category (place an `x` in each of the `[ ]`)

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've run python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes. <---- Ran up until test_interactions_websocket_client.py and then hung again

codecov · 2021-06-24T01:14:17Z

Codecov Report

Merging #1047 (d410e8d) into main (a087d71) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            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

Impacted Files	Coverage Δ
slack_sdk/webhook/async_client.py	`93.65% <ø> (ø)`
slack_sdk/webhook/client.py	`94.44% <ø> (ø)`
slack_sdk/webhook/internal_utils.py	`82.60% <ø> (-0.73%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a087d71...d410e8d. Read the comment docs.

seratch

Can you have some tests that just run the code in this scenario? Having those helps us debug / detect future regressions.

seratch · 2021-06-24T01:19:51Z

slack_sdk/web/internal_utils.py

 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


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

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

@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!

slack_sdk/web/internal_utils.py

slack_sdk/webhook/__init__.py

seratch · 2021-06-24T01:47:08Z

Ran up until test_interactions_websocket_client.py and then hung again

Hmm, this test basically takes much longer than others (it verifies many patters of data chunk size). You can see the progress in logs/pytest.log. Also, for convenience, you can run specific tests by ./scripts/run_unit_tests.sh tests/slack_sdk/webhook/.

srajiang · 2021-06-24T20:37:19Z

integration_tests/webhook/test_webhook.py

-        self.assertEqual(200, response.status_code)
-        self.assertEqual("ok", response.body)
-
+    def __get_channel_id(self):


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.

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

integration_tests/webhook/test_webhook.py

srajiang · 2021-06-24T20:53:50Z

@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!

seratch

@srajiang Thanks for the updates. Really nice!

Can you add some unit tests as well?

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.

seratch · 2021-06-24T22:55:29Z

integration_tests/webhook/test_webhook.py

-        self.assertEqual(200, response.status_code)
-        self.assertEqual("ok", response.body)
-
+    def __get_channel_id(self):


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

integration_tests/webhook/test_webhook.py

srajiang · 2021-06-28T21:06:03Z

@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 channel_id logic to setUp(). Thanks for the detailed feedback!

seratch

LGTM

srajiang added 4 commits June 23, 2021 14:40

Adds unfurl optional fields to webhook clients

e65f9b6

Add reference comments

da827cd

Merge branch 'main' into sj_update_webhook_clients

dae98dd

Integer 0|1 instead of string

75efbee

gitwave bot added the untriaged label Jun 24, 2021

srajiang requested a review from seratch June 24, 2021 01:11

seratch added enhancement M-T: A feature request for new functionality Version: 3x web-client and removed untriaged labels Jun 24, 2021

seratch added this to the 3.8.0 milestone Jun 24, 2021

seratch suggested changes Jun 24, 2021

View reviewed changes

srajiang added 2 commits June 23, 2021 18:29

Revert import for async module

4782359

Removes test script + updates fxn return type

5ad43b9

srajiang added 2 commits June 24, 2021 13:03

Add example integration test for comment

80f61c1

Remove repetetive logic

12f07c6

srajiang commented Jun 24, 2021

View reviewed changes

integration_tests/webhook/test_webhook.py Show resolved Hide resolved

srajiang commented Jun 24, 2021

View reviewed changes

integration_tests/webhook/test_webhook.py Show resolved Hide resolved

srajiang commented Jun 24, 2021

View reviewed changes

integration_tests/webhook/test_webhook.py Show resolved Hide resolved

srajiang commented Jun 24, 2021

View reviewed changes

integration_tests/webhook/test_webhook.py Show resolved Hide resolved

Remove boolean conversion and revert web util

52c44f8

Revert type

326ca99

srajiang requested a review from seratch June 24, 2021 21:03

seratch reviewed Jun 24, 2021

View reviewed changes

srajiang added 2 commits June 28, 2021 09:29

Cleanup comments

9d32daa

Add setup

ac4f8a4

srajiang and others added 6 commits June 28, 2021 13:06

Add async webhook integration test

d8389f4

Use async test case class

da13780

Add unit tests

693b27b

Remove dep

784bee9

Merge branch 'slackapi:main' into sj_update_webhook_clients

4d5d95b

Remove async class

d410e8d

srajiang requested a review from seratch June 28, 2021 21:06

seratch approved these changes Jun 28, 2021

View reviewed changes

seratch merged commit 3422a23 into slackapi:main Jun 28, 2021

srajiang deleted the sj_update_webhook_clients branch August 9, 2021 21:15

Conversation

srajiang commented Jun 24, 2021

Summary

Changes:

To recreate the bug:

Category (place an x in each of the [ ])

Requirements (place an x in each [ ])

Uh oh!

codecov bot commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

seratch Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

seratch Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

srajiang Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

seratch commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srajiang Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

seratch Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

srajiang commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

seratch Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

srajiang commented Jun 28, 2021

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Category (place an `x` in each of the `[ ]`)

Requirements (place an `x` in each `[ ]`)

codecov bot commented Jun 24, 2021 •

edited

Loading

srajiang Jun 24, 2021 •

edited

Loading

seratch commented Jun 24, 2021 •

edited

Loading

srajiang commented Jun 24, 2021 •

edited

Loading