From f8855ca179920963f2531d78054f820896971e7c Mon Sep 17 00:00:00 2001 From: Gloria Elston Date: Thu, 10 Oct 2019 12:03:09 -0500 Subject: [PATCH] api: Remove legacy emoji reactions endpoint. The original/legacy emoji reactions endpoints made use of HTTP PUT and didn't have an API that could correctly handle situations where the emoji names change over time. We stopped using the legacy endpoints some time ago, so we can remove them now. This requires straightforward updates to older tests that were still written against the legacy API. Fixes #12940. --- zerver/lib/emoji.py | 3 - zerver/lib/test_classes.py | 4 - zerver/tests/test_narrow.py | 7 +- zerver/tests/test_openapi.py | 1 - zerver/tests/test_reactions.py | 148 +++++++++++++++++++++++++++------ zerver/views/reactions.py | 49 +---------- zproject/urls.py | 11 +-- 7 files changed, 131 insertions(+), 92 deletions(-) diff --git a/zerver/lib/emoji.py b/zerver/lib/emoji.py index 8ac96c9e12..b0f646bbd6 100644 --- a/zerver/lib/emoji.py +++ b/zerver/lib/emoji.py @@ -51,9 +51,6 @@ def emoji_name_to_emoji_code(realm: Realm, emoji_name: str) -> Tuple[str, str]: return name_to_codepoint[emoji_name], Reaction.UNICODE_EMOJI raise JsonableError(_("Emoji '%s' does not exist") % (emoji_name,)) -def check_valid_emoji(realm: Realm, emoji_name: str) -> None: - emoji_name_to_emoji_code(realm, emoji_name) - def check_emoji_request(realm: Realm, emoji_name: str, emoji_code: str, emoji_type: str) -> None: # For a given realm and emoji type, checks whether an emoji diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index adc15bb1d9..51b6ffef30 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -434,10 +434,6 @@ class ZulipTestCase(TestCase): kwargs['HTTP_AUTHORIZATION'] = self.encode_credentials(identifier, kwargs.get('subdomain', 'zulip')) return self.client_patch(*args, **kwargs) - def api_put(self, identifier: str, *args: Any, **kwargs: Any) -> HttpResponse: - kwargs['HTTP_AUTHORIZATION'] = self.encode_credentials(identifier, kwargs.get('subdomain', 'zulip')) - return self.client_put(*args, **kwargs) - def api_delete(self, identifier: str, *args: Any, **kwargs: Any) -> HttpResponse: kwargs['HTTP_AUTHORIZATION'] = self.encode_credentials(identifier, kwargs.get('subdomain', 'zulip')) return self.client_delete(*args, **kwargs) diff --git a/zerver/tests/test_narrow.py b/zerver/tests/test_narrow.py index b2601a8aa2..9d1af83b74 100644 --- a/zerver/tests/test_narrow.py +++ b/zerver/tests/test_narrow.py @@ -1123,9 +1123,12 @@ class GetOldMessagesTest(ZulipTestCase): self.login(self.example_email("othello")) reaction_name = 'thumbs_up' + reaction_info = { + 'emoji_name': reaction_name + } - url = '/json/messages/{}/emoji_reactions/{}'.format(message_id, reaction_name) - payload = self.client_put(url) + url = '/json/messages/{}/reactions'.format(message_id) + payload = self.client_post(url, reaction_info) self.assert_json_success(payload) self.login(self.example_email("hamlet")) diff --git a/zerver/tests/test_openapi.py b/zerver/tests/test_openapi.py index a4cbba10b0..b2c59007f6 100644 --- a/zerver/tests/test_openapi.py +++ b/zerver/tests/test_openapi.py @@ -217,7 +217,6 @@ class OpenAPIArgumentsTest(ZulipTestCase): '/invites/{prereg_id}/resend', '/invites/multiuse/{invite_id}', '/users/me/subscriptions/{stream_id}', - '/messages/{message_id}/emoji_reactions/{emoji_name}', '/attachments/{attachment_id}', '/user_groups/{user_group_id}/members', '/streams/{stream_id}/members', diff --git a/zerver/tests/test_reactions.py b/zerver/tests/test_reactions.py index 6ce946e3c5..ed66560a0f 100644 --- a/zerver/tests/test_reactions.py +++ b/zerver/tests/test_reactions.py @@ -17,7 +17,12 @@ class ReactionEmojiTest(ZulipTestCase): Sending reaction without emoji fails """ sender = self.example_email("hamlet") - result = self.api_put(sender, '/api/v1/messages/1/emoji_reactions/') + reaction_info = { + 'emoji_name': '' + } + + result = self.api_post(sender, '/api/v1/messages/1/reactions', + reaction_info) self.assertEqual(result.status_code, 400) def test_add_invalid_emoji(self) -> None: @@ -25,7 +30,12 @@ class ReactionEmojiTest(ZulipTestCase): Sending invalid emoji fails """ sender = self.example_email("hamlet") - result = self.api_put(sender, '/api/v1/messages/1/emoji_reactions/foo') + reaction_info = { + 'emoji_name': 'foo' + } + + result = self.api_post(sender, '/api/v1/messages/1/reactions', + reaction_info) self.assert_json_error(result, "Emoji 'foo' does not exist") def test_add_deactivated_realm_emoji(self) -> None: @@ -36,7 +46,13 @@ class ReactionEmojiTest(ZulipTestCase): emoji.deactivated = True emoji.save(update_fields=['deactivated']) sender = self.example_email("hamlet") - result = self.api_put(sender, '/api/v1/messages/1/emoji_reactions/green_tick') + reaction_info = { + 'emoji_name': 'green_tick', + 'reaction_type': 'realm_emoji' + } + + result = self.api_post(sender, '/api/v1/messages/1/reactions', + reaction_info) self.assert_json_error(result, "Emoji 'green_tick' does not exist") def test_valid_emoji(self) -> None: @@ -44,7 +60,12 @@ class ReactionEmojiTest(ZulipTestCase): Reacting with valid emoji succeeds """ sender = self.example_email("hamlet") - result = self.api_put(sender, '/api/v1/messages/1/emoji_reactions/smile') + reaction_info = { + 'emoji_name': 'smile' + } + + result = self.api_post(sender, '/api/v1/messages/1/reactions', + reaction_info) self.assert_json_success(result) self.assertEqual(200, result.status_code) @@ -53,7 +74,13 @@ class ReactionEmojiTest(ZulipTestCase): Reacting with zulip emoji succeeds """ sender = self.example_email("hamlet") - result = self.api_put(sender, '/api/v1/messages/1/emoji_reactions/zulip') + reaction_info = { + 'emoji_name': 'zulip', + 'reaction_type': 'zulip_extra_emoji' + } + + result = self.api_post(sender, '/api/v1/messages/1/reactions', + reaction_info) self.assert_json_success(result) self.assertEqual(200, result.status_code) @@ -73,7 +100,12 @@ class ReactionEmojiTest(ZulipTestCase): message_id=message_id).exists()) # Have hamlet react to the message - result = self.api_put(sender, '/api/v1/messages/%s/emoji_reactions/smile' % (message_id,)) + reaction_info = { + 'emoji_name': 'smile' + } + + result = self.api_post(sender, '/api/v1/messages/%s/reactions' % (message_id,), + reaction_info) self.assert_json_success(result) # Fetch the now-created UserMessage object to confirm it exists and is historical @@ -87,9 +119,14 @@ class ReactionEmojiTest(ZulipTestCase): Reacting with valid realm emoji succeeds """ sender = self.example_email("hamlet") - emoji_name = 'green_tick' - result = self.api_put(sender, '/api/v1/messages/1/emoji_reactions/%s' % (emoji_name,)) + reaction_info = { + 'emoji_name': 'green_tick', + 'reaction_type': 'realm_emoji' + } + + result = self.api_post(sender, '/api/v1/messages/1/reactions', + reaction_info) self.assert_json_success(result) def test_emoji_name_to_emoji_code(self) -> None: @@ -153,7 +190,12 @@ class ReactionMessageIDTest(ZulipTestCase): Reacting without a message_id fails """ sender = self.example_email("hamlet") - result = self.api_put(sender, '/api/v1/messages//emoji_reactions/smile') + reaction_info = { + 'emoji_name': 'smile' + } + + result = self.api_post(sender, '/api/v1/messages//reactions', + reaction_info) self.assertEqual(result.status_code, 404) def test_invalid_message_id(self) -> None: @@ -161,7 +203,12 @@ class ReactionMessageIDTest(ZulipTestCase): Reacting to an invalid message id fails """ sender = self.example_email("hamlet") - result = self.api_put(sender, '/api/v1/messages/-1/emoji_reactions/smile') + reaction_info = { + 'emoji_name': 'smile' + } + + result = self.api_post(sender, '/api/v1/messages/-1/reactions', + reaction_info) self.assertEqual(result.status_code, 404) def test_inaccessible_message_id(self) -> None: @@ -178,7 +225,12 @@ class ReactionMessageIDTest(ZulipTestCase): "to": pm_recipient}) self.assert_json_success(result) pm_id = result.json()['id'] - result = self.api_put(reaction_sender, '/api/v1/messages/%s/emoji_reactions/smile' % (pm_id,)) + reaction_info = { + 'emoji_name': 'smile' + } + + result = self.api_post(reaction_sender, '/api/v1/messages/%s/reactions' % (pm_id,), + reaction_info) self.assert_json_error(result, "Invalid message(s)") class ReactionTest(ZulipTestCase): @@ -198,10 +250,18 @@ class ReactionTest(ZulipTestCase): content = ujson.loads(pm.content) pm_id = content['id'] - first = self.api_put(reaction_sender, '/api/v1/messages/%s/emoji_reactions/smile' % (pm_id,)) + + reaction_info = { + 'emoji_name': 'smile' + } + + first = self.api_post(reaction_sender, '/api/v1/messages/%s/reactions' % (pm_id,), + reaction_info) self.assert_json_success(first) - second = self.api_put(reaction_sender, '/api/v1/messages/%s/emoji_reactions/smile' % (pm_id,)) - self.assert_json_error(second, "Reaction already exists") + + second = self.api_post(reaction_sender, '/api/v1/messages/%s/reactions' % (pm_id,), + reaction_info) + self.assert_json_error(second, "Reaction already exists.") def test_remove_nonexisting_reaction(self) -> None: """ @@ -216,28 +276,44 @@ class ReactionTest(ZulipTestCase): "content": "Test message", "to": pm_recipient}) self.assert_json_success(pm) + content = ujson.loads(pm.content) pm_id = content['id'] - add = self.api_put(reaction_sender, '/api/v1/messages/%s/emoji_reactions/smile' % (pm_id,)) + reaction_info = { + 'emoji_name': 'smile' + } + + add = self.api_post(reaction_sender, '/api/v1/messages/%s/reactions' % (pm_id,), + reaction_info) self.assert_json_success(add) - first = self.api_delete(reaction_sender, '/api/v1/messages/%s/emoji_reactions/smile' % (pm_id,)) + first = self.api_delete(reaction_sender, '/api/v1/messages/%s/reactions' % (pm_id,), + reaction_info) self.assert_json_success(first) - second = self.api_delete(reaction_sender, '/api/v1/messages/%s/emoji_reactions/smile' % (pm_id,)) - self.assert_json_error(second, "Reaction does not exist") + second = self.api_delete(reaction_sender, '/api/v1/messages/%s/reactions' % (pm_id,), + reaction_info) + self.assert_json_error(second, "Reaction doesn't exist.") def test_remove_existing_reaction_with_renamed_emoji(self) -> None: """ Removes an old existing reaction but the name of emoji got changed during various emoji infra changes. """ + realm = get_realm('zulip') sender = self.example_email("hamlet") - result = self.api_put(sender, '/api/v1/messages/1/emoji_reactions/smile') + emoji_code, reaction_type = emoji_name_to_emoji_code(realm, 'smile') + reaction_info = { + 'emoji_name': 'smile', + 'emoji_code': emoji_code, + 'reaction_type': reaction_type + } + + result = self.api_post(sender, '/api/v1/messages/1/reactions', reaction_info) self.assert_json_success(result) with mock.patch('zerver.lib.emoji.name_to_codepoint', name_to_codepoint={}): - result = self.api_delete(sender, '/api/v1/messages/1/emoji_reactions/smile') + result = self.api_delete(sender, '/api/v1/messages/1/reactions', reaction_info) self.assert_json_success(result) def test_remove_existing_reaction_with_deactivated_realm_emoji(self) -> None: @@ -245,15 +321,22 @@ class ReactionTest(ZulipTestCase): Removes an old existing reaction but the realm emoji used there has been deactivated. """ sender = self.example_email("hamlet") - result = self.api_put(sender, '/api/v1/messages/1/emoji_reactions/green_tick') + + emoji = RealmEmoji.objects.get(name="green_tick") + + reaction_info = { + 'emoji_name': 'green_tick', + 'emoji_code': str(emoji.id), + 'reaction_type': 'realm_emoji' + } + + result = self.api_post(sender, '/api/v1/messages/1/reactions', reaction_info) self.assert_json_success(result) # Deactivate realm emoji. - emoji = RealmEmoji.objects.get(name="green_tick") emoji.deactivated = True emoji.save(update_fields=['deactivated']) - - result = self.api_delete(sender, '/api/v1/messages/1/emoji_reactions/green_tick') + result = self.api_delete(sender, '/api/v1/messages/1/reactions', reaction_info) self.assert_json_success(result) class ReactionEventTest(ZulipTestCase): @@ -275,9 +358,14 @@ class ReactionEventTest(ZulipTestCase): expected_recipient_ids = set([pm_sender.id, pm_recipient.id]) + reaction_info = { + 'emoji_name': 'smile' + } + events = [] # type: List[Mapping[str, Any]] with tornado_redirected_to_list(events): - result = self.api_put(reaction_sender.email, '/api/v1/messages/%s/emoji_reactions/smile' % (pm_id,)) + result = self.api_post(reaction_sender.email, '/api/v1/messages/%s/reactions' % (pm_id,), + reaction_info) self.assert_json_success(result) self.assertEqual(len(events), 1) @@ -310,12 +398,18 @@ class ReactionEventTest(ZulipTestCase): expected_recipient_ids = set([pm_sender.id, pm_recipient.id]) - add = self.api_put(reaction_sender.email, '/api/v1/messages/%s/emoji_reactions/smile' % (pm_id,)) + reaction_info = { + 'emoji_name': 'smile' + } + + add = self.api_post(reaction_sender.email, '/api/v1/messages/%s/reactions' % (pm_id,), + reaction_info) self.assert_json_success(add) events = [] # type: List[Mapping[str, Any]] with tornado_redirected_to_list(events): - result = self.api_delete(reaction_sender.email, '/api/v1/messages/%s/emoji_reactions/smile' % (pm_id,)) + result = self.api_delete(reaction_sender.email, '/api/v1/messages/%s/reactions' % (pm_id,), + reaction_info) self.assert_json_success(result) self.assertEqual(len(events), 1) diff --git a/zerver/views/reactions.py b/zerver/views/reactions.py index 8da23b3978..b7fb7ec00e 100644 --- a/zerver/views/reactions.py +++ b/zerver/views/reactions.py @@ -3,10 +3,8 @@ from django.utils.translation import ugettext as _ from zerver.decorator import \ has_request_variables, REQ -from zerver.lib.actions import do_add_reaction, do_add_reaction_legacy, \ - do_remove_reaction, do_remove_reaction_legacy -from zerver.lib.emoji import check_emoji_request, check_valid_emoji, \ - emoji_name_to_emoji_code +from zerver.lib.actions import do_add_reaction, do_remove_reaction +from zerver.lib.emoji import check_emoji_request, emoji_name_to_emoji_code from zerver.lib.message import access_message from zerver.lib.request import JsonableError from zerver.lib.response import json_success @@ -118,46 +116,3 @@ def remove_reaction(request: HttpRequest, user_profile: UserProfile, message_id: do_remove_reaction(user_profile, message, emoji_code, reaction_type) return json_success() - -@has_request_variables -def add_reaction_legacy(request: HttpRequest, user_profile: UserProfile, - message_id: int, emoji_name: str) -> HttpResponse: - - # access_message will throw a JsonableError exception if the user - # cannot see the message (e.g. for messages to private streams). - message, user_message = access_message(user_profile, message_id) - - check_valid_emoji(message.sender.realm, emoji_name) - - # We could probably just make this check be a try/except for the - # IntegrityError from it already existing, but this is a bit cleaner. - if Reaction.objects.filter(user_profile=user_profile, - message=message, - emoji_name=emoji_name).exists(): - raise JsonableError(_("Reaction already exists")) - - if user_message is None: - create_historical_message(user_profile, message) - - do_add_reaction_legacy(user_profile, message, emoji_name) - - return json_success() - -@has_request_variables -def remove_reaction_legacy(request: HttpRequest, user_profile: UserProfile, - message_id: int, emoji_name: str) -> HttpResponse: - - # 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] - - # We could probably just make this check be a try/except for the - # IntegrityError from it already existing, but this is a bit cleaner. - if not Reaction.objects.filter(user_profile=user_profile, - message=message, - emoji_name=emoji_name).exists(): - raise JsonableError(_("Reaction does not exist")) - - do_remove_reaction_legacy(user_profile, message, emoji_name) - - return json_success() diff --git a/zproject/urls.py b/zproject/urls.py index b4ca06e111..f896d069af 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -213,19 +213,14 @@ v1_api_and_json_patterns = [ {'POST': 'zerver.views.submessage.process_submessage'}), # New endpoint for handling reactions. + # reactions -> zerver.view.reactions + # POST adds a reaction to a message + # DELETE removes a reaction from a message url(r'^messages/(?P[0-9]+)/reactions$', rest_dispatch, {'POST': 'zerver.views.reactions.add_reaction', 'DELETE': 'zerver.views.reactions.remove_reaction'}), - # reactions -> zerver.view.reactions - # PUT adds a reaction to a message - # DELETE removes a reaction from a message - url(r'^messages/(?P[0-9]+)/emoji_reactions/(?P.*)$', - rest_dispatch, - {'PUT': 'zerver.views.reactions.add_reaction_legacy', - 'DELETE': 'zerver.views.reactions.remove_reaction_legacy'}), - # attachments -> zerver.views.attachments url(r'^attachments$', rest_dispatch, {'GET': 'zerver.views.attachments.list_by_user'}),