From 9bb36614a661d08409f86514228de228e927c370 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 12 Aug 2019 18:35:46 -0300 Subject: [PATCH] MM-17438: allow attaching nouser files to posts (#11837) * tweak AttachToPost formatting * test attaching files uploaded by nouser * MM-17438: allow attaching nouser files to posts --- api4/post_test.go | 54 ++++++++++++++++++++++++++++++ app/plugin_api_test.go | 35 +++++++++++++++++++ store/sqlstore/file_info_store.go | 23 ++++++++----- store/storetest/file_info_store.go | 41 +++++++++++++++++++---- 4 files changed, 138 insertions(+), 15 deletions(-) diff --git a/api4/post_test.go b/api4/post_test.go index 2cc2c83e6a..b00878b50a 100644 --- a/api4/post_test.go +++ b/api4/post_test.go @@ -76,6 +76,60 @@ func TestCreatePost(t *testing.T) { t.Fatal("create at should not match") } + t.Run("with file uploaded by same user", func(t *testing.T) { + fileResp, subResponse := Client.UploadFile([]byte("data"), th.BasicChannel.Id, "test") + CheckNoError(t, subResponse) + fileId := fileResp.FileInfos[0].Id + + postWithFiles, subResponse := Client.CreatePost(&model.Post{ + ChannelId: th.BasicChannel.Id, + Message: "with files", + FileIds: model.StringArray{fileId}, + }) + CheckNoError(t, subResponse) + assert.Equal(t, model.StringArray{fileId}, postWithFiles.FileIds) + + actualPostWithFiles, subResponse := Client.GetPost(postWithFiles.Id, "") + CheckNoError(t, subResponse) + assert.Equal(t, model.StringArray{fileId}, actualPostWithFiles.FileIds) + }) + + t.Run("with file uploaded by different user", func(t *testing.T) { + fileResp, subResponse := th.SystemAdminClient.UploadFile([]byte("data"), th.BasicChannel.Id, "test") + CheckNoError(t, subResponse) + fileId := fileResp.FileInfos[0].Id + + postWithFiles, subResponse := Client.CreatePost(&model.Post{ + ChannelId: th.BasicChannel.Id, + Message: "with files", + FileIds: model.StringArray{fileId}, + }) + CheckNoError(t, subResponse) + assert.Empty(t, postWithFiles.FileIds) + + actualPostWithFiles, subResponse := Client.GetPost(postWithFiles.Id, "") + CheckNoError(t, subResponse) + assert.Empty(t, actualPostWithFiles.FileIds) + }) + + t.Run("with file uploaded by nouser", func(t *testing.T) { + fileInfo, err := th.App.UploadFile([]byte("data"), th.BasicChannel.Id, "test") + require.Nil(t, err) + fileId := fileInfo.Id + + postWithFiles, subResponse := Client.CreatePost(&model.Post{ + ChannelId: th.BasicChannel.Id, + Message: "with files", + FileIds: model.StringArray{fileId}, + }) + CheckNoError(t, subResponse) + assert.Equal(t, model.StringArray{fileId}, postWithFiles.FileIds) + + actualPostWithFiles, subResponse := Client.GetPost(postWithFiles.Id, "") + CheckNoError(t, subResponse) + assert.Equal(t, model.StringArray{fileId}, actualPostWithFiles.FileIds) + }) + post.RootId = "" post.ParentId = "" post.Type = model.POST_SYSTEM_GENERIC diff --git a/app/plugin_api_test.go b/app/plugin_api_test.go index 616b149f8a..69401f64cc 100644 --- a/app/plugin_api_test.go +++ b/app/plugin_api_test.go @@ -319,6 +319,7 @@ func TestPluginAPIGetFile(t *testing.T) { require.NotNil(t, err) require.Nil(t, data) } + func TestPluginAPISavePluginConfig(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() @@ -1244,3 +1245,37 @@ func TestPluginCreateBot(t *testing.T) { require.NotNil(t, err) } + +func TestPluginCreatePostWithUploadedFile(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + api := th.SetupPluginAPI() + + data := []byte("Hello World") + channelId := th.BasicChannel.Id + filename := "testGetFile" + fileInfo, err := api.UploadFile(data, channelId, filename) + require.Nil(t, err) + defer func() { + th.App.Srv.Store.FileInfo().PermanentDelete(fileInfo.Id) + th.App.RemoveFile(fileInfo.Path) + }() + + actualData, err := api.GetFile(fileInfo.Id) + require.Nil(t, err) + assert.Equal(t, data, actualData) + + userId := th.BasicUser.Id + post, err := api.CreatePost(&model.Post{ + Message: "test", + UserId: userId, + ChannelId: channelId, + FileIds: model.StringArray{fileInfo.Id}, + }) + require.Nil(t, err) + assert.Equal(t, model.StringArray{fileInfo.Id}, post.FileIds) + + actualPost, err := api.GetPost(post.Id) + require.Nil(t, err) + assert.Equal(t, model.StringArray{fileInfo.Id}, actualPost.FileIds) +} diff --git a/store/sqlstore/file_info_store.go b/store/sqlstore/file_info_store.go index 6e9298b313..156840968d 100644 --- a/store/sqlstore/file_info_store.go +++ b/store/sqlstore/file_info_store.go @@ -196,15 +196,20 @@ func (fs SqlFileInfoStore) GetForUser(userId string) ([]*model.FileInfo, *model. } func (fs SqlFileInfoStore) AttachToPost(fileId, postId, creatorId string) *model.AppError { - sqlResult, err := fs.GetMaster().Exec( - `UPDATE - FileInfo - SET - PostId = :PostId - WHERE - Id = :Id - AND PostId = '' - AND CreatorId = :CreatorId`, map[string]interface{}{"PostId": postId, "Id": fileId, "CreatorId": creatorId}) + sqlResult, err := fs.GetMaster().Exec(` + UPDATE + FileInfo + SET + PostId = :PostId + WHERE + Id = :Id + AND PostId = '' + AND (CreatorId = :CreatorId OR CreatorId = 'nouser') + `, map[string]interface{}{ + "PostId": postId, + "Id": fileId, + "CreatorId": creatorId, + }) if err != nil { return model.NewAppError("SqlFileInfoStore.AttachToPost", "store.sql_file_info.attach_to_post.app_error", nil, "post_id="+postId+", file_id="+fileId+", err="+err.Error(), http.StatusInternalServerError) diff --git a/store/storetest/file_info_store.go b/store/storetest/file_info_store.go index f7c56174fc..237525116a 100644 --- a/store/storetest/file_info_store.go +++ b/store/storetest/file_info_store.go @@ -5,6 +5,7 @@ package storetest import ( "fmt" + "sort" "testing" "github.com/mattermost/mattermost-server/model" @@ -255,6 +256,12 @@ func testFileInfoGetForUser(t *testing.T, ss store.Store) { assert.Len(t, userPosts, 1) } +type byFileInfoId []*model.FileInfo + +func (a byFileInfoId) Len() int { return len(a) } +func (a byFileInfoId) Swap(i, j int) { a[i], a[j] = a[j], a[i] } +func (a byFileInfoId) Less(i, j int) bool { return a[i].Id < a[j].Id } + func testFileInfoAttachToPost(t *testing.T, ss store.Store) { t.Run("should attach files", func(t *testing.T) { userId := model.NewId() @@ -276,16 +283,19 @@ func testFileInfoAttachToPost(t *testing.T, ss store.Store) { err = ss.FileInfo().AttachToPost(info1.Id, postId, userId) assert.Nil(t, err) + info1.PostId = postId err = ss.FileInfo().AttachToPost(info2.Id, postId, userId) assert.Nil(t, err) + info2.PostId = postId data, err := ss.FileInfo().GetForPost(postId, true, false, false) - assert.Nil(t, err) + require.Nil(t, err) - assert.Len(t, data, 2) - assert.True(t, data[0].Id == info1.Id || data[0].Id == info2.Id) - assert.True(t, data[1].Id == info1.Id || data[1].Id == info2.Id) + expected := []*model.FileInfo{info1, info2} + sort.Sort(byFileInfoId(expected)) + sort.Sort(byFileInfoId(data)) + assert.Equal(t, expected, data) }) t.Run("should not attach files to multiple posts", func(t *testing.T) { @@ -301,10 +311,10 @@ func testFileInfoAttachToPost(t *testing.T, ss store.Store) { require.Equal(t, "", info.PostId) err = ss.FileInfo().AttachToPost(info.Id, model.NewId(), userId) - assert.Nil(t, err) + require.Nil(t, err) err = ss.FileInfo().AttachToPost(info.Id, postId, userId) - assert.NotNil(t, err) + require.NotNil(t, err) }) t.Run("should not attach files owned from a different user", func(t *testing.T) { @@ -322,6 +332,25 @@ func testFileInfoAttachToPost(t *testing.T, ss store.Store) { err = ss.FileInfo().AttachToPost(info.Id, postId, userId) assert.NotNil(t, err) }) + + t.Run("should attach files uploaded by nouser", func(t *testing.T) { + postId := model.NewId() + + info, err := ss.FileInfo().Save(&model.FileInfo{ + CreatorId: "nouser", + Path: "file.txt", + }) + require.Nil(t, err) + assert.Equal(t, "", info.PostId) + + err = ss.FileInfo().AttachToPost(info.Id, postId, model.NewId()) + require.Nil(t, err) + + data, err := ss.FileInfo().GetForPost(postId, true, false, false) + require.Nil(t, err) + info.PostId = postId + assert.Equal(t, []*model.FileInfo{info}, data) + }) } func testFileInfoDeleteForPost(t *testing.T, ss store.Store) {