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.

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

Fix #1258 Tuple value for blocks argument does not work for Web API calls #1259

Merged
merged 2 commits into from
Sep 1, 2022
Merged

Fix #1258 Tuple value for blocks argument does not work for Web API calls #1259

merged 2 commits into from
Sep 1, 2022

Conversation

tommasobertoni
Copy link
Contributor

@tommasobertoni tommasobertoni commented Aug 31, 2022

Summary

Fix for Tuple value for blocks argument does not work for Web API calls

blocks: Sequence[Block] = (
    SectionBlock(text="foo"),
    SectionBlock(text="bar"),
)

# fixes calls with non-list blocks and attachments
client.chat_postMessage(channel="#channel", blocks=blocks)

Resolves #1258

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

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /docs-src (Documents, have you run ./scripts/docs.sh?)
  • /docs-src-v2 (Documents, have you run ./scripts/docs-v2.sh?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

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.

@tommasobertoni
Copy link
Contributor Author

💡 Additionally, the SDK could accept an Iterable instead of Sequence, therefore allowing generators like:

messages_to_send: list[str] = ...

client.chat_postMessage(
    channel="#channel",
    blocks=(SectionBlock(text=some_text) for some_text in messages_to_send)
)

@seratch seratch changed the title fix: issue 1258 Fix #1258 Tuple value for blocks argument does not work for Web API calls Aug 31, 2022
@seratch seratch self-assigned this Aug 31, 2022
@seratch seratch added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented web-client Version: 3x labels Aug 31, 2022
@seratch seratch added this to the 3.18.2 milestone Aug 31, 2022
@@ -16,3 +25,33 @@ def test_build_unexpected_body_error_message(self):
assert message.startswith(
"""Received a response in a non-JSON format: <!DOCTYPE html><html lang="en"><head><meta charset="utf-8">"""
)


@pytest.mark.parametrize("initial_blocks", [
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@seratch
Copy link
Member

seratch commented Aug 31, 2022

Additionally, the SDK could accept an Iterable instead of Sequence, therefore allowing generators

Yeah, this can be a great addition for some specific use cases but not this time!

@seratch
Copy link
Member

seratch commented Aug 31, 2022

@tommasobertoni Thanks a lot for sending this PR! The changes already look great to me. Once the CI builds pass, we will merge the change into the main branch.

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #1259 (beaf082) into main (d2fd7ac) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1259      +/-   ##
==========================================
+ Coverage   86.56%   86.64%   +0.07%     
==========================================
  Files         111      111              
  Lines       11026    11026              
==========================================
+ Hits         9545     9553       +8     
+ Misses       1481     1473       -8     
Impacted Files Coverage Δ
slack_sdk/web/internal_utils.py 94.44% <100.00%> (ø)
slack_sdk/socket_mode/builtin/client.py 90.24% <0.00%> (+0.60%) ⬆️
slack_sdk/socket_mode/builtin/connection.py 67.03% <0.00%> (+2.59%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@seratch seratch merged commit 9c2ae69 into slackapi:main Sep 1, 2022
@ydshieh
Copy link

ydshieh commented Sep 5, 2022

Hi @tommasobertoni and @seratch , since the release of 3.18.2 (hence this PR being merged I believe), our CIs failed to send CI reports to our slack channels. The failing payload is provided below. The same payload was able to be sent to slack channels if I changed to slack_sdk==3.18.1.

Is this an intended breaking change?

The payload working with 3.18.1 but not 3.18.2

{"blocks": [{"type": "header", "text": {"type": "plain_text", "text": "\ud83e\udd17 Results of the Past CI - pytorch-1.11 tests."}}, {"type": "section", "text": {"type": "plain_text", "text": "There were 68 failures, out of 28636 tests.\nThe suite ran in 2h34m15s.", "emoji": true}, "accessory": {"type": "button", "text": {"type": "plain_text", "text": "Check Action results", "emoji": true}, "url": "https://github.com/huggingface/transformers/actions/runs/2970011128"}}, {"type": "section", "text": {"type": "mrkdwn", "text": "The following modeling categories had failures:\n```\nSingle |  Multi | Category\n    14 |     15 | PyTorch\n     3 |      3 | TensorFlow\n     1 |      1 | Tokenizers\n     1 |      0 | Trainer\n     1 |      1 | Auto\n    14 |     14 | Unclassified\n```\n"}}, {"type": "section", "text": {"type": "mrkdwn", "text": "These following model modules had failures:\n```\nSingle PT |  Multi PT | Single TF |  Multi TF |     Other | Category\n        0 |         0 |         0 |         0 |         1 | trainer\n        0 |         0 |         0 |         0 |         2 | models_layoutlmv2\n        0 |         0 |         0 |         0 |        30 | models_wav2vec2_with_lm\n        0 |         0 |         3 |         3 |         0 | models_opt\n        1 |         1 |         0 |         0 |         0 | models_wav2vec2\n        3 |         3 |         0 |         0 |         0 | models_bloom\n       10 |        11 |         0 |         0 |         0 | models_owlvit\n```\n"}}, {"type": "section", "text": {"type": "mrkdwn", "text": "The following non-model modules had failures:\n```\nSingle |  Multi | Category\n     1 |      0 | trainer\n```\n"}}]}

Error

Traceback (most recent call last):
  File "utils/notification_service.py", line 901, in <module>
    message.post()
  File "utils/notification_service.py", line 445, in post
    self.thread_ts = client.chat_postMessage(
  File "/home/runner/.local/lib/python3.8/site-packages/slack_sdk/web/client.py", line 2066, in chat_postMessage
    return self.api_call("chat.postMessage", json=kwargs)
  File "/home/runner/.local/lib/python3.8/site-packages/slack_sdk/web/base_client.py", line 156, in api_call
    return self._sync_send(api_url=api_url, req_args=req_args)
  File "/home/runner/.local/lib/python3.8/site-packages/slack_sdk/web/base_client.py", line 187, in _sync_send
    return self._urllib_api_call(
  File "/home/runner/.local/lib/python3.8/site-packages/slack_sdk/web/base_client.py", line [30](https://github.com/huggingface/transformers/runs/8138562211?check_suite_focus=true#step:5:31)9, in _urllib_api_call
    return SlackResponse(
  File "/home/runner/.local/lib/python3.8/site-packages/slack_sdk/web/slack_response.py", line 189, in validate
    raise e.SlackApiError(message=msg, response=self)
slack_sdk.errors.SlackApiError: The request to the Slack API failed. (url: https://www.slack.com/api/chat.postMessage)
The server responded with: {'ok': False, 'error': 'invalid_blocks_format'}

@seratch
Copy link
Member

seratch commented Sep 5, 2022

Hi @ydshieh, thanks for reporting the issue. Definitely any existing code should work without any changes. I will look into this but, with given information, I am still unsure about how to reproduce your situation. If you can share the code snippet to reproduce the issue, that'd be greatly helpful for us.

@ydshieh
Copy link

ydshieh commented Sep 5, 2022

@seratch Thanks, I will try to give a code snippet.

@ydshieh
Copy link

ydshieh commented Sep 5, 2022

Hi @seratch

The script to reproduce the issue under slack_sdk==3.18.2
https://github.com/huggingface/transformers/blob/0b1d41e499dcfa6825f6e415c25d0d454e5ad05e/utils/notification_service.py

If helpful, this workflow file triggers the above script
https://github.com/huggingface/transformers/blob/reproduce-slack-api-issue/.github/workflows/self-scheduled.yml

Here are the job run results:

Let me know if you need more information, thanks

@seratch
Copy link
Member

seratch commented Sep 6, 2022

@ydshieh Thanks for sharing the details! It was very helpful for us to understand your use case. Using str value for blocks/attachments was not supported by design but we will support it in future versions. See also: #1261

@seratch
Copy link
Member

seratch commented Sep 6, 2022

@ydshieh Your issue should be resolved in the latest version 3.18.3. Thanks again for sharing this!

@ydshieh
Copy link

ydshieh commented Sep 6, 2022

Thanks a lot, @seratch! Confirmed it works again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 3x web-client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tuple value for blocks argument does not work for Web API calls
3 participants