From 325bff11762cba2bc95c7a7c37bc9388efa6e87c Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Fri, 30 Oct 2020 11:14:06 +0530 Subject: [PATCH] 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 --- api4/apitestlib.go | 2 +- api4/user_test.go | 4 ++-- app/bot.go | 22 ++++++++++++++++++++-- app/bot_test.go | 2 +- app/user.go | 9 ++++++++- app/user_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ i18n/en.json | 8 ++++++++ 7 files changed, 82 insertions(+), 7 deletions(-) diff --git a/api4/apitestlib.go b/api4/apitestlib.go index 06820a81b5..1aa5d3ada7 100644 --- a/api4/apitestlib.go +++ b/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) { diff --git a/api4/user_test.go b/api4/user_test.go index d839ad722c..362a14f451 100644 --- a/api4/user_test.go +++ b/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 = "" diff --git a/app/bot.go b/app/bot.go index c0f8cb4dfa..dacc8af45d 100644 --- a/app/bot.go +++ b/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) } diff --git a/app/bot_test.go b/app/bot_test.go index 39d7eaf4e8..3b413e0428 100644 --- a/app/bot_test.go +++ b/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) }) } diff --git a/app/user.go b/app/user.go index 2c3539d920..c80485df51 100644 --- a/app/user.go +++ b/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) } diff --git a/app/user_test.go b/app/user_test.go index 5a4b640140..38125cec62 100644 --- a/app/user_test.go +++ b/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() diff --git a/i18n/en.json b/i18n/en.json index 08cc91de0b..128c9107d7 100644 --- a/i18n/en.json +++ b/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."