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

@@ -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

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)

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)
+        url = '/json/messages/{}/reactions'.format(message_id)
-        payload = self.client_put(url)
+        payload = self.client_post(url, reaction_info)
         self.assert_json_success(payload)
 
         self.login(self.example_email("hamlet"))

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',

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,))
+        second = self.api_delete(reaction_sender, '/api/v1/messages/%s/reactions' % (pm_id,),
-        self.assert_json_error(second, "Reaction does not exist")
+                                 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/reactions', reaction_info)
-        result = self.api_delete(sender, '/api/v1/messages/1/emoji_reactions/green_tick')
         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)
 

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, \
+from zerver.lib.actions import do_add_reaction, do_remove_reaction
-    do_remove_reaction, do_remove_reaction_legacy
+from zerver.lib.emoji import check_emoji_request, emoji_name_to_emoji_code
-from zerver.lib.emoji import check_emoji_request, check_valid_emoji, \
-    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()

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<message_id>[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<message_id>[0-9]+)/emoji_reactions/(?P<emoji_name>.*)$',
-        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'}),