From 5c32054415fd2dbc02ebd3a20b79efbc531018c0 Mon Sep 17 00:00:00 2001 From: Rohitt Vashishtha Date: Thu, 19 Mar 2020 02:39:30 +0530 Subject: [PATCH] bots: Do not reply in group PMs unless explicitly mentioned. Previously, if a bot was accidentally added to a group PM, we would have no option but to leave that conversation because the bot would reply to all the messages sent in that conversation. This also has potential to cause infinite loops in case two bots are added to a group PM since they could keep on replying to each other's messages. Fixes #551. --- zulip_bots/zulip_bots/lib.py | 12 +++++++----- zulip_bots/zulip_bots/tests/test_lib.py | 24 +++++++++++++++++++++++- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/zulip_bots/zulip_bots/lib.py b/zulip_bots/zulip_bots/lib.py index 1911e58..f8dc626 100644 --- a/zulip_bots/zulip_bots/lib.py +++ b/zulip_bots/zulip_bots/lib.py @@ -268,7 +268,7 @@ def extract_query_without_mention(message: Dict[str, Any], client: ExternalBotHa return None -def is_private_message_from_another_user(message_dict: Dict[str, Any], current_user_id: int) -> bool: +def is_private_message_but_not_group_pm(message_dict: Dict[str, Any], current_user: ExternalBotHandler) -> bool: """ Checks whether a message dict represents a PM from another user. @@ -276,9 +276,11 @@ def is_private_message_from_another_user(message_dict: Dict[str, Any], current_u zulip/zulip project, so refactor with care. See the comments in extract_query_without_mention. """ - if message_dict['type'] == 'private': - return current_user_id != message_dict['sender_id'] - return False + if not message_dict['type'] == 'private': + return False + is_message_from_self = current_user.user_id == message_dict['sender_id'] + recipients = [x['email'] for x in message_dict['display_recipient'] if current_user.email != x['email']] + return len(recipients) == 1 and not is_message_from_self def display_config_file_errors(error_msg: str, config_file: str) -> None: @@ -346,7 +348,7 @@ def run_message_handler_for_bot( # `mentioned` will be in `flags` if the bot is mentioned at ANY position # (not necessarily the first @mention in the message). is_mentioned = 'mentioned' in flags - is_private_message = is_private_message_from_another_user(message, restricted_client.user_id) + is_private_message = is_private_message_but_not_group_pm(message, restricted_client) # Provide bots with a way to access the full, unstripped message message['full_content'] = message['content'] diff --git a/zulip_bots/zulip_bots/tests/test_lib.py b/zulip_bots/zulip_bots/tests/test_lib.py index 9d60ccf..c6116ae 100644 --- a/zulip_bots/zulip_bots/tests/test_lib.py +++ b/zulip_bots/zulip_bots/tests/test_lib.py @@ -4,7 +4,8 @@ from zulip_bots.lib import ( ExternalBotHandler, StateHandler, run_message_handler_for_bot, - extract_query_without_mention + extract_query_without_mention, + is_private_message_but_not_group_pm ) import io @@ -209,6 +210,27 @@ class LibTest(TestCase): message = {'content': "Not at start @**Alice|alice** Hello World"} self.assertEqual(extract_query_without_mention(message, handler), None) + def test_is_private_message_but_not_group_pm(self): + client = FakeClient() + handler = ExternalBotHandler( + client=client, + root_dir=None, + bot_details=None, + bot_config_file=None + ) + message = {} + message['display_recipient'] = 'some stream' + message['type'] = 'stream' + self.assertFalse(is_private_message_but_not_group_pm(message, handler)) + message['type'] = 'private' + message['display_recipient'] = [{'email': 'a1@b.com'}] + message['sender_id'] = handler.user_id + self.assertFalse(is_private_message_but_not_group_pm(message, handler)) + message['sender_id'] = 0 # someone else + self.assertTrue(is_private_message_but_not_group_pm(message, handler)) + message['display_recipient'] = [{'email': 'a1@b.com'}, {'email': 'a2@b.com'}] + self.assertFalse(is_private_message_but_not_group_pm(message, handler)) + def _create_client_and_handler_for_file_upload(self): client = FakeClient() client.upload_file = MagicMock()