From 3dbd634626fe9b86525bb7be0ba741eed067b4da Mon Sep 17 00:00:00 2001 From: Guillaume Charest <1690085+gcharest@users.noreply.github.com> Date: Thu, 1 Aug 2024 15:42:52 +0000 Subject: [PATCH] fix: introduce method to paginate for users.list --- app/integrations/slack/users.py | 61 ++++++++- app/tests/integrations/slack/test_users.py | 145 ++++++++++++++++++--- 2 files changed, 182 insertions(+), 24 deletions(-) diff --git a/app/integrations/slack/users.py b/app/integrations/slack/users.py index fab677db..66abfab3 100644 --- a/app/integrations/slack/users.py +++ b/app/integrations/slack/users.py @@ -5,9 +5,50 @@ import re from slack_sdk import WebClient +from logging import getLogger SLACK_USER_ID_REGEX = r"^[A-Z0-9]+$" +logger = getLogger(__name__) + + +def get_all_users(client: WebClient, deleted=False, is_bot=False): + """Get all users from the Slack workspace. + + Args: + client (WebClient): The Slack client instance. + deleted (bool, optional): Include deleted users. Defaults to False. + is_bot (bool, optional): Include bot users. Defaults to False. + + Returns: + list: A list of active users. + """ + + users_list = [] + cursor = None + try: + while True: + response = client.users_list(cursor=cursor, limit=200) + if response["ok"]: + if "members" in response: + users_list.extend(response["members"]) + cursor = response["response_metadata"]["next_cursor"] + if not cursor: + break + else: + logger.error(f"Failed to get users list: {response['error']}") + break + except Exception as e: + logger.error(f"Failed to get users list: {e}") + + # filters + if not deleted: + users_list = [user for user in users_list if not user["deleted"]] + if not is_bot: + users_list = [user for user in users_list if not user["is_bot"]] + + return users_list + def get_user_id_from_request(body): """ @@ -50,18 +91,28 @@ def get_user_email_from_body(client: WebClient, body): return user_info["user"]["profile"]["email"] -def get_user_email_from_handle(client: WebClient, user_handle: str): - user_handle = user_handle.lstrip("@") +def get_user_email_from_handle(client: WebClient, user_handle: str) -> str | None: + """ + Returns the user email from a user handle, otherwise None. + + Args: + client (WebClient): The Slack client instance. + user_handle (str): The user handle. - users_list = client.users_list() + Returns: + str | None: The user email or None. + """ + user_handle = user_handle.lstrip("@") - for member in users_list["members"]: + users_list = get_all_users(client) + for member in users_list: if "name" in member and member["name"] == user_handle: user_id = member["id"] user_info = client.users_info(user=user_id) - return user_info["user"]["profile"]["email"] + if user_info["ok"]: + return user_info["user"]["profile"]["email"] return None diff --git a/app/tests/integrations/slack/test_users.py b/app/tests/integrations/slack/test_users.py index 9016f705..1a2b6725 100644 --- a/app/tests/integrations/slack/test_users.py +++ b/app/tests/integrations/slack/test_users.py @@ -1,9 +1,109 @@ -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch from integrations.slack import users from slack_sdk import WebClient +def test_get_all_users(): + client = MagicMock() + client.users_list.return_value = { + "ok": True, + "members": [ + {"id": "U00AAAAAAA0", "name": "user1", "deleted": False, "is_bot": False}, + {"id": "U00AAAAAAA1", "name": "user2", "deleted": True, "is_bot": False}, + {"id": "U00AAAAAAA2", "name": "user3", "deleted": False, "is_bot": True}, + ], + } + assert users.get_all_users(client) == [ + {"id": "U00AAAAAAA0", "name": "user1", "deleted": False, "is_bot": False} + ] + + +def test_get_all_users_with_pagination(): + client = MagicMock() + client.users_list.side_effect = [ + { + "ok": True, + "members": [ + { + "id": "U00AAAAAAA0", + "name": "user1", + "deleted": False, + "is_bot": False, + }, + { + "id": "U00AAAAAAA1", + "name": "user2", + "deleted": False, + "is_bot": False, + }, + ], + "response_metadata": {"next_cursor": "cursor"}, + }, + { + "ok": True, + "members": [ + { + "id": "U00AAAAAAA2", + "name": "user3", + "deleted": False, + "is_bot": False, + } + ], + "response_metadata": { + "next_cursor": "", + }, + }, + ] + assert users.get_all_users(client) == [ + {"id": "U00AAAAAAA0", "name": "user1", "deleted": False, "is_bot": False}, + {"id": "U00AAAAAAA1", "name": "user2", "deleted": False, "is_bot": False}, + {"id": "U00AAAAAAA2", "name": "user3", "deleted": False, "is_bot": False}, + ] + + assert client.users_list.call_count == 2 + + +def test_get_all_users_with_deleted(): + client = MagicMock() + client.users_list.return_value = { + "ok": True, + "members": [ + {"id": "U00AAAAAAA0", "name": "user1", "deleted": False, "is_bot": False}, + {"id": "U00AAAAAAA1", "name": "user2", "deleted": True, "is_bot": False}, + {"id": "U00AAAAAAA2", "name": "user3", "deleted": False, "is_bot": True}, + ], + } + assert users.get_all_users(client, deleted=True) == [ + {"id": "U00AAAAAAA0", "name": "user1", "deleted": False, "is_bot": False}, + {"id": "U00AAAAAAA1", "name": "user2", "deleted": True, "is_bot": False}, + ] + + +def test_get_all_users_with_bot(): + client = MagicMock() + client.users_list.return_value = { + "ok": True, + "members": [ + {"id": "U00AAAAAAA0", "name": "user1", "deleted": False, "is_bot": False}, + {"id": "U00AAAAAAA1", "name": "user2", "deleted": True, "is_bot": False}, + {"id": "U00AAAAAAA2", "name": "user3", "deleted": False, "is_bot": True}, + ], + } + assert users.get_all_users(client, is_bot=True) == [ + {"id": "U00AAAAAAA0", "name": "user1", "deleted": False, "is_bot": False}, + {"id": "U00AAAAAAA2", "name": "user3", "deleted": False, "is_bot": True}, + ] + + +def test_get_all_users_with_error(): + client = MagicMock() + with patch("integrations.slack.users.logger") as mock_logger: + client.users_list.return_value = {"ok": False, "error": "error"} + assert users.get_all_users(client) == [] + mock_logger.error.assert_called_once_with("Failed to get users list: error") + + def test_get_user_id_from_request_with_user_id(): body = {"user_id": "U00AAAAAAA0"} assert users.get_user_id_from_request(body) == "U00AAAAAAA0" @@ -130,8 +230,14 @@ def users_profile_get(user): assert users.replace_user_id_with_handle(client, message) == expected_message -def test_get_user_email(): +@patch("integrations.slack.users.get_all_users") +def test_get_user_email(mock_get_all_users): # Mock the WebClient + + mock_get_all_users.return_value = [ + {"id": "U1234", "name": "user_name1", "profile": {"email": "email1@test.com"}}, + {"id": "U5678", "name": "user_name2", "profile": {"email": "email2@test.com"}}, + ] client = MagicMock(spec=WebClient) # Test when the user ID is found in the request body and the users_info call is successful @@ -152,25 +258,26 @@ def test_get_user_email(): assert users.get_user_email_from_body(client, body) is None -def test_get_user_email_from_handle(): +@patch("integrations.slack.users.get_all_users") +def test_get_user_email_from_handle(mock_get_all_users): + client = MagicMock(spec=WebClient) - client.users_list.return_value = { - "members": [ - { - "id": "U1234", - "name": "user_name1", - "profile": {"email": "user_email1@test.com"}, - }, - { - "id": "U5678", - "name": "user_name2", - "profile": {"email": "user_email2@test.com"}, - }, - ] - } + mock_get_all_users.return_value = [ + { + "id": "U1234", + "name": "user_name1", + "profile": {"email": "user_email1@test.com"}, + }, + { + "id": "U5678", + "name": "user_name2", + "profile": {"email": "user_email2@test.com"}, + }, + ] + client.users_info.side_effect = lambda user: { - "U1234": {"user": {"profile": {"email": "user_email1@test.com"}}}, - "U5678": {"user": {"profile": {"email": "user_email2@test.com"}}}, + "U1234": {"ok": True, "user": {"profile": {"email": "user_email1@test.com"}}}, + "U5678": {"ok": True, "user": {"profile": {"email": "user_email2@test.com"}}}, }.get(user, {}) assert (