Modified updateUserNotifyProps to directly update the field (#18097)

* Modified updateUserNotifyProps to directly update the field

The method was only being used during import and it unnecessarily
made multiple queries to the DB.

Changed to a separate query that just updated the props field.

https://community-daily.mattermost.com/plugins/focalboard/workspace/zyoahc9uapdn3xdptac6jb69ic?id=285b80a3-257d-41f6-8cf4-ed80ca9d92e5&v=495cdb4d-c13a-4992-8eb9-80cfee2819a4&c=e4f9a891-85d6-4886-8590-1e327f7f8b8f

```release-note
NONE
```

* invalidating cache

```release-note
NONE
```

app/app_iface.go

@@ -1082,7 +1082,6 @@ type AppIface interface {
 	UpdateUserActive(c *request.Context, userID string, active bool) *model.AppError
 	UpdateUserAsUser(user *model.User, asAdmin bool) (*model.User, *model.AppError)
 	UpdateUserAuth(userID string, userAuth *model.UserAuth) (*model.UserAuth, *model.AppError)
-	UpdateUserNotifyProps(userID string, props map[string]string, sendNotifications bool) (*model.User, *model.AppError)
 	UpdateUserRoles(userID string, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError)
 	UpdateUserRolesWithUser(user *model.User, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError)
 	UploadData(c *request.Context, us *model.UploadSession, rd io.Reader) (*model.FileInfo, *model.AppError)

app/import_functions.go

@@ -527,7 +527,10 @@ func (a *App) importUser(data *UserImportData, dryRun bool) *model.AppError {
 			}
 		}
 		if hasNotifyPropsChanged {
-			if savedUser, appErr = a.UpdateUserNotifyProps(user.Id, user.NotifyProps, false); appErr != nil {
+			if appErr = a.updateUserNotifyProps(user.Id, user.NotifyProps); appErr != nil {
+				return appErr
+			}
+			if savedUser, appErr = a.GetUser(user.Id); appErr != nil {
 				return appErr
 			}
 		}

app/opentracing/opentracing_layer.go

@@ -16816,28 +16816,6 @@ func (a *OpenTracingAppLayer) UpdateUserAuth(userID string, userAuth *model.User
 	return resultVar0, resultVar1
 }
 
-func (a *OpenTracingAppLayer) UpdateUserNotifyProps(userID string, props map[string]string, sendNotifications bool) (*model.User, *model.AppError) {
-	origCtx := a.ctx
-	span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UpdateUserNotifyProps")
-
-	a.ctx = newCtx
-	a.app.Srv().Store.SetContext(newCtx)
-	defer func() {
-		a.app.Srv().Store.SetContext(origCtx)
-		a.ctx = origCtx
-	}()
-
-	defer span.Finish()
-	resultVar0, resultVar1 := a.app.UpdateUserNotifyProps(userID, props, sendNotifications)
-
-	if resultVar1 != nil {
-		span.LogFields(spanlog.Error(resultVar1))
-		ext.Error.Set(span, true)
-	}
-
-	return resultVar0, resultVar1
-}
-
 func (a *OpenTracingAppLayer) UpdateUserRoles(userID string, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) {
 	origCtx := a.ctx
 	span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UpdateUserRoles")

app/user.go

@@ -1143,20 +1143,22 @@ func (a *App) UpdateUserActive(c *request.Context, userID string, active bool) *
 	return nil
 }
 
-func (a *App) UpdateUserNotifyProps(userID string, props map[string]string, sendNotifications bool) (*model.User, *model.AppError) {
-	user, err := a.GetUser(userID)
+func (a *App) updateUserNotifyProps(userID string, props map[string]string) *model.AppError {
+	err := a.srv.userService.UpdateUserNotifyProps(userID, props)
 	if err != nil {
-		return nil, err
+		var appErr *model.AppError
+		switch {
+		case errors.As(err, &appErr):
+			return appErr
+		default:
+			return model.NewAppError("UpdateUser", "app.user.update.finding.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
 	}
 
-	user.NotifyProps = props
+	a.InvalidateCacheForUser(userID)
+	a.onUserProfileChange(userID)
 
-	ruser, err := a.UpdateUser(user, sendNotifications)
-	if err != nil {
-		return nil, err
-	}
-
-	return ruser, nil
+	return nil
 }
 
 func (a *App) UpdateMfa(activate bool, userID, token string) *model.AppError {

services/users/users.go

@@ -193,6 +193,10 @@ func (us *UserService) UpdateUser(user *model.User, allowRoleUpdate bool) (*mode
 	return us.store.Update(user, allowRoleUpdate)
 }
 
+func (us *UserService) UpdateUserNotifyProps(userID string, props map[string]string) error {
+	return us.store.UpdateNotifyProps(userID, props)
+}
+
 func (us *UserService) DeactivateAllGuests() ([]string, error) {
 	users, err := us.store.DeactivateGuests()
 	if err != nil {

store/opentracinglayer/opentracinglayer.go

@@ -10685,6 +10685,24 @@ func (s *OpenTracingLayerUserStore) UpdateMfaSecret(userID string, secret string
 	return err
 }
 
+func (s *OpenTracingLayerUserStore) UpdateNotifyProps(userID string, props map[string]string) error {
+	origCtx := s.Root.Store.Context()
+	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "UserStore.UpdateNotifyProps")
+	s.Root.Store.SetContext(newCtx)
+	defer func() {
+		s.Root.Store.SetContext(origCtx)
+	}()
+
+	defer span.Finish()
+	err := s.UserStore.UpdateNotifyProps(userID, props)
+	if err != nil {
+		span.LogFields(spanlog.Error(err))
+		ext.Error.Set(span, true)
+	}
+
+	return err
+}
+
 func (s *OpenTracingLayerUserStore) UpdatePassword(userID string, newPassword string) error {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "UserStore.UpdatePassword")

store/retrylayer/retrylayer.go

@@ -11610,6 +11610,26 @@ func (s *RetryLayerUserStore) UpdateMfaSecret(userID string, secret string) erro
 
 }
 
+func (s *RetryLayerUserStore) UpdateNotifyProps(userID string, props map[string]string) error {
+
+	tries := 0
+	for {
+		err := s.UserStore.UpdateNotifyProps(userID, props)
+		if err == nil {
+			return nil
+		}
+		if !isRepeatableError(err) {
+			return err
+		}
+		tries++
+		if tries >= 3 {
+			err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures")
+			return err
+		}
+	}
+
+}
+
 func (s *RetryLayerUserStore) UpdatePassword(userID string, newPassword string) error {
 
 	tries := 0

store/sqlstore/user_store.go

@@ -226,6 +226,18 @@ func (us SqlUserStore) Update(user *model.User, trustedUpdateData bool) (*model.
 	return &model.UserUpdate{New: user, Old: oldUser}, nil
 }
 
+func (us SqlUserStore) UpdateNotifyProps(userID string, props map[string]string) error {
+	if _, err := us.GetMaster().Exec(`UPDATE Users
+		SET NotifyProps = :NotifyProps
+		WHERE Id = :UserId`, map[string]interface{}{
+		"NotifyProps": model.MapToJson(props),
+		"UserId":      userID}); err != nil {
+		return errors.Wrapf(err, "failed to update User with userId=%s", userID)
+	}
+
+	return nil
+}
+
 func (us SqlUserStore) UpdateLastPictureUpdate(userId string) error {
 	curTime := model.GetMillis()
 

store/store.go

@@ -356,6 +356,7 @@ type PostStore interface {
 type UserStore interface {
 	Save(user *model.User) (*model.User, error)
 	Update(user *model.User, allowRoleUpdate bool) (*model.UserUpdate, error)
+	UpdateNotifyProps(userID string, props map[string]string) error
 	UpdateLastPictureUpdate(userID string) error
 	ResetLastPictureUpdate(userID string) error
 	UpdatePassword(userID, newPassword string) error

store/storetest/mocks/UserStore.go

@@ -1383,6 +1383,20 @@ func (_m *UserStore) UpdateMfaSecret(userID string, secret string) error {
 	return r0
 }
 
+// UpdateNotifyProps provides a mock function with given fields: userID, props
+func (_m *UserStore) UpdateNotifyProps(userID string, props map[string]string) error {
+	ret := _m.Called(userID, props)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(string, map[string]string) error); ok {
+		r0 = rf(userID, props)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
 // UpdatePassword provides a mock function with given fields: userID, newPassword
 func (_m *UserStore) UpdatePassword(userID string, newPassword string) error {
 	ret := _m.Called(userID, newPassword)

store/storetest/user_store.go

@@ -215,6 +215,22 @@ func testUserStoreUpdate(t *testing.T, ss store.Store) {
 
 	err = ss.User().UpdateLastPictureUpdate(u1.Id)
 	require.NoError(t, err, "Update should not have failed")
+
+	// Test UpdateNotifyProps
+	u1, err = ss.User().Get(context.Background(), u1.Id)
+	require.NoError(t, err)
+
+	props := u1.NotifyProps
+	props["hello"] = "world"
+
+	err = ss.User().UpdateNotifyProps(u1.Id, props)
+	require.NoError(t, err)
+
+	ss.User().InvalidateProfileCacheForUser(u1.Id)
+
+	uNew, err := ss.User().Get(context.Background(), u1.Id)
+	require.NoError(t, err)
+	assert.Equal(t, props, uNew.NotifyProps)
 }
 
 func testUserStoreUpdateUpdateAt(t *testing.T, ss store.Store) {

store/timerlayer/timerlayer.go

@@ -9634,6 +9634,22 @@ func (s *TimerLayerUserStore) UpdateMfaSecret(userID string, secret string) erro
 	return err
 }
 
+func (s *TimerLayerUserStore) UpdateNotifyProps(userID string, props map[string]string) error {
+	start := timemodule.Now()
+
+	err := s.UserStore.UpdateNotifyProps(userID, props)
+
+	elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second)
+	if s.Root.Metrics != nil {
+		success := "false"
+		if err == nil {
+			success = "true"
+		}
+		s.Root.Metrics.ObserveStoreMethodDuration("UserStore.UpdateNotifyProps", success, elapsed)
+	}
+	return err
+}
+
 func (s *TimerLayerUserStore) UpdatePassword(userID string, newPassword string) error {
 	start := timemodule.Now()