Skip and warn on emoji import when name conflicts with system emoji (#19516)

This changes import behavior related to emoji imports when the name
conflicts with the name of a system emoji. Previously, the import
would fail, but now a warning is logged and the conflicting emoji
is skipped.

app/import_functions.go

@@ -1813,8 +1813,13 @@ func (a *App) importMultipleDirectPostLines(c *request.Context, lines []LineImpo
 }
 
 func (a *App) importEmoji(data *EmojiImportData, dryRun bool) *model.AppError {
-	if err := validateEmojiImportData(data); err != nil {
-		return err
+	aerr := validateEmojiImportData(data)
+	if aerr != nil {
+		if aerr.Id == "model.emoji.system_emoji_name.app_error" {
+			mlog.Warn("Skipping emoji import due to name conflict with system emoji", mlog.String("emoji_name", *data.Name))
+			return nil
+		}
+		return aerr
 	}
 
 	// If this is a Dry Run, do not continue any further.

app/import_functions_test.go

@@ -4075,6 +4075,10 @@ func TestImportImportEmoji(t *testing.T) {
 
 	err = th.App.importEmoji(&data, false)
 	assert.Nil(t, err, "Second run should have succeeded apply mode")
+
+	data = EmojiImportData{Name: ptrStr("smiley"), Image: ptrStr(testImage)}
+	err = th.App.importEmoji(&data, false)
+	assert.Nil(t, err, "System emoji should not fail")
 }
 
 func TestImportAttachment(t *testing.T) {

app/import_validators.go

@@ -560,6 +560,8 @@ func validateDirectPostImportData(data *DirectPostImportData, maxPostSize int) *
 	return nil
 }
 
+// validateEmojiImportData validates emoji data and returns if the import name
+// conflicts with a system emoji.
 func validateEmojiImportData(data *EmojiImportData) *model.AppError {
 	if data == nil {
 		return model.NewAppError("BulkImport", "app.import.validate_emoji_import_data.empty.error", nil, "", http.StatusBadRequest)
@@ -569,14 +571,14 @@ func validateEmojiImportData(data *EmojiImportData) *model.AppError {
 		return model.NewAppError("BulkImport", "app.import.validate_emoji_import_data.name_missing.error", nil, "", http.StatusBadRequest)
 	}
 
-	if err := model.IsValidEmojiName(*data.Name); err != nil {
-		return err
-	}
-
 	if data.Image == nil || *data.Image == "" {
 		return model.NewAppError("BulkImport", "app.import.validate_emoji_import_data.image_missing.error", nil, "", http.StatusBadRequest)
 	}
 
+	if err := model.IsValidEmojiName(*data.Name); err != nil {
+		return err
+	}
+
 	return nil
 }
 

app/import_validators_test.go

@@ -1343,34 +1343,37 @@ func TestImportValidateDirectPostImportData(t *testing.T) {
 }
 
 func TestImportValidateEmojiImportData(t *testing.T) {
-	data := EmojiImportData{
-		Name:  ptrStr("parrot2"),
-		Image: ptrStr("/path/to/image"),
+	var testCases = []struct {
+		testName          string
+		name              *string
+		image             *string
+		expectError       bool
+		expectSystemEmoji bool
+	}{
+		{"success", ptrStr("parrot2"), ptrStr("/path/to/image"), false, false},
+		{"system emoji", ptrStr("smiley"), ptrStr("/path/to/image"), true, true},
+		{"empty name", ptrStr(""), ptrStr("/path/to/image"), true, false},
+		{"empty image", ptrStr("parrot2"), ptrStr(""), true, false},
+		{"empty name and image", ptrStr(""), ptrStr(""), true, false},
+		{"nil name", nil, ptrStr("/path/to/image"), true, false},
+		{"nil image", ptrStr("parrot2"), nil, true, false},
+		{"nil name and image", nil, nil, true, false},
 	}
 
-	err := validateEmojiImportData(&data)
-	assert.Nil(t, err, "Validation should succeed")
+	for _, tc := range testCases {
+		t.Run(tc.testName, func(t *testing.T) {
+			data := EmojiImportData{
+				Name:  tc.name,
+				Image: tc.image,
+			}
 
-	*data.Name = "smiley"
-	err = validateEmojiImportData(&data)
-	assert.NotNil(t, err)
-
-	*data.Name = ""
-	err = validateEmojiImportData(&data)
-	assert.NotNil(t, err)
-
-	*data.Name = ""
-	*data.Image = ""
-	err = validateEmojiImportData(&data)
-	assert.NotNil(t, err)
-
-	*data.Image = "/path/to/image"
-	data.Name = nil
-	err = validateEmojiImportData(&data)
-	assert.NotNil(t, err)
-
-	data.Name = ptrStr("parrot")
-	data.Image = nil
-	err = validateEmojiImportData(&data)
-	assert.NotNil(t, err)
+			err := validateEmojiImportData(&data)
+			if tc.expectError {
+				require.NotNil(t, err)
+				assert.Equal(t, tc.expectSystemEmoji, err.Id == "model.emoji.system_emoji_name.app_error")
+			} else {
+				assert.Nil(t, err)
+			}
+		})
+	}
 }

i18n/en.json

@@ -8363,6 +8363,10 @@
     "id": "model.emoji.name.app_error",
     "translation": "Name must be 1 to 64 lowercase alphanumeric characters."
   },
+  {
+    "id": "model.emoji.system_emoji_name.app_error",
+    "translation": "Name conflicts with existing system emoji name."
+  },
   {
     "id": "model.emoji.update_at.app_error",
     "translation": "Update at must be a valid time."

model/emoji.go

@@ -78,9 +78,12 @@ func (emoji *Emoji) IsValid() *AppError {
 }
 
 func IsValidEmojiName(name string) *AppError {
-	if name == "" || len(name) > EmojiNameMaxLength || !IsValidAlphaNumHyphenUnderscorePlus(name) || inSystemEmoji(name) {
+	if name == "" || len(name) > EmojiNameMaxLength || !IsValidAlphaNumHyphenUnderscorePlus(name) {
 		return NewAppError("Emoji.IsValid", "model.emoji.name.app_error", nil, "", http.StatusBadRequest)
 	}
+	if inSystemEmoji(name) {
+		return NewAppError("Emoji.IsValid", "model.emoji.system_emoji_name.app_error", nil, "", http.StatusBadRequest)
+	}
 
 	return nil
 }