From f3af0fe63516377f754c13650c25317269472fae Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Wed, 22 Mar 2017 20:15:32 -0700 Subject: [PATCH] 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. --- zerver/lib/actions.py | 5 +++++ zerver/tests/test_reactions.py | 30 +++++++++++++++++++++++++++++- zerver/views/reactions.py | 14 ++++++++++++-- 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 5cfa8b925f..e5a8f89d84 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -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]) diff --git a/zerver/tests/test_reactions.py b/zerver/tests/test_reactions.py index e16179092e..01e10f859b 100644 --- a/zerver/tests/test_reactions.py +++ b/zerver/tests/test_reactions.py @@ -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 """ diff --git a/zerver/views/reactions.py b/zerver/views/reactions.py index d822440b10..b48328fa40 100644 --- a/zerver/views/reactions.py +++ b/zerver/views/reactions.py @@ -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()