[MM-54497]Fix updatePost props (#24543)

* Fix updatePost props and plugin hooks

* Add more tests

* Minor naming improvement

* Revert plugin hooks changes

* Rename post variable

* Fix app layers

---------

Co-authored-by: Mattermost Build <build@mattermost.com>

server/channels/app/app_iface.go

@@ -1135,7 +1135,7 @@ type AppIface interface {
 	UpdatePasswordAsUser(c request.CTX, userID, currentPassword, newPassword string) *model.AppError
 	UpdatePasswordByUserIdSendEmail(c request.CTX, userID, newPassword, method string) *model.AppError
 	UpdatePasswordSendEmail(c request.CTX, user *model.User, newPassword, method string) *model.AppError
-	UpdatePost(c *request.Context, post *model.Post, safeUpdate bool) (*model.Post, *model.AppError)
+	UpdatePost(c *request.Context, receivedUpdatedPost *model.Post, safeUpdate bool) (*model.Post, *model.AppError)
 	UpdatePreferences(userID string, preferences model.Preferences) *model.AppError
 	UpdateRemoteCluster(rc *model.RemoteCluster) (*model.RemoteCluster, *model.AppError)
 	UpdateRemoteClusterTopics(remoteClusterId string, topics string) (*model.RemoteCluster, *model.AppError)

server/channels/app/opentracing/opentracing_layer.go

@@ -17706,7 +17706,7 @@ func (a *OpenTracingAppLayer) UpdatePasswordSendEmail(c request.CTX, user *model
 	return resultVar0
 }
 
-func (a *OpenTracingAppLayer) UpdatePost(c *request.Context, post *model.Post, safeUpdate bool) (*model.Post, *model.AppError) {
+func (a *OpenTracingAppLayer) UpdatePost(c *request.Context, receivedUpdatedPost *model.Post, safeUpdate bool) (*model.Post, *model.AppError) {
 	origCtx := a.ctx
 	span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UpdatePost")
 
@@ -17718,7 +17718,7 @@ func (a *OpenTracingAppLayer) UpdatePost(c *request.Context, post *model.Post, s
 	}()
 
 	defer span.Finish()
-	resultVar0, resultVar1 := a.app.UpdatePost(c, post, safeUpdate)
+	resultVar0, resultVar1 := a.app.UpdatePost(c, receivedUpdatedPost, safeUpdate)
 
 	if resultVar1 != nil {
 		span.LogFields(spanlog.Error(resultVar1))

server/channels/app/post.go

@@ -625,10 +625,10 @@ func (a *App) DeleteEphemeralPost(userID, postID string) {
 	a.Publish(message)
 }
 
-func (a *App) UpdatePost(c *request.Context, post *model.Post, safeUpdate bool) (*model.Post, *model.AppError) {
+func (a *App) UpdatePost(c *request.Context, receivedUpdatedPost *model.Post, safeUpdate bool) (*model.Post, *model.AppError) {
-	post.SanitizeProps()
+	receivedUpdatedPost.SanitizeProps()
 
-	postLists, nErr := a.Srv().Store().Post().Get(context.Background(), post.Id, model.GetPostsOptions{}, "", a.Config().GetSanitizeOptions())
+	postLists, nErr := a.Srv().Store().Post().Get(context.Background(), receivedUpdatedPost.Id, model.GetPostsOptions{}, "", a.Config().GetSanitizeOptions())
 	if nErr != nil {
 		var nfErr *store.ErrNotFound
 		var invErr *store.ErrInvalidInput
@@ -641,21 +641,21 @@ func (a *App) UpdatePost(c *request.Context, post *model.Post, safeUpdate bool)
 			return nil, model.NewAppError("UpdatePost", "app.post.get.app_error", nil, "", http.StatusInternalServerError).Wrap(nErr)
 		}
 	}
-	oldPost := postLists.Posts[post.Id]
+	oldPost := postLists.Posts[receivedUpdatedPost.Id]
 
 	var err *model.AppError
 	if oldPost == nil {
-		err = model.NewAppError("UpdatePost", "api.post.update_post.find.app_error", nil, "id="+post.Id, http.StatusBadRequest)
+		err = model.NewAppError("UpdatePost", "api.post.update_post.find.app_error", nil, "id="+receivedUpdatedPost.Id, http.StatusBadRequest)
 		return nil, err
 	}
 
 	if oldPost.DeleteAt != 0 {
-		err = model.NewAppError("UpdatePost", "api.post.update_post.permissions_details.app_error", map[string]any{"PostId": post.Id}, "", http.StatusBadRequest)
+		err = model.NewAppError("UpdatePost", "api.post.update_post.permissions_details.app_error", map[string]any{"PostId": receivedUpdatedPost.Id}, "", http.StatusBadRequest)
 		return nil, err
 	}
 
 	if oldPost.IsSystemMessage() {
-		err = model.NewAppError("UpdatePost", "api.post.update_post.system_message.app_error", nil, "id="+post.Id, http.StatusBadRequest)
+		err = model.NewAppError("UpdatePost", "api.post.update_post.system_message.app_error", nil, "id="+receivedUpdatedPost.Id, http.StatusBadRequest)
 		return nil, err
 	}
 
@@ -670,17 +670,17 @@ func (a *App) UpdatePost(c *request.Context, post *model.Post, safeUpdate bool)
 
 	newPost := oldPost.Clone()
 
-	if newPost.Message != post.Message {
+	if newPost.Message != receivedUpdatedPost.Message {
-		newPost.Message = post.Message
+		newPost.Message = receivedUpdatedPost.Message
 		newPost.EditAt = model.GetMillis()
-		newPost.Hashtags, _ = model.ParseHashtags(post.Message)
+		newPost.Hashtags, _ = model.ParseHashtags(receivedUpdatedPost.Message)
 	}
 
 	if !safeUpdate {
-		newPost.IsPinned = post.IsPinned
+		newPost.IsPinned = receivedUpdatedPost.IsPinned
-		newPost.HasReactions = post.HasReactions
+		newPost.HasReactions = receivedUpdatedPost.HasReactions
-		newPost.FileIds = post.FileIds
+		newPost.FileIds = receivedUpdatedPost.FileIds
-		newPost.SetProps(post.GetProps())
+		newPost.SetProps(receivedUpdatedPost.GetProps())
 	}
 
 	// Avoid deep-equal checks if EditAt was already modified through message change
@@ -688,19 +688,19 @@ func (a *App) UpdatePost(c *request.Context, post *model.Post, safeUpdate bool)
 		newPost.EditAt = model.GetMillis()
 	}
 
-	if err = a.FillInPostProps(c, post, nil); err != nil {
+	if err = a.FillInPostProps(c, newPost, nil); err != nil {
 		return nil, err
 	}
 
-	if post.IsRemote() {
+	if receivedUpdatedPost.IsRemote() {
-		oldPost.RemoteId = model.NewString(*post.RemoteId)
+		oldPost.RemoteId = model.NewString(*receivedUpdatedPost.RemoteId)
 	}
 
 	var rejectionReason string
 	pluginContext := pluginContext(c)
 	a.ch.RunMultiHook(func(hooks plugin.Hooks) bool {
 		newPost, rejectionReason = hooks.MessageWillBeUpdated(pluginContext, newPost.ForPlugin(), oldPost.ForPlugin())
-		return post != nil
+		return receivedUpdatedPost != nil
 	}, plugin.MessageWillBeUpdatedID)
 	if newPost == nil {
 		return nil, model.NewAppError("UpdatePost", "Post rejected by plugin. "+rejectionReason, nil, "", http.StatusBadRequest)

server/channels/app/post_test.go

@@ -418,7 +418,15 @@ func TestPostChannelMentions(t *testing.T) {
 		TeamId:      th.BasicTeam.Id,
 	}, false)
 	require.Nil(t, err)
+	channelToMention2, err := th.App.CreateChannel(th.Context, &model.Channel{
+		DisplayName: "Mention Test2",
+		Name:        "mention-test2",
+		Type:        model.ChannelTypeOpen,
+		TeamId:      th.BasicTeam.Id,
+	}, false)
+	require.Nil(t, err)
 	defer th.App.PermanentDeleteChannel(th.Context, channelToMention)
+	defer th.App.PermanentDeleteChannel(th.Context, channelToMention2)
 
 	_, err = th.App.AddUserToChannel(th.Context, user, channel, false)
 	require.Nil(t, err)
@@ -440,15 +448,20 @@ func TestPostChannelMentions(t *testing.T) {
 		},
 	}, post.GetProp("channel_mentions"))
 
-	post.Message = fmt.Sprintf("goodbye, ~%v!", channelToMention.Name)
+	post.Message = fmt.Sprintf("goodbye, ~%v!", channelToMention2.Name)
 	result, err := th.App.UpdatePost(th.Context, post, false)
 	require.Nil(t, err)
 	assert.Equal(t, map[string]any{
-		"mention-test": map[string]any{
+		"mention-test2": map[string]any{
-			"display_name": "Mention Test",
+			"display_name": "Mention Test2",
 			"team_name":    th.BasicTeam.Name,
 		},
 	}, result.GetProp("channel_mentions"))
+
+	result.Message = "no more mentions!"
+	result, err = th.App.UpdatePost(th.Context, result, false)
+	require.Nil(t, err)
+	assert.Nil(t, result.GetProp("channel_mentions"))
 }
 
 func TestImageProxy(t *testing.T) {