Add Slack connect API support + tests by srajiang · Pull Request #1089 · slackapi/python-slack-sdk

srajiang · 2021-08-12T01:01:09Z

Summary

Fixes #1058 by adding support for recently added Slack Connect AP methods.

conversations.acceptSharedInvite
conversations.approveSharedInvite
conversations.declineSharedInvite
conversations.inviteShared
conversations.listConnectInvites

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.

codecov · 2021-08-12T01:22:59Z

Codecov Report

Merging #1089 (32663c5) into main (cbafdd1) will increase coverage by 0.11%.
The diff coverage is 97.53%.

@@            Coverage Diff             @@
##             main    #1089      +/-   ##
==========================================
+ Coverage   86.07%   86.19%   +0.11%     
==========================================
  Files         110      110              
  Lines        9847     9928      +81     
==========================================
+ Hits         8476     8557      +81     
  Misses       1371     1371

Impacted Files	Coverage Δ
slack_sdk/web/async_client.py	`92.80% <92.59%> (-0.01%)`	⬇️
slack_sdk/web/client.py	`94.40% <100.00%> (+0.19%)`	⬆️
slack_sdk/web/legacy_client.py	`94.04% <100.00%> (+0.20%)`	⬆️
slack_sdk/socket_mode/builtin/connection.py	`36.13% <0.00%> (+0.42%)`	⬆️
slack_sdk/socket_mode/builtin/client.py	`79.86% <0.00%> (+0.67%)`	⬆️

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 cbafdd1...32663c5. Read the comment docs.

seratch

Left a few (early) comments. I hope these were helpful to you!

seratch · 2021-08-12T01:23:20Z

slack_sdk/web/client.py

+        channel_id: str = None,
+        invite_id: str = None,


I know some existing methods (e.g., views_update) are not yet consistent about this but let's use Optional type hint for the value that can be None.

Suggested change

channel_id: str = None,

invite_id: str = None,

channel_id: Optional[str] = None,

invite_id: Optional[str] = None,

For existing others, we are happy to change the type hint this time! We don't call it a breaking change. It's a correction that does not affect runtime behavior.

Sure thing. I see that the type hint is another way of declaring a union type with None. I've included that for other existing methods that don't currently use the Optional type hint as well (including views_update) in the latest changes.

seratch · 2021-08-12T01:25:59Z

slack_sdk/web/client.py

+            kwargs.update({"channel_id": channel_id})
+        else:
+            kwargs.update({"invite_id": invite_id})
+        return self.api_call("conversations.acceptSharedInvite", json=kwargs)


If there is no strong reason to use json request body here, let's use params for new ones. Although this method does not have any array arguments at this moment, we may encounter #1054 with future updates of the endpoint.

Thanks for the context with #1054. I've modified acceptSharedInvite and approveSharedInvite to accept params instead.

seratch · 2021-08-12T01:29:08Z

integration_tests/web/test_conversations.py

@@ -0,0 +1,129 @@
+import logging


As this file has only tests for Slack Connect APIs, how about naming it to test_conversations_connect.py? If we will have integration tests for other conversations.* endpoints in the future, we can have them in a different file.

seratch · 2021-08-12T01:31:19Z

integration_tests/web/test_conversations.py

+            invite_id=invite["invite_id"],
+        )
+
+        # clean up created channel


How about having this part with if channel_id is not None: in finally clause? We can have a new local variable channel_id: Optional[str] outside the try/finally block.

Yes, that is much nicer - and we will always attempt to cleanup any channels in the course of the test. Thanks for the suggestion - I've included these changes!

srajiang

Thanks always for the prompt feedback! I've included some updates based on that feedback.

srajiang · 2021-08-12T16:36:50Z

slack_sdk/web/client.py

+        channel_id: str = None,
+        invite_id: str = None,


Sure thing. I see that the type hint is another way of declaring a union type with None. I've included that for other existing methods that don't currently use the Optional type hint as well (including views_update) in the latest changes.

srajiang · 2021-08-12T16:37:02Z

slack_sdk/web/client.py

+            kwargs.update({"channel_id": channel_id})
+        else:
+            kwargs.update({"invite_id": invite_id})
+        return self.api_call("conversations.acceptSharedInvite", json=kwargs)


Thanks for the context with #1054. I've modified acceptSharedInvite and approveSharedInvite to accept params instead.

srajiang · 2021-08-12T16:37:17Z

integration_tests/web/test_conversations.py

+            invite_id=invite["invite_id"],
+        )
+
+        # clean up created channel


Yes, that is much nicer - and we will always attempt to cleanup any channels in the course of the test. Thanks for the suggestion - I've included these changes!

seratch

@srajiang Finally comments before merging this!

seratch · 2021-08-12T21:27:37Z

integration_tests/web/test_conversations_connect.py

+    def test_sync(self):
+        sender = self.sender_sync_client
+        receiver = self.receiver_sync_client
+        channel_id: Optional[str]


If there is no assignment and we access the value in finally clause, it should raise an exception.

Suggested change

channel_id: Optional[str]

channel_id: Optional[str] = None

seratch · 2021-08-12T21:28:09Z

integration_tests/env_variable_names.py

 )
+
+# For Slack Connect shared tests
+SLACK_SDK_TEST_SENDER_BOT_TOKEN = "SLACK_SDK_TEST_SENDER_BOT_TOKEN"


Can we make these names more specific?

Suggested change

SLACK_SDK_TEST_SENDER_BOT_TOKEN = "SLACK_SDK_TEST_SENDER_BOT_TOKEN"

SLACK_SDK_TEST_CONNECT_INVITE_SENDER_BOT_TOKEN = "SLACK_SDK_TEST_CONNECT_INVITE_SENDER_BOT_TOKEN"

seratch · 2021-08-12T21:29:35Z

slack_sdk/web/async_client.py

        date: Optional[str] = None,
        metadata_only: Optional[bool] = None,
-        **kwargs
+        **kwargs,


This is fine to have these changes but, out of curiosity, do you use some code formatter that does this? I don't believe that you manually added these commas.

Right, these were added by the Python extension I have in my VSCode, it seems. But I kept it in there because it seemed better style to leave the trailing commas in there.

Got it. Thanks!

seratch · 2021-08-12T21:32:34Z

Probably, we can merge this PR soon. I've moved this one to v3.9 milestone.

srajiang · 2021-08-12T21:53:04Z

Probably, we can merge this PR soon. I've moved this one to v3.9 milestone.

Thanks @seratch - I think it should be ready now with the last set of tweaks based on your last comments.

seratch · 2021-08-12T22:08:02Z

slack_sdk/web/async_client.py

        date: Optional[str] = None,
        metadata_only: Optional[bool] = None,
-        **kwargs
+        **kwargs,


Got it. Thanks!

gitwave bot added the untriaged label Aug 12, 2021

srajiang added 2 commits August 11, 2021 18:07

Add Slack connect API support + tests

479f151

Update

9eae59a

srajiang force-pushed the sj-add-slack-connect-api branch from c253296 to 9eae59a Compare August 12, 2021 01:18

Tweak

9967662

srajiang added enhancement M-T: A feature request for new functionality and removed untriaged labels Aug 12, 2021

srajiang added this to the 3.10.0 milestone Aug 12, 2021

Tweak

6dd4bed

seratch reviewed Aug 12, 2021

View reviewed changes

srajiang added 2 commits August 11, 2021 18:44

Rename tests for clarity

34d8714

Fixes

8c294d5

srajiang commented Aug 12, 2021

View reviewed changes

Adds more test coverage

9a822ce

srajiang marked this pull request as ready for review August 12, 2021 17:21

srajiang self-assigned this Aug 12, 2021

srajiang added the Version: 3x label Aug 12, 2021

srajiang requested review from seratch and stevengill August 12, 2021 17:28

Add test coverage

ec977ca

seratch reviewed Aug 12, 2021

View reviewed changes

seratch modified the milestones: 3.10.0, 3.9.0 Aug 12, 2021

Fixes

32663c5

seratch approved these changes Aug 12, 2021

View reviewed changes

seratch merged commit b2364d2 into slackapi:main Aug 12, 2021

	SLACK_SDK_TEST_SENDER_BOT_TOKEN = "SLACK_SDK_TEST_SENDER_BOT_TOKEN"
	SLACK_SDK_TEST_CONNECT_INVITE_SENDER_BOT_TOKEN = "SLACK_SDK_TEST_CONNECT_INVITE_SENDER_BOT_TOKEN"

Conversation

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

Uh oh!

Summary

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

Requirements (place an x in each [ ])

Uh oh!

codecov bot commented Aug 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srajiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch commented Aug 12, 2021

Uh oh!

srajiang commented Aug 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

srajiang commented Aug 12, 2021 •

edited

Loading

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

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

codecov bot commented Aug 12, 2021 •

edited

Loading