-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only update needed columns when update user #2296
Conversation
// UpdateUserCols update user according special columns | ||
func UpdateUserCols(u *User, cols ...string) error { | ||
// Organization does not need email | ||
u.Email = strings.ToLower(u.Email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we iterate over cols array and with a switch only do needed operation when matching col ?
Example : https://play.golang.org/p/4pRMeP89Re
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't get what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is overkill
// UpdateUserCols update user according special columns
func UpdateUserCols(u *User, cols ...string) error {
//Format only when needed
for _, c := range cols {
switch c {
case "email":
// Organization does not need email
u.Email = strings.ToLower(u.Email)
if !u.IsOrganization() {
if len(u.AvatarEmail) == 0 {
u.AvatarEmail = u.Email
}
}
case "name":
u.LowerName = strings.ToLower(u.Name)
case "location":
u.Location = base.TruncateString(u.Location, 255)
case "website":
u.Website = base.TruncateString(u.Website, 255)
case "description":
u.Description = base.TruncateString(u.Description, 255)
}
}
if !u.IsOrganization() {
if len(u.AvatarEmail) > 0 {
u.Avatar = base.HashEmail(u.AvatarEmail)
}
}
_, err := x.Id(u.ID).Cols(cols...).Update(u)
return err
}
models/user.go
Outdated
u.Website = base.TruncateString(u.Website, 255) | ||
u.Description = base.TruncateString(u.Description, 255) | ||
|
||
_, err := x.Id(u.ID).Cols(cols...).Update(u) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated_unix
column won't be updated. I think we need to do something similar to #2204
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
Adding a new function seems like overkill to me, is there a reason we can't just fix the |
@ethantkoenig We could add a new func generateAvatar that is call before updateUser only when email is change or needed ? |
@ethantkoenig I think |
@sapk that could be another PR. |
LGTM |
LGTM |
Make LG-TM work |
Also fix #1882. After this merged, I will back ported to branch release/v1.1.