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

feat: add support for conversations.requestShared approve, deny & list #1530

Merged

Conversation

WilliamBergamin
Copy link
Contributor

@WilliamBergamin WilliamBergamin commented Jul 9, 2024

Summary

This PR aims to introduce support for conversations.requestShared.approve/deny/list API methods

Testing

N/A

Category

  • 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?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements

  • 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.

Copy link

codecov bot commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 53.33333% with 21 lines in your changes missing coverage. Please review.

Project coverage is 84.85%. Comparing base (062b8c2) to head (4568864).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
slack_sdk/web/async_client.py 53.33% 7 Missing ⚠️
slack_sdk/web/client.py 53.33% 7 Missing ⚠️
slack_sdk/web/legacy_client.py 53.33% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1530      +/-   ##
==========================================
- Coverage   85.04%   84.85%   -0.20%     
==========================================
  Files         113      113              
  Lines       12501    12546      +45     
==========================================
+ Hits        10632    10646      +14     
- Misses       1869     1900      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seratch seratch modified the milestones: 3.31.1, 3.32.0 Aug 29, 2024
@WilliamBergamin WilliamBergamin changed the title feat: add support for conversations.requestShared approve and deny feat: add support for conversations.requestShared approve, deny & list Sep 4, 2024
@WilliamBergamin WilliamBergamin marked this pull request as ready for review September 4, 2024 20:00
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for taking this on 🙇

invite_id: str,
channel_id: Optional[str] = None,
is_external_limited: Optional[str] = None,
message: Optional[Dict[str, Any]] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

one note about message here is that if it is provided, text inside of message is required. not sure if that is possible to model in Python but figured I'd mention it

Copy link
Member

Choose a reason for hiding this comment

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

No way to describe such in Python's type hints. The only approaches we can think of would be 1) accept both dict and custom class that has "text" property, 2) add runtime validation that checks if the key is not missing. That said, as long as the server-side returns a clear error code/message for the pattern, I don't think we should do extra on the client code side.

@srajiang
Copy link
Member

srajiang commented Sep 4, 2024

Not a blocker, but are we planning on releasing this after the methods are public?

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Looks good to me; we can ship it once the API deployments to customers are 100% done

"message": message,
}
)
if message is not None:
Copy link
Member

Choose a reason for hiding this comment

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

When you have these lines of code, the above "message": message, is unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch 💯

invite_id: str,
channel_id: Optional[str] = None,
is_external_limited: Optional[str] = None,
message: Optional[Dict[str, Any]] = None,
Copy link
Member

Choose a reason for hiding this comment

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

No way to describe such in Python's type hints. The only approaches we can think of would be 1) accept both dict and custom class that has "text" property, 2) add runtime validation that checks if the key is not missing. That said, as long as the server-side returns a clear error code/message for the pattern, I don't think we should do extra on the client code side.

@WilliamBergamin WilliamBergamin merged commit 0e8a686 into slackapi:main Sep 6, 2024
12 checks passed
@WilliamBergamin WilliamBergamin deleted the slack-connect-invite-automation branch September 6, 2024 15:35
@WilliamBergamin WilliamBergamin modified the milestones: 3.32.0, 3.31.1 Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants