From e5500a2226091707d46e61cda691101364dec200 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Thu, 14 Apr 2022 14:54:01 -0700 Subject: [PATCH] actions: Split out zerver.actions.reactions. Signed-off-by: Anders Kaseorg --- zerver/actions/reactions.py | 170 ++++++++++++++++++ zerver/lib/actions.py | 165 +---------------- zerver/lib/onboarding.py | 3 +- zerver/openapi/curl_param_value_generators.py | 2 +- zerver/tests/test_events.py | 3 +- zerver/tests/test_import_export.py | 8 +- zerver/tests/test_management_commands.py | 2 +- zerver/tests/test_message_edit.py | 2 +- zerver/tests/test_reactions.py | 6 +- zerver/views/reactions.py | 2 +- .../commands/add_mock_conversation.py | 2 +- 11 files changed, 184 insertions(+), 181 deletions(-) create mode 100644 zerver/actions/reactions.py diff --git a/zerver/actions/reactions.py b/zerver/actions/reactions.py new file mode 100644 index 0000000000..df16fe31b8 --- /dev/null +++ b/zerver/actions/reactions.py @@ -0,0 +1,170 @@ +from typing import Any, Dict, Optional + +from django.db import transaction +from django.utils.translation import gettext as _ + +from zerver.actions.create_user import create_historical_user_messages +from zerver.lib.emoji import check_emoji_request, emoji_name_to_emoji_code +from zerver.lib.exceptions import JsonableError +from zerver.lib.message import access_message, update_to_dict_cache +from zerver.lib.stream_subscription import subscriber_ids_with_stream_history_access +from zerver.models import Message, Reaction, Recipient, Stream, UserMessage, UserProfile +from zerver.tornado.django_api import send_event + + +def notify_reaction_update( + user_profile: UserProfile, message: Message, reaction: Reaction, op: str +) -> None: + user_dict = { + "user_id": user_profile.id, + "email": user_profile.email, + "full_name": user_profile.full_name, + } + + event: Dict[str, Any] = { + "type": "reaction", + "op": op, + "user_id": user_profile.id, + # TODO: We plan to remove this redundant user_dict object once + # clients are updated to support accessing use user_id. See + # https://github.com/zulip/zulip/pull/14711 for details. + "user": user_dict, + "message_id": message.id, + "emoji_name": reaction.emoji_name, + "emoji_code": reaction.emoji_code, + "reaction_type": reaction.reaction_type, + } + + # Update the cached message since new reaction is added. + update_to_dict_cache([message]) + + # Recipients for message update events, including reactions, are + # everyone who got the original message, plus subscribers of + # streams with the access to stream's full history. + # + # This means reactions won't live-update in preview narrows for a + # stream the user isn't yet subscribed to; this is the right + # performance tradeoff to avoid sending every reaction to public + # stream messages to all users. + # + # 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, even if they are not + # subscribed to the stream. + user_ids = set( + UserMessage.objects.filter(message=message.id).values_list("user_profile_id", flat=True) + ) + if message.recipient.type == Recipient.STREAM: + stream_id = message.recipient.type_id + stream = Stream.objects.get(id=stream_id) + user_ids |= subscriber_ids_with_stream_history_access(stream) + + transaction.on_commit(lambda: send_event(user_profile.realm, event, list(user_ids))) + + +def do_add_reaction( + user_profile: UserProfile, + message: Message, + emoji_name: str, + emoji_code: str, + reaction_type: str, +) -> None: + """Should be called while holding a SELECT FOR UPDATE lock + (e.g. via access_message(..., lock_message=True)) on the + Message row, to prevent race conditions. + """ + + reaction = Reaction( + user_profile=user_profile, + message=message, + emoji_name=emoji_name, + emoji_code=emoji_code, + reaction_type=reaction_type, + ) + + reaction.save() + + notify_reaction_update(user_profile, message, reaction, "add") + + +def check_add_reaction( + user_profile: UserProfile, + message_id: int, + emoji_name: str, + emoji_code: Optional[str], + reaction_type: Optional[str], +) -> None: + message, user_message = access_message(user_profile, message_id, lock_message=True) + + if emoji_code is None: + # The emoji_code argument is only required for rare corner + # cases discussed in the long block comment below. For simple + # API clients, we allow specifying just the name, and just + # look up the code using the current name->code mapping. + emoji_code = emoji_name_to_emoji_code(message.sender.realm, emoji_name)[0] + + if reaction_type is None: + reaction_type = emoji_name_to_emoji_code(message.sender.realm, emoji_name)[1] + + if Reaction.objects.filter( + user_profile=user_profile, + message=message, + emoji_code=emoji_code, + reaction_type=reaction_type, + ).exists(): + raise JsonableError(_("Reaction already exists.")) + + query = Reaction.objects.filter( + message=message, emoji_code=emoji_code, reaction_type=reaction_type + ) + if query.exists(): + # If another user has already reacted to this message with + # same emoji code, we treat the new reaction as a vote for the + # existing reaction. So the emoji name used by that earlier + # reaction takes precedence over whatever was passed in this + # request. This is necessary to avoid a message having 2 + # "different" emoji reactions with the same emoji code (and + # thus same image) on the same message, which looks ugly. + # + # In this "voting for an existing reaction" case, we shouldn't + # check whether the emoji code and emoji name match, since + # it's possible that the (emoji_type, emoji_name, emoji_code) + # triple for this existing reaction may not pass validation + # now (e.g. because it is for a realm emoji that has been + # since deactivated). We still want to allow users to add a + # vote any old reaction they see in the UI even if that is a + # deactivated custom emoji, so we just use the emoji name from + # the existing reaction with no further validation. + reaction = query.first() + assert reaction is not None + emoji_name = reaction.emoji_name + else: + # Otherwise, use the name provided in this request, but verify + # it is valid in the user's realm (e.g. not a deactivated + # realm emoji). + check_emoji_request(user_profile.realm, emoji_name, emoji_code, reaction_type) + + if user_message is None: + # See called function for more context. + create_historical_user_messages(user_id=user_profile.id, message_ids=[message.id]) + + do_add_reaction(user_profile, message, emoji_name, emoji_code, reaction_type) + + +def do_remove_reaction( + user_profile: UserProfile, message: Message, emoji_code: str, reaction_type: str +) -> None: + """Should be called while holding a SELECT FOR UPDATE lock + (e.g. via access_message(..., lock_message=True)) on the + Message row, to prevent race conditions. + """ + reaction = Reaction.objects.filter( + user_profile=user_profile, + message=message, + emoji_code=emoji_code, + reaction_type=reaction_type, + ).get() + reaction.delete() + + notify_reaction_update(user_profile, message, reaction, "remove") diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 101c3e1c81..756b7ecb2d 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -34,7 +34,6 @@ from zerver.lib.bulk_create import create_users from zerver.lib.cache import flush_user_profile from zerver.lib.create_user import get_display_email_address from zerver.lib.email_validation import email_reserved_for_system_bots_error -from zerver.lib.emoji import check_emoji_request, emoji_name_to_emoji_code from zerver.lib.exceptions import JsonableError from zerver.lib.markdown import MessageRenderingResult, topic_links from zerver.lib.markdown import version as markdown_version @@ -55,10 +54,7 @@ from zerver.lib.retention import move_messages_to_archive from zerver.lib.send_email import FromAddress, send_email_to_admins from zerver.lib.server_initialization import create_internal_realm, server_initialized from zerver.lib.sessions import delete_user_sessions -from zerver.lib.stream_subscription import ( - get_active_subscriptions_for_stream_id, - subscriber_ids_with_stream_history_access, -) +from zerver.lib.stream_subscription import get_active_subscriptions_for_stream_id from zerver.lib.stream_topic import StreamTopicTarget from zerver.lib.streams import ( access_stream_by_id, @@ -98,7 +94,6 @@ from zerver.models import ( RealmAuditLog, RealmDomain, RealmUserDefault, - Recipient, ScheduledEmail, Stream, UserMessage, @@ -515,164 +510,6 @@ def do_scrub_realm(realm: Realm, *, acting_user: Optional[UserProfile]) -> None: ) -def notify_reaction_update( - user_profile: UserProfile, message: Message, reaction: Reaction, op: str -) -> None: - user_dict = { - "user_id": user_profile.id, - "email": user_profile.email, - "full_name": user_profile.full_name, - } - - event: Dict[str, Any] = { - "type": "reaction", - "op": op, - "user_id": user_profile.id, - # TODO: We plan to remove this redundant user_dict object once - # clients are updated to support accessing use user_id. See - # https://github.com/zulip/zulip/pull/14711 for details. - "user": user_dict, - "message_id": message.id, - "emoji_name": reaction.emoji_name, - "emoji_code": reaction.emoji_code, - "reaction_type": reaction.reaction_type, - } - - # Update the cached message since new reaction is added. - update_to_dict_cache([message]) - - # Recipients for message update events, including reactions, are - # everyone who got the original message, plus subscribers of - # streams with the access to stream's full history. - # - # This means reactions won't live-update in preview narrows for a - # stream the user isn't yet subscribed to; this is the right - # performance tradeoff to avoid sending every reaction to public - # stream messages to all users. - # - # 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, even if they are not - # subscribed to the stream. - user_ids = set( - UserMessage.objects.filter(message=message.id).values_list("user_profile_id", flat=True) - ) - if message.recipient.type == Recipient.STREAM: - stream_id = message.recipient.type_id - stream = Stream.objects.get(id=stream_id) - user_ids |= subscriber_ids_with_stream_history_access(stream) - - transaction.on_commit(lambda: send_event(user_profile.realm, event, list(user_ids))) - - -def do_add_reaction( - user_profile: UserProfile, - message: Message, - emoji_name: str, - emoji_code: str, - reaction_type: str, -) -> None: - """Should be called while holding a SELECT FOR UPDATE lock - (e.g. via access_message(..., lock_message=True)) on the - Message row, to prevent race conditions. - """ - - reaction = Reaction( - user_profile=user_profile, - message=message, - emoji_name=emoji_name, - emoji_code=emoji_code, - reaction_type=reaction_type, - ) - - reaction.save() - - notify_reaction_update(user_profile, message, reaction, "add") - - -def check_add_reaction( - user_profile: UserProfile, - message_id: int, - emoji_name: str, - emoji_code: Optional[str], - reaction_type: Optional[str], -) -> None: - message, user_message = access_message(user_profile, message_id, lock_message=True) - - if emoji_code is None: - # The emoji_code argument is only required for rare corner - # cases discussed in the long block comment below. For simple - # API clients, we allow specifying just the name, and just - # look up the code using the current name->code mapping. - emoji_code = emoji_name_to_emoji_code(message.sender.realm, emoji_name)[0] - - if reaction_type is None: - reaction_type = emoji_name_to_emoji_code(message.sender.realm, emoji_name)[1] - - if Reaction.objects.filter( - user_profile=user_profile, - message=message, - emoji_code=emoji_code, - reaction_type=reaction_type, - ).exists(): - raise JsonableError(_("Reaction already exists.")) - - query = Reaction.objects.filter( - message=message, emoji_code=emoji_code, reaction_type=reaction_type - ) - if query.exists(): - # If another user has already reacted to this message with - # same emoji code, we treat the new reaction as a vote for the - # existing reaction. So the emoji name used by that earlier - # reaction takes precedence over whatever was passed in this - # request. This is necessary to avoid a message having 2 - # "different" emoji reactions with the same emoji code (and - # thus same image) on the same message, which looks ugly. - # - # In this "voting for an existing reaction" case, we shouldn't - # check whether the emoji code and emoji name match, since - # it's possible that the (emoji_type, emoji_name, emoji_code) - # triple for this existing reaction may not pass validation - # now (e.g. because it is for a realm emoji that has been - # since deactivated). We still want to allow users to add a - # vote any old reaction they see in the UI even if that is a - # deactivated custom emoji, so we just use the emoji name from - # the existing reaction with no further validation. - reaction = query.first() - assert reaction is not None - emoji_name = reaction.emoji_name - else: - # Otherwise, use the name provided in this request, but verify - # it is valid in the user's realm (e.g. not a deactivated - # realm emoji). - check_emoji_request(user_profile.realm, emoji_name, emoji_code, reaction_type) - - if user_message is None: - # See called function for more context. - create_historical_user_messages(user_id=user_profile.id, message_ids=[message.id]) - - do_add_reaction(user_profile, message, emoji_name, emoji_code, reaction_type) - - -def do_remove_reaction( - user_profile: UserProfile, message: Message, emoji_code: str, reaction_type: str -) -> None: - """Should be called while holding a SELECT FOR UPDATE lock - (e.g. via access_message(..., lock_message=True)) on the - Message row, to prevent race conditions. - """ - reaction = Reaction.objects.filter( - user_profile=user_profile, - message=message, - emoji_code=emoji_code, - reaction_type=reaction_type, - ).get() - reaction.delete() - - notify_reaction_update(user_profile, message, reaction, "remove") - - def validate_message_edit_payload( message: Message, stream_id: Optional[int], diff --git a/zerver/lib/onboarding.py b/zerver/lib/onboarding.py index 62bb1952a2..a4cc2a8620 100644 --- a/zerver/lib/onboarding.py +++ b/zerver/lib/onboarding.py @@ -10,7 +10,8 @@ from zerver.actions.message_send import ( internal_prep_stream_message_by_name, internal_send_private_message, ) -from zerver.lib.actions import do_add_reaction, setup_realm_internal_bots +from zerver.actions.reactions import do_add_reaction +from zerver.lib.actions import setup_realm_internal_bots from zerver.lib.emoji import emoji_name_to_emoji_code from zerver.lib.message import SendMessageRequest from zerver.models import Message, Realm, UserProfile, get_system_bot diff --git a/zerver/openapi/curl_param_value_generators.py b/zerver/openapi/curl_param_value_generators.py index 0a2b252592..ae52d90456 100644 --- a/zerver/openapi/curl_param_value_generators.py +++ b/zerver/openapi/curl_param_value_generators.py @@ -12,9 +12,9 @@ from django.utils.timezone import now as timezone_now from zerver.actions.create_user import do_create_user from zerver.actions.presence import update_user_presence +from zerver.actions.reactions import do_add_reaction from zerver.actions.realm_linkifiers import do_add_linkifier from zerver.actions.realm_playgrounds import do_add_realm_playground -from zerver.lib.actions import do_add_reaction from zerver.lib.events import do_events_register from zerver.lib.initial_password import initial_password from zerver.lib.test_classes import ZulipTestCase diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index d4db54be71..5a28959ed8 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -40,6 +40,7 @@ from zerver.actions.invites import ( do_revoke_user_invite, ) from zerver.actions.presence import do_update_user_presence, do_update_user_status +from zerver.actions.reactions import do_add_reaction, do_remove_reaction from zerver.actions.realm_emoji import check_add_realm_emoji, do_remove_realm_emoji from zerver.actions.realm_icon import do_change_icon_source from zerver.actions.realm_linkifiers import ( @@ -86,7 +87,6 @@ from zerver.actions.users import ( ) from zerver.actions.video_calls import do_set_zoom_token from zerver.lib.actions import ( - do_add_reaction, do_add_realm_domain, do_change_bot_owner, do_change_default_all_public_streams, @@ -97,7 +97,6 @@ from zerver.lib.actions import ( do_deactivate_realm, do_delete_messages, do_mute_user, - do_remove_reaction, do_remove_realm_domain, do_set_realm_authentication_methods, do_set_realm_message_editing, diff --git a/zerver/tests/test_import_export.py b/zerver/tests/test_import_export.py index 6698b200d3..536600de97 100644 --- a/zerver/tests/test_import_export.py +++ b/zerver/tests/test_import_export.py @@ -17,6 +17,7 @@ from zerver.actions.custom_profile_fields import ( try_add_realm_custom_profile_field, ) from zerver.actions.presence import do_update_user_presence, do_update_user_status +from zerver.actions.reactions import check_add_reaction, do_add_reaction from zerver.actions.realm_emoji import check_add_realm_emoji from zerver.actions.realm_icon import do_change_icon_source from zerver.actions.realm_logo import do_change_logo_source @@ -24,12 +25,7 @@ from zerver.actions.user_activity import do_update_user_activity, do_update_user from zerver.actions.user_topics import do_mute_topic from zerver.actions.users import do_deactivate_user from zerver.lib import upload -from zerver.lib.actions import ( - check_add_reaction, - do_add_reaction, - do_change_realm_plan_type, - do_mute_user, -) +from zerver.lib.actions import do_change_realm_plan_type, do_mute_user from zerver.lib.avatar_hash import user_avatar_path from zerver.lib.bot_config import set_bot_config from zerver.lib.bot_lib import StateHandler diff --git a/zerver/tests/test_management_commands.py b/zerver/tests/test_management_commands.py index 9996d4b8b4..4232150c0a 100644 --- a/zerver/tests/test_management_commands.py +++ b/zerver/tests/test_management_commands.py @@ -14,7 +14,7 @@ from django.utils.timezone import now as timezone_now from confirmation.models import RealmCreationKey, generate_realm_creation_url from zerver.actions.create_user import do_create_user -from zerver.lib.actions import do_add_reaction +from zerver.actions.reactions import do_add_reaction from zerver.lib.management import ZulipBaseCommand, check_config from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_helpers import most_recent_message, stdout_suppressed diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index f2af201e8f..adc5963c9d 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -8,11 +8,11 @@ from django.db import IntegrityError from django.http import HttpResponse from django.utils.timezone import now as timezone_now +from zerver.actions.reactions import do_add_reaction from zerver.actions.streams import do_change_stream_post_policy, do_deactivate_stream from zerver.actions.users import do_change_user_role from zerver.lib.actions import ( check_update_message, - do_add_reaction, do_change_realm_plan_type, do_delete_messages, do_set_realm_property, diff --git a/zerver/tests/test_reactions.py b/zerver/tests/test_reactions.py index 335942d03a..79b049cd4f 100644 --- a/zerver/tests/test_reactions.py +++ b/zerver/tests/test_reactions.py @@ -4,8 +4,8 @@ from unittest import mock import orjson from django.http import HttpResponse +from zerver.actions.reactions import notify_reaction_update from zerver.actions.streams import do_change_stream_permission -from zerver.lib.actions import notify_reaction_update from zerver.lib.cache import cache_get, to_dict_cache_key_id from zerver.lib.emoji import emoji_name_to_emoji_code from zerver.lib.exceptions import JsonableError @@ -1035,7 +1035,7 @@ class ReactionAPIEventTest(EmojiReactionBase): } events: List[Mapping[str, Any]] = [] with self.tornado_redirected_to_list(events, expected_num_events=1): - with mock.patch("zerver.lib.actions.send_event") as m: + with mock.patch("zerver.actions.reactions.send_event") as m: m.side_effect = AssertionError( "Events should be sent only after the transaction commits!" ) @@ -1119,7 +1119,7 @@ class ReactionAPIEventTest(EmojiReactionBase): ) with self.tornado_redirected_to_list([], expected_num_events=1): - with mock.patch("zerver.lib.actions.send_event") as m: + with mock.patch("zerver.actions.reactions.send_event") as m: m.side_effect = AssertionError( "Events should be sent only after the transaction commits." ) diff --git a/zerver/views/reactions.py b/zerver/views/reactions.py index a1fd401433..6d93e2ffa5 100644 --- a/zerver/views/reactions.py +++ b/zerver/views/reactions.py @@ -4,7 +4,7 @@ from django.db import transaction from django.http import HttpRequest, HttpResponse from django.utils.translation import gettext as _ -from zerver.lib.actions import check_add_reaction, do_remove_reaction +from zerver.actions.reactions import check_add_reaction, do_remove_reaction from zerver.lib.emoji import emoji_name_to_emoji_code from zerver.lib.exceptions import JsonableError from zerver.lib.message import access_message diff --git a/zilencer/management/commands/add_mock_conversation.py b/zilencer/management/commands/add_mock_conversation.py index ac3f3c2eaf..6d5492c37c 100644 --- a/zilencer/management/commands/add_mock_conversation.py +++ b/zilencer/management/commands/add_mock_conversation.py @@ -4,9 +4,9 @@ from django.core.management.base import BaseCommand from zerver.actions.create_user import do_create_user from zerver.actions.message_send import do_send_messages, internal_prep_stream_message +from zerver.actions.reactions import do_add_reaction from zerver.actions.streams import bulk_add_subscriptions from zerver.actions.user_settings import do_change_avatar_fields -from zerver.lib.actions import do_add_reaction from zerver.lib.emoji import emoji_name_to_emoji_code from zerver.lib.streams import ensure_stream from zerver.lib.upload import upload_avatar_image