emoji: Migrate realm emoji to be addressed by id rather than name.

This commit migrates realm emoji to be addressed by their `id` rather
than their name. This fixes a long standing issue which was causing
an error on uploading an emoji with same name as a deactivated realm
emoji.

Fixes: #6977.

frontend_tests/casper_tests/10-admin.js

@@ -172,7 +172,7 @@ casper.then(function () {
 casper.then(function () {
     casper.waitUntilVisible('tr#emoji_mouseface', function () {
         casper.test.assertSelectorHasText('tr#emoji_mouseface .emoji_name', 'mouseface');
-        casper.test.assertExists('.emoji_row img[src="/user_avatars/1/emoji/mouseface.png"]');
+        casper.test.assertExists('tr#emoji_mouseface img');
         casper.click('tr#emoji_mouseface button.delete');
     });
 });

frontend_tests/node_tests/markdown.js

@@ -24,8 +24,11 @@ set_global('window', {
 set_global('page_params', {
     realm_users: [],
     realm_emoji: {
-        burrito: {display_url: '/static/generated/emoji/images/emoji/burrito.png',
-                  source_url: '/static/generated/emoji/images/emoji/burrito.png'},
+        1: {id: 1,
+            name: 'burrito',
+            source_url: '/static/generated/emoji/images/emoji/burrito.png',
+            deactivated: false,
+        },
     },
     realm_filters: [
         [

frontend_tests/node_tests/reactions.js

@@ -10,13 +10,13 @@ set_global('emoji', {
         smile: '1f604',
     },
     all_realm_emojis: {
-        realm_emoji: {
+        991: {
             id: '991',
             emoji_name: 'realm_emoji',
             emoji_url: 'TBD',
             deactivated: false,
         },
-        inactive_realm_emoji: {
+        992: {
             id: '992',
             emoji_name: 'inactive_realm_emoji',
             emoji_url: 'TBD',

static/js/emoji.js

@@ -31,28 +31,28 @@ var EMOTICON_CONVERSIONS = {
 };
 
 exports.update_emojis = function update_emojis(realm_emojis) {
-    // exports.all_realm_emojis is emptied before adding the realm-specific emoji to it.
-    // This makes sure that in case of deletion, the deleted realm_emojis don't
-    // persist in exports.all_realm_emojis or exports.active_realm_emojis.
+    // exports.all_realm_emojis is emptied before adding the realm-specific emoji
+    // to it. This makes sure that in case of deletion, the deleted realm_emojis
+    // don't persist in exports.active_realm_emojis.
     exports.all_realm_emojis = {};
     exports.active_realm_emojis = {};
 
     // Copy the default emoji list and add realm-specific emoji to it
     exports.emojis = default_emojis.slice(0);
-    _.each(realm_emojis, function (data, name) {
-        exports.all_realm_emojis[name] = {id: data.id,
-                                          emoji_name: name,
-                                          emoji_url: data.source_url,
-                                          deactivated: data.deactivated};
+    _.each(realm_emojis, function (data) {
+        exports.all_realm_emojis[data.id] = {id: data.id,
+                                             emoji_name: data.name,
+                                             emoji_url: data.source_url,
+                                             deactivated: data.deactivated};
         if (data.deactivated !== true) {
             // export.emojis are used in composebox autocomplete. This condition makes sure
             // that deactivated emojis don't appear in the autocomplete.
-            exports.emojis.push({emoji_name: name,
+            exports.emojis.push({emoji_name: data.name,
                                  emoji_url: data.source_url,
                                  is_realm_emoji: true});
-            exports.active_realm_emojis[name] = {id: data.id,
-                                                 emoji_name: name,
-                                                 emoji_url: data.source_url};
+            exports.active_realm_emojis[data.name] = {id: data.id,
+                                                      emoji_name: data.name,
+                                                      emoji_url: data.source_url};
         }
     });
     // Add the Zulip emoji to the realm emojis list

static/js/hotkey.js

@@ -709,7 +709,12 @@ exports.process_hotkey = function (e, hotkey) {
             // Use canonical name.
             var thumbs_up_codepoint = '1f44d';
             var canonical_name = emoji_codes.codepoint_to_name[thumbs_up_codepoint];
-            reactions.toggle_emoji_reaction(msg.id, canonical_name);
+            var reaction_info = {
+                emoji_name: canonical_name,
+                emoji_code: thumbs_up_codepoint,
+                reaction_type: 'unicode_emoji',
+            };
+            reactions.toggle_emoji_reaction(msg.id, reaction_info);
             return true;
         case 'toggle_mute':
             muting_ui.toggle_mute(msg);

static/js/reactions.js

@@ -108,12 +108,16 @@ function get_user_list_for_message_reaction(message, local_id) {
 }
 
 exports.toggle_emoji_reaction = function (message_id, emoji_name) {
-    // This toggles the current user's reaction to the clicked emoji.
+    // This codepath doesn't support toggling a deactivated realm emoji.
+    // Since an user can interact with a deactivated realm emoji only by
+    // clicking on a reaction and that is handled by `process_reaction_click()`
+    // method. This codepath is to be used only where there is no chance of an
+    // user interacting with a deactivated realm emoji like emoji picker.
     var reaction_info = {
         emoji_name: emoji_name,
     };
 
-    if (emoji.all_realm_emojis.hasOwnProperty(emoji_name)) {
+    if (emoji.active_realm_emojis.hasOwnProperty(emoji_name)) {
         if (emoji_name === 'zulip') {
             reaction_info.reaction_type = 'zulip_extra_emoji';
         } else {
@@ -270,7 +274,7 @@ exports.view.insert_new_reaction = function (opts) {
 
     if (opts.reaction_type !== 'unicode_emoji') {
         context.is_realm_emoji = true;
-        context.url = emoji.all_realm_emojis[emoji_name].emoji_url;
+        context.url = emoji.all_realm_emojis[emoji_code].emoji_url;
     }
 
     context.count = 1;
@@ -410,7 +414,7 @@ exports.get_message_reactions = function (message) {
 
         if (reaction.reaction_type !== 'unicode_emoji') {
             reaction.is_realm_emoji = true;
-            reaction.url = emoji.all_realm_emojis[reaction.emoji_name].emoji_url;
+            reaction.url = emoji.all_realm_emojis[reaction.emoji_code].emoji_url;
         }
         if (reaction.user_ids.indexOf(page_params.user_id) !== -1) {
             reaction.class = "message_reaction reacted";

static/js/settings_emoji.js

@@ -47,11 +47,11 @@ exports.populate_emoji = function (emoji_data) {
 
     var emoji_table = $('#admin_emoji_table').expectOne();
     emoji_table.find('tr.emoji_row').remove();
-    _.each(emoji_data, function (data, name) {
+    _.each(emoji_data, function (data) {
         if (data.deactivated !== true) {
             emoji_table.append(templates.render('admin_emoji_list', {
                 emoji: {
-                    name: name, source_url: data.source_url,
+                    name: data.name, source_url: data.source_url,
                     display_url: data.source_url,
                     author: data.author || '',
                     can_admin_emoji: can_admin_emoji(data),

zerver/lib/actions.py

@@ -11,6 +11,7 @@ from django.utils.html import escape
 from django.utils.translation import ugettext as _
 from django.conf import settings
 from django.core import validators
+from django.core.files import File
 from analytics.lib.counts import COUNT_STATS, do_increment_logging_stat, \
     RealmCount
 
@@ -34,7 +35,7 @@ from zerver.lib.cache import (
     to_dict_cache_key_id,
 )
 from zerver.lib.context_managers import lockfile
-from zerver.lib.emoji import emoji_name_to_emoji_code
+from zerver.lib.emoji import emoji_name_to_emoji_code, get_emoji_file_name
 from zerver.lib.hotspots import get_next_hotspots
 from zerver.lib.message import (
     access_message,
@@ -117,7 +118,7 @@ from zerver.lib.narrow import check_supported_events_narrow_filter
 from zerver.lib.exceptions import JsonableError, ErrorCode
 from zerver.lib.sessions import delete_user_sessions
 from zerver.lib.upload import attachment_url_re, attachment_url_to_path_id, \
-    claim_attachment, delete_message_image
+    claim_attachment, delete_message_image, upload_emoji_image
 from zerver.lib.str_utils import NonBinaryStr, force_str
 from zerver.tornado.event_queue import request_event_queue, send_event
 
@@ -4269,15 +4270,29 @@ def notify_realm_emoji(realm: Realm) -> None:
 
 def check_add_realm_emoji(realm: Realm,
                           name: Text,
-                          file_name: Text,
-                          author: Optional[UserProfile]=None) -> None:
-    emoji = RealmEmoji(realm=realm, name=name, file_name=file_name, author=author)
-    emoji.full_clean()
-    emoji.save()
-    notify_realm_emoji(realm)
+                          author: UserProfile,
+                          image_file: File) -> Optional[RealmEmoji]:
+    realm_emoji = RealmEmoji(realm=realm, name=name, author=author)
+    realm_emoji.full_clean()
+    realm_emoji.save()
+
+    emoji_file_name = get_emoji_file_name(image_file.name, realm_emoji.id)
+    emoji_uploaded_successfully = False
+    try:
+        upload_emoji_image(image_file, emoji_file_name, author)
+        emoji_uploaded_successfully = True
+    finally:
+        if not emoji_uploaded_successfully:
+            realm_emoji.delete()
+            return None
+        else:
+            realm_emoji.file_name = emoji_file_name
+            realm_emoji.save(update_fields=['file_name'])
+            notify_realm_emoji(realm_emoji.realm)
+    return realm_emoji
 
 def do_remove_realm_emoji(realm: Realm, name: Text) -> None:
-    emoji = RealmEmoji.objects.get(realm=realm, name=name)
+    emoji = RealmEmoji.objects.get(realm=realm, name=name, deactivated=False)
     emoji.deactivated = True
     emoji.save(update_fields=['deactivated'])
     notify_realm_emoji(realm)

zerver/lib/emoji.py

@@ -65,11 +65,11 @@ def check_emoji_request(realm: Realm, emoji_name: str, emoji_code: str,
     # code is valid for new reactions, or not.
     if emoji_type == "realm_emoji":
         realm_emojis = realm.get_emoji()
-        realm_emoji = realm_emojis.get(emoji_name)
+        realm_emoji = realm_emojis.get(emoji_code)
         if realm_emoji is None:
             raise JsonableError(_("Invalid custom emoji."))
-        if realm_emoji["id"] != emoji_code:
-            raise JsonableError(_("Invalid custom emoji id."))
+        if realm_emoji["name"] != emoji_name:
+            raise JsonableError(_("Invalid custom emoji name."))
         if realm_emoji["deactivated"]:
             raise JsonableError(_("This custom emoji has been deactivated."))
     elif emoji_type == "zulip_extra_emoji":
@@ -117,6 +117,6 @@ def get_emoji_url(emoji_file_name: Text, realm_id: int) -> Text:
     return upload_backend.get_emoji_url(emoji_file_name, realm_id)
 
 
-def get_emoji_file_name(emoji_file_name: Text, emoji_name: Text) -> Text:
+def get_emoji_file_name(emoji_file_name: Text, emoji_id: int) -> Text:
     _, image_ext = os.path.splitext(emoji_file_name)
-    return ''.join((emoji_name, image_ext))
+    return ''.join((str(emoji_id), image_ext))

zerver/migrations/0149_realm_emoji_drop_unique_constraint.py

@@ -0,0 +1,111 @@
+# -*- coding: utf-8 -*-
+import os
+import shutil
+import tempfile
+
+from boto.s3.connection import S3Connection
+from django.conf import settings
+from django.db import migrations, models
+from django.db.backends.postgresql_psycopg2.schema import DatabaseSchemaEditor
+from django.db.migrations.state import StateApps
+
+class Uploader:
+    def __init__(self) -> None:
+        self.old_orig_image_path_template = "{realm_id}/emoji/{emoji_file_name}.original"
+        self.old_path_template = "{realm_id}/emoji/{emoji_file_name}"
+        self.new_orig_image_path_template = "{realm_id}/emoji/images/{emoji_file_name}.original"
+        self.new_path_template = "{realm_id}/emoji/images/{emoji_file_name}"
+
+    def copy_files(self, src_path: str, dst_path: str) -> None:
+        raise NotImplementedError()
+
+    def ensure_emoji_images(self, realm_id: int, old_filename: str, new_filename: str) -> None:
+        # Copy original image file.
+        old_file_path = self.old_orig_image_path_template.format(realm_id=realm_id,
+                                                                 emoji_file_name=old_filename)
+        new_file_path = self.new_orig_image_path_template.format(realm_id=realm_id,
+                                                                 emoji_file_name=new_filename)
+        self.copy_files(old_file_path, new_file_path)
+
+        # Copy resized image file.
+        old_file_path = self.old_path_template.format(realm_id=realm_id,
+                                                      emoji_file_name=old_filename)
+        new_file_path = self.new_path_template.format(realm_id=realm_id,
+                                                      emoji_file_name=new_filename)
+        self.copy_files(old_file_path, new_file_path)
+
+class LocalUploader(Uploader):
+    def __init__(self) -> None:
+        super().__init__()
+
+    @staticmethod
+    def mkdirs(path: str) -> None:
+        dirname = os.path.dirname(path)
+        if not os.path.isdir(dirname):
+            os.makedirs(dirname)
+
+    def copy_files(self, src_path: str, dst_path: str) -> None:
+        src_path = os.path.join(settings.LOCAL_UPLOADS_DIR, 'avatars', src_path)
+        self.mkdirs(src_path)
+        dst_path = os.path.join(settings.LOCAL_UPLOADS_DIR, 'avatars', dst_path)
+        self.mkdirs(dst_path)
+        shutil.copyfile(src_path, dst_path)
+
+class S3Uploader(Uploader):
+    def __init__(self) -> None:
+        super().__init__()
+        conn = S3Connection(settings.S3_KEY, settings.S3_SECRET_KEY)
+        bucket_name = settings.S3_AVATAR_BUCKET
+        self.bucket = conn.get_bucket(bucket_name, validate=False)
+
+    def copy_files(self, src_key: str, dst_key: str) -> None:
+        self.bucket.copy_key(src_key, self.bucket, dst_key)
+
+def get_uploader() -> Uploader:
+    if settings.LOCAL_UPLOADS_DIR is None:
+        return S3Uploader()
+    return LocalUploader()
+
+def get_emoji_file_name(emoji_file_name: str, new_name: str) -> str:
+    _, image_ext = os.path.splitext(emoji_file_name)
+    return ''.join((new_name, image_ext))
+
+def migrate_realm_emoji_image_files(apps: StateApps, schema_editor: DatabaseSchemaEditor) -> None:
+    RealmEmoji = apps.get_model('zerver', 'RealmEmoji')
+    uploader = get_uploader()
+    for realm_emoji in RealmEmoji.objects.all():
+        old_file_name = realm_emoji.file_name
+        new_file_name = get_emoji_file_name(old_file_name, str(realm_emoji.id))
+        uploader.ensure_emoji_images(realm_emoji.realm_id, old_file_name, new_file_name)
+        realm_emoji.file_name = new_file_name
+        realm_emoji.save(update_fields=['file_name'])
+
+def reversal(apps: StateApps, schema_editor: DatabaseSchemaEditor) -> None:
+    # Ensures that migration can be re-run in case of a failure.
+    RealmEmoji = apps.get_model('zerver', 'RealmEmoji')
+    for realm_emoji in RealmEmoji.objects.all():
+        corrupt_file_name = realm_emoji.file_name
+        correct_file_name = get_emoji_file_name(corrupt_file_name, realm_emoji.name)
+        realm_emoji.file_name = correct_file_name
+        realm_emoji.save(update_fields=['file_name'])
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('zerver', '0148_max_invites_forget_default'),
+    ]
+
+    operations = [
+        migrations.AlterUniqueTogether(
+            name='realmemoji',
+            unique_together=set([]),
+        ),
+        migrations.AlterField(
+            model_name='realmemoji',
+            name='file_name',
+            field=models.TextField(db_index=True, null=True, blank=True),
+        ),
+        migrations.RunPython(
+            migrate_realm_emoji_image_files,
+            reverse_code=reversal),
+    ]

zerver/models.py

@@ -418,13 +418,10 @@ class RealmEmoji(models.Model):
     name = models.TextField(validators=[MinLengthValidator(1),
                                         RegexValidator(regex=r'^[0-9a-z.\-_]+(?<![.\-_])$',
                                                        message=_("Invalid characters in emoji name"))])  # type: Text
-    file_name = models.TextField(db_index=True, null=True)  # type: Optional[Text]
+    file_name = models.TextField(db_index=True, null=True, blank=True)  # type: Optional[Text]
     deactivated = models.BooleanField(default=False)  # type: bool
 
-    PATH_ID_TEMPLATE = "{realm_id}/emoji/{emoji_file_name}"
-
-    class Meta:
-        unique_together = ("realm", "name")
+    PATH_ID_TEMPLATE = "{realm_id}/emoji/images/{emoji_file_name}"
 
     def __str__(self) -> Text:
         return "<RealmEmoji(%s): %s %s %s %s>" % (self.realm.string_id,
@@ -449,18 +446,22 @@ def get_realm_emoji_dicts(realm: Realm,
                 'email': realm_emoji.author.email,
                 'full_name': realm_emoji.author.full_name}
         emoji_url = get_emoji_url(realm_emoji.file_name, realm_emoji.realm_id)
-        d[realm_emoji.name] = dict(id=str(realm_emoji.id),
-                                   name=realm_emoji.name,
-                                   source_url=emoji_url,
-                                   deactivated=realm_emoji.deactivated,
-                                   author=author)
+        d[str(realm_emoji.id)] = dict(id=str(realm_emoji.id),
+                                      name=realm_emoji.name,
+                                      source_url=emoji_url,
+                                      deactivated=realm_emoji.deactivated,
+                                      author=author)
     return d
 
 def get_realm_emoji_uncached(realm: Realm) -> Dict[str, Dict[str, Any]]:
     return get_realm_emoji_dicts(realm)
 
 def get_active_realm_emoji_uncached(realm: Realm) -> Dict[str, Dict[str, Any]]:
-    return get_realm_emoji_dicts(realm, only_active_emojis=True)
+    realm_emojis = get_realm_emoji_dicts(realm, only_active_emojis=True)
+    d = {}
+    for emoji_id, emoji_dict in realm_emojis.items():
+        d[emoji_dict['name']] = emoji_dict
+    return d
 
 def flush_realm_emoji(sender: Any, **kwargs: Any) -> None:
     realm = kwargs['instance'].realm

zerver/tests/test_bugdown.py

@@ -37,6 +37,7 @@ from zerver.models import (
     Message,
     Stream,
     Realm,
+    RealmEmoji,
     RealmFilter,
     Recipient,
     UserProfile,
@@ -600,7 +601,10 @@ class BugdownTest(ZulipTestCase):
         # Needs to mock an actual message because that's how bugdown obtains the realm
         msg = Message(sender=self.example_user('hamlet'))
         converted = bugdown.convert(":green_tick:", message_realm=realm, message=msg)
-        self.assertEqual(converted, '<p>%s</p>' % (emoji_img(':green_tick:', 'green_tick.png', realm.id)))
+        realm_emoji = RealmEmoji.objects.filter(realm=realm,
+                                                name='green_tick',
+                                                deactivated=False).get()
+        self.assertEqual(converted, '<p>%s</p>' % (emoji_img(':green_tick:', realm_emoji.file_name, realm.id)))
 
         # Deactivate realm emoji.
         do_remove_realm_emoji(realm, 'green_tick')

zerver/tests/test_events.py

@@ -97,7 +97,7 @@ from zerver.lib.message import (
     UnreadMessagesResult,
 )
 from zerver.lib.test_helpers import POSTRequestMock, get_subscription, \
-    stub_event_queue_user_events, queries_captured
+    get_test_image_file, stub_event_queue_user_events, queries_captured
 from zerver.lib.test_classes import (
     ZulipTestCase,
 )
@@ -1488,8 +1488,12 @@ class EventsRegisterTest(ZulipTestCase):
             ('op', equals('update')),
             ('realm_emoji', check_dict([])),
         ])
-        events = self.do_test(lambda: check_add_realm_emoji(get_realm("zulip"), "my_emoji",
-                                                            "https://realm.com/my_emoji"))
+        author = self.example_user('iago')
+        with get_test_image_file('img.png') as img_file:
+            events = self.do_test(lambda: check_add_realm_emoji(get_realm("zulip"),
+                                                                "my_emoji",
+                                                                author,
+                                                                img_file))
         error = schema_checker('events[0]', events[0])
         self.assert_on_error(error)
 

zerver/tests/test_notifications.py

@@ -422,7 +422,9 @@ class TestMissedMessages(ZulipTestCase):
         msg_id = self.send_personal_message(
             self.example_email('othello'), self.example_email('hamlet'),
             'Extremely personal message with a realm emoji :green_tick:!')
-        body = '<img alt=":green_tick:" src="http://zulip.testserver/user_avatars/1/emoji/green_tick.png" title="green tick" style="height: 20px;">'
+        realm_emoji_id = get_realm('zulip').get_active_emoji()['green_tick']['id']
+        realm_emoji_url = "http://zulip.testserver/user_avatars/1/emoji/images/%s.png" % (realm_emoji_id,)
+        body = '<img alt=":green_tick:" src="%s" title="green tick" style="height: 20px;">' % (realm_emoji_url,)
         subject = 'Othello, the Moor of Venice sent you a message'
         self._test_cases(tokens, msg_id, body, subject, send_as_user=False, verify_html_body=True)
 

zerver/tests/test_reactions.py

@@ -590,15 +590,16 @@ class RealmEmojiReactionTests(EmojiReactionBase):
             'emoji_code': '9999',
         }
         result = self.post_reaction(reaction_info)
-        self.assert_json_error(result, 'Invalid custom emoji id.')
+        self.assert_json_error(result, 'Invalid custom emoji.')
 
     def test_add_realm_emoji_invalid_name(self) -> None:
+        green_tick_emoji = RealmEmoji.objects.get(name="green_tick")
         reaction_info = {
             'emoji_name': 'bogus_name',
-            'emoji_code': '1',
+            'emoji_code': str(green_tick_emoji.id),
         }
         result = self.post_reaction(reaction_info)
-        self.assert_json_error(result, 'Invalid custom emoji.')
+        self.assert_json_error(result, 'Invalid custom emoji name.')
 
     def test_add_deactivated_realm_emoji(self) -> None:
         emoji = RealmEmoji.objects.get(name="green_tick")

zerver/tests/test_realm_emoji.py

@@ -1,17 +1,33 @@
 # -*- coding: utf-8 -*-
 
+import mock
+
 from zerver.lib.actions import get_realm, check_add_realm_emoji
 from zerver.lib.test_classes import ZulipTestCase
 from zerver.lib.test_helpers import get_test_image_file, get_user
-from zerver.models import RealmEmoji
+from zerver.models import Realm, RealmEmoji, UserProfile
 
 class RealmEmojiTest(ZulipTestCase):
 
+    def create_test_emoji(self, name: str, author: UserProfile) -> RealmEmoji:
+        with get_test_image_file('img.png') as img_file:
+            realm_emoji = check_add_realm_emoji(realm=author.realm,
+                                                name=name,
+                                                author=author,
+                                                image_file=img_file)
+            if realm_emoji is None:
+                raise Exception("Error creating test emoji.")   # nocoverage
+        return realm_emoji
+
+    def create_test_emoji_with_no_author(self, name: str, realm: Realm) -> RealmEmoji:
+        realm_emoji = RealmEmoji.objects.create(realm=realm, name=name)
+        return realm_emoji
+
     def test_list(self) -> None:
-        email = self.example_email('iago')
-        self.login(email)
-        realm = get_realm('zulip')
-        check_add_realm_emoji(realm, "my_emoji", "my_emoji")
+        emoji_author = self.example_user('iago')
+        self.login(emoji_author.email)
+        self.create_test_emoji('my_emoji', emoji_author)
+
         result = self.client_get("/json/realm/emoji")
         self.assert_json_success(result)
         self.assertEqual(200, result.status_code)
@@ -21,25 +37,31 @@ class RealmEmojiTest(ZulipTestCase):
         email = self.example_email('iago')
         self.login(email)
         realm = get_realm('zulip')
-        RealmEmoji.objects.create(realm=realm, name='my_emoji', file_name='my_emoji')
+        realm_emoji = self.create_test_emoji_with_no_author('my_emoji', realm)
+
         result = self.client_get("/json/realm/emoji")
         self.assert_json_success(result)
         content = result.json()
         self.assertEqual(len(content["emoji"]), 2)
-        self.assertIsNone(content["emoji"]['my_emoji']['author'])
+        test_emoji = content["emoji"][str(realm_emoji.id)]
+        self.assertIsNone(test_emoji['author'])
 
     def test_list_admins_only(self) -> None:
+        # Test that realm emoji list is public and realm emojis
+        # having no author are also there in the list.
         email = self.example_email('othello')
         self.login(email)
         realm = get_realm('zulip')
         realm.add_emoji_by_admins_only = True
         realm.save()
-        check_add_realm_emoji(realm, 'my_emoji', 'my_emoji')
+        realm_emoji = self.create_test_emoji_with_no_author('my_emoji', realm)
+
         result = self.client_get("/json/realm/emoji")
         self.assert_json_success(result)
         content = result.json()
         self.assertEqual(len(content["emoji"]), 2)
-        self.assertIsNone(content["emoji"]['my_emoji']['author'])
+        test_emoji = content["emoji"][str(realm_emoji.id)]
+        self.assertIsNone(test_emoji['author'])
 
     def test_upload(self) -> None:
         email = self.example_email('iago')
@@ -49,22 +71,23 @@ class RealmEmojiTest(ZulipTestCase):
             result = self.client_post('/json/realm/emoji/my_emoji', info=emoji_data)
         self.assert_json_success(result)
         self.assertEqual(200, result.status_code)
-        emoji = RealmEmoji.objects.get(name="my_emoji")
-        self.assertEqual(emoji.author.email, email)
+        realm_emoji = RealmEmoji.objects.get(name="my_emoji")
+        self.assertEqual(realm_emoji.author.email, email)
 
         result = self.client_get("/json/realm/emoji")
         content = result.json()
         self.assert_json_success(result)
         self.assertEqual(len(content["emoji"]), 2)
-        self.assertIn('author', content["emoji"]['my_emoji'])
-        self.assertEqual(
-            content["emoji"]['my_emoji']['author']['email'], email)
+        test_emoji = content["emoji"][str(realm_emoji.id)]
+        self.assertIn('author', test_emoji)
+        self.assertEqual(test_emoji['author']['email'], email)
 
-        realm_emoji = RealmEmoji.objects.get(name='my_emoji')
-        file_name = realm_emoji.name + '.png'
+    def test_realm_emoji_repr(self) -> None:
+        realm_emoji = RealmEmoji.objects.get(name='green_tick')
+        file_name = str(realm_emoji.id) + '.png'
         self.assertEqual(
             str(realm_emoji),
-            '<RealmEmoji(zulip): %s my_emoji False %s>' % (realm_emoji.id, file_name)
+            '<RealmEmoji(zulip): %s green_tick False %s>' % (realm_emoji.id, file_name)
         )
 
     def test_upload_exception(self) -> None:
@@ -106,11 +129,10 @@ class RealmEmojiTest(ZulipTestCase):
         self.assert_json_success(result)
 
     def test_delete(self) -> None:
-        email = self.example_email('iago')
-        self.login(email)
-        realm = get_realm('zulip')
-        check_add_realm_emoji(realm, "my_emoji", "my_emoji.png")
-        result = self.client_delete("/json/realm/emoji/my_emoji")
+        emoji_author = self.example_user('iago')
+        self.login(emoji_author.email)
+        realm_emoji = self.create_test_emoji('my_emoji', emoji_author)
+        result = self.client_delete('/json/realm/emoji/my_emoji')
         self.assert_json_success(result)
 
         result = self.client_get("/json/realm/emoji")
@@ -119,39 +141,49 @@ class RealmEmojiTest(ZulipTestCase):
         # We only mark an emoji as deactivated instead of
         # removing it from the database.
         self.assertEqual(len(emojis), 2)
-        self.assertEqual(emojis["my_emoji"]["deactivated"], True)
+        test_emoji = emojis[str(realm_emoji.id)]
+        self.assertEqual(test_emoji["deactivated"], True)
+
+    def test_delete_no_author(self) -> None:
+        email = self.example_email('iago')
+        self.login(email)
+        realm = get_realm('zulip')
+        self.create_test_emoji_with_no_author('my_emoji', realm)
+        result = self.client_delete('/json/realm/emoji/my_emoji')
+        self.assert_json_success(result)
 
     def test_delete_admins_only(self) -> None:
-        email = self.example_email('othello')
-        self.login(email)
+        emoji_author = self.example_user('othello')
+        self.login(emoji_author.email)
         realm = get_realm('zulip')
         realm.add_emoji_by_admins_only = True
         realm.save()
-        check_add_realm_emoji(realm, "my_emoji", "my_emoji.png")
+        self.create_test_emoji_with_no_author("my_emoji", realm)
         result = self.client_delete("/json/realm/emoji/my_emoji")
         self.assert_json_error(result, 'Must be an organization administrator')
 
     def test_delete_admin_or_author(self) -> None:
         # If any user in a realm can upload the emoji then the user who
         # uploaded it as well as the admin should be able to delete it.
+        emoji_author = self.example_user('othello')
         realm = get_realm('zulip')
-        author = get_user('othello@zulip.com', realm)
         realm.add_emoji_by_admins_only = False
         realm.save()
-        check_add_realm_emoji(realm, "my_emoji_1", "my_emoji.png", author)
-        self.login('othello@zulip.com')
+
+        self.create_test_emoji('my_emoji_1', emoji_author)
+        self.login(emoji_author.email)
         result = self.client_delete("/json/realm/emoji/my_emoji_1")
         self.assert_json_success(result)
         self.logout()
 
-        check_add_realm_emoji(realm, "my_emoji_2", "my_emoji.png", author)
-        self.login('iago@zulip.com')
+        self.create_test_emoji('my_emoji_2', emoji_author)
+        self.login(self.example_email('iago'))
         result = self.client_delete("/json/realm/emoji/my_emoji_2")
         self.assert_json_success(result)
         self.logout()
 
-        check_add_realm_emoji(realm, "my_emoji_3", "my_emoji.png", author)
-        self.login('cordelia@zulip.com')
+        self.create_test_emoji('my_emoji_3', emoji_author)
+        self.login(self.example_email('cordelia'))
         result = self.client_delete("/json/realm/emoji/my_emoji_3")
         self.assert_json_error(result, 'Must be an organization administrator or emoji author')
 
@@ -182,4 +214,35 @@ class RealmEmojiTest(ZulipTestCase):
         with get_test_image_file('img.png') as fp1:
             emoji_data = {'f1': fp1}
             result = self.client_post('/json/realm/emoji/green_tick', info=emoji_data)
-        self.assert_json_error(result, 'Realm emoji with this Realm and Name already exists.')
+        self.assert_json_error(result, 'A custom emoji with this name already exists.')
+
+    def test_reupload(self) -> None:
+        # An user should be able to reupload an emoji with same name.
+        email = self.example_email('iago')
+        self.login(email)
+        with get_test_image_file('img.png') as fp1:
+            emoji_data = {'f1': fp1}
+            result = self.client_post('/json/realm/emoji/my_emoji', info=emoji_data)
+        self.assert_json_success(result)
+
+        result = self.client_delete("/json/realm/emoji/my_emoji")
+        self.assert_json_success(result)
+
+        with get_test_image_file('img.png') as fp1:
+            emoji_data = {'f1': fp1}
+            result = self.client_post('/json/realm/emoji/my_emoji', info=emoji_data)
+        self.assert_json_success(result)
+
+        result = self.client_get("/json/realm/emoji")
+        emojis = result.json()["emoji"]
+        self.assert_json_success(result)
+        self.assertEqual(len(emojis), 3)
+
+    def test_failed_file_upload(self) -> None:
+        email = self.example_email('iago')
+        self.login(email)
+        with mock.patch('zerver.lib.upload.write_local_file', side_effect=Exception()):
+            with get_test_image_file('img.png') as fp1:
+                emoji_data = {'f1': fp1}
+                result = self.client_post('/json/realm/emoji/my_emoji', info=emoji_data)
+        self.assert_json_error(result, "Image file upload failed.")

zerver/views/realm_emoji.py

@@ -5,10 +5,8 @@ from django.http import HttpRequest, HttpResponse
 from django.utils.translation import ugettext as _
 from typing import Text
 
-from zerver.lib.upload import upload_emoji_image
-from zerver.models import UserProfile
-from zerver.lib.emoji import check_emoji_admin, check_valid_emoji_name, check_valid_emoji, \
-    get_emoji_file_name
+from zerver.models import RealmEmoji, UserProfile
+from zerver.lib.emoji import check_emoji_admin, check_valid_emoji_name, check_valid_emoji
 from zerver.lib.request import JsonableError, REQ, has_request_variables
 from zerver.lib.response import json_success, json_error
 from zerver.lib.actions import check_add_realm_emoji, do_remove_realm_emoji
@@ -26,24 +24,32 @@ def upload_emoji(request: HttpRequest, user_profile: UserProfile,
                  emoji_name: Text=REQ()) -> HttpResponse:
     check_valid_emoji_name(emoji_name)
     check_emoji_admin(user_profile)
+    if RealmEmoji.objects.filter(realm=user_profile.realm,
+                                 name=emoji_name,
+                                 deactivated=False).exists():
+        return json_error(_("A custom emoji with this name already exists."))
     if len(request.FILES) != 1:
         return json_error(_("You must upload exactly one file."))
     emoji_file = list(request.FILES.values())[0]
     if (settings.MAX_EMOJI_FILE_SIZE * 1024 * 1024) < emoji_file.size:
         return json_error(_("Uploaded file is larger than the allowed limit of %s MB") % (
             settings.MAX_EMOJI_FILE_SIZE))
-    emoji_file_name = get_emoji_file_name(emoji_file.name, emoji_name)
-    upload_emoji_image(emoji_file, emoji_file_name, user_profile)
-    try:
-        check_add_realm_emoji(user_profile.realm, emoji_name, emoji_file_name, author=user_profile)
-    except ValidationError as e:
-        return json_error(e.messages[0])
+
+    realm_emoji = check_add_realm_emoji(user_profile.realm,
+                                        emoji_name,
+                                        user_profile,
+                                        emoji_file)
+    if realm_emoji is None:
+        return json_error(_("Image file upload failed."))
     return json_success()
 
 
 def delete_emoji(request: HttpRequest, user_profile: UserProfile,
-                 emoji_name: Text) -> HttpResponse:
-    check_valid_emoji(user_profile.realm, emoji_name)
+                 emoji_name: str) -> HttpResponse:
+    if not RealmEmoji.objects.filter(realm=user_profile.realm,
+                                     name=emoji_name,
+                                     deactivated=False).exists():
+        raise JsonableError(_("Emoji '%s' does not exist" % (emoji_name,)))
     check_emoji_admin(user_profile, emoji_name)
     do_remove_realm_emoji(user_profile.realm, emoji_name)
     return json_success()

zilencer/management/commands/populate_db.py

@@ -11,7 +11,7 @@ from django.db.models import F, Max
 from django.utils.timezone import now as timezone_now
 from django.utils.timezone import timedelta as timezone_timedelta
 
-from zerver.lib.actions import STREAM_ASSIGNMENT_COLORS, \
+from zerver.lib.actions import STREAM_ASSIGNMENT_COLORS, check_add_realm_emoji, \
     do_change_is_admin, do_send_messages, do_update_user_custom_profile_data, \
     try_add_realm_custom_profile_field
 from zerver.lib.bulk_create import bulk_create_streams, bulk_create_users
@@ -280,16 +280,8 @@ class Command(BaseCommand):
 
         # Create a test realm emoji.
         IMAGE_FILE_PATH = os.path.join(settings.STATIC_ROOT, 'images', 'test-images', 'checkbox.png')
-        UPLOADED_EMOJI_FILE_NAME = 'green_tick.png'
         with open(IMAGE_FILE_PATH, 'rb') as fp:
-            upload_backend.upload_emoji_image(fp, UPLOADED_EMOJI_FILE_NAME, iago)
-            RealmEmoji.objects.create(
-                name='green_tick',
-                author=iago,
-                realm=zulip_realm,
-                deactivated=False,
-                file_name=UPLOADED_EMOJI_FILE_NAME,
-            )
+            check_add_realm_emoji(zulip_realm, 'green_tick', iago, fp)
 
         if not options["test_suite"]:
             # Populate users with some bar data