From d645d5c0ecdf0b133ab41b634c071a8337252041 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Wed, 10 May 2023 08:17:55 +0530 Subject: [PATCH] message_edit: Fix code to set visibility policy on moving messages. The code for updating visibility policy values on moving messages had two bugs. - There was a typo in elif condition where "user_profile" was being used instead of "user_profile_with_policy". This commit fixes the typo. - It was assumed that there would be no UserTopic rows for target topic if the target topic didn't exist. But there can be such case where some messages were sent to that topic and the user muted the topic. But then the messages in that topic was deleted. In such case there can be UserTopic rows for a stream-topic pair that does not exist. This commit fixes the code to handle such case as well and set the visibility policy of new topic to what was set for the original topic. This change simplifies the condition to just check whether new_visibility_policy is equal to target_topic_visibility_policy and skip if so, and update the visibility policy otherwise. Due to this change, we now do not try to mute the already muted topic if the topic is moved to a topic which didn't exist previously and thus we modify the existing test to not expect any INFO logs. --- zerver/actions/message_edit.py | 11 ++++++----- zerver/tests/test_message_edit.py | 26 +++++++++----------------- 2 files changed, 15 insertions(+), 22 deletions(-) diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index f3ff86c7e8..a20abb5c5a 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -837,7 +837,7 @@ def do_update_message( target_topic_user_profile_to_visibility_policy[ user_profile_with_policy ] = UserTopic.VisibilityPolicy.INHERIT - elif user_profile not in orig_topic_user_profile_to_visibility_policy: + elif user_profile_with_policy not in orig_topic_user_profile_to_visibility_policy: orig_topic_user_profile_to_visibility_policy[ user_profile_with_policy ] = UserTopic.VisibilityPolicy.INHERIT @@ -917,11 +917,12 @@ def do_update_message( ) else: # This corresponds to the case when messages are moved - # to a stream-topic pair that didn't exist. - if new_visibility_policy == UserTopic.VisibilityPolicy.INHERIT: + # to a stream-topic pair that didn't exist. There can + # still be UserTopic rows for the stream-topic pair + # that didn't exist if the messages in that topic had + # been deleted. + if new_visibility_policy == target_topic_visibility_policy: # This avoids unnecessary db operations and INFO logs. - # As INHERIT is the default visibility_policy in the new - # stream-topic pair for users. continue bulk_do_set_user_topic_visibility_policy( user_profiles, diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 9dd2f42e00..19e41842a4 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -1427,23 +1427,15 @@ class EditMessageTest(EditMessageTestCase): assert_is_topic_muted(hamlet, stream.id, change_later_topic_name, muted=True) # Make sure we safely handle the case of the new topic being already muted. - with self.assertLogs(level="INFO") as info_logs: - check_update_message( - user_profile=hamlet, - message_id=message_id, - stream_id=None, - topic_name=already_muted_topic, - propagate_mode="change_all", - send_notification_to_old_thread=False, - send_notification_to_new_thread=False, - content=None, - ) - self.assertEqual( - set(info_logs.output), - { - f"INFO:root:User {hamlet.id} tried to set visibility_policy to its current value of {UserTopic.VisibilityPolicy.MUTED}", - f"INFO:root:User {cordelia.id} tried to set visibility_policy to its current value of {UserTopic.VisibilityPolicy.MUTED}", - }, + check_update_message( + user_profile=hamlet, + message_id=message_id, + stream_id=None, + topic_name=already_muted_topic, + propagate_mode="change_all", + send_notification_to_old_thread=False, + send_notification_to_new_thread=False, + content=None, ) assert_is_topic_muted(hamlet, stream.id, change_later_topic_name, muted=False) assert_is_topic_muted(hamlet, stream.id, already_muted_topic, muted=True)