MM-30041: Return correct error message for user save (#16117)

* MM-30041: Return correct error message for user save

We were collapsing all types of user conflict into a single
error message. Fixed it by inspecting the field of the invalidError type
and returning the correct message.

https://mattermost.atlassian.net/browse/MM-30041

```release-note
NONE
```

* Fix test errors

* Fix wrong message in test when comparing error messages

Co-authored-by: Rodrigo Villablanca <villa061004@gmail.com>

api4/apitestlib.go

@@ -881,7 +881,7 @@ func CheckErrorMessage(t *testing.T, resp *model.Response, errorId string) {
 	t.Helper()
 
 	require.NotNilf(t, resp.Error, "should have errored with message: %s", errorId)
-	require.Equalf(t, resp.Error.Id, errorId, "incorrect error message, actual: %s, expected: %s", resp.Error.Id, errorId)
+	require.Equalf(t, errorId, resp.Error.Id, "incorrect error message, actual: %s, expected: %s", resp.Error.Id, errorId)
 }
 
 func CheckStartsWith(t *testing.T, value, prefix, message string) {

api4/user_test.go

@@ -53,13 +53,13 @@ func TestCreateUser(t *testing.T) {
 	ruser.Username = GenerateTestUsername()
 	ruser.Password = "passwd1"
 	_, resp = th.Client.CreateUser(ruser)
-	CheckErrorMessage(t, resp, "app.user.save.existing.app_error")
+	CheckErrorMessage(t, resp, "app.user.save.email_exists.app_error")
 	CheckBadRequestStatus(t, resp)
 
 	ruser.Email = th.GenerateTestEmail()
 	ruser.Username = user.Username
 	_, resp = th.Client.CreateUser(ruser)
-	CheckErrorMessage(t, resp, "app.user.save.existing.app_error")
+	CheckErrorMessage(t, resp, "app.user.save.username_exists.app_error")
 	CheckBadRequestStatus(t, resp)
 
 	ruser.Email = ""

app/bot.go

@@ -26,7 +26,16 @@ func (a *App) CreateBot(bot *model.Bot) (*model.Bot, *model.AppError) {
 		case errors.As(nErr, &appErr):
 			return nil, appErr
 		case errors.As(nErr, &invErr):
-			return nil, model.NewAppError("CreateBot", "app.user.save.existing.app_error", nil, invErr.Error(), http.StatusBadRequest)
+			code := ""
+			switch invErr.Field {
+			case "email":
+				code = "app.user.save.email_exists.app_error"
+			case "username":
+				code = "app.user.save.username_exists.app_error"
+			default:
+				code = "app.user.save.existing.app_error"
+			}
+			return nil, model.NewAppError("CreateBot", code, nil, invErr.Error(), http.StatusBadRequest)
 		default:
 			return nil, model.NewAppError("CreateBot", "app.user.save.app_error", nil, nErr.Error(), http.StatusInternalServerError)
 		}
@@ -93,7 +102,16 @@ func (a *App) getOrCreateWarnMetricsBot(botDef *model.Bot) (*model.Bot, *model.A
 			case errors.As(nErr, &appError):
 				return nil, appError
 			case errors.As(nErr, &invErr):
-				return nil, model.NewAppError("getOrCreateWarnMetricsBot", "app.user.save.existing.app_error", nil, invErr.Error(), http.StatusBadRequest)
+				code := ""
+				switch invErr.Field {
+				case "email":
+					code = "app.user.save.email_exists.app_error"
+				case "username":
+					code = "app.user.save.username_exists.app_error"
+				default:
+					code = "app.user.save.existing.app_error"
+				}
+				return nil, model.NewAppError("getOrCreateWarnMetricsBot", code, nil, invErr.Error(), http.StatusBadRequest)
 			default:
 				return nil, model.NewAppError("getOrCreateWarnMetricsBot", "app.user.save.app_error", nil, nErr.Error(), http.StatusInternalServerError)
 			}

app/bot_test.go

@@ -97,7 +97,7 @@ func TestCreateBot(t *testing.T) {
 			OwnerId:     th.BasicUser.Id,
 		})
 		require.NotNil(t, err)
-		require.Equal(t, "app.user.save.existing.app_error", err.Id)
+		require.Equal(t, "app.user.save.username_exists.app_error", err.Id)
 	})
 }
 

app/user.go

@@ -306,7 +306,14 @@ func (a *App) createUser(user *model.User) (*model.User, *model.AppError) {
 		case errors.As(nErr, &appErr):
 			return nil, appErr
 		case errors.As(nErr, &invErr):
-			return nil, model.NewAppError("createUser", "app.user.save.existing.app_error", nil, invErr.Error(), http.StatusBadRequest)
+			switch invErr.Field {
+			case "email":
+				return nil, model.NewAppError("createUser", "app.user.save.email_exists.app_error", nil, invErr.Error(), http.StatusBadRequest)
+			case "username":
+				return nil, model.NewAppError("createUser", "app.user.save.username_exists.app_error", nil, invErr.Error(), http.StatusBadRequest)
+			default:
+				return nil, model.NewAppError("createUser", "app.user.save.existing.app_error", nil, invErr.Error(), http.StatusBadRequest)
+			}
 		default:
 			return nil, model.NewAppError("createUser", "app.user.save.app_error", nil, nErr.Error(), http.StatusInternalServerError)
 		}

app/user_test.go

@@ -6,6 +6,7 @@ package app
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"image"
 	"image/color"
 	"strings"
@@ -18,6 +19,7 @@ import (
 	"github.com/mattermost/mattermost-server/v5/einterfaces"
 	"github.com/mattermost/mattermost-server/v5/model"
 	oauthgitlab "github.com/mattermost/mattermost-server/v5/model/gitlab"
+	"github.com/mattermost/mattermost-server/v5/store"
 	"github.com/mattermost/mattermost-server/v5/utils/testutils"
 )
 
@@ -361,6 +363,46 @@ func TestUpdateOAuthUserAttrs(t *testing.T) {
 	})
 }
 
+func TestCreateUserConflict(t *testing.T) {
+	th := Setup(t)
+	defer th.TearDown()
+
+	user := &model.User{
+		Email:    "test@localhost",
+		Username: model.NewId(),
+	}
+	user, err := th.App.Srv().Store.User().Save(user)
+	require.NoError(t, err)
+	username := user.Username
+
+	var invErr *store.ErrInvalidInput
+	// Same id
+	_, err = th.App.Srv().Store.User().Save(user)
+	require.Error(t, err)
+	require.True(t, errors.As(err, &invErr))
+	assert.Equal(t, "id", invErr.Field)
+
+	// Same email
+	user = &model.User{
+		Email:    "test@localhost",
+		Username: model.NewId(),
+	}
+	_, err = th.App.Srv().Store.User().Save(user)
+	require.Error(t, err)
+	require.True(t, errors.As(err, &invErr))
+	assert.Equal(t, "email", invErr.Field)
+
+	// Same username
+	user = &model.User{
+		Email:    "test2@localhost",
+		Username: username,
+	}
+	_, err = th.App.Srv().Store.User().Save(user)
+	require.Error(t, err)
+	require.True(t, errors.As(err, &invErr))
+	assert.Equal(t, "username", invErr.Field)
+}
+
 func TestUpdateUserEmail(t *testing.T) {
 	th := Setup(t)
 	defer th.TearDown()

i18n/en.json

@@ -5466,10 +5466,18 @@
     "id": "app.user.save.app_error",
     "translation": "Unable to save the account."
   },
+  {
+    "id": "app.user.save.email_exists.app_error",
+    "translation": "An account with that email already exists."
+  },
   {
     "id": "app.user.save.existing.app_error",
     "translation": "Must call update for existing user."
   },
+  {
+    "id": "app.user.save.username_exists.app_error",
+    "translation": "An account with that username already exists."
+  },
   {
     "id": "app.user.search.app_error",
     "translation": "Unable to find any user matching the search parameters."