Skip to content

Commit

Permalink
mute user: Mark as read new messages.
Browse files Browse the repository at this point in the history
Messages sent by muted users are marked as read
as soon as they are sent (or, more accurately,
while creating the database entries itself), regardless
of type (stream/huddle/PM).

ede73ee, makes it easy to
pass a list to `do_send_messages` containing user-ids for
whom the message should be marked as read.
We add the contents of this list to the set of muter IDs,
and then pass it on to `create_user_messages`.

This benefits from the caching behaviour of `get_muting_users`
and should not cause performance issues long term.

The consequence is that messages sent by muted users will
not contribute to unread counts and notifications.

This commit does not affect the unread messages
(if any) present just before muting, but only handles
subsequent messages. Old unreads will be handled in
further commits.
  • Loading branch information
abhijeetbodas2001 authored and timabbott committed Apr 13, 2021
1 parent b140c17 commit 2f56f8d
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 16 deletions.
13 changes: 9 additions & 4 deletions zerver/lib/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@
upload_emoji_image,
)
from zerver.lib.user_groups import access_user_group_by_id, create_user_group
from zerver.lib.user_mutes import add_user_mute, get_user_mutes
from zerver.lib.user_mutes import add_user_mute, get_muting_users, get_user_mutes
from zerver.lib.user_status import update_user_status
from zerver.lib.users import (
check_bot_name_available,
Expand Down Expand Up @@ -1848,14 +1848,19 @@ def do_send_messages(
# Service bots (outgoing webhook bots and embedded bots) don't store UserMessage rows;
# they will be processed later.
mentioned_user_ids = send_request.message.mentions_user_ids

# Extend the set with users who have muted the sender.
mark_as_read_for_users = get_muting_users(send_request.message.sender)
mark_as_read_for_users.update(mark_as_read)

user_messages = create_user_messages(
message=send_request.message,
um_eligible_user_ids=send_request.um_eligible_user_ids,
long_term_idle_user_ids=send_request.long_term_idle_user_ids,
stream_push_user_ids=send_request.stream_push_user_ids,
stream_email_user_ids=send_request.stream_email_user_ids,
mentioned_user_ids=mentioned_user_ids,
mark_as_read=mark_as_read,
mark_as_read_for_users=mark_as_read_for_users,
)

for um in user_messages:
Expand Down Expand Up @@ -2030,7 +2035,7 @@ def create_user_messages(
stream_push_user_ids: AbstractSet[int],
stream_email_user_ids: AbstractSet[int],
mentioned_user_ids: AbstractSet[int],
mark_as_read: Sequence[int] = [],
mark_as_read_for_users: Set[int],
) -> List[UserMessageLite]:
ums_to_create = []
for user_profile_id in um_eligible_user_ids:
Expand All @@ -2049,7 +2054,7 @@ def create_user_messages(
for um in ums_to_create:
if (
um.user_profile_id == message.sender.id and message.sent_by_human()
) or um.user_profile_id in mark_as_read:
) or um.user_profile_id in mark_as_read_for_users:
um.flags |= UserMessage.flags.read
if wildcard:
um.flags |= UserMessage.flags.wildcard_mentioned
Expand Down
4 changes: 2 additions & 2 deletions zerver/tests/test_message_edit.py
Original file line number Diff line number Diff line change
Expand Up @@ -1171,8 +1171,8 @@ def test_move_message_to_stream_and_topic(self) -> None:
"topic": "new topic",
},
)
self.assertEqual(len(queries), 47)
self.assertEqual(len(cache_tries), 11)
self.assertEqual(len(queries), 48)
self.assertEqual(len(cache_tries), 13)

messages = get_topic_messages(user_profile, old_stream, "test")
self.assertEqual(len(messages), 1)
Expand Down
2 changes: 1 addition & 1 deletion zerver/tests/test_message_send.py
Original file line number Diff line number Diff line change
Expand Up @@ -1557,7 +1557,7 @@ def test_not_too_many_queries(self) -> None:
body=content,
)

self.assert_length(queries, 12)
self.assert_length(queries, 13)

def test_stream_message_dict(self) -> None:
user_profile = self.example_user("iago")
Expand Down
41 changes: 40 additions & 1 deletion zerver/tests/test_muting_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from zerver.lib.test_classes import ZulipTestCase
from zerver.lib.timestamp import datetime_to_timestamp
from zerver.lib.user_mutes import get_mute_object, get_muting_users, get_user_mutes
from zerver.models import RealmAuditLog
from zerver.models import RealmAuditLog, UserMessage, UserProfile


class MutedUsersTests(ZulipTestCase):
Expand Down Expand Up @@ -185,3 +185,42 @@ def test_get_muting_users(self) -> None:
self.assertEqual(None, cache_get(get_muting_users_cache_key(cordelia)))
self.assertEqual(set(), get_muting_users(cordelia))
self.assertEqual(set(), cache_get(get_muting_users_cache_key(cordelia))[0])

def assert_usermessage_read_flag(self, user: UserProfile, message: int, flag: bool) -> None:
usermesaage = UserMessage.objects.get(
user_profile=user,
message=message,
)
self.assertTrue(usermesaage.flags.read == flag)

def test_new_messages_from_muted_user_marked_as_read(self) -> None:
hamlet = self.example_user("hamlet")
self.login_user(hamlet)
cordelia = self.example_user("cordelia")
othello = self.example_user("othello")

self.make_stream("general")
self.subscribe(hamlet, "general")
self.subscribe(cordelia, "general")
self.subscribe(othello, "general")

# Hamlet mutes Cordelia.
url = "/api/v1/users/me/muted_users/{}".format(cordelia.id)
result = self.api_post(hamlet, url)
self.assert_json_success(result)

# Have Cordelia send messages to Hamlet and Othello.
stream_message = self.send_stream_message(cordelia, "general", "Spam in stream")
huddle_message = self.send_huddle_message(cordelia, [hamlet, othello], "Spam in huddle")
pm_to_hamlet = self.send_personal_message(cordelia, hamlet, "Spam in PM")
pm_to_othello = self.send_personal_message(cordelia, othello, "Spam in PM")

# These should be marked as read for Hamlet, since he has muted Cordelia.
self.assert_usermessage_read_flag(hamlet, stream_message, True)
self.assert_usermessage_read_flag(hamlet, huddle_message, True)
self.assert_usermessage_read_flag(hamlet, pm_to_hamlet, True)

# These messages should be unreads for Othello, since he hasn't muted Cordelia.
self.assert_usermessage_read_flag(othello, stream_message, False)
self.assert_usermessage_read_flag(othello, huddle_message, False)
self.assert_usermessage_read_flag(othello, pm_to_othello, False)
4 changes: 2 additions & 2 deletions zerver/tests/test_signup.py
Original file line number Diff line number Diff line change
Expand Up @@ -746,13 +746,13 @@ def test_register(self) -> None:
with queries_captured() as queries, cache_tries_captured() as cache_tries:
self.register(self.nonreg_email("test"), "test")
# Ensure the number of queries we make is not O(streams)
self.assertEqual(len(queries), 70)
self.assertEqual(len(queries), 71)

# We can probably avoid a couple cache hits here, but there doesn't
# seem to be any O(N) behavior. Some of the cache hits are related
# to sending messages, such as getting the welcome bot, looking up
# the alert words for a realm, etc.
self.assertEqual(len(cache_tries), 15)
self.assertEqual(len(cache_tries), 16)

user_profile = self.nonreg_user("test")
self.assert_logged_in_user_id(user_profile.id)
Expand Down
8 changes: 4 additions & 4 deletions zerver/tests/test_subs.py
Original file line number Diff line number Diff line change
Expand Up @@ -3531,7 +3531,7 @@ def test_multi_user_subscription(self) -> None:
streams_to_sub,
dict(principals=orjson.dumps([user1.id, user2.id]).decode()),
)
self.assert_length(queries, 34)
self.assert_length(queries, 35)

self.assert_length(events, 5)
for ev in [x for x in events if x["event"]["type"] not in ("message", "stream")]:
Expand Down Expand Up @@ -4421,7 +4421,7 @@ def test_subscriptions_query_count(self) -> None:
[new_streams[0]],
dict(principals=orjson.dumps([user1.id, user2.id]).decode()),
)
self.assert_length(queries, 34)
self.assert_length(queries, 35)

# Test creating private stream.
with queries_captured() as queries:
Expand All @@ -4431,7 +4431,7 @@ def test_subscriptions_query_count(self) -> None:
dict(principals=orjson.dumps([user1.id, user2.id]).decode()),
invite_only=True,
)
self.assert_length(queries, 36)
self.assert_length(queries, 37)

# Test creating a public stream with announce when realm has a notification stream.
notifications_stream = get_stream(self.streams[0], self.test_realm)
Expand All @@ -4446,7 +4446,7 @@ def test_subscriptions_query_count(self) -> None:
principals=orjson.dumps([user1.id, user2.id]).decode(),
),
)
self.assert_length(queries, 42)
self.assert_length(queries, 43)


class GetStreamsTest(ZulipTestCase):
Expand Down
4 changes: 2 additions & 2 deletions zerver/tests/test_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -819,8 +819,8 @@ def test_create_user_with_multiple_streams(self) -> None:
acting_user=None,
)

self.assert_length(queries, 68)
self.assert_length(cache_tries, 20)
self.assert_length(queries, 70)
self.assert_length(cache_tries, 22)
self.assert_length(events, 7)

peer_add_events = [event for event in events if event["event"].get("op") == "peer_add"]
Expand Down

0 comments on commit 2f56f8d

Please sign in to comment.