message_send: Fix wildcard_mentioned flag unset for few participants.

For topic wildcard mentions, the 'wildcard_mentioned' flag is set
for those user messages having 'user_profile_id' in
'topic_participant_user_ids', i.e. all topic participants.

Earlier, the flag was set if the 'user_profile_id' exists in
'all_topic_wildcard_mention_user_ids'.
'all_topic_wildcard_mention_user_ids' contains the ids of those
users who are topic participants and have enabled notifications
for '@topic' mentions.

The earlier approach was incorrect, as it would set the
'wildcard_mentioned' flag only for those topic participants
who have enabled the notifications for '@topic' mention instead
of setting the flag for all the topic participants.

The bug was introduced in 4c9d26c.
This commit is contained in:
Prakhar Pratyush
2023-08-16 09:15:05 +05:30
committed by Tim Abbott
parent 0988751d6c
commit 379a08eb1e
4 changed files with 58 additions and 9 deletions

View File

@@ -179,6 +179,7 @@ class RecipientInfoResult:
default_bot_user_ids: Set[int] default_bot_user_ids: Set[int]
service_bot_tuples: List[Tuple[int, int]] service_bot_tuples: List[Tuple[int, int]]
all_bot_user_ids: Set[int] all_bot_user_ids: Set[int]
topic_participant_user_ids: Set[int]
class ActiveUserDict(TypedDict): class ActiveUserDict(TypedDict):
@@ -210,6 +211,7 @@ def get_recipient_info(
topic_wildcard_mention_in_followed_topic_user_ids: Set[int] = set() topic_wildcard_mention_in_followed_topic_user_ids: Set[int] = set()
stream_wildcard_mention_in_followed_topic_user_ids: Set[int] = set() stream_wildcard_mention_in_followed_topic_user_ids: Set[int] = set()
muted_sender_user_ids: Set[int] = get_muting_users(sender_id) muted_sender_user_ids: Set[int] = get_muting_users(sender_id)
topic_participant_user_ids: Set[int] = set()
if recipient.type == Recipient.PERSONAL: if recipient.type == Recipient.PERSONAL:
# The sender and recipient may be the same id, so # The sender and recipient may be the same id, so
@@ -223,7 +225,6 @@ def get_recipient_info(
# of this function for different message types. # of this function for different message types.
assert stream_topic is not None assert stream_topic is not None
topic_participant_user_ids: Set[int] = set()
if possible_topic_wildcard_mention: if possible_topic_wildcard_mention:
# A topic participant is anyone who either sent or reacted to messages in the topic. # A topic participant is anyone who either sent or reacted to messages in the topic.
# It is expensive to call `participants_for_topic` if the topic has a large number # It is expensive to call `participants_for_topic` if the topic has a large number
@@ -464,6 +465,7 @@ def get_recipient_info(
default_bot_user_ids=default_bot_user_ids, default_bot_user_ids=default_bot_user_ids,
service_bot_tuples=service_bot_tuples, service_bot_tuples=service_bot_tuples,
all_bot_user_ids=all_bot_user_ids, all_bot_user_ids=all_bot_user_ids,
topic_participant_user_ids=topic_participant_user_ids,
) )
@@ -625,9 +627,11 @@ def build_message_send_dict(
topic_wildcard_mention_in_followed_topic_user_ids = ( topic_wildcard_mention_in_followed_topic_user_ids = (
info.topic_wildcard_mention_in_followed_topic_user_ids info.topic_wildcard_mention_in_followed_topic_user_ids
) )
topic_participant_user_ids = info.topic_participant_user_ids
else: else:
topic_wildcard_mention_user_ids = set() topic_wildcard_mention_user_ids = set()
topic_wildcard_mention_in_followed_topic_user_ids = set() topic_wildcard_mention_in_followed_topic_user_ids = set()
topic_participant_user_ids = set()
""" """
Once we have the actual list of mentioned ids from message Once we have the actual list of mentioned ids from message
rendering, we can patch in "default bots" (aka normal bots) rendering, we can patch in "default bots" (aka normal bots)
@@ -670,6 +674,7 @@ def build_message_send_dict(
widget_content=widget_content_dict, widget_content=widget_content_dict,
limit_unread_user_ids=limit_unread_user_ids, limit_unread_user_ids=limit_unread_user_ids,
disable_external_notifications=disable_external_notifications, disable_external_notifications=disable_external_notifications,
topic_participant_user_ids=topic_participant_user_ids,
) )
return message_send_dict return message_send_dict
@@ -688,17 +693,13 @@ def create_user_messages(
mark_as_read_user_ids: Set[int], mark_as_read_user_ids: Set[int],
limit_unread_user_ids: Optional[Set[int]], limit_unread_user_ids: Optional[Set[int]],
scheduled_message_to_self: bool, scheduled_message_to_self: bool,
topic_wildcard_mention_user_ids: Set[int], topic_participant_user_ids: Set[int],
topic_wildcard_mention_in_followed_topic_user_ids: Set[int],
) -> List[UserMessageLite]: ) -> List[UserMessageLite]:
# These properties on the Message are set via # These properties on the Message are set via
# render_markdown by code in the Markdown inline patterns # render_markdown by code in the Markdown inline patterns
ids_with_alert_words = rendering_result.user_ids_with_alert_words ids_with_alert_words = rendering_result.user_ids_with_alert_words
sender_id = message.sender.id sender_id = message.sender.id
is_stream_message = message.is_stream_message() is_stream_message = message.is_stream_message()
all_topic_wildcard_mention_user_ids = topic_wildcard_mention_user_ids.union(
topic_wildcard_mention_in_followed_topic_user_ids
)
base_flags = 0 base_flags = 0
if rendering_result.mentions_stream_wildcard: if rendering_result.mentions_stream_wildcard:
@@ -747,7 +748,7 @@ def create_user_messages(
flags |= UserMessage.flags.has_alert_word flags |= UserMessage.flags.has_alert_word
if ( if (
rendering_result.mentions_topic_wildcard rendering_result.mentions_topic_wildcard
and user_profile_id in all_topic_wildcard_mention_user_ids and user_profile_id in topic_participant_user_ids
): ):
flags |= UserMessage.flags.wildcard_mentioned flags |= UserMessage.flags.wildcard_mentioned
@@ -875,8 +876,7 @@ def do_send_messages(
mark_as_read_user_ids=mark_as_read_user_ids, mark_as_read_user_ids=mark_as_read_user_ids,
limit_unread_user_ids=send_request.limit_unread_user_ids, limit_unread_user_ids=send_request.limit_unread_user_ids,
scheduled_message_to_self=scheduled_message_to_self, scheduled_message_to_self=scheduled_message_to_self,
topic_wildcard_mention_user_ids=send_request.topic_wildcard_mention_user_ids, topic_participant_user_ids=send_request.topic_participant_user_ids,
topic_wildcard_mention_in_followed_topic_user_ids=send_request.topic_wildcard_mention_in_followed_topic_user_ids,
) )
for um in user_messages: for um in user_messages:

View File

@@ -191,6 +191,8 @@ class SendMessageRequest:
# (having stream wildcard) is being sent to, and have the # (having stream wildcard) is being sent to, and have the
# 'followed_topic_wildcard_mentions_notify' setting ON. # 'followed_topic_wildcard_mentions_notify' setting ON.
stream_wildcard_mention_in_followed_topic_user_ids: Set[int] stream_wildcard_mention_in_followed_topic_user_ids: Set[int]
# A topic participant is anyone who either sent or reacted to messages in the topic.
topic_participant_user_ids: Set[int]
links_for_embed: Set[str] links_for_embed: Set[str]
widget_content: Optional[Dict[str, Any]] widget_content: Optional[Dict[str, Any]]
submessages: List[Dict[str, Any]] = field(default_factory=list) submessages: List[Dict[str, Any]] = field(default_factory=list)

View File

@@ -29,6 +29,7 @@ from zerver.actions.message_send import (
from zerver.actions.realm_settings import do_set_realm_property from zerver.actions.realm_settings import do_set_realm_property
from zerver.actions.streams import do_change_stream_post_policy from zerver.actions.streams import do_change_stream_post_policy
from zerver.actions.user_groups import add_subgroups_to_user_group, check_add_user_group from zerver.actions.user_groups import add_subgroups_to_user_group, check_add_user_group
from zerver.actions.user_settings import do_change_user_setting
from zerver.actions.users import do_change_can_forge_sender, do_deactivate_user from zerver.actions.users import do_change_can_forge_sender, do_deactivate_user
from zerver.lib.addressee import Addressee from zerver.lib.addressee import Addressee
from zerver.lib.exceptions import JsonableError from zerver.lib.exceptions import JsonableError
@@ -1805,6 +1806,51 @@ class StreamMessagesTest(ZulipTestCase):
self.send_and_verify_stream_wildcard_mention_message("iago", test_fails=True) self.send_and_verify_stream_wildcard_mention_message("iago", test_fails=True)
self.send_and_verify_stream_wildcard_mention_message("iago", sub_count=10) self.send_and_verify_stream_wildcard_mention_message("iago", sub_count=10)
def test_wildcard_mentioned_flag_topic_wildcard_mention(self) -> None:
# For topic wildcard mentions, the 'wildcard_mentioned' flag should be
# set for all the user messages for topic participants, irrespective of
# their notifications settings.
cordelia = self.example_user("cordelia")
hamlet = self.example_user("hamlet")
iago = self.example_user("iago")
for user_profile in [cordelia, hamlet, iago]:
self.subscribe(user_profile, "Denmark")
# user | topic participant | wildcard_mentions_notify setting
# -------- | ----------------- | ----------------------------------
# cordelia | YES | True
# hamlet | YES | False
# iago | NO | True
self.send_stream_message(cordelia, "Denmark", content="test", topic_name="topic-1")
do_change_user_setting(cordelia, "wildcard_mentions_notify", True, acting_user=None)
self.send_stream_message(hamlet, "Denmark", content="Hi @**topic**", topic_name="topic-1")
message = most_recent_message(cordelia)
self.assertTrue(
UserMessage.objects.get(
user_profile=cordelia, message=message
).flags.wildcard_mentioned.is_set
)
self.send_stream_message(hamlet, "Denmark", content="test", topic_name="topic-2")
do_change_user_setting(hamlet, "wildcard_mentions_notify", False, acting_user=None)
self.send_stream_message(cordelia, "Denmark", content="Hi @**topic**", topic_name="topic-2")
message = most_recent_message(hamlet)
self.assertTrue(
UserMessage.objects.get(
user_profile=hamlet, message=message
).flags.wildcard_mentioned.is_set
)
do_change_user_setting(iago, "wildcard_mentions_notify", True, acting_user=None)
self.send_stream_message(hamlet, "Denmark", content="Hi @**topic**", topic_name="topic-3")
message = most_recent_message(iago)
self.assertFalse(
UserMessage.objects.get(
user_profile=iago, message=message
).flags.wildcard_mentioned.is_set
)
def test_invalid_wildcard_mention_policy(self) -> None: def test_invalid_wildcard_mention_policy(self) -> None:
cordelia = self.example_user("cordelia") cordelia = self.example_user("cordelia")
self.login_user(cordelia) self.login_user(cordelia)

View File

@@ -1859,6 +1859,7 @@ class RecipientInfoTest(ZulipTestCase):
default_bot_user_ids=set(), default_bot_user_ids=set(),
service_bot_tuples=[], service_bot_tuples=[],
all_bot_user_ids=set(), all_bot_user_ids=set(),
topic_participant_user_ids=set(),
) )
self.assertEqual(info, expected_info) self.assertEqual(info, expected_info)