From 49092dfa79e10aa8fa28e01d6991e9bba065464c Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Mon, 25 Sep 2023 14:57:15 +0530 Subject: [PATCH] unread_msgs: Fix all unreads in muted stream being treated as muted. Earlier, 'is_row_muted' returned 'true' if the message was in a muted stream or muted topic. If the message is in an unmuted or followed topic in a muted stream, such topics should be treated as not muted topics in an unmuted stream. This commit fixes the incorrect behavior. Now, for wildcard mentions, 'unread_msgs.mentions' exclude the IDs in muted streams only if the message is in default or muted topic. Also, 'unread_msgs.count' takes into account the unreads in unmuted or followed topics in muted streams too. Documents that this bug was fixed in the API changelog. --- api_docs/changelog.md | 6 +++ version.py | 2 +- zerver/lib/message.py | 30 ++++++++++----- zerver/lib/user_topics.py | 15 ++++++++ zerver/openapi/zulip.yaml | 22 +++++++---- zerver/tests/test_events.py | 24 +++++++++++- zerver/tests/test_message_flags.py | 59 +++++++++++++++++++++++++----- 7 files changed, 128 insertions(+), 30 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index c8b0555df1..1edfeed023 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 8.0 +**Feature level 213** + +* [`POST /register`](/api/register-queue): Fixed incorrect handling of + unmuted and followed topics in calculating the `mentions` and + `count` fields of the `unread_msgs` object. + **Feature level 212** * [`GET /events`](/api/get-events), [`POST /register`](/api/register-queue), diff --git a/version.py b/version.py index 4ebbf00ba8..5bfed5f098 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 212 +API_FEATURE_LEVEL = 213 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/lib/message.py b/zerver/lib/message.py index cd109c95ee..0b35b5ceb5 100644 --- a/zerver/lib/message.py +++ b/zerver/lib/message.py @@ -53,7 +53,7 @@ from zerver.lib.topic import DB_TOPIC_NAME, MESSAGE__TOPIC, TOPIC_LINKS, TOPIC_N from zerver.lib.types import DisplayRecipientT, EditHistoryEvent, UserDisplayRecipient from zerver.lib.url_preview.types import UrlEmbedData from zerver.lib.user_groups import is_user_in_group -from zerver.lib.user_topics import build_get_topic_visibility_policy, topic_has_visibility_policy +from zerver.lib.user_topics import build_get_topic_visibility_policy, get_topic_visibility_policy from zerver.models import ( MAX_TOPIC_NAME_LENGTH, Message, @@ -1114,10 +1114,19 @@ def extract_unread_data_from_um_rows( get_topic_visibility_policy = build_get_topic_visibility_policy(user_profile) def is_row_muted(stream_id: int, recipient_id: int, topic: str) -> bool: - if stream_id in muted_stream_ids: + stream_muted = stream_id in muted_stream_ids + visibility_policy = get_topic_visibility_policy(recipient_id, topic) + + if stream_muted and visibility_policy in [ + UserTopic.VisibilityPolicy.UNMUTED, + UserTopic.VisibilityPolicy.FOLLOWED, + ]: + return False + + if stream_muted: return True - visibility_policy = get_topic_visibility_policy(recipient_id, topic) + # muted topic in unmuted stream if visibility_policy == UserTopic.VisibilityPolicy.MUTED: return True @@ -1317,12 +1326,15 @@ def apply_unread_message_event( topic=topic, ) - if ( - stream_id not in state["muted_stream_ids"] - # This next check hits the database. - and not topic_has_visibility_policy( - user_profile, stream_id, topic, UserTopic.VisibilityPolicy.MUTED - ) + stream_muted = stream_id in state["muted_stream_ids"] + visibility_policy = get_topic_visibility_policy(user_profile, stream_id, topic) + # A stream message is unmuted if it belongs to: + # * a not muted topic in a normal stream + # * an unmuted or followed topic in a muted stream + if (not stream_muted and visibility_policy != UserTopic.VisibilityPolicy.MUTED) or ( + stream_muted + and visibility_policy + in [UserTopic.VisibilityPolicy.UNMUTED, UserTopic.VisibilityPolicy.FOLLOWED] ): state["unmuted_stream_msgs"].add(message_id) diff --git a/zerver/lib/user_topics.py b/zerver/lib/user_topics.py index 554a0bb871..44b2b5f04c 100644 --- a/zerver/lib/user_topics.py +++ b/zerver/lib/user_topics.py @@ -109,6 +109,21 @@ def set_topic_visibility_policy( ) +def get_topic_visibility_policy( + user_profile: UserProfile, + stream_id: int, + topic_name: str, +) -> int: + try: + user_topic = UserTopic.objects.get( + user_profile=user_profile, stream_id=stream_id, topic_name__iexact=topic_name + ) + visibility_policy = user_topic.visibility_policy + except UserTopic.DoesNotExist: + visibility_policy = UserTopic.VisibilityPolicy.INHERIT + return visibility_policy + + @transaction.atomic(savepoint=False) def bulk_set_user_topic_visibility_policy_in_database( user_profiles: List[UserProfile], diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 631ee8db7f..757d880cdd 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -11851,9 +11851,11 @@ paths: count: type: integer description: | - The total number of unread messages to display; this includes one-on-one - and group direct messages, as well as all messages to unmuted topics - on unmuted streams. + The total number of unread messages to display. This includes one-on-one and group + direct messages, as well as stream messages that are not [muted](/help/mute-a-topic). + + **Changes**: Before Zulip 8.0 (feature level 213), the unmute and follow + topic features were not handled correctly in calculating this field. pms: type: array description: | @@ -11897,8 +11899,9 @@ paths: streams: type: array description: | - An array of objects where each object contains details of unread - messages of a single subscribed stream, including muted streams. + An array of dictionaries where each dictionary contains details of all + unread messages of a single subscribed stream. This includes muted streams + and muted topics, even though those messages are excluded from count. **Changes**: Prior to Zulip 5.0 (feature level 90), these objects included a `sender_ids` property, which listed the set of IDs of @@ -11947,9 +11950,12 @@ paths: mentions: type: array description: | - Array containing the IDs of all unread messages in which the user has been - mentioned. For muted streams, wildcard mentions will not be considered for - this array. + Array containing the IDs of all unread messages in which the user was + mentioned directly, and unread [non-muted](/help/mute-a-topic) messages + in which the user was mentioned through a wildcard. + + **Changes**: Before Zulip 8.0 (feature level 213), the unmute and follow + topic features were not handled correctly in calculating this field. items: type: integer old_unreads_missing: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index b2f88b2062..81a4cbb8e8 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -529,8 +529,9 @@ class NormalActionsTest(BaseAction): ) def test_stream_send_message_events(self) -> None: + user_profile = self.example_user("hamlet") events = self.verify_action( - lambda: self.send_stream_message(self.example_user("hamlet"), "Verona", "hello"), + lambda: self.send_stream_message(user_profile, "Verona", "hello"), client_gravatar=False, ) check_message("events[0]", events[0]) @@ -544,12 +545,31 @@ class NormalActionsTest(BaseAction): ) events = self.verify_action( - lambda: self.send_stream_message(self.example_user("hamlet"), "Verona", "hello"), + lambda: self.send_stream_message(user_profile, "Verona", "hello"), client_gravatar=True, ) check_message("events[0]", events[0]) assert events[0]["message"]["avatar_url"] is None + # Here we add coverage for the case where 'apply_unread_message_event' + # should be called and unread messages in unmuted or followed topic in + # muted stream is treated as unmuted stream message, thus added to 'unmuted_stream_msgs'. + stream = get_stream("Verona", user_profile.realm) + sub = get_subscription(stream.name, user_profile) + do_change_subscription_property( + user_profile, sub, stream, "is_muted", True, acting_user=None + ) + do_set_user_topic_visibility_policy( + user_profile, + stream, + "test", + visibility_policy=UserTopic.VisibilityPolicy.UNMUTED, + ) + self.verify_action( + lambda: self.send_stream_message(self.example_user("aaron"), "Verona", "hello"), + state_change_expected=True, + ) + def test_stream_update_message_events(self) -> None: self.send_stream_message(self.example_user("hamlet"), "Verona", "hello") diff --git a/zerver/tests/test_message_flags.py b/zerver/tests/test_message_flags.py index d517d55de7..a85438c691 100644 --- a/zerver/tests/test_message_flags.py +++ b/zerver/tests/test_message_flags.py @@ -704,7 +704,9 @@ class GetUnreadMsgsTest(ZulipTestCase): subscription.is_muted = True subscription.save() - def mute_topic(self, user_profile: UserProfile, stream_name: str, topic_name: str) -> None: + def set_topic_visibility_policy( + self, user_profile: UserProfile, stream_name: str, topic_name: str, visibility_policy: int + ) -> None: realm = user_profile.realm stream = get_stream(stream_name, realm) @@ -712,7 +714,7 @@ class GetUnreadMsgsTest(ZulipTestCase): user_profile, stream, topic_name, - visibility_policy=UserTopic.VisibilityPolicy.MUTED, + visibility_policy=visibility_policy, ) def test_raw_unread_stream(self) -> None: @@ -752,10 +754,11 @@ class GetUnreadMsgsTest(ZulipTestCase): stream=get_stream("test here", realm), ) - self.mute_topic( + self.set_topic_visibility_policy( user_profile=hamlet, stream_name="devel", topic_name="ruby", + visibility_policy=UserTopic.VisibilityPolicy.MUTED, ) raw_unread_data = get_raw_unread_data( @@ -960,7 +963,12 @@ class GetUnreadMsgsTest(ZulipTestCase): muted_stream = self.subscribe(user_profile, "Muted stream") self.subscribe(sender, muted_stream.name) self.mute_stream(user_profile, muted_stream) - self.mute_topic(user_profile, "Denmark", "muted-topic") + self.set_topic_visibility_policy( + user_profile, "Denmark", "muted-topic", UserTopic.VisibilityPolicy.MUTED + ) + self.set_topic_visibility_policy( + user_profile, "Muted stream", "unmuted-topic", UserTopic.VisibilityPolicy.UNMUTED + ) stream_message_id = self.send_stream_message(sender, "Denmark", "hello") muted_stream_message_id = self.send_stream_message(sender, "Muted stream", "hello") @@ -970,6 +978,12 @@ class GetUnreadMsgsTest(ZulipTestCase): topic_name="muted-topic", content="hello", ) + unmuted_topic_muted_stream_message_id = self.send_stream_message( + sender, + "Muted stream", + topic_name="unmuted-topic", + content="hello", + ) huddle_message_id = self.send_huddle_message( sender, @@ -982,17 +996,18 @@ class GetUnreadMsgsTest(ZulipTestCase): aggregated_data = aggregate_unread_data(raw_unread_data) return aggregated_data - with mock.patch("zerver.lib.message.MAX_UNREAD_MESSAGES", 4): + with mock.patch("zerver.lib.message.MAX_UNREAD_MESSAGES", 5): result = get_unread_data() - self.assertEqual(result["count"], 2) + self.assertEqual(result["count"], 3) self.assertTrue(result["old_unreads_missing"]) result = get_unread_data() - # The count here reflects the count of unread messages that we will - # report to users in the bankruptcy dialog, and it excludes unread messages - # from muted streams and muted topics. - self.assertEqual(result["count"], 4) + # The count here reflects the count of unread messages that we will report + # to users in the bankruptcy dialog, and it excludes unread messages from: + # * muted topics in unmuted streams + # * default or muted topics in muted streams + self.assertEqual(result["count"], 5) self.assertFalse(result["old_unreads_missing"]) unread_pm = result["pms"][0] @@ -1016,6 +1031,15 @@ class GetUnreadMsgsTest(ZulipTestCase): self.assertEqual(unread_stream["topic"], "test") self.assertEqual(unread_stream["unread_message_ids"], [muted_stream_message_id]) + unread_stream = result["streams"][3] + self.assertEqual( + unread_stream["stream_id"], get_stream("Muted stream", user_profile.realm).id + ) + self.assertEqual(unread_stream["topic"], "unmuted-topic") + self.assertEqual( + unread_stream["unread_message_ids"], [unmuted_topic_muted_stream_message_id] + ) + huddle_string = ",".join( str(uid) for uid in sorted([sender_id, user_profile.id, othello.id]) ) @@ -1076,6 +1100,21 @@ class GetUnreadMsgsTest(ZulipTestCase): result = get_unread_data() self.assertEqual(result["mentions"], []) + # Test with a unmuted topic in muted stream + um = UserMessage.objects.get( + user_profile_id=user_profile.id, + message_id=unmuted_topic_muted_stream_message_id, + ) + um.flags = UserMessage.flags.wildcard_mentioned + um.save() + result = get_unread_data() + self.assertEqual(result["mentions"], [unmuted_topic_muted_stream_message_id]) + + um.flags = 0 + um.save() + result = get_unread_data() + self.assertEqual(result["mentions"], []) + # Test with a muted topic um = UserMessage.objects.get( user_profile_id=user_profile.id,