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

@@ -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),

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

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_muted = stream_id in state["muted_stream_ids"]
-            stream_id not in state["muted_stream_ids"]
+        visibility_policy = get_topic_visibility_policy(user_profile, stream_id, topic)
-            # This next check hits the database.
+        # A stream message is unmuted if it belongs to:
-            and not topic_has_visibility_policy(
+        # * a not muted topic in a normal stream
-                user_profile, stream_id, topic, UserTopic.VisibilityPolicy.MUTED
+        # * 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)
 

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],

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
+                              The total number of unread messages to display. This includes one-on-one and group
-                              and group direct messages, as well as all messages to unmuted topics
+                              direct messages, as well as stream messages that are not [muted](/help/mute-a-topic).
-                              on unmuted streams.
+
+                              **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
+                              An array of dictionaries where each dictionary contains details of all
-                              messages of a single subscribed stream, including muted streams.
+                              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
+                              Array containing the IDs of all unread messages in which the user was
-                              mentioned. For muted streams, wildcard mentions will not be considered for
+                              mentioned directly, and unread [non-muted](/help/mute-a-topic) messages
-                              this array.
+                              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:

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")
 

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
+        # The count here reflects the count of unread messages that we will report
-        # report to users in the bankruptcy dialog, and it excludes unread messages
+        # to users in the bankruptcy dialog, and it excludes unread messages from:
-        # from muted streams and muted topics.
+        # * muted topics in unmuted streams
-        self.assertEqual(result["count"], 4)
+        # * 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,