Repository: slackapi/python-slackclient
Pull request #678 (2020-05-11 03:49:31)

Change list args to be either str or list in a consistent manner
###  Summary

Currently, Some API methods in `WebClient` accept only str values for "channel_ids", "users" etc while others accept list values for those. This pull request improves them to be more consistent. Also, this pull request fixes #444 .

### Requirements (place an `x` in each `[ ]`)

* [x] I've read and understood the [Contributing Guidelines](https://github.com/slackapi/python-slackclient/blob/master/.github/contributing.md) and have done my best effort to follow them.
* [x] I've read and agree to the [Code of Conduct](https://slackhq.github.io/code-of-conduct).

codecov[bot]: # [Codecov](https://codecov.io/gh/slackapi/python-slackclient/pull/678?src=pr&el=h1) Report
> Merging [#678](https://codecov.io/gh/slackapi/python-slackclient/pull/678?src=pr&el=desc) into [master](https://codecov.io/gh/slackapi/python-slackclient/commit/250045a7090c8531da05978ba505f3eb8f6ecc6b&el=desc) will **increase** coverage by `0.14%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/slackapi/python-slackclient/pull/678/graphs/tree.svg?width=650&height=150&src=pr&token=F0O7meYmtD)](https://codecov.io/gh/slackapi/python-slackclient/pull/678?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #678      +/-   ##
==========================================
+ Coverage   87.09%   87.23%   +0.14%     
==========================================
  Files          15       15              
  Lines        1836     1857      +21     
  Branches      104      112       +8     
==========================================
+ Hits         1599     1620      +21     
  Misses        203      203              
  Partials       34       34              
```


| [Impacted Files](https://codecov.io/gh/slackapi/python-slackclient/pull/678?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [slack/web/client.py](https://codecov.io/gh/slackapi/python-slackclient/pull/678/diff?src=pr&el=tree#diff-c2xhY2svd2ViL2NsaWVudC5weQ==) | `96.60% <100.00%> (+0.14%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/slackapi/python-slackclient/pull/678?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/slackapi/python-slackclient/pull/678?src=pr&el=footer). Last update [250045a...0baec0d](https://codecov.io/gh/slackapi/python-slackclient/pull/678?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/slack/web/client.py b/slack/web/client.py
index f418924..ce329a0 100644
--- a/slack/web/client.py
+++ b/slack/web/client.py
@@ -1,14 +1,12 @@
 """A Python module for interacting with Slack's Web API."""
 
-# Standard Imports
 import os
-from typing import Union, List
-from io import IOBase
 from asyncio import Future
+from io import IOBase
+from typing import Union, List
 
-# Internal Imports
-from slack.web.base_client import BaseClient, SlackResponse
 import slack.errors as e
+from slack.web.base_client import BaseClient, SlackResponse
 
 
 class WebClient(BaseClient):
@@ -236,16 +234,20 @@ class WebClient(BaseClient):
         return self.api_call("admin.teams.settings.info", json=kwargs)
 
     def admin_teams_settings_setDefaultChannels(
-        self, *, team_id: str, channel_ids: List[str], **kwargs
+        self, *, team_id: str, channel_ids: Union[str, List[str]], **kwargs
     ) -> Union[Future, SlackResponse]:
         """Set the default channels of a workspace.
 
         Args:
             team_id (str): ID of the team.
-            channel_ids (list): A list of channel_ids.
+            channel_ids (str or list): A list of channel_ids.
                 At least one channel is required. e.g. ['C1A2B3C4D', 'C26Z25Y24']
         """
-        kwargs.update({"team_id": team_id, "channel_ids": ",".join(channel_ids)})
+        kwargs.update({"team_id": team_id})
+        if isinstance(channel_ids, list):
+            kwargs.update({"channel_ids": ",".join(channel_ids)})
+        else:
+            kwargs.update({"channel_ids": channel_ids})
         return self.api_call(
             "admin.teams.settings.setDefaultChannels", http_verb="GET", params=kwargs
         )
@@ -314,19 +316,21 @@ class WebClient(BaseClient):
         return self.api_call("admin.users.assign", json=kwargs)
 
     def admin_users_invite(
-        self, *, team_id: str, email: str, channel_ids: List[str], **kwargs
+        self, *, team_id: str, email: str, channel_ids: Union[str, List[str]], **kwargs
     ) -> Union[Future, SlackResponse]:
         """Invite a user to a workspace.
 
         Args:
             team_id (str): ID of the team. e.g. 'T1234'
             email (str): The email address of the person to invite. e.g. 'joe@email.com'
-            channel_ids (list): A list of channel_ids for this user to join.
+            channel_ids (str or list): A list of channel_ids for this user to join.
                 At least one channel is required. e.g. ['C1A2B3C4D', 'C26Z25Y24']
         """
-        kwargs.update(
-            {"team_id": team_id, "email": email, "channel_ids": ",".join(channel_ids)}
-        )
+        kwargs.update({"team_id": team_id, "email": email})
+        if isinstance(channel_ids, list):
+            kwargs.update({"channel_ids": ",".join(channel_ids)})
+        else:
+            kwargs.update({"channel_ids": channel_ids})
         return self.api_call("admin.users.invite", json=kwargs)
 
     def admin_users_list(
@@ -764,15 +768,19 @@ class WebClient(BaseClient):
         return self.api_call("conversations.info", http_verb="GET", params=kwargs)
 
     def conversations_invite(
-        self, *, channel: str, users: List[str], **kwargs
+        self, *, channel: str, users: Union[str, List[str]], **kwargs
     ) -> Union[Future, SlackResponse]:
         """Invites users to a channel.
 
         Args:
             channel (str): The channel id. e.g. 'C1234567890'
-            users (list): An list of user id's to invite. e.g. ['U2345678901', 'U3456789012']
+            users (str or list): An list of user id's to invite. e.g. ['U2345678901', 'U3456789012']
         """
-        kwargs.update({"channel": channel, "users": users})
+        kwargs.update({"channel": channel})
+        if isinstance(users, list):
+            kwargs.update({"users": ",".join(users)})
+        else:
+            kwargs.update({"users": users})
         return self.api_call("conversations.invite", json=kwargs)
 
     def conversations_join(
@@ -941,8 +949,18 @@ class WebClient(BaseClient):
         kwargs.update({"num_minutes": num_minutes})
         return self.api_call("dnd.setSnooze", http_verb="GET", params=kwargs)
 
-    def dnd_teamInfo(self, **kwargs) -> Union[Future, SlackResponse]:
-        """Retrieves the Do Not Disturb status for users on a team."""
+    def dnd_teamInfo(
+        self, users: Union[str, List[str]], **kwargs
+    ) -> Union[Future, SlackResponse]:
+        """Retrieves the Do Not Disturb status for users on a team.
+
+        Args:
+            users (str or list): User IDs to fetch information e.g. 'U123,U234' or ["U123", "U234"]
+        """
+        if isinstance(users, list):
+            kwargs.update({"users": ",".join(users)})
+        else:
+            kwargs.update({"users": users})
         return self.api_call("dnd.teamInfo", http_verb="GET", params=kwargs)
 
     def emoji_list(self, **kwargs) -> Union[Future, SlackResponse]:
@@ -1031,10 +1049,13 @@ class WebClient(BaseClient):
         """Share a remote file into a channel.
 
         Args:
-            channels (list): Comma-separated list of channel IDs where the file will be shared.
+            channels (str or list): Comma-separated list of channel IDs where the file will be shared.
                 e.g. ['C1234567890', 'C2345678901']
         """
-        kwargs.update({"channels": channels})
+        if isinstance(channels, list):
+            kwargs.update({"channels": ",".join(channels)})
+        else:
+            kwargs.update({"channels": channels})
         return self.api_call("files.remote.share", http_verb="GET", params=kwargs)
 
     def files_revokePublicURL(
@@ -1310,15 +1331,18 @@ class WebClient(BaseClient):
         return self.api_call("im.replies", http_verb="GET", params=kwargs)
 
     def migration_exchange(
-        self, *, users: List[str], **kwargs
+        self, *, users: Union[str, List[str]], **kwargs
     ) -> Union[Future, SlackResponse]:
         """For Enterprise Grid workspaces, map local user IDs to global user IDs
 
         Args:
-            users (list): A list of user ids, up to 400 per request.
+            users (str or list): A list of user ids, up to 400 per request.
                 e.g. ['W1234567890', 'U2345678901', 'U3456789012']
         """
-        kwargs.update({"users": users})
+        if isinstance(users, list):
+            kwargs.update({"users": ",".join(users)})
+        else:
+            kwargs.update({"users": users})
         return self.api_call("migration.exchange", http_verb="GET", params=kwargs)
 
     def mpim_close(self, *, channel: str, **kwargs) -> Union[Future, SlackResponse]:
@@ -1357,15 +1381,20 @@ class WebClient(BaseClient):
         kwargs.update({"channel": channel, "ts": ts})
         return self.api_call("mpim.mark", json=kwargs)
 
-    def mpim_open(self, *, users: List[str], **kwargs) -> Union[Future, SlackResponse]:
+    def mpim_open(
+        self, *, users: Union[str, List[str]], **kwargs
+    ) -> Union[Future, SlackResponse]:
         """This method opens a multiparty direct message.
 
         Args:
-            users (list): A lists of user ids. The ordering of the users
+            users (str or list): A lists of user ids. The ordering of the users
                 is preserved whenever a MPIM group is returned.
                 e.g. ['W1234567890', 'U2345678901', 'U3456789012']
         """
-        kwargs.update({"users": users})
+        if isinstance(users, list):
+            kwargs.update({"users": ",".join(users)})
+        else:
+            kwargs.update({"users": users})
         return self.api_call("mpim.open", json=kwargs)
 
     def mpim_replies(
@@ -1681,17 +1710,21 @@ class WebClient(BaseClient):
         return self.api_call("usergroups.users.list", http_verb="GET", params=kwargs)
 
     def usergroups_users_update(
-        self, *, usergroup: str, users: List[str], **kwargs
+        self, *, usergroup: str, users: Union[str, List[str]], **kwargs
     ) -> Union[Future, SlackResponse]:
         """Update the list of users for a User Group
 
         Args:
             usergroup (str): The encoded ID of the User Group to update.
                 e.g. 'S0604QSJC'
-            users (list): A list user IDs that represent the entire list of
+            users (str or list): A list user IDs that represent the entire list of
                 users for the User Group. e.g. ['U060R4BJ4', 'U060RNRCZ']
         """
-        kwargs.update({"usergroup": usergroup, "users": users})
+        kwargs.update({"usergroup": usergroup})
+        if isinstance(users, list):
+            kwargs.update({"users": ",".join(users)})
+        else:
+            kwargs.update({"users": users})
         return self.api_call("usergroups.users.update", json=kwargs)
 
     def users_conversations(self, **kwargs) -> Union[Future, SlackResponse]:
@@ -1775,7 +1808,7 @@ class WebClient(BaseClient):
     def views_open(
         self, *, trigger_id: str, view: dict, **kwargs
     ) -> Union[Future, SlackResponse]:
-        """Open a view for a user.​
+        """Open a view for a user.
 
         Open a modal with a user by exchanging a trigger_id received
         from another interaction.


Test patch:
diff --git a/tests/web/test_web_client_coverage.py b/tests/web/test_web_client_coverage.py
index 366cc65..438d5f2 100644
--- a/tests/web/test_web_client_coverage.py
+++ b/tests/web/test_web_client_coverage.py
@@ -1,12 +1,12 @@
-# Standard Imports
+import asyncio
+import json
 import os
+import re
 import unittest
+from urllib.parse import parse_qsl
 
-# ThirdParty Imports
-import asyncio
 from aiohttp import web
 
-# Internal Imports
 import slack
 
 
@@ -54,6 +54,22 @@ class TestWebClientCoverage(unittest.TestCase):
         self.site = web.TCPSite(runner, "localhost", 8765)
         await self.site.start()
 
+    @staticmethod
+    def parse_payload(body: str, content_type: str) -> dict:
+        if not body:
+            return {}
+        if content_type == "application/json" or body.startswith("{"):
+            return json.loads(body)
+        if content_type == "application/x-www-form-urlencoded":
+            if "payload" in body:
+                params = dict(parse_qsl(body))
+                if "payload" in params:
+                    return json.loads(params.get("payload"))
+                return {}
+            return dict(parse_qsl(body))
+
+        return {}
+
     async def handler(self, request):
         content_type = request.content_type
         assert content_type in [
@@ -62,7 +78,18 @@ class TestWebClientCoverage(unittest.TestCase):
             "multipart/form-data",
         ]
         # This `api_method` is done
-        self.api_methods_to_call.remove(request.path.replace("/", ""))
+        method = request.path.replace("/", "")
+        if method in self.api_methods_to_call:
+            self.api_methods_to_call.remove(method)
+
+        body = (await request.read()).decode("utf-8")
+        body = self.parse_payload(body, content_type)
+        ids = ["channels", "users", "channel_ids"]
+        for k, v in body.items():
+            if k in ids:
+                self.assertTrue(
+                    re.compile(r"^[^,\[\]]+?,[^,\[\]]+$").match(v),
+                    f"The parameter {k} is not a comma-separated string value: {v}")
         return web.json_response({"ok": True})
 
     def test_coverage(self):
@@ -90,6 +117,7 @@ class TestWebClientCoverage(unittest.TestCase):
                     method(team_id="T123")
                 elif method_name == "admin_teams_settings_setDefaultChannels":
                     method(team_id="T123", channel_ids=["C123", "C234"])
+                    method(team_id="T123", channel_ids="C123")
                 elif method_name == "admin_teams_settings_setDescription":
                     method(team_id="T123", description="Workspace for an awesome team")
                 elif method_name == "admin_teams_settings_setDiscoverability":
@@ -109,6 +137,11 @@ class TestWebClientCoverage(unittest.TestCase):
                         email="test@example.com",
                         channel_ids=["C1A2B3C4D", "C26Z25Y24"],
                     )
+                    method(
+                        team_id="T123",
+                        email="test@example.com",
+                        channel_ids="C1A2B3C4D,C26Z25Y24",
+                    )
                 elif method_name == "admin_users_list":
                     method(team_id="T123")
                 elif method_name == "admin_users_remove":
@@ -159,6 +192,7 @@ class TestWebClientCoverage(unittest.TestCase):
                     method(channel="C123")
                 elif method_name == "conversations_invite":
                     method(channel="C123", users=["U2345678901", "U3456789012"])
+                    method(channel="C123", users="U2345678901,U3456789012")
                 elif method_name == "conversations_join":
                     method(channel="C123")
                 elif method_name == "conversations_kick":
@@ -181,6 +215,9 @@ class TestWebClientCoverage(unittest.TestCase):
                     method(dialog={}, trigger_id="123")
                 elif method_name == "dnd_setSnooze":
                     method(num_minutes=120)
+                elif method_name == "dnd_teamInfo":
+                    method(users=["123", "U234"])
+                    method(users="U123,U234")
                 elif method_name == "files_comments_delete":
                     method(file="F123", id="FC123")
                 elif method_name == "files_delete":
@@ -200,8 +237,13 @@ class TestWebClientCoverage(unittest.TestCase):
                         title="File title",
                     )
                 elif method_name == "files_remote_share":
+                    method(channels=["C123", "G123"])
                     method(channels="C123,G123")
                 elif method_name == "migration_exchange":
+                    method(users=["U123", "U234"])
+                    method(users="U123,U234")
+                elif method_name == "mpim_open":
+                    method(users=["U123", "U234"])
                     method(users="U123,U234")
                 elif method_name == "oauth_access":
                     method = getattr(self.no_token_client, method_name, None)
@@ -245,6 +287,7 @@ class TestWebClientCoverage(unittest.TestCase):
                     method(usergroup="UG123")
                 elif method_name == "usergroups_users_update":
                     method(usergroup="UG123", users=["U123", "U234"])
+                    method(usergroup="UG123", users="U123,U234")
                 elif method_name == "users_getPresence":
                     method(user="U123")
                 elif method_name == "users_info":
@@ -339,6 +382,7 @@ class TestWebClientCoverage(unittest.TestCase):
                     method(channel="D123", ts="123.123")
                 elif method_name == "mpim_open":
                     method(users=["U123", "U234"])
+                    method(users="U123,U234")
                 elif method_name == "mpim_replies":
                     method(channel="D123", thread_ts="123.123")
                 else: