[PLT-6102] Increase maximum length of field User.Position (#6044)

* Increase maximum length of field User.Position

* Rename method

2 files changed, 58 insertions, 37 deletions

diff --git a/model/user.go b/model/user.go
index a4a37b387..2c2ba734b 100644
--- a/model/user.go
+++ b/model/user.go
@@ -26,13 +26,12 @@ const (
 	PUSH_NOTIFY_PROP        = "push"
 	EMAIL_NOTIFY_PROP       = "email"
 
-	DEFAULT_LOCALE             = "en"
-	USER_AUTH_SERVICE_EMAIL    = "email"
-	USER_AUTH_SERVICE_USERNAME = "username"
+	DEFAULT_LOCALE          = "en"
+	USER_AUTH_SERVICE_EMAIL = "email"
 
 	USER_EMAIL_MAX_LENGTH     = 128
 	USER_NICKNAME_MAX_RUNES   = 64
-	USER_POSITION_MAX_RUNES   = 35
+	USER_POSITION_MAX_RUNES   = 64
 	USER_FIRST_NAME_MAX_RUNES = 64
 	USER_LAST_NAME_MAX_RUNES  = 64
 	USER_AUTH_DATA_MAX_LENGTH = 128
@@ -85,56 +84,65 @@ type UserPatch struct {
 func (u *User) IsValid() *AppError {
 
 	if len(u.Id) != 26 {
-		return NewAppError("User.IsValid", "model.user.is_valid.id.app_error", nil, "", http.StatusBadRequest)
+		return InvalidUserError("id", "")
 	}
 
 	if u.CreateAt == 0 {
-		return NewAppError("User.IsValid", "model.user.is_valid.create_at.app_error", nil, "user_id="+u.Id, http.StatusBadRequest)
+		return InvalidUserError("create_at", u.Id)
 	}
 
 	if u.UpdateAt == 0 {
-		return NewAppError("User.IsValid", "model.user.is_valid.update_at.app_error", nil, "user_id="+u.Id, http.StatusBadRequest)
+		return InvalidUserError("update_at", u.Id)
 	}
 
 	if !IsValidUsername(u.Username) {
-		return NewAppError("User.IsValid", "model.user.is_valid.username.app_error", nil, "user_id="+u.Id, http.StatusBadRequest)
+		return InvalidUserError("username", u.Id)
 	}
 
 	if len(u.Email) > USER_EMAIL_MAX_LENGTH || len(u.Email) == 0 {
-		return NewAppError("User.IsValid", "model.user.is_valid.email.app_error", nil, "user_id="+u.Id, http.StatusBadRequest)
+		return InvalidUserError("email", u.Id)
 	}
 
 	if utf8.RuneCountInString(u.Nickname) > USER_NICKNAME_MAX_RUNES {
-		return NewAppError("User.IsValid", "model.user.is_valid.nickname.app_error", nil, "user_id="+u.Id, http.StatusBadRequest)
+		return InvalidUserError("nickname", u.Id)
 	}
 
 	if utf8.RuneCountInString(u.Position) > USER_POSITION_MAX_RUNES {
-		return NewAppError("User.IsValid", "model.user.is_valid.position.app_error", nil, "user_id="+u.Id, http.StatusBadRequest)
+		return InvalidUserError("position", u.Id)
 	}
 
 	if utf8.RuneCountInString(u.FirstName) > USER_FIRST_NAME_MAX_RUNES {
-		return NewAppError("User.IsValid", "model.user.is_valid.first_name.app_error", nil, "user_id="+u.Id, http.StatusBadRequest)
+		return InvalidUserError("first_name", u.Id)
 	}
 
 	if utf8.RuneCountInString(u.LastName) > USER_LAST_NAME_MAX_RUNES {
-		return NewAppError("User.IsValid", "model.user.is_valid.last_name.app_error", nil, "user_id="+u.Id, http.StatusBadRequest)
+		return InvalidUserError("last_name", u.Id)
 	}
 
 	if u.AuthData != nil && len(*u.AuthData) > USER_AUTH_DATA_MAX_LENGTH {
-		return NewAppError("User.IsValid", "model.user.is_valid.auth_data.app_error", nil, "user_id="+u.Id, http.StatusBadRequest)
+		return InvalidUserError("auth_data", u.Id)
 	}
 
 	if u.AuthData != nil && len(*u.AuthData) > 0 && len(u.AuthService) == 0 {
-		return NewAppError("User.IsValid", "model.user.is_valid.auth_data_type.app_error", nil, "user_id="+u.Id, http.StatusBadRequest)
+		return InvalidUserError("auth_data_type", u.Id)
 	}
 
 	if len(u.Password) > 0 && u.AuthData != nil && len(*u.AuthData) > 0 {
-		return NewAppError("User.IsValid", "model.user.is_valid.auth_data_pwd.app_error", nil, "user_id="+u.Id, http.StatusBadRequest)
+		return InvalidUserError("auth_data_pwd", u.Id)
 	}
 
 	return nil
 }
 
+func InvalidUserError(fieldName string, userId string) *AppError {
+	id := fmt.Sprintf("model.user.is_valid.%s.app_error", fieldName)
+	details := ""
+	if userId != "" {
+		details = "user_id=" + userId
+	}
+	return NewAppError("User.IsValid", id, nil, details, http.StatusBadRequest)
+}
+
 // PreSave will set the Id and Username if missing.  It will also fill
 // in the CreateAt, UpdateAt times.  It will also hash the password.  It should
 // be run before saving the user to the db.
diff --git a/model/user_test.go b/model/user_test.go
index a42ec3da5..3eb931f5d 100644
--- a/model/user_test.go
+++ b/model/user_test.go
@@ -4,6 +4,8 @@
 package model
 
 import (
+	"fmt"
+	"net/http"
 	"strings"
 	"testing"
 )
@@ -71,43 +73,43 @@ func TestUserUpdateMentionKeysFromUsername(t *testing.T) {
 func TestUserIsValid(t *testing.T) {
 	user := User{}
 
-	if err := user.IsValid(); err == nil {
-		t.Fatal()
+	if err := user.IsValid(); !HasExpectedUserIsValidError(err, "id", "") {
+		t.Fatal(err)
 	}
 
 	user.Id = NewId()
-	if err := user.IsValid(); err == nil {
+	if err := user.IsValid(); !HasExpectedUserIsValidError(err, "create_at", user.Id) {
 		t.Fatal()
 	}
 
 	user.CreateAt = GetMillis()
-	if err := user.IsValid(); err == nil {
+	if err := user.IsValid(); !HasExpectedUserIsValidError(err, "update_at", user.Id) {
 		t.Fatal()
 	}
 
 	user.UpdateAt = GetMillis()
-	if err := user.IsValid(); err == nil {
+	if err := user.IsValid(); !HasExpectedUserIsValidError(err, "username", user.Id) {
 		t.Fatal()
 	}
 
 	user.Username = NewId() + "^hello#"
-	if err := user.IsValid(); err == nil {
+	if err := user.IsValid(); !HasExpectedUserIsValidError(err, "username", user.Id) {
 		t.Fatal()
 	}
 
 	user.Username = "n" + NewId()
-	user.Email = strings.Repeat("01234567890", 20)
-	if err := user.IsValid(); err == nil {
+	user.Email = strings.Repeat("a", 129)
+	if err := user.IsValid(); !HasExpectedUserIsValidError(err, "email", user.Id) {
 		t.Fatal()
 	}
 
-	user.Email = "test@nowhere.com"
-	user.Nickname = strings.Repeat("01234567890", 20)
-	if err := user.IsValid(); err == nil {
+	user.Email = strings.Repeat("a", 128)
+	user.Nickname = strings.Repeat("a", 65)
+	if err := user.IsValid(); !HasExpectedUserIsValidError(err, "nickname", user.Id) {
 		t.Fatal()
 	}
 
-	user.Nickname = ""
+	user.Nickname = strings.Repeat("a", 64)
 	if err := user.IsValid(); err != nil {
 		t.Fatal(err)
 	}
@@ -118,29 +120,40 @@ func TestUserIsValid(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	user.FirstName = strings.Repeat("01234567890", 20)
-	if err := user.IsValid(); err == nil {
+	user.FirstName = strings.Repeat("a", 65)
+	if err := user.IsValid(); !HasExpectedUserIsValidError(err, "first_name", user.Id) {
 		t.Fatal(err)
 	}
 
-	user.FirstName = ""
-	user.LastName = strings.Repeat("01234567890", 20)
-	if err := user.IsValid(); err == nil {
+	user.FirstName = strings.Repeat("a", 64)
+	user.LastName = strings.Repeat("a", 65)
+	if err := user.IsValid(); !HasExpectedUserIsValidError(err, "last_name", user.Id) {
 		t.Fatal(err)
 	}
 
-	user.LastName = ""
-	user.Position = ""
+	user.LastName = strings.Repeat("a", 64)
+	user.Position = strings.Repeat("a", 64)
 	if err := user.IsValid(); err != nil {
 		t.Fatal(err)
 	}
 
-	user.Position = strings.Repeat("01234567890", 20)
-	if err := user.IsValid(); err == nil {
+	user.Position = strings.Repeat("a", 65)
+	if err := user.IsValid(); !HasExpectedUserIsValidError(err, "position", user.Id) {
 		t.Fatal(err)
 	}
 }
 
+func HasExpectedUserIsValidError(err *AppError, fieldName string, userId string) bool {
+	if err == nil {
+		return false
+	}
+
+	return err.Where == "User.IsValid" &&
+		err.Id == fmt.Sprintf("model.user.is_valid.%s.app_error", fieldName) &&
+		err.StatusCode == http.StatusBadRequest &&
+		(userId == "" || err.DetailedError == "user_id="+userId)
+}
+
 func TestUserGetFullName(t *testing.T) {
 	user := User{}