From 5a89960b640ef9efb98df68a441b828301016a57 Mon Sep 17 00:00:00 2001 From: Rajat Dabade Date: Thu, 9 Feb 2023 12:50:44 +0530 Subject: [PATCH] Fixed duplicate attachment in board template (#4444) * Fixed duplicate attachment in board template * Linter fixes * Duplicate attachment fix * Code optimismed * Linter fixes * Updated test cases * update some error handling, update attachments on duplicate card * Fixed attachment section width --------- Co-authored-by: Mattermost Build Co-authored-by: Scott Bishel Co-authored-by: Harshil Sharma --- server/app/blocks.go | 42 ++++++++++++++++--- server/app/boards.go | 22 ++++++---- webapp/cypress/integration/cardURLProperty.ts | 4 -- .../src/components/cardDetail/attachment.scss | 2 +- webapp/src/mutator.ts | 3 ++ webapp/src/pages/boardPage/boardPage.tsx | 10 +++-- webapp/src/store/attachments.ts | 4 +- 7 files changed, 64 insertions(+), 23 deletions(-) diff --git a/server/app/blocks.go b/server/app/blocks.go index 9031190ed..3ed9d39d4 100644 --- a/server/app/blocks.go +++ b/server/app/blocks.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "path/filepath" + "strings" "github.com/mattermost/focalboard/server/model" "github.com/mattermost/focalboard/server/services/notify" @@ -309,14 +310,26 @@ func (a *App) CopyCardFiles(sourceBoardID string, copiedBlocks []*model.Block) e for i := range copiedBlocks { block := copiedBlocks[i] + fileName := "" + isOk := false - fileName, ok := block.Fields["fileId"] - if !ok || fileName == "" { - continue // doesn't have a file attachment + switch block.Type { + case model.TypeImage: + fileName, isOk = block.Fields["fileId"].(string) + if !isOk || fileName == "" { + continue + } + case model.TypeAttachment: + fileName, isOk = block.Fields["attachmentId"].(string) + if !isOk || fileName == "" { + continue + } + default: + continue } // create unique filename in case we are copying cards within the same board. - ext := filepath.Ext(fileName.(string)) + ext := filepath.Ext(fileName) destFilename := utils.NewID(utils.IDTypeNone) + ext if destBoardID == "" || block.BoardID != destBoardID { @@ -328,7 +341,7 @@ func (a *App) CopyCardFiles(sourceBoardID string, copiedBlocks []*model.Block) e destTeamID = destBoard.TeamID } - sourceFilePath := filepath.Join(sourceBoard.TeamID, sourceBoard.ID, fileName.(string)) + sourceFilePath := filepath.Join(sourceBoard.TeamID, sourceBoard.ID, fileName) destinationFilePath := filepath.Join(destTeamID, block.BoardID, destFilename) a.logger.Debug( @@ -345,7 +358,24 @@ func (a *App) CopyCardFiles(sourceBoardID string, copiedBlocks []*model.Block) e mlog.Err(err), ) } - block.Fields["fileId"] = destFilename + if block.Type == model.TypeAttachment { + block.Fields["attachmentId"] = destFilename + parts := strings.Split(fileName, ".") + fileInfoID := parts[0][1:] + fileInfo, err := a.store.GetFileInfo(fileInfoID) + if err != nil { + return fmt.Errorf("CopyCardFiles: cannot retrieve original fileinfo: %w", err) + } + newParts := strings.Split(destFilename, ".") + newFileID := newParts[0][1:] + fileInfo.Id = newFileID + err = a.store.SaveFileInfo(fileInfo) + if err != nil { + return fmt.Errorf("CopyCardFiles: cannot create fileinfo: %w", err) + } + } else { + block.Fields["fileId"] = destFilename + } } return nil diff --git a/server/app/boards.go b/server/app/boards.go index 479926447..bb5d3ff34 100644 --- a/server/app/boards.go +++ b/server/app/boards.go @@ -202,13 +202,21 @@ func (a *App) DuplicateBoard(boardID, userID, toTeam string, asTemplate bool) (* blockPatches := make([]model.BlockPatch, 0) for _, block := range bab.Blocks { - if fileID, ok := block.Fields["fileId"]; ok { - blockIDs = append(blockIDs, block.ID) - blockPatches = append(blockPatches, model.BlockPatch{ - UpdatedFields: map[string]interface{}{ - "fileId": fileID, - }, - }) + fieldName := "" + if block.Type == model.TypeImage { + fieldName = "fileId" + } else if block.Type == model.TypeAttachment { + fieldName = "attachmentId" + } + if fieldName != "" { + if fieldID, ok := block.Fields[fieldName]; ok { + blockIDs = append(blockIDs, block.ID) + blockPatches = append(blockPatches, model.BlockPatch{ + UpdatedFields: map[string]interface{}{ + fieldName: fieldID, + }, + }) + } } } a.logger.Debug("Duplicate boards patching file IDs", mlog.Int("count", len(blockIDs))) diff --git a/webapp/cypress/integration/cardURLProperty.ts b/webapp/cypress/integration/cardURLProperty.ts index d6a53bba1..61fc2bc20 100644 --- a/webapp/cypress/integration/cardURLProperty.ts +++ b/webapp/cypress/integration/cardURLProperty.ts @@ -98,13 +98,9 @@ describe('Card URL Property', () => { const addView = (type: ViewType) => { cy.log(`**Add ${type} view**`) - // Intercept and wait for getUser request because it is the last one in the effects for BoardPage - // After this last request the BoardPage component will not have additional rerenders - cy.intercept('POST', '/api/v2/users').as('getUser') cy.findByRole('button', {name: 'View menu'}).click() cy.findByText('Add view').realHover() cy.findByRole('button', {name: type}).click() - cy.wait('@getUser') cy.findByRole('textbox', {name: `${type} view`}).should('exist') } diff --git a/webapp/src/components/cardDetail/attachment.scss b/webapp/src/components/cardDetail/attachment.scss index 9c70985f6..ff8902815 100644 --- a/webapp/src/components/cardDetail/attachment.scss +++ b/webapp/src/components/cardDetail/attachment.scss @@ -1,5 +1,6 @@ .Attachment { display: block; + width: 100%; .attachment-header { display: flex; @@ -13,7 +14,6 @@ padding-bottom: 20px; display: flex; overflow-x: auto; - width: 550px; } .attachment-plus-icon { diff --git a/webapp/src/mutator.ts b/webapp/src/mutator.ts index 4110c9981..3dea2ae41 100644 --- a/webapp/src/mutator.ts +++ b/webapp/src/mutator.ts @@ -12,6 +12,7 @@ import {BoardView, ISortOption, createBoardView, KanbanCalculationFields} from ' import {Card, createCard} from './blocks/card' import {ContentBlock} from './blocks/contentBlock' import {CommentBlock} from './blocks/commentBlock' +import {AttachmentBlock} from './blocks/attachmentBlock' import {FilterGroup} from './blocks/filterGroup' import octoClient from './octoClient' import undoManager from './undomanager' @@ -26,6 +27,7 @@ import store from './store' import {updateBoards} from './store/boards' import {updateViews} from './store/views' import {updateCards} from './store/cards' +import {updateAttachments} from './store/attachments' import {updateComments} from './store/comments' import {updateContents} from './store/contents' import {addBoardUsers, removeBoardUsersById} from './store/users' @@ -35,6 +37,7 @@ function updateAllBoardsAndBlocks(boards: Board[], blocks: Block[]) { store.dispatch(updateBoards(boards.filter((b: Board) => b.deleteAt !== 0) as Board[])) store.dispatch(updateViews(blocks.filter((b: Block) => b.type === 'view' || b.deleteAt !== 0) as BoardView[])) store.dispatch(updateCards(blocks.filter((b: Block) => b.type === 'card' || b.deleteAt !== 0) as Card[])) + store.dispatch(updateAttachments(blocks.filter((b: Block) => b.type === 'attachment' || b.deleteAt !== 0) as AttachmentBlock[])) store.dispatch(updateComments(blocks.filter((b: Block) => b.type === 'comment' || b.deleteAt !== 0) as CommentBlock[])) store.dispatch(updateContents(blocks.filter((b: Block) => b.type !== 'card' && b.type !== 'view' && b.type !== 'board' && b.type !== 'comment') as ContentBlock[])) }) diff --git a/webapp/src/pages/boardPage/boardPage.tsx b/webapp/src/pages/boardPage/boardPage.tsx index af3806d25..3e6bbdd44 100644 --- a/webapp/src/pages/boardPage/boardPage.tsx +++ b/webapp/src/pages/boardPage/boardPage.tsx @@ -215,13 +215,15 @@ const BoardPage = (props: Props): JSX.Element => { UserSettings.setLastViewId(match.params.boardId, viewId) } } - - if (!props.readonly && me) { - loadOrJoinBoard(me.id, teamId, match.params.boardId) - } } }, [teamId, match.params.boardId, viewId, me?.id]) + useEffect(() => { + if (match.params.boardId && !props.readonly && me) { + loadOrJoinBoard(me.id, teamId, match.params.boardId) + } + }, [teamId, match.params.boardId, me?.id]) + const handleUnhideBoard = async (boardID: string) => { if (!me || !category) { return diff --git a/webapp/src/store/attachments.ts b/webapp/src/store/attachments.ts index 918867371..879e364e3 100644 --- a/webapp/src/store/attachments.ts +++ b/webapp/src/store/attachments.ts @@ -26,7 +26,9 @@ const attachmentSlice = createSlice({ state.attachmentsByCard[attachment.parentId] = [attachment] return } - state.attachmentsByCard[attachment.parentId].push(attachment) + if (state.attachmentsByCard[attachment.parentId].findIndex((a) => a.id === attachment.id) === -1) { + state.attachmentsByCard[attachment.parentId].push(attachment) + } } else { const parentId = state.attachments[attachment.id]?.parentId if (!state.attachmentsByCard[parentId]) {