Fixed critical bugs in Mattermost Boards plugin

server/api/files.go

@@ -173,6 +173,12 @@ func (a *API) handleServeFile(w http.ResponseWriter, r *http.Request) {
 		// if err is still not nil then it is an error other than `not found` so we must
 		// return the error to the requestor.  fileReader and Fileinfo are nil in this case.
 		a.errorResponse(w, r, err)
+		return
+	}
+
+	if fileReader == nil {
+		a.errorResponse(w, r, model.NewErrNotFound("file reader is nil"))
+		return
 	}
 
 	defer fileReader.Close()

server/app/files.go

@@ -213,6 +213,19 @@ func (a *App) GetFilePath(teamID, boardID, fileName string) (*mm_model.FileInfo,
 			return nil, "", fmt.Errorf("invalid fileName in GetFilePath: %w", err)
 		}
 		filePath = filepath.Join(teamID, boardID, fileName)
+
+		// FIXUP: Check the deprecated old location for template files (GlobalTeamID)
+		// This matches the logic in GetFileReader to handle legacy template files
+		if teamID == model.GlobalTeamID {
+			exists, err := a.filesBackend.FileExists(filePath)
+			if err == nil && !exists {
+				oldExists, err2 := a.filesBackend.FileExists(fileName)
+				if err2 == nil && oldExists {
+					// File exists in old location, return that path
+					filePath = fileName
+				}
+			}
+		}
 	}
 
 	return fileInfo, filePath, nil
@@ -285,7 +298,7 @@ func (a *App) GetFileReader(teamID, boardID, filename string) (filestore.ReadClo
 		return nil, err
 	}
 	// FIXUP: Check the deprecated old location
-	if teamID == "0" && !exists {
+	if teamID == model.GlobalTeamID && !exists {
 		oldExists, err2 := a.filesBackend.FileExists(filename)
 		if err2 != nil {
 			return nil, err2
@@ -346,58 +359,179 @@ func (a *App) MoveFile(channelID, teamID, boardID, filename string) error {
 	return nil
 }
 
+// processFileIDPatch handles patching for fileID field in a block.
+func (a *App) processFileIDPatch(block *model.Block, fileID string, newFileNames map[string]string, blockIDs []string, blockPatches []model.BlockPatch) ([]string, []model.BlockPatch, error) {
+	if err := model.ValidateFileId(fileID); err != nil {
+		errMessage := fmt.Sprintf("invalid characters in block with key: %s, %s", block.Fields[model.BlockFieldFileId], err)
+		return nil, nil, model.NewErrBadRequest(errMessage)
+	}
+
+	// Only patch if the file was successfully copied (exists in newFileNames)
+	if newFileName, exists := newFileNames[fileID]; exists && newFileName != "" {
+		blockIDs = append(blockIDs, block.ID)
+		blockPatches = append(blockPatches, model.BlockPatch{
+			UpdatedFields: map[string]interface{}{
+				model.BlockFieldFileId: newFileName,
+			},
+			DeletedFields: []string{model.BlockFieldAttachmentId},
+		})
+	} else {
+		// File was skipped (doesn't exist or copy failed)
+		// Remove the file reference from the block so frontend doesn't try to load missing file
+		blockIDs = append(blockIDs, block.ID)
+		blockPatches = append(blockPatches, model.BlockPatch{
+			DeletedFields: []string{model.BlockFieldFileId, model.BlockFieldAttachmentId},
+		})
+		a.logger.Debug(
+			"CopyAndUpdateCardFiles: Removing file reference from block (file not found/copied)",
+			mlog.String("blockID", block.ID),
+			mlog.String("fileID", fileID),
+		)
+	}
+
+	return blockIDs, blockPatches, nil
+}
+
+// processAttachmentIDPatch handles patching for attachmentID field in a block.
+func (a *App) processAttachmentIDPatch(block *model.Block, attachmentID string, newFileNames map[string]string, blockIDs []string, blockPatches []model.BlockPatch) ([]string, []model.BlockPatch, error) {
+	if err := model.ValidateFileId(attachmentID); err != nil {
+		errMessage := fmt.Sprintf("invalid characters in block with key: %s, %s", block.Fields[model.BlockFieldAttachmentId], err)
+		return nil, nil, model.NewErrBadRequest(errMessage)
+	}
+
+	// Only patch if the file was successfully copied (exists in newFileNames)
+	if newFileName, exists := newFileNames[attachmentID]; exists && newFileName != "" {
+		// Check if this block is already in the patch list (has fileID)
+		blockAlreadyPatched := false
+		for i, bid := range blockIDs {
+			if bid == block.ID {
+				// Block already being patched, update the patch instead
+				blockPatches[i].UpdatedFields[model.BlockFieldAttachmentId] = newFileName
+				// Remove attachmentId from DeletedFields if it's there
+				for j, df := range blockPatches[i].DeletedFields {
+					if df == model.BlockFieldAttachmentId {
+						blockPatches[i].DeletedFields = append(blockPatches[i].DeletedFields[:j], blockPatches[i].DeletedFields[j+1:]...)
+						break
+					}
+				}
+				blockAlreadyPatched = true
+				break
+			}
+		}
+		if !blockAlreadyPatched {
+			blockIDs = append(blockIDs, block.ID)
+			blockPatches = append(blockPatches, model.BlockPatch{
+				UpdatedFields: map[string]interface{}{
+					model.BlockFieldAttachmentId: newFileName,
+				},
+				DeletedFields: []string{model.BlockFieldFileId},
+			})
+		}
+	} else {
+		// File was skipped (doesn't exist or copy failed)
+		// Check if block is already in patch list (might have fileID patch)
+		blockAlreadyInPatch := false
+		for i, bid := range blockIDs {
+			if bid == block.ID {
+				// Block already being patched, just add attachmentId to DeletedFields
+				blockAlreadyInPatch = true
+				// Check if attachmentId is not already in DeletedFields
+				alreadyDeleted := false
+				for _, df := range blockPatches[i].DeletedFields {
+					if df == model.BlockFieldAttachmentId {
+						alreadyDeleted = true
+						break
+					}
+				}
+				if !alreadyDeleted {
+					blockPatches[i].DeletedFields = append(blockPatches[i].DeletedFields, model.BlockFieldAttachmentId)
+				}
+				break
+			}
+		}
+		if !blockAlreadyInPatch {
+			// Remove the attachment reference from the block
+			blockIDs = append(blockIDs, block.ID)
+			blockPatches = append(blockPatches, model.BlockPatch{
+				DeletedFields: []string{model.BlockFieldFileId, model.BlockFieldAttachmentId},
+			})
+		}
+		a.logger.Debug(
+			"CopyAndUpdateCardFiles: Removing attachment reference from block (file not found/copied)",
+			mlog.String("blockID", block.ID),
+			mlog.String("attachmentID", attachmentID),
+		)
+	}
+
+	return blockIDs, blockPatches, nil
+}
+
+// updateInMemoryBlocks applies patches to in-memory block objects.
+func (a *App) updateInMemoryBlocks(blocks []*model.Block, blockIDs []string, blockPatches []model.BlockPatch) {
+	// Create a map for quick lookup of patches by block ID
+	patchMap := make(map[string]*model.BlockPatch)
+	for i, blockID := range blockIDs {
+		patchMap[blockID] = &blockPatches[i]
+	}
+
+	// CRITICAL: Update blocks in memory so frontend receives updated file IDs
+	for _, block := range blocks {
+		if patch, exists := patchMap[block.ID]; exists {
+			for key, value := range patch.UpdatedFields {
+				block.Fields[key] = value
+			}
+			for _, key := range patch.DeletedFields {
+				delete(block.Fields, key)
+			}
+		}
+	}
+}
+
 func (a *App) CopyAndUpdateCardFiles(boardID, userID string, blocks []*model.Block, asTemplate bool) error {
 	newFileNames, err := a.CopyCardFiles(boardID, blocks, asTemplate)
 	if err != nil {
 		a.logger.Error("Could not copy files while duplicating board", mlog.String("BoardID", boardID), mlog.Err(err))
 	}
 
 	// blocks now has updated file ids for any blocks containing files.  We need to update the database for them.
+	// Only patch blocks whose files were successfully copied (exist in newFileNames)
 	blockIDs := make([]string, 0)
 	blockPatches := make([]model.BlockPatch, 0)
 	for _, block := range blocks {
 		if block.Type == model.TypeImage || block.Type == model.TypeAttachment {
-			if fileID, ok := block.Fields[model.BlockFieldFileId].(string); ok {
-				if err = model.ValidateFileId(fileID); err == nil {
-					blockIDs = append(blockIDs, block.ID)
-					blockPatches = append(blockPatches, model.BlockPatch{
-						UpdatedFields: map[string]interface{}{
-							model.BlockFieldFileId: newFileNames[fileID],
-						},
-						DeletedFields: []string{model.BlockFieldAttachmentId},
-					})
-				} else {
-					errMessage := fmt.Sprintf("invalid characters in block with key: %s, %s", block.Fields[model.BlockFieldFileId], err)
-					return model.NewErrBadRequest(errMessage)
+			if fileID, ok := block.Fields[model.BlockFieldFileId].(string); ok && fileID != "" {
+				var patchErr error
+				blockIDs, blockPatches, patchErr = a.processFileIDPatch(block, fileID, newFileNames, blockIDs, blockPatches)
+				if patchErr != nil {
+					return patchErr
 				}
 			}
 
-			if attachmentID, ok := block.Fields[model.BlockFieldAttachmentId].(string); ok {
-				if err = model.ValidateFileId(attachmentID); err == nil {
-					blockIDs = append(blockIDs, block.ID)
-					blockPatches = append(blockPatches, model.BlockPatch{
-						UpdatedFields: map[string]interface{}{
-							model.BlockFieldAttachmentId: newFileNames[attachmentID],
-						},
-						DeletedFields: []string{model.BlockFieldFileId},
-					})
-				} else {
-					errMessage := fmt.Sprintf("invalid characters in block with key: %s, %s", block.Fields[model.BlockFieldAttachmentId], err)
-					return model.NewErrBadRequest(errMessage)
+			if attachmentID, ok := block.Fields[model.BlockFieldAttachmentId].(string); ok && attachmentID != "" {
+				var patchErr error
+				blockIDs, blockPatches, patchErr = a.processAttachmentIDPatch(block, attachmentID, newFileNames, blockIDs, blockPatches)
+				if patchErr != nil {
+					return patchErr
 				}
 			}
 		}
 	}
-	a.logger.Debug("Duplicate boards patching file IDs", mlog.Int("count", len(blockIDs)))
-
 	if len(blockIDs) != 0 {
 		patches := &model.BlockPatchBatch{
 			BlockIDs:     blockIDs,
 			BlockPatches: blockPatches,
 		}
 		if err := a.store.PatchBlocks(patches, userID); err != nil {
+			a.logger.Error(
+				"CopyAndUpdateCardFiles: Failed to patch blocks",
+				mlog.String("boardID", boardID),
+				mlog.Err(err),
+			)
 			return fmt.Errorf("could not patch file IDs while duplicating board %s: %w", boardID, err)
 		}
+
+		a.updateInMemoryBlocks(blocks, blockIDs, blockPatches)
+		a.logger.Debug("Duplicate boards patching file IDs", mlog.Int("count", len(blockIDs)))
 	}
 
 	return nil
@@ -431,6 +565,11 @@ func (a *App) CopyCardFiles(sourceBoardID string, copiedBlocks []*model.Block, a
 			}
 		}
 
+		// Check if we've already processed this fileID (multiple blocks can reference same file)
+		if _, alreadyProcessed := newFileNames[fileID]; alreadyProcessed {
+			continue
+		}
+
 		if err = model.ValidateFileId(fileID); err != nil {
 			errMessage := fmt.Sprintf("Could not validate file ID while duplicating board with fileId: %s", fileID)
 			return nil, model.NewErrBadRequest(errMessage)
@@ -462,30 +601,77 @@ func (a *App) CopyCardFiles(sourceBoardID string, copiedBlocks []*model.Block, a
 			return nil, fmt.Errorf("invalid destination file path: %w", pathErr)
 		}
 
+		// Check if source file exists before attempting to copy
+		exists, err := a.filesBackend.FileExists(sourceFilePath)
+		if err != nil {
+			a.logger.Error(
+				"CopyCardFiles failed to check if source file exists",
+				mlog.String("sourceFilePath", sourceFilePath),
+				mlog.String("fileID", fileID),
+				mlog.Err(err),
+			)
+			continue
+		}
+		if !exists {
+			a.logger.Warn(
+				"CopyCardFiles: source file does not exist, skipping copy",
+				mlog.String("sourceFilePath", sourceFilePath),
+				mlog.String("fileID", fileID),
+				mlog.String("boardID", sourceBoardID),
+			)
+			continue
+		}
+
+		// Copy the file FIRST before saving FileInfo to ensure file exists when FileInfo is queried
+		if err = a.filesBackend.CopyFile(sourceFilePath, destinationFilePath); err != nil {
+			a.logger.Error(
+				"CopyCardFiles failed to copy file",
+				mlog.String("sourceFilePath", sourceFilePath),
+				mlog.String("destinationFilePath", destinationFilePath),
+				mlog.String("fileID", fileID),
+				mlog.Err(err),
+			)
+			continue
+		}
+
+		// Verify the copied file exists before saving FileInfo
+		destExists, err := a.filesBackend.FileExists(destinationFilePath)
+		if err != nil {
+			a.logger.Error(
+				"CopyCardFiles failed to verify copied file exists",
+				mlog.String("destinationFilePath", destinationFilePath),
+				mlog.String("fileID", fileID),
+				mlog.Err(err),
+			)
+			continue
+		}
+		if !destExists {
+			a.logger.Error(
+				"CopyCardFiles: copied file does not exist after copy operation",
+				mlog.String("destinationFilePath", destinationFilePath),
+				mlog.String("fileID", fileID),
+			)
+			continue
+		}
+
+		// Save FileInfo AFTER successful file copy to ensure file exists when FileInfo is queried
 		if fileInfo == nil {
 			fileInfo = model.NewFileInfo(destFilename)
 		}
 		fileInfo.Id = getFileInfoID(fileInfoID)
 		fileInfo.Path = destinationFilePath
 		err = a.store.SaveFileInfo(fileInfo)
 		if err != nil {
-			return nil, fmt.Errorf("CopyCardFiles: cannot create fileinfo: %w", err)
-		}
-
-		a.logger.Debug(
-			"Copying card file",
-			mlog.String("sourceFilePath", sourceFilePath),
-			mlog.String("destinationFilePath", destinationFilePath),
-		)
-
-		if err := a.filesBackend.CopyFile(sourceFilePath, destinationFilePath); err != nil {
 			a.logger.Error(
-				"CopyCardFiles failed to copy file",
-				mlog.String("sourceFilePath", sourceFilePath),
+				"CopyCardFiles: cannot save fileinfo after successful copy",
 				mlog.String("destinationFilePath", destinationFilePath),
+				mlog.String("fileID", fileID),
+				mlog.String("fileInfoID", fileInfo.Id),
 				mlog.Err(err),
 			)
+			// Continue even if FileInfo save fails - file is copied, block can still reference it
 		}
+
 		newFileNames[fileID] = destFilename
 	}
 

server/model/properties.go

@@ -110,11 +110,18 @@ func (pd PropDef) GetValue(v interface{}, resolver PropValueResolver) (string, e
 			usernames := make([]string, len(userIDs))
 
 			for i, userIDInterface := range userIDs {
-				userID := userIDInterface.(string)
+				userID, ok := userIDInterface.(string)
+				if !ok {
+					// Handle non-string values gracefully
+					usernames[i] = fmt.Sprintf("%v", userIDInterface)
+					continue
+				}
 
 				user, err := resolver.GetUserByID(userID)
 				if err != nil {
-					return "", err
+					// If user lookup fails, use the userID as fallback
+					usernames[i] = userID
+					continue
 				}
 				if user == nil {
 					usernames[i] = userID
@@ -125,6 +132,8 @@ func (pd PropDef) GetValue(v interface{}, resolver PropValueResolver) (string, e
 
 			return strings.Join(usernames, ", "), nil
 		}
+		// If no resolver, return empty string for multiPerson
+		return "", nil
 
 	case "multiSelect":
 		// v is a slice of strings containing option ids
@@ -155,9 +164,17 @@ func (pd PropDef) GetValue(v interface{}, resolver PropValueResolver) (string, e
 func (pd PropDef) ParseDate(s string) (string, error) {
 	// s is a JSON snippet of the form: {"from":1642161600000, "to":1642161600000} in milliseconds UTC
 	// The UI does not yet support date ranges.
+
+	// Handle empty or whitespace-only strings
+	s = strings.TrimSpace(s)
+	if s == "" {
+		return "", ErrInvalidDate
+	}
+
 	var m map[string]int64
 	if err := json.Unmarshal([]byte(s), &m); err != nil {
-		return s, err
+		// If it's not valid JSON, return the original string with error.
+		return s, fmt.Errorf("invalid date format (expected JSON): %w", err)
 	}
 	tsFrom, ok := m["from"]
 	if !ok {