diff --git a/zerver/data_import/slack.py b/zerver/data_import/slack.py index 183f2861d1..364f78083a 100755 --- a/zerver/data_import/slack.py +++ b/zerver/data_import/slack.py @@ -187,7 +187,8 @@ def users_to_zerver_userprofile(slack_data_dir: str, users: List[ZerverFieldsT], userprofile = UserProfile( full_name=get_user_full_name(user), short_name=user['name'], - is_active=not user['deleted'], + is_active=not user.get('deleted', False) and not user["is_mirror_dummy"], + is_mirror_dummy=user["is_mirror_dummy"], id=user_id, email=email, delivery_email=email, @@ -287,6 +288,8 @@ def process_customprofilefields(customprofilefield: List[ZerverFieldsT], def get_user_email(user: ZerverFieldsT, domain_name: str) -> str: if 'email' in user['profile']: return user['profile']['email'] + if user['is_mirror_dummy']: + return "{}@{}.slack.com".format(user["name"], user["team_domain"]) if 'bot_id' in user['profile']: if 'real_name_normalized' in user['profile']: slack_bot_name = user['profile']['real_name_normalized'] @@ -609,7 +612,7 @@ def convert_slack_workspace_messages(slack_data_dir: str, users: List[ZerverFiel logging.info('######### IMPORTING MESSAGES FINISHED #########\n') return total_reactions, total_uploads, total_attachments -def get_messages_iterator(slack_data_dir: str, added_channels: AddedChannelsT, +def get_messages_iterator(slack_data_dir: str, added_channels: Dict[str, Any], added_mpims: AddedMPIMsT, dm_members: DMMembersT) -> Iterator[ZerverFieldsT]: """This function is an iterator that returns all the messages across all Slack channels, in order by timestamp. It's important to @@ -950,6 +953,45 @@ def get_message_sending_user(message: ZerverFieldsT) -> Optional[str]: return message['file'].get('user') return None +def fetch_shared_channel_users(user_list: List[ZerverFieldsT], slack_data_dir: str, token: str) -> None: + normal_user_ids = set() + mirror_dummy_user_ids = set() + added_channels = {} + team_id_to_domain = {} # type: Dict[str, str] + for user in user_list: + user["is_mirror_dummy"] = False + normal_user_ids.add(user["id"]) + + public_channels = get_data_file(slack_data_dir + '/channels.json') + try: + private_channels = get_data_file(slack_data_dir + '/groups.json') + except FileNotFoundError: + private_channels = [] + for channel in public_channels + private_channels: + added_channels[channel["name"]] = True + for user_id in channel["members"]: + if user_id not in normal_user_ids: + mirror_dummy_user_ids.add(user_id) + + all_messages = get_messages_iterator(slack_data_dir, added_channels, {}, {}) + for message in all_messages: + user_id = get_message_sending_user(message) + if user_id is None or user_id in normal_user_ids: + continue + mirror_dummy_user_ids.add(user_id) + + # Fetch data on the mirror_dummy_user_ids from the Slack API (it's + # not included in the data export file). + for user_id in mirror_dummy_user_ids: + user = get_slack_api_data("https://slack.com/api/users.info", "user", token=token, user=user_id) + team_id = user["team_id"] + if team_id not in team_id_to_domain: + team = get_slack_api_data("https://slack.com/api/team.info", "team", token=token, team=team_id) + team_id_to_domain[team_id] = team["domain"] + user["team_domain"] = team_id_to_domain[team_id] + user["is_mirror_dummy"] = True + user_list.append(user) + def do_convert_data(slack_zip_file: str, output_dir: str, token: str, threads: int=6) -> None: # Subdomain is set by the user while running the import command realm_subdomain = "" @@ -972,6 +1014,7 @@ def do_convert_data(slack_zip_file: str, output_dir: str, token: str, threads: i # We get the user data from the legacy token method of slack api, which is depreciated # but we use it as the user email data is provided only in this method user_list = get_slack_api_data("https://slack.com/api/users.list", "members", token=token) + fetch_shared_channel_users(user_list, slack_data_dir, token) # Get custom emoji from slack api custom_emoji_list = get_slack_api_data("https://slack.com/api/emoji.list", "emoji", token=token) diff --git a/zerver/data_import/slack_message_conversion.py b/zerver/data_import/slack_message_conversion.py index 55f8ac6b9d..3a01dcc6a4 100644 --- a/zerver/data_import/slack_message_conversion.py +++ b/zerver/data_import/slack_message_conversion.py @@ -57,11 +57,10 @@ SLACK_BOLD_REGEX = r""" """ def get_user_full_name(user: ZerverFieldsT) -> str: - if user['deleted'] is False: - if user['real_name'] == '': - return user['name'] - else: - return user['real_name'] + if "deleted" in user and user['deleted'] is False: + return user['real_name'] or user['name'] + elif user["is_mirror_dummy"]: + return user["profile"].get("real_name", user["name"]) else: return user['name'] diff --git a/zerver/tests/fixtures/slack_fixtures/channels.json b/zerver/tests/fixtures/slack_fixtures/channels.json index 8d635b27ac..59214848fe 100644 --- a/zerver/tests/fixtures/slack_fixtures/channels.json +++ b/zerver/tests/fixtures/slack_fixtures/channels.json @@ -43,10 +43,10 @@ }, { "id": "C061A0HJG", - "name": "feedback", + "name": "sharedchannel", "created": "1433558359", "is_general": false, - "members": ["U061A3E0G"], + "members": ["U061A3E0G", "U061A1R2R"], "is_archived": false, "topic": { "value": "" diff --git a/zerver/tests/test_slack_importer.py b/zerver/tests/test_slack_importer.py index a9b542b6da..b3c1fedc8e 100644 --- a/zerver/tests/test_slack_importer.py +++ b/zerver/tests/test_slack_importer.py @@ -6,6 +6,7 @@ from zerver.data_import.slack import ( get_slack_api_data, get_admin, get_user_timezone, + fetch_shared_channel_users, users_to_zerver_userprofile, get_subscription, channels_to_zerver_stream, @@ -51,7 +52,7 @@ import logging import shutil import os import mock -from mock import ANY +from mock import ANY, call from typing import Any, Dict, List, Set, Tuple, Iterator def remove_folder(path: str) -> None: @@ -91,6 +92,10 @@ class SlackImporter(ZulipTestCase): get_slack_api_data(slack_user_list_url, "members", token=token) self.assertEqual(invalid.exception.args, ('Enter a valid token!',),) + with self.assertRaises(Exception) as invalid: + get_slack_api_data(slack_user_list_url, "members") + self.assertEqual(invalid.exception.args, ('Pass slack token in kwargs',),) + token = 'status404' wrong_url = "https://slack.com/api/wrong" with self.assertRaises(Exception) as invalid: @@ -128,6 +133,57 @@ class SlackImporter(ZulipTestCase): self.assertEqual(get_user_timezone(user_timezone_none), "America/New_York") self.assertEqual(get_user_timezone(user_no_timezone), "America/New_York") + @mock.patch("zerver.data_import.slack.get_data_file") + @mock.patch("zerver.data_import.slack.get_slack_api_data") + @mock.patch("zerver.data_import.slack.get_messages_iterator") + def test_fetch_shared_channel_users(self, messages_mock: mock.Mock, api_mock: mock.Mock, + data_file_mock: mock.Mock) -> None: + users = [{"id": "U061A1R2R"}, {"id": "U061A5N1G"}, {"id": "U064KUGRJ"}] + data_file_mock.side_effect = [ + [ + {"name": "general", "members": ["U061A1R2R", "U061A5N1G"]}, + {"name": "sharedchannel", "members": ["U061A1R2R", "U061A3E0G"]} + ], + [] + ] + api_mock.side_effect = [ + {"id": "U061A3E0G", "team_id": "T6LARQE2Z"}, + {"domain": "foreignteam1"}, + {"id": "U061A8H1G", "team_id": "T7KJRQE8Y"}, + {"domain": "foreignteam2"}, + ] + messages_mock.return_value = [ + {"user": "U061A1R2R"}, + {"user": "U061A5N1G"}, + {"user": "U061A8H1G"}, + ] + slack_data_dir = self.fixture_file_name('', type='slack_fixtures') + fetch_shared_channel_users(users, slack_data_dir, "token") + + # Normal users + self.assertEqual(len(users), 5) + self.assertEqual(users[0]["id"], "U061A1R2R") + self.assertEqual(users[0]["is_mirror_dummy"], False) + self.assertFalse("team_domain" in users[0]) + self.assertEqual(users[1]["id"], "U061A5N1G") + self.assertEqual(users[2]["id"], "U064KUGRJ") + + # Shared channel users + self.assertEqual(users[3]["id"], "U061A3E0G") + self.assertEqual(users[3]["team_domain"], "foreignteam1") + self.assertEqual(users[3]["is_mirror_dummy"], True) + self.assertEqual(users[4]["id"], "U061A8H1G") + self.assertEqual(users[4]["team_domain"], "foreignteam2") + self.assertEqual(users[4]["is_mirror_dummy"], True) + + api_calls = [ + call("https://slack.com/api/users.info", "user", token="token", user="U061A3E0G"), + call("https://slack.com/api/team.info", "team", token="token", team="T6LARQE2Z"), + call("https://slack.com/api/users.info", "user", token="token", user="U061A8H1G"), + call("https://slack.com/api/team.info", "team", token="token", team="T7KJRQE8Y") + ] + api_mock.assert_has_calls(api_calls, any_order=True) + @mock.patch("zerver.data_import.slack.get_data_file") def test_users_to_zerver_userprofile(self, mock_get_data_file: mock.Mock) -> None: custom_profile_field_user1 = {"Xf06054BBB": {"value": "random1"}, @@ -138,6 +194,7 @@ class SlackImporter(ZulipTestCase): "team_id": "T5YFFM2QY", "name": "john", "deleted": False, + "is_mirror_dummy": False, "real_name": "John Doe", "profile": {"image_32": "", "email": "jon@gmail.com", "avatar_hash": "hash", "phone": "+1-123-456-77-868", @@ -151,6 +208,7 @@ class SlackImporter(ZulipTestCase): 'name': 'Jane', "real_name": "Jane Doe", "deleted": False, + "is_mirror_dummy": False, "profile": {"image_32": "https://secure.gravatar.com/avatar/random.png", "fields": custom_profile_field_user2, "email": "jane@foo.com", "avatar_hash": "hash"}}, @@ -160,16 +218,26 @@ class SlackImporter(ZulipTestCase): "real_name": "Bot", "is_bot": True, "deleted": False, + "is_mirror_dummy": False, "profile": {"image_32": "https://secure.gravatar.com/avatar/random1.png", "skype": "test_skype_name", - "email": "bot1@zulipchat.com", "avatar_hash": "hash"}}] + "email": "bot1@zulipchat.com", "avatar_hash": "hash"}}, + {"id": "UHSG7OPQN", + "team_id": "T6LARQE2Z", + 'name': 'matt.perry', + "color": '9d8eee', + "is_bot": False, + "is_app_user": False, + "is_mirror_dummy": True, + "team_domain": "foreignteam", + "profile": {"image_32": "https://secure.gravatar.com/avatar/random6.png", + "avatar_hash": "hash", "first_name": "Matt", "last_name": "Perry", + "real_name": "Matt Perry", "display_name": "matt.perry", "team": "T6LARQE2Z"}}] mock_get_data_file.return_value = user_data # As user with slack_id 'U0CBK5KAT' is the primary owner, that user should be imported first # and hence has zulip_id = 1 - test_added_users = {'U08RGD1RD': 1, - 'U0CBK5KAT': 0, - 'U09TYF5Sk': 2} + test_added_users = {'U08RGD1RD': 1, 'U0CBK5KAT': 0, 'U09TYF5Sk': 2, 'UHSG7OPQN': 3} slack_data_dir = './random_path' timestamp = int(timezone_now().timestamp()) mock_get_data_file.return_value = user_data @@ -196,20 +264,44 @@ class SlackImporter(ZulipTestCase): # test that the primary owner should always be imported first self.assertDictEqual(added_users, test_added_users) - self.assertEqual(len(avatar_list), 3) + self.assertEqual(len(avatar_list), 4) + + self.assertEqual(len(zerver_userprofile), 4) + + self.assertEqual(zerver_userprofile[0]['is_staff'], False) + self.assertEqual(zerver_userprofile[0]['is_bot'], False) + self.assertEqual(zerver_userprofile[0]['is_active'], True) + self.assertEqual(zerver_userprofile[0]['is_mirror_dummy'], False) + self.assertEqual(zerver_userprofile[0]['is_realm_admin'], False) + self.assertEqual(zerver_userprofile[0]['enable_desktop_notifications'], True) + self.assertEqual(zerver_userprofile[0]['email'], 'jon@gmail.com') + self.assertEqual(zerver_userprofile[0]['full_name'], 'John Doe') self.assertEqual(zerver_userprofile[1]['id'], test_added_users['U0CBK5KAT']) - self.assertEqual(len(zerver_userprofile), 3) - self.assertEqual(zerver_userprofile[1]['id'], 0) self.assertEqual(zerver_userprofile[1]['is_realm_admin'], True) self.assertEqual(zerver_userprofile[1]['is_staff'], False) self.assertEqual(zerver_userprofile[1]['is_active'], True) - self.assertEqual(zerver_userprofile[0]['is_staff'], False) - self.assertEqual(zerver_userprofile[0]['is_bot'], False) - self.assertEqual(zerver_userprofile[0]['enable_desktop_notifications'], True) + self.assertEqual(zerver_userprofile[0]['is_mirror_dummy'], False) + + self.assertEqual(zerver_userprofile[2]['id'], test_added_users['U09TYF5Sk']) + self.assertEqual(zerver_userprofile[2]['is_bot'], True) + self.assertEqual(zerver_userprofile[2]['is_active'], True) + self.assertEqual(zerver_userprofile[2]['is_mirror_dummy'], False) + self.assertEqual(zerver_userprofile[2]['email'], 'bot1@zulipchat.com') self.assertEqual(zerver_userprofile[2]['bot_type'], 1) self.assertEqual(zerver_userprofile[2]['avatar_source'], 'U') + self.assertEqual(zerver_userprofile[3]['id'], test_added_users['UHSG7OPQN']) + self.assertEqual(zerver_userprofile[3]['is_realm_admin'], False) + self.assertEqual(zerver_userprofile[3]['is_staff'], False) + self.assertEqual(zerver_userprofile[3]['is_active'], False) + self.assertEqual(zerver_userprofile[3]['email'], 'matt.perry@foreignteam.slack.com') + self.assertEqual(zerver_userprofile[3]['realm'], 1) + self.assertEqual(zerver_userprofile[3]['full_name'], 'Matt Perry') + self.assertEqual(zerver_userprofile[3]['short_name'], 'matt.perry') + self.assertEqual(zerver_userprofile[3]['is_mirror_dummy'], True) + self.assertEqual(zerver_userprofile[3]['is_api_super_user'], False) + def test_build_defaultstream(self) -> None: realm_id = 1 stream_id = 1 @@ -266,7 +358,7 @@ class SlackImporter(ZulipTestCase): added_recipient = channels_to_zerver_stream(self.fixture_file_name("", "slack_fixtures"), realm_id, {"zerver_userpresence": []}, added_users, zerver_userprofile) - test_added_channels = {'feedback': ("C061A0HJG", 3), 'general': ("C061A0YJG", 1), + test_added_channels = {'sharedchannel': ("C061A0HJG", 3), 'general': ("C061A0YJG", 1), 'general1': ("C061A0YJP", 2), 'random': ("C061A0WJG", 0)} test_added_mpims = {'mpdm-user9--user2--user10-1': ('G9HBG2A5D', 0), 'mpdm-user6--user7--user4-1': ('G6H1Z0ZPS', 1), diff --git a/zerver/tests/test_slack_message_conversion.py b/zerver/tests/test_slack_message_conversion.py index 28e31ead57..859b4a72af 100644 --- a/zerver/tests/test_slack_message_conversion.py +++ b/zerver/tests/test_slack_message_conversion.py @@ -57,13 +57,16 @@ class SlackMessageConversion(ZulipTestCase): users = [{"id": "U0CBK5KAT", "name": "aaron.anzalone", "deleted": False, + "is_mirror_dummy": False, "real_name": ""}, {"id": "U08RGD1RD", "name": "john", "deleted": False, + "is_mirror_dummy": False, "real_name": "John Doe"}, {"id": "U09TYF5Sk", "name": "Jane", + "is_mirror_dummy": False, "deleted": True}] # Deleted users don't have 'real_name' key in Slack channel_map = {'general': ('C5Z73A7RA', 137)} message = 'Hi <@U08RGD1RD|john>: How are you? <#C5Z73A7RA|general>'