diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index 36c0f23422..cd7b1ee6cd 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -687,65 +687,61 @@ def do_update_message( changed_message_ids = list(changed_messages.values_list("id", flat=True)) changed_messages_count = len(changed_message_ids) - if new_stream is not None: - assert stream_being_edited is not None - if gaining_usermessage_user_ids: - ums_to_create = [] - for message_id in changed_message_ids: - for user_profile_id in gaining_usermessage_user_ids: - # The fact that the user didn't have a UserMessage originally means we can infer that the user - # was not mentioned in the original message (even if mention syntax was present, it would not - # take effect for a user who was not subscribed). If we were editing the message's content, we - # would rerender the message and then use the new stream's data to determine whether this is - # a mention of a subscriber; but as we are not doing so, we choose to preserve the "was this - # mention syntax an actual mention" decision made during the original rendering for implementation - # simplicity. As a result, the only flag to consider applying here is read. - um = UserMessageLite( - user_profile_id=user_profile_id, - message_id=message_id, - flags=UserMessage.flags.read, - ) - ums_to_create.append(um) - bulk_insert_ums(ums_to_create) + if new_stream is not None: + assert stream_being_edited is not None + if gaining_usermessage_user_ids: + ums_to_create = [] + for message_id in changed_message_ids: + for user_profile_id in gaining_usermessage_user_ids: + # The fact that the user didn't have a UserMessage originally means we can infer that the user + # was not mentioned in the original message (even if mention syntax was present, it would not + # take effect for a user who was not subscribed). If we were editing the message's content, we + # would rerender the message and then use the new stream's data to determine whether this is + # a mention of a subscriber; but as we are not doing so, we choose to preserve the "was this + # mention syntax an actual mention" decision made during the original rendering for implementation + # simplicity. As a result, the only flag to consider applying here is read. + um = UserMessageLite( + user_profile_id=user_profile_id, + message_id=message_id, + flags=UserMessage.flags.read, + ) + ums_to_create.append(um) + bulk_insert_ums(ums_to_create) - # Delete UserMessage objects for users who will no - # longer have access to these messages. Note: This could be - # very expensive, since it's N guest users x M messages. - UserMessage.objects.filter( - user_profile_id__in=[sub.user_profile_id for sub in subs_losing_usermessages], - message_id__in=changed_messages, - ).delete() + # Delete UserMessage objects for users who will no + # longer have access to these messages. Note: This could be + # very expensive, since it's N guest users x M messages. + UserMessage.objects.filter( + user_profile_id__in=[sub.user_profile_id for sub in subs_losing_usermessages], + message_id__in=changed_messages, + ).delete() - delete_event: DeleteMessagesEvent = { - "type": "delete_message", - "message_ids": changed_message_ids, - "message_type": "stream", - "stream_id": stream_being_edited.id, - "topic": orig_topic_name, - } - send_event(user_profile.realm, delete_event, losing_access_user_ids) + delete_event: DeleteMessagesEvent = { + "type": "delete_message", + "message_ids": changed_message_ids, + "message_type": "stream", + "stream_id": stream_being_edited.id, + "topic": orig_topic_name, + } + send_event(user_profile.realm, delete_event, losing_access_user_ids) - # Reset the Attachment.is_*_public caches for all messages - # moved to another stream with different access permissions. - if new_stream.invite_only != stream_being_edited.invite_only: - Attachment.objects.filter(messages__in=changed_messages.values("id")).update( - is_realm_public=None, - ) - ArchivedAttachment.objects.filter( - messages__in=changed_messages.values("id") - ).update( - is_realm_public=None, - ) + # Reset the Attachment.is_*_public caches for all messages + # moved to another stream with different access permissions. + if new_stream.invite_only != stream_being_edited.invite_only: + Attachment.objects.filter(messages__in=changed_messages.values("id")).update( + is_realm_public=None, + ) + ArchivedAttachment.objects.filter(messages__in=changed_messages.values("id")).update( + is_realm_public=None, + ) - if new_stream.is_web_public != stream_being_edited.is_web_public: - Attachment.objects.filter(messages__in=changed_messages.values("id")).update( - is_web_public=None, - ) - ArchivedAttachment.objects.filter( - messages__in=changed_messages.values("id") - ).update( - is_web_public=None, - ) + if new_stream.is_web_public != stream_being_edited.is_web_public: + Attachment.objects.filter(messages__in=changed_messages.values("id")).update( + is_web_public=None, + ) + ArchivedAttachment.objects.filter(messages__in=changed_messages.values("id")).update( + is_web_public=None, + ) # This does message.save(update_fields=[...]) save_message_for_edit_use_case(message=target_message) diff --git a/zerver/tests/test_message_move_stream.py b/zerver/tests/test_message_move_stream.py index 7e570b4d9a..90abbbd148 100644 --- a/zerver/tests/test_message_move_stream.py +++ b/zerver/tests/test_message_move_stream.py @@ -1460,6 +1460,7 @@ class MessageMoveStreamTest(ZulipTestCase): history_public_to_subscribers: bool, user_messages_created: bool, to_invite_only: bool = True, + propagate_mode: str = "change_all", ) -> None: admin_user = self.example_user("iago") user_losing_access = self.example_user("cordelia") @@ -1484,6 +1485,9 @@ class MessageMoveStreamTest(ZulipTestCase): ) self.send_stream_message(admin_user, old_stream.name, topic_name="test", content="Second") + self.assert_length(get_topic_messages(admin_user, old_stream, "test"), 2) + self.assert_length(get_topic_messages(admin_user, new_stream, "test"), 0) + self.assertEqual( UserMessage.objects.filter( user_profile_id=user_losing_access.id, @@ -1503,16 +1507,18 @@ class MessageMoveStreamTest(ZulipTestCase): f"/json/messages/{msg_id}", { "stream_id": new_stream.id, - "propagate_mode": "change_all", + "propagate_mode": propagate_mode, }, ) self.assert_json_success(result) - messages = get_topic_messages(admin_user, old_stream, "test") - self.assert_length(messages, 0) - - messages = get_topic_messages(admin_user, new_stream, "test") - self.assert_length(messages, 3) + # We gain one more message than we moved because of a notification-bot message. + if propagate_mode == "change_one": + self.assert_length(get_topic_messages(admin_user, old_stream, "test"), 1) + self.assert_length(get_topic_messages(admin_user, new_stream, "test"), 2) + else: + self.assert_length(get_topic_messages(admin_user, old_stream, "test"), 0) + self.assert_length(get_topic_messages(admin_user, new_stream, "test"), 3) self.assertEqual( UserMessage.objects.filter( @@ -1545,6 +1551,22 @@ class MessageMoveStreamTest(ZulipTestCase): user_messages_created=False, ) + def test_move_one_message_from_public_to_private_stream_not_shared_history(self) -> None: + self.parameterized_test_move_message_involving_private_stream( + from_invite_only=False, + history_public_to_subscribers=False, + user_messages_created=True, + propagate_mode="change_one", + ) + + def test_move_one_message_from_public_to_private_stream_shared_history(self) -> None: + self.parameterized_test_move_message_involving_private_stream( + from_invite_only=False, + history_public_to_subscribers=True, + user_messages_created=False, + propagate_mode="change_one", + ) + def test_move_message_from_private_to_private_stream_not_shared_history(self) -> None: self.parameterized_test_move_message_involving_private_stream( from_invite_only=True,