MM-17412: update post incorrectly removing files (#11834)

* leverage testify

* improve TestUpdatePost tests

* improve TestPatchPost

* assert unchanged fileIds after UpdatePost

* MM-17412: retain existing FileIds on post update

api4/post.go

@@ -501,9 +501,6 @@ func updatePost(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// Updating the file_ids of a post is not a supported operation and will be ignored
-	post.FileIds = nil
-
 	if !c.App.SessionHasPermissionToChannelByPost(c.App.Session, c.Params.PostId, model.PERMISSION_EDIT_POST) {
 		c.SetPermissionError(model.PERMISSION_EDIT_POST)
 		return
@@ -515,6 +512,9 @@ func updatePost(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Updating the file_ids of a post is not a supported operation and will be ignored
+	post.FileIds = originalPost.FileIds
+
 	if c.App.Session.UserId != originalPost.UserId {
 		if !c.App.SessionHasPermissionToChannelByPost(c.App.Session, c.Params.PostId, model.PERMISSION_EDIT_OTHERS_POSTS) {
 			c.SetPermissionError(model.PERMISSION_EDIT_OTHERS_POSTS)

api4/post_test.go

@@ -587,95 +587,154 @@ func TestUpdatePost(t *testing.T) {
 
 	th.App.SetLicense(model.NewTestLicense())
 
-	post := &model.Post{ChannelId: channel.Id, Message: "zz" + model.NewId() + "a"}
+	fileIds := make([]string, 3)
-	rpost, resp := Client.CreatePost(post)
+	data, err := testutils.ReadTestFile("test.png")
+	require.Nil(t, err)
+	for i := 0; i < len(fileIds); i++ {
+		fileResp, resp := Client.UploadFile(data, channel.Id, "test.png")
 		CheckNoError(t, resp)
-
+		fileIds[i] = fileResp.FileInfos[0].Id
-	if rpost.Message != post.Message {
-		t.Fatal("full name didn't match")
 	}
 
-	if rpost.EditAt != 0 {
+	rpost, err := th.App.CreatePost(&model.Post{
-		t.Fatal("Newly created post shouldn't have EditAt set")
+		UserId:    th.BasicUser.Id,
-	}
+		ChannelId: channel.Id,
+		Message:   "zz" + model.NewId() + "a",
+		FileIds:   fileIds,
+	}, channel, false)
+	require.Nil(t, err)
 
+	assert.Equal(t, rpost.Message, rpost.Message, "full name didn't match")
+	assert.EqualValues(t, 0, rpost.EditAt, "Newly created post shouldn't have EditAt set")
+	assert.Equal(t, model.StringArray(fileIds), rpost.FileIds, "FileIds should have been set")
+
+	t.Run("same message, fewer files", func(t *testing.T) {
 		msg := "zz" + model.NewId() + " update post"
 		rpost.Message = msg
 		rpost.UserId = ""
 
-	rupost, resp := Client.UpdatePost(rpost.Id, rpost)
+		rupost, resp := Client.UpdatePost(rpost.Id, &model.Post{
+			Id:      rpost.Id,
+			Message: rpost.Message,
+			FileIds: fileIds[0:2], // one fewer file id
+		})
 		CheckNoError(t, resp)
 
-	if rupost.Message != msg {
+		assert.Equal(t, rupost.Message, msg, "failed to updates")
-		t.Fatal("failed to updates")
+		assert.NotEqual(t, 0, rupost.EditAt, "EditAt not updated for post")
-	}
+		assert.Equal(t, model.StringArray(fileIds), rupost.FileIds, "FileIds should have not have been updated")
-	if rupost.EditAt == 0 {
-		t.Fatal("EditAt not updated for post")
-	}
 
+		actual, resp := Client.GetPost(rpost.Id, "")
+		CheckNoError(t, resp)
+
+		assert.Equal(t, actual.Message, msg, "failed to updates")
+		assert.NotEqual(t, 0, actual.EditAt, "EditAt not updated for post")
+		assert.Equal(t, model.StringArray(fileIds), actual.FileIds, "FileIds should have not have been updated")
+	})
+
+	t.Run("new message, invalid props", func(t *testing.T) {
 		msg1 := "#hashtag a" + model.NewId() + " update post again"
 		rpost.Message = msg1
 		rpost.Props[model.PROPS_ADD_CHANNEL_MEMBER] = "no good"
 		rrupost, resp := Client.UpdatePost(rpost.Id, rpost)
 		CheckNoError(t, resp)
 
-	if rrupost.Message != msg1 && rrupost.Hashtags != "#hashtag" {
+		assert.Equal(t, msg1, rrupost.Message, "failed to update message")
-		t.Fatal("failed to updates")
+		assert.Equal(t, "#hashtag", rrupost.Hashtags, "failed to update hashtags")
-	}
+		assert.Nil(t, rrupost.Props[model.PROPS_ADD_CHANNEL_MEMBER], "failed to sanitize Props['add_channel_member'], should be nil")
 
-	if rrupost.Props[model.PROPS_ADD_CHANNEL_MEMBER] != nil {
+		actual, resp := Client.GetPost(rpost.Id, "")
-		t.Fatal("failed to sanitize Props['add_channel_member'], should be nil")
+		CheckNoError(t, resp)
-	}
 
-	rpost2, err := th.App.CreatePost(&model.Post{ChannelId: channel.Id, Message: "zz" + model.NewId() + "a", Type: model.POST_JOIN_LEAVE, UserId: th.BasicUser.Id}, channel, false)
+		assert.Equal(t, msg1, actual.Message, "failed to update message")
-	if err != nil {
+		assert.Equal(t, "#hashtag", actual.Hashtags, "failed to update hashtags")
-		t.Fatal(err)
+		assert.Nil(t, actual.Props[model.PROPS_ADD_CHANNEL_MEMBER], "failed to sanitize Props['add_channel_member'], should be nil")
-	}
+	})
 
-	up2 := &model.Post{Id: rpost2.Id, ChannelId: channel.Id, Message: "zz" + model.NewId() + " update post 2"}
+	t.Run("join/leave post", func(t *testing.T) {
-	_, resp = Client.UpdatePost(rpost2.Id, up2)
+		rpost2, err := th.App.CreatePost(&model.Post{
+			ChannelId: channel.Id,
+			Message:   "zz" + model.NewId() + "a",
+			Type:      model.POST_JOIN_LEAVE,
+			UserId:    th.BasicUser.Id,
+		}, channel, false)
+		require.Nil(t, err)
+
+		up2 := &model.Post{
+			Id:        rpost2.Id,
+			ChannelId: channel.Id,
+			Message:   "zz" + model.NewId() + " update post 2",
+		}
+		_, resp := Client.UpdatePost(rpost2.Id, up2)
 		CheckBadRequestStatus(t, resp)
+	})
 
-	rpost3, err := th.App.CreatePost(&model.Post{ChannelId: channel.Id, Message: "zz" + model.NewId() + "a", UserId: th.BasicUser.Id}, channel, false)
+	rpost3, err := th.App.CreatePost(&model.Post{
-	if err != nil {
+		ChannelId: channel.Id,
-		t.Fatal(err)
+		Message:   "zz" + model.NewId() + "a",
+		UserId:    th.BasicUser.Id,
+	}, channel, false)
+	require.Nil(t, err)
+
+	t.Run("new message, add files", func(t *testing.T) {
+		up3 := &model.Post{
+			Id:        rpost3.Id,
+			ChannelId: channel.Id,
+			Message:   "zz" + model.NewId() + " update post 3",
+			FileIds:   fileIds[0:2],
 		}
-
-	fileIds := model.StringArray{"abcdef", "geh"}
-	up3 := &model.Post{Id: rpost3.Id, ChannelId: channel.Id, Message: "zz" + model.NewId() + " update post 3", FileIds: fileIds}
 		rrupost3, resp := Client.UpdatePost(rpost3.Id, up3)
 		CheckNoError(t, resp)
 		assert.Empty(t, rrupost3.FileIds)
 
-	up4 := &model.Post{Id: rpost3.Id, ChannelId: channel.Id, Message: "zz" + model.NewId() + " update post 3"}
+		actual, resp := Client.GetPost(rpost.Id, "")
+		CheckNoError(t, resp)
+		assert.Equal(t, model.StringArray(fileIds), actual.FileIds)
+	})
+
+	t.Run("add slack attachments", func(t *testing.T) {
+		up4 := &model.Post{
+			Id:        rpost3.Id,
+			ChannelId: channel.Id,
+			Message:   "zz" + model.NewId() + " update post 3",
+		}
 		up4.AddProp("attachments", []model.SlackAttachment{
 			{
 				Text: "Hello World",
 			},
 		})
-	rrupost3, resp = Client.UpdatePost(rpost3.Id, up4)
+		rrupost3, resp := Client.UpdatePost(rpost3.Id, up4)
 		CheckNoError(t, resp)
 		assert.NotEqual(t, rpost3.EditAt, rrupost3.EditAt)
 		assert.NotEqual(t, rpost3.Attachments(), rrupost3.Attachments())
+	})
 
+	t.Run("logged out", func(t *testing.T) {
 		Client.Logout()
-	_, resp = Client.UpdatePost(rpost.Id, rpost)
+		_, resp := Client.UpdatePost(rpost.Id, rpost)
 		CheckUnauthorizedStatus(t, resp)
+	})
 
+	t.Run("different user", func(t *testing.T) {
 		th.LoginBasic2()
-	_, resp = Client.UpdatePost(rpost.Id, rpost)
+		_, resp := Client.UpdatePost(rpost.Id, rpost)
 		CheckForbiddenStatus(t, resp)
 
 		Client.Logout()
+	})
 
+	t.Run("different user, but team admin", func(t *testing.T) {
 		th.LoginTeamAdmin()
-	_, resp = Client.UpdatePost(rpost.Id, rpost)
+		_, resp := Client.UpdatePost(rpost.Id, rpost)
 		CheckForbiddenStatus(t, resp)
 
 		Client.Logout()
+	})
 
-	_, resp = th.SystemAdminClient.UpdatePost(rpost.Id, rpost)
+	t.Run("different user, but system admin", func(t *testing.T) {
+		_, resp := th.SystemAdminClient.UpdatePost(rpost.Id, rpost)
 		CheckNoError(t, resp)
+	})
 }
 
 func TestUpdateOthersPostInDirectMessageChannel(t *testing.T) {
@@ -711,98 +770,105 @@ func TestPatchPost(t *testing.T) {
 
 	th.App.SetLicense(model.NewTestLicense())
 
+	fileIds := make([]string, 3)
+	data, err := testutils.ReadTestFile("test.png")
+	require.Nil(t, err)
+	for i := 0; i < len(fileIds); i++ {
+		fileResp, resp := Client.UploadFile(data, channel.Id, "test.png")
+		CheckNoError(t, resp)
+		fileIds[i] = fileResp.FileInfos[0].Id
+	}
+
 	post := &model.Post{
 		ChannelId:    channel.Id,
 		IsPinned:     true,
 		Message:      "#hashtag a message",
 		Props:        model.StringInterface{"channel_header": "old_header"},
-		FileIds:      model.StringArray{"file1", "file2"},
+		FileIds:      fileIds[0:2],
 		HasReactions: true,
 	}
 	post, _ = Client.CreatePost(post)
 
+	var rpost *model.Post
+	t.Run("new message, props, files, HasReactions bit", func(t *testing.T) {
 		patch := &model.PostPatch{}
 
 		patch.IsPinned = model.NewBool(false)
 		patch.Message = model.NewString("#otherhashtag other message")
-	patch.Props = new(model.StringInterface)
+		patch.Props = &model.StringInterface{"channel_header": "new_header"}
-	*patch.Props = model.StringInterface{"channel_header": "new_header"}
+		patchFileIds := model.StringArray(fileIds) // one extra file
-	patch.FileIds = new(model.StringArray)
+		patch.FileIds = &patchFileIds
-	*patch.FileIds = model.StringArray{"file1", "otherfile2", "otherfile3"}
 		patch.HasReactions = model.NewBool(false)
 
-	rpost, resp := Client.PatchPost(post.Id, patch)
+		var resp *model.Response
+		rpost, resp = Client.PatchPost(post.Id, patch)
 		CheckNoError(t, resp)
 
-	if rpost.IsPinned {
+		assert.False(t, rpost.IsPinned, "IsPinned did not update properly")
-		t.Fatal("IsPinned did not update properly")
+		assert.Equal(t, "#otherhashtag other message", rpost.Message, "Message did not update properly")
-	}
+		assert.Equal(t, *patch.Props, rpost.Props, "Props did not update properly")
-	if rpost.Message != "#otherhashtag other message" {
+		assert.Equal(t, "#otherhashtag", rpost.Hashtags, "Message did not update properly")
-		t.Fatal("Message did not update properly")
+		assert.Equal(t, model.StringArray(fileIds[0:2]), rpost.FileIds, "FileIds should not update")
-	}
+		assert.False(t, rpost.HasReactions, "HasReactions did not update properly")
-	if len(rpost.Props) != 1 {
+	})
-		t.Fatal("Props did not update properly")
-	}
-	if !reflect.DeepEqual(rpost.Props, *patch.Props) {
-		t.Fatal("Props did not update properly")
-	}
-	if rpost.Hashtags != "#otherhashtag" {
-		t.Fatal("Message did not update properly")
-	}
-	if len(rpost.FileIds) == 3 {
-		t.Fatal("FileIds should not update properly")
-	}
-	if reflect.DeepEqual(rpost.FileIds, *patch.FileIds) {
-		t.Fatal("FileIds should not update")
-	}
-	if rpost.HasReactions {
-		t.Fatal("HasReactions did not update properly")
-	}
 
+	t.Run("add slack attachments", func(t *testing.T) {
 		patch2 := &model.PostPatch{}
 		attachments := []model.SlackAttachment{
 			{
 				Text: "Hello World",
 			},
 		}
-	patch2.Props = new(model.StringInterface)
+		patch2.Props = &model.StringInterface{"attachments": attachments}
-	*patch2.Props = model.StringInterface{"attachments": attachments}
 
 		rpost2, resp := Client.PatchPost(post.Id, patch2)
 		CheckNoError(t, resp)
 		assert.NotEmpty(t, rpost2.Props["attachments"])
 		assert.NotEqual(t, rpost.EditAt, rpost2.EditAt)
+	})
 
-	if r, err := Client.DoApiPut("/posts/"+post.Id+"/patch", "garbage"); err == nil {
+	t.Run("invalid requests", func(t *testing.T) {
-		t.Fatal("should have errored")
+		r, err := Client.DoApiPut("/posts/"+post.Id+"/patch", "garbage")
-	} else {
+		require.EqualError(t, err, ": Invalid or missing post in request body, ")
-		if r.StatusCode != http.StatusBadRequest {
+		require.Equal(t, http.StatusBadRequest, r.StatusCode, "wrong status code")
-			t.Log("actual: " + strconv.Itoa(r.StatusCode))
-			t.Log("expected: " + strconv.Itoa(http.StatusBadRequest))
-			t.Fatal("wrong status code")
-		}
-	}
 
-	_, resp = Client.PatchPost("junk", patch)
+		patch := &model.PostPatch{}
+		_, resp := Client.PatchPost("junk", patch)
 		CheckBadRequestStatus(t, resp)
+	})
 
-	_, resp = Client.PatchPost(GenerateTestId(), patch)
+	t.Run("unknown post", func(t *testing.T) {
+		patch := &model.PostPatch{}
+		_, resp := Client.PatchPost(GenerateTestId(), patch)
 		CheckForbiddenStatus(t, resp)
+	})
 
+	t.Run("logged out", func(t *testing.T) {
 		Client.Logout()
-	_, resp = Client.PatchPost(post.Id, patch)
+		patch := &model.PostPatch{}
+		_, resp := Client.PatchPost(post.Id, patch)
 		CheckUnauthorizedStatus(t, resp)
+	})
 
+	t.Run("different user", func(t *testing.T) {
 		th.LoginBasic2()
-	_, resp = Client.PatchPost(post.Id, patch)
+		patch := &model.PostPatch{}
+		_, resp := Client.PatchPost(post.Id, patch)
 		CheckForbiddenStatus(t, resp)
+	})
 
+	t.Run("different user, but team admin", func(t *testing.T) {
 		th.LoginTeamAdmin()
-	_, resp = Client.PatchPost(post.Id, patch)
+		patch := &model.PostPatch{}
+		_, resp := Client.PatchPost(post.Id, patch)
 		CheckForbiddenStatus(t, resp)
+	})
 
-	_, resp = th.SystemAdminClient.PatchPost(post.Id, patch)
+	t.Run("different user, but system admin", func(t *testing.T) {
+		patch := &model.PostPatch{}
+		_, resp := th.SystemAdminClient.PatchPost(post.Id, patch)
 		CheckNoError(t, resp)
+	})
 }
 
 func TestPinPost(t *testing.T) {