reaction tests: Clean up optional parameters.

This fixes up some complex helpers that may
have had some value before f-strings come along,
but they mostly obscured the logic for
people reading the tests.

We still keep really simple helpers for the
common cases, but there are no optional
parameters for them.

One goal of this fix is to remove the
short_name concept, and we just explicitly
set senders everywhere we need them.

We also now have each test just explicitly set
its reaction_type.

For cases where we have custom message ids
or senders, we just inline the simple call
to api_post.

zerver/tests/test_reactions.py

@@ -9,7 +9,7 @@ from zerver.lib.emoji import emoji_name_to_emoji_code
 from zerver.lib.message import extract_message_dict
 from zerver.lib.request import JsonableError
 from zerver.lib.test_classes import ZulipTestCase
-from zerver.lib.test_helpers import tornado_redirected_to_list
+from zerver.lib.test_helpers import tornado_redirected_to_list, zulip_reaction_info
 from zerver.models import Message, Reaction, RealmEmoji, UserMessage, get_realm
 
 
@@ -480,44 +480,36 @@ class EmojiReactionBase(ZulipTestCase):
     changing this: It's used in test_retention.py as well."""
 
     def __init__(self, *args: Any, **kwargs: Any) -> None:
-        self.reaction_type = 'realm_emoji'
         super().__init__(*args, **kwargs)
 
-    def post_reaction(self, reaction_info: Dict[str, str], message_id: int=1,
-                      sender: str='hamlet') -> HttpResponse:
-        if 'reaction_type' not in reaction_info:
-            reaction_info['reaction_type'] = self.reaction_type
-        sender = self.example_user(sender)
-        result = self.api_post(sender, f'/api/v1/messages/{message_id}/reactions',
-                               reaction_info)
+    def post_reaction(self, reaction_info: Dict[str, str]) -> HttpResponse:
+        message_id = 1
+
+        result = self.api_post(
+            self.example_user('hamlet'),
+            f'/api/v1/messages/{message_id}/reactions',
+            reaction_info
+        )
         return result
 
-    def post_zulip_reaction(self, message_id: int=1, sender: str='hamlet') -> HttpResponse:
-        reaction_info = {
-            'emoji_name': 'zulip',
-            'emoji_code': 'zulip',
-            'reaction_type': 'zulip_extra_emoji',
-        }
-        result = self.post_reaction(reaction_info, message_id, sender)
+    def post_other_reaction(self, reaction_info: Dict[str, str]) -> HttpResponse:
+        message_id = 1
 
+        result = self.api_post(
+            self.example_user('AARON'),
+            f'/api/v1/messages/{message_id}/reactions',
+            reaction_info
+        )
         return result
 
-    def delete_reaction(self, reaction_info: Dict[str, str], message_id: int=1,
-                        sender: str='hamlet') -> HttpResponse:
-        if 'reaction_type' not in reaction_info:
-            reaction_info['reaction_type'] = self.reaction_type
-        sender = self.example_user(sender)
-        result = self.api_delete(sender, f'/api/v1/messages/{message_id}/reactions',
-                                 reaction_info)
-        return result
+    def delete_reaction(self, reaction_info: Dict[str, str]) -> HttpResponse:
+        message_id = 1
 
-    def delete_zulip_reaction(self, message_id: int=1, sender: str='hamlet') -> HttpResponse:
-        reaction_info = {
-            'emoji_name': 'zulip',
-            'emoji_code': 'zulip',
-            'reaction_type': 'zulip_extra_emoji',
-        }
-        result = self.delete_reaction(reaction_info, message_id, sender)
+        result = self.api_delete(
+            self.example_user('hamlet'),
+            f'/api/v1/messages/{message_id}/reactions',
+            reaction_info
+        )
         return result
 
     def get_message_reactions(self, message_id: int, emoji_code: str,
@@ -531,8 +523,8 @@ class EmojiReactionBase(ZulipTestCase):
 class DefaultEmojiReactionTests(EmojiReactionBase):
     def setUp(self) -> None:
         super().setUp()
-        self.reaction_type = 'unicode_emoji'
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': 'hamburger',
             'emoji_code': '1f354',
         }
@@ -541,6 +533,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
 
     def test_add_default_emoji_reaction(self) -> None:
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': 'thumbs_up',
             'emoji_code': '1f44d',
         }
@@ -549,6 +542,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
 
     def test_add_default_emoji_invalid_code(self) -> None:
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': 'hamburger',
             'emoji_code': 'TBD',
         }
@@ -557,6 +551,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
 
     def test_add_default_emoji_invalid_name(self) -> None:
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': 'non-existent',
             'emoji_code': '1f44d',
         }
@@ -573,10 +568,11 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
                                            reaction_type='unicode_emoji',
                                            )
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': 'smiley',
             'emoji_code': '1f603',
         }
-        result = self.post_reaction(reaction_info, sender='AARON')
+        result = self.post_other_reaction(reaction_info)
         self.assert_json_success(result)
 
         reactions = self.get_message_reactions(1, '1f603', 'unicode_emoji')
@@ -585,6 +581,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
 
     def test_add_duplicate_reaction(self) -> None:
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': 'non-existent',
             'emoji_code': '1f354',
         }
@@ -593,6 +590,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
 
     def test_add_reaction_by_name(self) -> None:
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': '+1',
         }
         result = self.post_reaction(reaction_info)
@@ -609,6 +607,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
 
     def test_preserve_non_canonical_name(self) -> None:
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': '+1',
             'emoji_code': '1f44d',
         }
@@ -621,6 +620,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
 
     def test_reaction_name_collapse(self) -> None:
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': '+1',
             'emoji_code': '1f44d',
         }
@@ -628,7 +628,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
         self.assert_json_success(result)
 
         reaction_info['emoji_name'] = 'thumbs_up'
-        result = self.post_reaction(reaction_info, sender='AARON')
+        result = self.post_other_reaction(reaction_info)
         self.assert_json_success(result)
 
         reactions = self.get_message_reactions(1, '1f44d', 'unicode_emoji')
@@ -637,6 +637,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
 
     def test_delete_default_emoji_reaction(self) -> None:
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': 'hamburger',
             'emoji_code': '1f354',
         }
@@ -651,6 +652,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
 
     def test_delete_non_existing_emoji_reaction(self) -> None:
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': 'thumbs_up',
             'emoji_code': '1f44d',
         }
@@ -668,6 +670,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
                                 )
 
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': 'new_name',
             'emoji_code': '1f44f',
         }
@@ -685,6 +688,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
                                 )
 
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': '+1',
         }
         result = self.delete_reaction(reaction_info)
@@ -713,10 +717,16 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
 
         # Have hamlet react to the message
         reaction_info = {
+            'reaction_type': 'unicode_emoji',
             'emoji_name': 'hamburger',
             'emoji_code': '1f354',
         }
-        result = self.post_reaction(reaction_info, message_id=message_id)
+
+        result = self.api_post(
+            user_profile,
+            f'/api/v1/messages/{message_id}/reactions',
+            reaction_info
+        )
         self.assert_json_success(result)
 
         # Fetch the now-created UserMessage object to confirm it exists and is historical
@@ -727,14 +737,14 @@ class DefaultEmojiReactionTests(EmojiReactionBase):
 
 class ZulipExtraEmojiReactionTest(EmojiReactionBase):
     def test_add_zulip_emoji_reaction(self) -> None:
-        result = self.post_zulip_reaction()
+        result = self.post_reaction(zulip_reaction_info())
         self.assert_json_success(result)
 
     def test_add_duplicate_zulip_reaction(self) -> None:
-        result = self.post_zulip_reaction()
+        result = self.post_reaction(zulip_reaction_info())
         self.assert_json_success(result)
 
-        result = self.post_zulip_reaction()
+        result = self.post_reaction(zulip_reaction_info())
         self.assert_json_error(result, 'Reaction already exists.')
 
     def test_add_invalid_extra_emoji(self) -> None:
@@ -756,14 +766,14 @@ class ZulipExtraEmojiReactionTest(EmojiReactionBase):
         self.assert_json_error(result, 'Invalid emoji name.')
 
     def test_delete_zulip_emoji(self) -> None:
-        result = self.post_zulip_reaction()
+        result = self.post_reaction(zulip_reaction_info())
         self.assert_json_success(result)
 
-        result = self.delete_zulip_reaction()
+        result = self.delete_reaction(zulip_reaction_info())
         self.assert_json_success(result)
 
     def test_delete_non_existent_zulip_reaction(self) -> None:
-        result = self.delete_zulip_reaction()
+        result = self.delete_reaction(zulip_reaction_info())
         self.assert_json_error(result, "Reaction doesn't exist.")
 
 class RealmEmojiReactionTests(EmojiReactionBase):
@@ -771,6 +781,7 @@ class RealmEmojiReactionTests(EmojiReactionBase):
         super().setUp()
         green_tick_emoji = RealmEmoji.objects.get(name="green_tick")
         self.default_reaction_info = {
+            'reaction_type': 'realm_emoji',
             'emoji_name': 'green_tick',
             'emoji_code': str(green_tick_emoji.id),
         }
@@ -781,6 +792,7 @@ class RealmEmojiReactionTests(EmojiReactionBase):
 
     def test_add_realm_emoji_invalid_code(self) -> None:
         reaction_info = {
+            'reaction_type': 'realm_emoji',
             'emoji_name': 'green_tick',
             'emoji_code': '9999',
         }
@@ -790,6 +802,7 @@ class RealmEmojiReactionTests(EmojiReactionBase):
     def test_add_realm_emoji_invalid_name(self) -> None:
         green_tick_emoji = RealmEmoji.objects.get(name="green_tick")
         reaction_info = {
+            'reaction_type': 'realm_emoji',
             'emoji_name': 'bogus_name',
             'emoji_code': str(green_tick_emoji.id),
         }
@@ -812,7 +825,7 @@ class RealmEmojiReactionTests(EmojiReactionBase):
         emoji.deactivated = True
         emoji.save(update_fields=['deactivated'])
 
-        result = self.post_reaction(self.default_reaction_info, sender='AARON')
+        result = self.post_other_reaction(self.default_reaction_info)
         self.assert_json_success(result)
 
         reactions = self.get_message_reactions(1,
@@ -840,6 +853,7 @@ class RealmEmojiReactionTests(EmojiReactionBase):
 
     def test_remove_non_existent_realm_emoji_reaction(self) -> None:
         reaction_info = {
+            'reaction_type': 'realm_emoji',
             'emoji_name': 'non_existent',
             'emoji_code': 'TBD',
         }
@@ -876,10 +890,12 @@ class ReactionAPIEventTest(EmojiReactionBase):
         }
         events: List[Mapping[str, Any]] = []
         with tornado_redirected_to_list(events):
-            result = self.post_reaction(reaction_info,
-                                        message_id=pm_id,
-                                        sender=reaction_sender.short_name)
-        self.assert_json_success(result)
+            self.api_post(
+                reaction_sender,
+                f'/api/v1/messages/{pm_id}/reactions',
+                reaction_info
+            )
+
         self.assertEqual(len(events), 1)
 
         event = events[0]['event']
@@ -911,14 +927,21 @@ class ReactionAPIEventTest(EmojiReactionBase):
             'emoji_code': '1f354',
             'reaction_type': 'unicode_emoji',
         }
-        add = self.post_reaction(reaction_info, message_id=pm_id, sender=reaction_sender.short_name)
+        add = self.api_post(
+            reaction_sender,
+            f'/api/v1/messages/{pm_id}/reactions',
+            reaction_info,
+        )
         self.assert_json_success(add)
 
         events: List[Mapping[str, Any]] = []
         with tornado_redirected_to_list(events):
-            result = self.delete_reaction(reaction_info,
-                                          message_id=pm_id,
-                                          sender=reaction_sender.short_name)
+            result = self.api_delete(
+                reaction_sender,
+                f'/api/v1/messages/{pm_id}/reactions',
+                reaction_info,
+            )
+
         self.assert_json_success(result)
         self.assertEqual(len(events), 1)