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 #1342 files_upload_v2 fails to share files in a channel #1343

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

seratch
Copy link
Member

@seratch seratch commented Mar 7, 2023

Summary

This pull request resolves #1342 . The root cause of the issue is miscoding in this SDK. The current implemenation of the v2 method internally passes "channel" parameter instead of "channel_id" parameter in files.completeUploadExternal API calls. Although "channel_id" is the correct one, the API server-side had been accepting both "channel" and "channel_id" until today. In the initial development of this v2 wrapper method on the SDK side, we overlooked the invalid parameter name due to the server-side behavior.

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.

@seratch seratch added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented web-client Version: 3x labels Mar 7, 2023
@seratch seratch added this to the 3.20.1 milestone Mar 7, 2023
@seratch seratch self-assigned this Mar 7, 2023
@@ -3143,9 +3143,10 @@ def files_upload_v2(
channel_to_share = channels[0]
completion = self.files_completeUploadExternal(
files=[{"id": f["file_id"], "title": f["title"]} for f in files],
channel=channel_to_share,
channel_id=channel_to_share,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the cause of the issue

initial_comment=initial_comment,
thread_ts=thread_ts,
token=kwargs.get("token"),
Copy link
Member Author

Choose a reason for hiding this comment

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

While fixing the issue, I've noticed that this one is missing here. With this, developers can pass "token" to v2 method to overwrite the token given when initializing WebClient. Current code does not response the parameter for this specific API call. That was a different bug.

@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #1343 (7b27a4d) into main (66e8359) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1343   +/-   ##
=======================================
  Coverage   85.40%   85.40%           
=======================================
  Files         111      111           
  Lines       11406    11406           
=======================================
  Hits         9741     9741           
  Misses       1665     1665           
Impacted Files Coverage Δ
slack_sdk/web/async_client.py 82.39% <ø> (ø)
slack_sdk/web/client.py 83.69% <ø> (ø)
slack_sdk/web/legacy_client.py 83.42% <ø> (ø)

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

@seratch seratch merged commit 9aaea27 into slackapi:main Mar 7, 2023
@seratch seratch deleted the issue-1342-files-upload-v2 branch March 7, 2023 06:32
@JSpiner
Copy link

JSpiner commented Mar 8, 2023

Thanks for your help. You saved my life.

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.

files_upload_v2 fails to share files in a channel
2 participants