From 670b0e4c9fb600bc0eb1831122a2cb7d13e4e8ce Mon Sep 17 00:00:00 2001 From: Nathan Date: Thu, 4 May 2023 08:14:26 -0600 Subject: [PATCH] [MM-44954] Regenerate default avatar (#22871) * Regenerate default profile picture if username has changed - Only actions is profile picture has not been changed - Adjusts ResetLastPictureUpdate store function to store -curTime instead of 0 - This is to support updating the default picture while still retaining the ability to discern a default image from a set one. - Changes SetDefaultProfileImage to leverage UpdateDefaultProfileImage - Test updates around updating user default profile pictures * App interface updates * Only display picture update date if non-negative - Ensures we don't display negative timestamps (default images) - Change ported for mono-repo changes * Remove duplicate test assertion --------- Co-authored-by: Nathan Geist Co-authored-by: Mattermost Build --- server/channels/api4/user_test.go | 11 ++++- server/channels/app/app_iface.go | 1 + .../app/opentracing/opentracing_layer.go | 22 ++++++++++ server/channels/app/user.go | 42 +++++++++++++++---- server/channels/app/user_test.go | 35 ++++++++++++++-- server/channels/store/sqlstore/user_store.go | 2 +- server/channels/store/storetest/user_store.go | 8 ++-- .../general/user_settings_general.tsx | 2 +- 8 files changed, 105 insertions(+), 18 deletions(-) diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index 0f9ce87d33..d64e8dd2a4 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -3664,9 +3664,16 @@ func TestSetDefaultProfileImage(t *testing.T) { defer th.TearDown() user := th.BasicUser + startTime := model.GetMillis() + time.Sleep(time.Millisecond) + _, err := th.Client.SetDefaultProfileImage(user.Id) require.NoError(t, err) + iuser, getUserErr := th.App.GetUser(user.Id) + require.Nil(t, getUserErr) + assert.Less(t, iuser.LastPictureUpdate, -startTime, "LastPictureUpdate should be set to -(current time in milliseconds)") + resp, err := th.Client.SetDefaultProfileImage(model.NewId()) require.Error(t, err) CheckForbiddenStatus(t, resp) @@ -3684,12 +3691,14 @@ func TestSetDefaultProfileImage(t *testing.T) { require.Fail(t, "Should have failed either forbidden or unauthorized") } + time.Sleep(time.Millisecond) + _, err = th.SystemAdminClient.SetDefaultProfileImage(user.Id) require.NoError(t, err) ruser, appErr := th.App.GetUser(user.Id) require.Nil(t, appErr) - assert.Equal(t, int64(0), ruser.LastPictureUpdate, "Picture should have reset to default") + assert.Less(t, ruser.LastPictureUpdate, iuser.LastPictureUpdate, "LastPictureUpdate should be updated to a lower negative number") info := &model.FileInfo{Path: "users/" + user.Id + "/profile.png"} err = th.cleanupTestFile(info) diff --git a/server/channels/app/app_iface.go b/server/channels/app/app_iface.go index 2c38a3afd8..0ca3855d0c 100644 --- a/server/channels/app/app_iface.go +++ b/server/channels/app/app_iface.go @@ -1120,6 +1120,7 @@ type AppIface interface { UpdateChannelPrivacy(c request.CTX, oldChannel *model.Channel, user *model.User) (*model.Channel, *model.AppError) UpdateCommand(oldCmd, updatedCmd *model.Command) (*model.Command, *model.AppError) UpdateConfig(f func(*model.Config)) + UpdateDefaultProfileImage(c request.CTX, user *model.User) *model.AppError UpdateEphemeralPost(c request.CTX, userID string, post *model.Post) *model.Post UpdateExpiredDNDStatuses() ([]*model.Status, error) UpdateGroup(group *model.Group) (*model.Group, *model.AppError) diff --git a/server/channels/app/opentracing/opentracing_layer.go b/server/channels/app/opentracing/opentracing_layer.go index c109b9a3ce..39827e4e9d 100644 --- a/server/channels/app/opentracing/opentracing_layer.go +++ b/server/channels/app/opentracing/opentracing_layer.go @@ -17376,6 +17376,28 @@ func (a *OpenTracingAppLayer) UpdateDNDStatusOfUsers() { a.app.UpdateDNDStatusOfUsers() } +func (a *OpenTracingAppLayer) UpdateDefaultProfileImage(c request.CTX, user *model.User) *model.AppError { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UpdateDefaultProfileImage") + + a.ctx = newCtx + a.app.Srv().Store().SetContext(newCtx) + defer func() { + a.app.Srv().Store().SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0 := a.app.UpdateDefaultProfileImage(c, user) + + if resultVar0 != nil { + span.LogFields(spanlog.Error(resultVar0)) + ext.Error.Set(span, true) + } + + return resultVar0 +} + func (a *OpenTracingAppLayer) UpdateEphemeralPost(c request.CTX, userID string, post *model.Post) *model.Post { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UpdateEphemeralPost") diff --git a/server/channels/app/user.go b/server/channels/app/user.go index 9e08824168..8787402a50 100644 --- a/server/channels/app/user.go +++ b/server/channels/app/user.go @@ -792,7 +792,7 @@ func (a *App) GetDefaultProfileImage(user *model.User) ([]byte, *model.AppError) return a.ch.srv.GetDefaultProfileImage(user) } -func (a *App) SetDefaultProfileImage(c request.CTX, user *model.User) *model.AppError { +func (a *App) UpdateDefaultProfileImage(c request.CTX, user *model.User) *model.AppError { img, appErr := a.GetDefaultProfileImage(user) if appErr != nil { return appErr @@ -809,6 +809,16 @@ func (a *App) SetDefaultProfileImage(c request.CTX, user *model.User) *model.App a.InvalidateCacheForUser(user.Id) + return nil +} + +func (a *App) SetDefaultProfileImage(c request.CTX, user *model.User) *model.AppError { + + if err := a.UpdateDefaultProfileImage(c, user); err != nil { + c.Logger().Error("Failed to update default profile image for user", mlog.String("user_id", user.Id), mlog.Err(err)) + return err + } + updatedUser, appErr := a.GetUser(user.Id) if appErr != nil { c.Logger().Warn("Error in getting users profile forcing logout", mlog.String("user_id", user.Id), mlog.Err(appErr)) @@ -1228,37 +1238,53 @@ func (a *App) UpdateUser(c request.CTX, user *model.User, sendNotifications bool } } + newUser := userUpdate.New + + if (newUser.Username != userUpdate.Old.Username) && (newUser.LastPictureUpdate <= 0) { + // When a username is updated and the profile is still using a default profile picture, generate a new one based on their username + if err := a.UpdateDefaultProfileImage(c, newUser); err != nil { + c.Logger().Warn("Error with updating default profile image", mlog.Err(err)) + } + + tempUser, getUserErr := a.GetUser(user.Id) + if getUserErr != nil { + c.Logger().Warn("Error when retrieving user after profile picture update, avatar may fail to update automatically on client applications.", mlog.Err(getUserErr)) + } else { + newUser = tempUser + } + } + if sendNotifications { - if userUpdate.New.Email != userUpdate.Old.Email || newEmail != "" { + if newUser.Email != userUpdate.Old.Email || newEmail != "" { if *a.Config().EmailSettings.RequireEmailVerification { a.Srv().Go(func() { - if err := a.SendEmailVerification(userUpdate.New, newEmail, ""); err != nil { + if err := a.SendEmailVerification(newUser, newEmail, ""); err != nil { c.Logger().Error("Failed to send email verification", mlog.Err(err)) } }) } else { a.Srv().Go(func() { - if err := a.Srv().EmailService.SendEmailChangeEmail(userUpdate.Old.Email, userUpdate.New.Email, userUpdate.New.Locale, a.GetSiteURL()); err != nil { + if err := a.Srv().EmailService.SendEmailChangeEmail(userUpdate.Old.Email, newUser.Email, newUser.Locale, a.GetSiteURL()); err != nil { c.Logger().Error("Failed to send email change email", mlog.Err(err)) } }) } } - if userUpdate.New.Username != userUpdate.Old.Username { + if newUser.Username != userUpdate.Old.Username { a.Srv().Go(func() { - if err := a.Srv().EmailService.SendChangeUsernameEmail(userUpdate.New.Username, userUpdate.New.Email, userUpdate.New.Locale, a.GetSiteURL()); err != nil { + if err := a.Srv().EmailService.SendChangeUsernameEmail(newUser.Username, newUser.Email, newUser.Locale, a.GetSiteURL()); err != nil { c.Logger().Error("Failed to send change username email", mlog.Err(err)) } }) } - a.sendUpdatedUserEvent(*userUpdate.New) + a.sendUpdatedUserEvent(*newUser) } a.InvalidateCacheForUser(user.Id) a.onUserProfileChange(user.Id) - return userUpdate.New, nil + return newUser, nil } func (a *App) UpdateUserActive(c request.CTX, userID string, active bool) *model.AppError { diff --git a/server/channels/app/user_test.go b/server/channels/app/user_test.go index 703a39db9b..da1938cd7e 100644 --- a/server/channels/app/user_test.go +++ b/server/channels/app/user_test.go @@ -85,9 +85,11 @@ func TestCreateOAuthUser(t *testing.T) { }) } -func TestSetDefaultProfileImage(t *testing.T) { +func TestUpdateDefaultProfileImage(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() + startTime := model.GetMillis() + time.Sleep(time.Millisecond) err := th.App.SetDefaultProfileImage(th.Context, &model.User{ Id: model.NewId(), @@ -98,11 +100,11 @@ func TestSetDefaultProfileImage(t *testing.T) { user := th.BasicUser - err = th.App.SetDefaultProfileImage(th.Context, user) + err = th.App.UpdateDefaultProfileImage(th.Context, user) require.Nil(t, err) user = getUserFromDB(th.App, user.Id, t) - assert.Equal(t, int64(0), user.LastPictureUpdate) + assert.Less(t, user.LastPictureUpdate, -startTime, "LastPictureUpdate should be set to -(current time in milliseconds)") } func TestAdjustProfileImage(t *testing.T) { @@ -194,6 +196,33 @@ func TestUpdateUser(t *testing.T) { require.NotNil(t, err) require.Nil(t, u) }) + + t.Run("fails if default profile picture is not updated when user has default profile picture and username is changed", func(t *testing.T) { + user.Username = "updatedUsername" + iLastPictureUpdate := user.LastPictureUpdate + require.Equal(t, iLastPictureUpdate, int64(0)) + u, err := th.App.UpdateUser(th.Context, user, false) + require.Nil(t, err) + require.NotNil(t, u) + require.Less(t, u.LastPictureUpdate, iLastPictureUpdate) + }) + + t.Run("fails if profile picture is updated when user has custom profile picture and username is changed", func(t *testing.T) { + // Give the user a LastPictureUpdate to mimic having a custom profile picture + err := th.App.Srv().Store().User().UpdateLastPictureUpdate(user.Id) + require.NoError(t, err) + iUser, errGetUser := th.App.GetUser(user.Id) + require.Nil(t, errGetUser) + iUser.Username = "updatedUsername" + iLastPictureUpdate := iUser.LastPictureUpdate + require.Greater(t, iLastPictureUpdate, int64(0)) + + // Attempt the update, ensure the LastPictureUpdate has not changed + updatedUser, errUpdateUser := th.App.UpdateUser(th.Context, iUser, false) + require.Nil(t, errUpdateUser) + require.NotNil(t, updatedUser) + require.Equal(t, updatedUser.LastPictureUpdate, iLastPictureUpdate) + }) } func TestUpdateUserMissingFields(t *testing.T) { diff --git a/server/channels/store/sqlstore/user_store.go b/server/channels/store/sqlstore/user_store.go index 9f063ad9e1..f6792d187a 100644 --- a/server/channels/store/sqlstore/user_store.go +++ b/server/channels/store/sqlstore/user_store.go @@ -285,7 +285,7 @@ func (us SqlUserStore) UpdateLastPictureUpdate(userId string) error { func (us SqlUserStore) ResetLastPictureUpdate(userId string) error { curTime := model.GetMillis() - if _, err := us.GetMasterX().Exec("UPDATE Users SET LastPictureUpdate = ?, UpdateAt = ? WHERE Id = ?", 0, curTime, userId); err != nil { + if _, err := us.GetMasterX().Exec("UPDATE Users SET LastPictureUpdate = ?, UpdateAt = ? WHERE Id = ?", -curTime, curTime, userId); err != nil { return errors.Wrapf(err, "failed to update User with userId=%s", userId) } diff --git a/server/channels/store/storetest/user_store.go b/server/channels/store/storetest/user_store.go index 1a1c64bc49..06069591a8 100644 --- a/server/channels/store/storetest/user_store.go +++ b/server/channels/store/storetest/user_store.go @@ -5895,6 +5895,7 @@ func testDeactivateGuests(t *testing.T, ss store.Store) { } func testUserStoreResetLastPictureUpdate(t *testing.T, ss store.Store) { + startTime := model.GetMillis() u1 := &model.User{} u1.Email = MakeEmail() _, err := ss.User().Save(u1) @@ -5909,8 +5910,7 @@ func testUserStoreResetLastPictureUpdate(t *testing.T, ss store.Store) { user, err := ss.User().Get(context.Background(), u1.Id) require.NoError(t, err) - assert.NotZero(t, user.LastPictureUpdate) - assert.NotZero(t, user.UpdateAt) + assert.GreaterOrEqual(t, user.LastPictureUpdate, startTime) // Ensure update at timestamp changes time.Sleep(time.Millisecond) @@ -5923,8 +5923,8 @@ func testUserStoreResetLastPictureUpdate(t *testing.T, ss store.Store) { user2, err := ss.User().Get(context.Background(), u1.Id) require.NoError(t, err) - assert.True(t, user2.UpdateAt > user.UpdateAt) - assert.Zero(t, user2.LastPictureUpdate) + assert.Greater(t, user2.UpdateAt, user.UpdateAt) + assert.Less(t, user2.LastPictureUpdate, -startTime) } func testGetKnownUsers(t *testing.T, ss store.Store) { diff --git a/webapp/channels/src/components/user_settings/general/user_settings_general.tsx b/webapp/channels/src/components/user_settings/general/user_settings_general.tsx index e6aa6758da..5d06120161 100644 --- a/webapp/channels/src/components/user_settings/general/user_settings_general.tsx +++ b/webapp/channels/src/components/user_settings/general/user_settings_general.tsx @@ -1335,7 +1335,7 @@ export class UserSettingsGeneralTab extends React.Component { if (Utils.isMobile()) { minMessage = formatMessage(holders.uploadImageMobile); } - if (user.last_picture_update) { + if (user.last_picture_update > 0) { minMessage = (