emoji: Rework reactions validation to have a single function.

This feels more readable to me, and also identified a couple cases
where we were missing test coverage.

zerver/lib/emoji.py

@@ -35,26 +35,8 @@ def check_valid_emoji(realm, emoji_name):
     # type: (Realm, Text) -> None
     emoji_name_to_emoji_code(realm, emoji_name)
 
-def check_emoji_name_consistency(emoji_name: str, emoji_code: str, emoji_type: str) -> None:
+def check_emoji_request(realm: Realm, emoji_name: str, emoji_code: str,
-    # Given a realm, emoji code and emoji type, checks whether the
+                        emoji_type: str) -> None:
-    # passed emoji name is a valid name for that emoji. It is assumed
-    # here that the emoji code has been checked for validity before
-    # calling this function.
-    if emoji_type == "realm_emoji":
-        if emoji_code == emoji_name:
-            return
-    elif emoji_type == "zulip_extra_emoji":
-        if emoji_name in ["zulip"]:
-            return
-    elif emoji_type == "unicode_emoji":
-        if name_to_codepoint.get(emoji_name) == emoji_code:
-            return
-    else:
-        raise AssertionError("Emoji type should have been checked previously.")
-
-    raise JsonableError(_("Invalid emoji name."))
-
-def check_emoji_code_consistency(realm: Realm, emoji_code: str, emoji_type: str) -> None:
     # For a given realm and emoji type, checks whether an emoji
     # code is valid for new reactions, or not.
     if emoji_type == "realm_emoji":
@@ -63,12 +45,18 @@ def check_emoji_code_consistency(realm: Realm, emoji_code: str, emoji_type: str)
             raise JsonableError(_("No such realm emoji found."))
         if realm_emojis[emoji_code]["deactivated"]:
             raise JsonableError(_("This realm emoji has been deactivated."))
+        if emoji_name != emoji_code:
+            raise JsonableError(_("Invalid emoji name."))
     elif emoji_type == "zulip_extra_emoji":
         if emoji_code not in ["zulip"]:
             raise JsonableError(_("No such extra emoji found."))
+        if emoji_name != emoji_code:
+            raise JsonableError(_("Invalid emoji name."))
     elif emoji_type == "unicode_emoji":
         if emoji_code not in codepoint_to_name:
             raise JsonableError(_("No unicode emoji with this emoji code found."))
+        if name_to_codepoint.get(emoji_name) != emoji_code:
+            raise JsonableError(_("Invalid emoji name."))
     else:
         # The above are the only valid emoji types
         raise JsonableError(_("Invalid emoji type."))

zerver/tests/test_reactions.py

@@ -576,6 +576,15 @@ class ZulipExtraEmojiReactionTest(EmojiReactionBase):
         result = self.post_reaction(reaction_info)
         self.assert_json_error(result, 'No such extra emoji found.')
 
+    def test_add_invalid_emoji_name(self) -> None:
+        reaction_info = {
+            'emoji_name': 'zulip_invalid',
+            'emoji_code': 'zulip',
+            'reaction_type': 'zulip_extra_emoji',
+        }
+        result = self.post_reaction(reaction_info)
+        self.assert_json_error(result, 'Invalid emoji name.')
+
     def test_delete_zulip_emoji(self) -> None:
         result = self.post_zulip_reaction()
         self.assert_json_success(result)
@@ -604,6 +613,14 @@ class RealmEmojiReactionTests(EmojiReactionBase):
         result = self.post_reaction(reaction_info)
         self.assert_json_error(result, 'No such realm emoji found.')
 
+    def test_add_realm_emoji_invalid_name(self) -> None:
+        reaction_info = {
+            'emoji_name': 'bogus_name',
+            'emoji_code': 'green_tick',
+        }
+        result = self.post_reaction(reaction_info)
+        self.assert_json_error(result, 'Invalid emoji name.')
+
     def test_add_deactivated_realm_emoji(self) -> None:
         emoji = RealmEmoji.objects.get(name="green_tick")
         emoji.deactivated = True

zerver/views/reactions.py

@@ -7,8 +7,7 @@ from zerver.decorator import \
     has_request_variables, REQ, to_non_negative_int
 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_code_consistency,\
+from zerver.lib.emoji import check_emoji_request, check_valid_emoji
-    check_emoji_name_consistency, check_valid_emoji
 from zerver.lib.message import access_message
 from zerver.lib.request import JsonableError
 from zerver.lib.response import json_success
@@ -64,8 +63,8 @@ def add_reaction(request: HttpRequest, user_profile: UserProfile, message_id: in
         # 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_code_consistency(message.sender.realm, emoji_code, reaction_type)
+        check_emoji_request(message.sender.realm, emoji_name,
-        check_emoji_name_consistency(emoji_name, emoji_code, reaction_type)
+                            emoji_code, reaction_type)
 
     if user_message is None:
         create_historical_message(user_profile, message)