mirror of https://github.com/zulip/zulip.git
reactions: Fix reacting to messages on streams you're not subscribed to.
We use the same strategy Zulip already uses for starred messages, namely, creating a new UserMessage row with the "historical" flag set (which basically means Zulip can ignore this row for most purposes that use UserMessage rows). The historical flag is ignored, however, in determining which users' browsers to notify about new reactions, and thus the user will get to see the reaction appear when they click a message (and any reactions other users later add, as well!). There's still something of a race here, in that if some users react to a message while the user is looking at the unsubscribed stream but before the user reacts to that message, those reactions will not be displayed to that user (so counts will be a bit lower, or something). This race feels small enough to ignore for now. Fixes #3345.
This commit is contained in:
parent
60326ca94c
commit
f3af0fe635
|
@ -993,6 +993,11 @@ def notify_reaction_update(user_profile, message, emoji_name, op):
|
|||
# performance tradeoff, since otherwise we'd need to send all
|
||||
# reactions to public stream messages to every browser for every
|
||||
# client in the organization, which doesn't scale.
|
||||
#
|
||||
# However, to ensure that reactions do live-update for any user
|
||||
# who has actually participated in reacting to a message, we add a
|
||||
# "historical" UserMessage row for any user who reacts to message,
|
||||
# subscribing them to future notifications.
|
||||
ums = UserMessage.objects.filter(message=message.id)
|
||||
send_event(event, [um.user_profile_id for um in ums])
|
||||
|
||||
|
|
|
@ -7,7 +7,7 @@ from six import string_types
|
|||
|
||||
from zerver.lib.test_helpers import tornado_redirected_to_list, get_display_recipient
|
||||
from zerver.lib.test_classes import ZulipTestCase
|
||||
from zerver.models import get_user_profile_by_email
|
||||
from zerver.models import get_realm, get_user_profile_by_email, Recipient, UserMessage
|
||||
|
||||
class ReactionEmojiTest(ZulipTestCase):
|
||||
def test_missing_emoji(self):
|
||||
|
@ -51,6 +51,34 @@ class ReactionEmojiTest(ZulipTestCase):
|
|||
self.assert_json_success(result)
|
||||
self.assertEqual(200, result.status_code)
|
||||
|
||||
def test_valid_emoji_react_historical(self):
|
||||
# type: () -> None
|
||||
"""
|
||||
Reacting with valid emoji on a historical message succeeds
|
||||
"""
|
||||
realm = get_realm("zulip")
|
||||
stream_name = "Saxony"
|
||||
self.subscribe_to_stream("cordelia@zulip.com", stream_name, realm=realm)
|
||||
message_id = self.send_message("cordelia@zulip.com", stream_name, Recipient.STREAM)
|
||||
|
||||
sender = 'hamlet@zulip.com'
|
||||
user_profile = get_user_profile_by_email(sender)
|
||||
|
||||
# Verify that hamlet did not receive the message.
|
||||
self.assertFalse(UserMessage.objects.filter(user_profile=user_profile,
|
||||
message_id=message_id).exists())
|
||||
|
||||
# Have hamlet react to the message
|
||||
result = self.client_put('/api/v1/messages/%s/emoji_reactions/smile' % (message_id,),
|
||||
**self.api_auth(sender))
|
||||
self.assert_json_success(result)
|
||||
|
||||
# Fetch the now-created UserMessage object to confirm it exists and is historical
|
||||
user_message = UserMessage.objects.get(user_profile=user_profile, message_id=message_id)
|
||||
self.assertTrue(user_message.flags.historical)
|
||||
self.assertTrue(user_message.flags.read)
|
||||
self.assertFalse(user_message.flags.starred)
|
||||
|
||||
def test_valid_realm_emoji(self):
|
||||
# type: () -> None
|
||||
"""
|
||||
|
|
|
@ -11,7 +11,7 @@ from zerver.lib.emoji import check_valid_emoji
|
|||
from zerver.lib.message import access_message
|
||||
from zerver.lib.request import JsonableError
|
||||
from zerver.lib.response import json_success
|
||||
from zerver.models import Reaction, UserProfile
|
||||
from zerver.models import Reaction, UserMessage, UserProfile
|
||||
|
||||
@has_request_variables
|
||||
def add_reaction_backend(request, user_profile, message_id, emoji_name):
|
||||
|
@ -19,7 +19,7 @@ def add_reaction_backend(request, user_profile, message_id, emoji_name):
|
|||
|
||||
# access_message will throw a JsonableError exception if the user
|
||||
# cannot see the message (e.g. for messages to private streams).
|
||||
message = access_message(user_profile, message_id)[0]
|
||||
message, user_message = access_message(user_profile, message_id)
|
||||
|
||||
check_valid_emoji(message.sender.realm, emoji_name)
|
||||
|
||||
|
@ -30,6 +30,16 @@ def add_reaction_backend(request, user_profile, message_id, emoji_name):
|
|||
emoji_name=emoji_name).exists():
|
||||
raise JsonableError(_("Reaction already exists"))
|
||||
|
||||
if user_message is None:
|
||||
# Users can see and react to messages sent to streams they
|
||||
# were not a subscriber to; in order to receive events for
|
||||
# those, we give the user a `historical` UserMessage objects
|
||||
# for the message. This is the same trick we use for starring
|
||||
# messages.
|
||||
UserMessage.objects.create(user_profile=user_profile,
|
||||
message=message,
|
||||
flags=UserMessage.flags.historical | UserMessage.flags.read)
|
||||
|
||||
do_add_reaction(user_profile, message, emoji_name)
|
||||
|
||||
return json_success()
|
||||
|
|
Loading…
Reference in New Issue