upload shared files in parallel, pass caption to server with the last file

Signed-off-by: Maksim Sukharev <[email protected]>

Author: Antreesy

src/components/NewMessage/NewMessageUploadEditor.vue

@@ -40,9 +40,9 @@
 					tag="div"
 					group>
 					<template v-for="file in files">
-						<FilePreview :key="file.temporaryMessage.id"
+						<FilePreview :key="file[1].temporaryMessage.id"
 							:token="token"
-							v-bind="file.temporaryMessage.messageParameters.file"
+							v-bind="file[1].temporaryMessage.messageParameters.file"
 							:is-upload-editor="true"
 							@remove-file="handleRemoveFileFromSelection" />
 					</template>
@@ -117,10 +117,7 @@ export default {
 		},
 
 		files() {
-			if (this.currentUploadId) {
-				return this.$store.getters.getInitialisedUploads(this.currentUploadId)
-			}
-			return []
+			return this.$store.getters.getInitialisedUploads(this.currentUploadId)
 		},
 
 		showModal() {
@@ -136,7 +133,7 @@ export default {
 		},
 
 		firstFile() {
-			return this.files[Object.keys(this.files)[0]]
+			return this.files?.at(0)?.at(1)
 		},
 
 		// Hide the plus button in case this editor is used while sending a voice message

src/store/fileUploadStore.js

@@ -48,36 +48,24 @@ const state = {
 
 const getters = {
 
-	getInitialisedUploads: (state) => (uploadId) => {
+	getUploadsArray: (state) => (uploadId) => {
 		if (state.uploads[uploadId]) {
-			const initialisedUploads = {}
-			for (const index in state.uploads[uploadId].files) {
-				const currentFile = state.uploads[uploadId].files[index]
-				if (currentFile.status === 'initialised') {
-					initialisedUploads[index] = (currentFile)
-				}
-			}
-			return initialisedUploads
+			return Object.entries(state.uploads[uploadId].files)
 		} else {
-			return {}
+			return []
 		}
 	},
 
+	getInitialisedUploads: (state, getters) => (uploadId) => {
+		return getters.getUploadsArray(uploadId)
+			.filter(([_index, uploadedFile]) => uploadedFile.status === 'initialised')
+	},
+
 	// Returns all the files that have been successfully uploaded provided an
 	// upload id
-	getShareableFiles: (state) => (uploadId) => {
-		if (state.uploads[uploadId]) {
-			const shareableFiles = {}
-			for (const index in state.uploads[uploadId].files) {
-				const currentFile = state.uploads[uploadId].files[index]
-				if (currentFile.status === 'successUpload') {
-					shareableFiles[index] = (currentFile)
-				}
-			}
-			return shareableFiles
-		} else {
-			return {}
-		}
+	getShareableFiles: (state, getters) => (uploadId) => {
+		return getters.getUploadsArray(uploadId)
+			.filter(([_index, uploadedFile]) => uploadedFile.status === 'successUpload')
 	},
 
 	// gets the current attachment folder
@@ -293,22 +281,27 @@ const actions = {
 
 		EventBus.$emit('upload-start')
 
-		// Tag the previously indexed files and add the temporary messages to the
-		// messages list
-		for (const index in state.uploads[uploadId].files) {
+		// Tag previously indexed files and add temporary messages to the MessagesList
+		// If caption is provided, attach to the last temporary message
+		const lastIndex = getters.getUploadsArray(uploadId).at(-1).at(0)
+		for (const [index, uploadedFile] of getters.getUploadsArray(uploadId)) {
 			// mark all files as uploading
 			commit('markFileAsUploading', { uploadId, index })
 			// Store the previously created temporary message
-			const temporaryMessage = state.uploads[uploadId].files[index].temporaryMessage
+			const temporaryMessage = {
+				...uploadedFile.temporaryMessage,
+				message: index === lastIndex ? caption : '{file}',
+			}
 			// Add temporary messages (files) to the messages list
 			dispatch('addTemporaryMessage', temporaryMessage)
 			// Scroll the message list
 			EventBus.$emit('scroll-chat-to-bottom')
 		}
+
 		// Iterate again and perform the uploads
-		for (const index in state.uploads[uploadId].files) {
+		await Promise.allSettled(getters.getUploadsArray(uploadId).map(async ([index, uploadedFile]) => {
 			// currentFile to be uploaded
-			const currentFile = state.uploads[uploadId].files[index].file
+			const currentFile = uploadedFile.file
 			// userRoot path
 			const userRoot = '/files/' + getters.getUserId()
 			const fileName = (currentFile.newName || currentFile.name)
@@ -345,41 +338,34 @@ const actions = {
 					showError(t('spreed', 'Error while uploading file "{fileName}"', { fileName }))
 				}
 
-				const temporaryMessage = state.uploads[uploadId].files[index].temporaryMessage
 				// Mark the upload as failed in the store
 				commit('markFileAsFailedUpload', { uploadId, index })
-				dispatch('markTemporaryMessageAsFailed', {
-					message: temporaryMessage,
-					reason,
-				})
+				dispatch('markTemporaryMessageAsFailed', { message: uploadedFile.temporaryMessage, reason })
 			}
-
-			// Get the files that have successfully been uploaded from the store
-			const shareableFiles = getters.getShareableFiles(uploadId)
-			// Share each of those files to the conversation
-			for (const index in shareableFiles) {
-				const path = shareableFiles[index].sharePath
-				const temporaryMessage = shareableFiles[index].temporaryMessage
-				const metadata = JSON.stringify({ messageType: temporaryMessage.messageType })
-				try {
-					const token = temporaryMessage.token
-					dispatch('markFileAsSharing', { uploadId, index })
-					await shareFile(path, token, temporaryMessage.referenceId, metadata)
-					dispatch('markFileAsShared', { uploadId, index })
-				} catch (error) {
-					if (error?.response?.status === 403) {
-						showError(t('spreed', 'You are not allowed to share files'))
-					} else {
-						showError(t('spreed', 'An error happened when trying to share your file'))
-					}
-					dispatch('markTemporaryMessageAsFailed', {
-						message: temporaryMessage,
-						reason: 'failed-share',
-					})
-					console.error('An error happened when trying to share your file: ', error)
+		}))
+
+		// Share the files, that have successfully been uploaded from the store, to the conversation
+		await Promise.all(getters.getShareableFiles(uploadId).map(async ([index, shareableFile]) => {
+			const path = shareableFile.sharePath
+			const temporaryMessage = shareableFile.temporaryMessage
+			const metadata = (caption && index === lastIndex)
+				? JSON.stringify({ messageType: temporaryMessage.messageType, caption })
+				: JSON.stringify({ messageType: temporaryMessage.messageType })
+			try {
+				const token = temporaryMessage.token
+				dispatch('markFileAsSharing', { uploadId, index })
+				await shareFile(path, token, temporaryMessage.referenceId, metadata)
+				dispatch('markFileAsShared', { uploadId, index })
+			} catch (error) {
+				if (error?.response?.status === 403) {
+					showError(t('spreed', 'You are not allowed to share files'))
+				} else {
+					showError(t('spreed', 'An error happened when trying to share your file'))
 				}
+				dispatch('markTemporaryMessageAsFailed', { message: temporaryMessage, reason: 'failed-share' })
+				console.error('An error happened when trying to share your file: ', error)
 			}
-		}
+		}))
 		EventBus.$emit('upload-finished')
 	},
 	/**

src/store/fileUploadStore.spec.js

@@ -111,29 +111,67 @@ describe('fileUploadStore', () => {
 					lastModified: Date.UTC(2021, 3, 25, 15, 30, 0),
 				},
 			]
+			const localUrls = ['local-url:pngimage.png', 'local-url:jpgimage.jpg', 'icon-url:text/plain']
+
 			await store.dispatch('initialiseUpload', {
 				uploadId: 'upload-id1',
 				token: 'XXTOKENXX',
 				files,
 			})
 
 			const uploads = store.getters.getInitialisedUploads('upload-id1')
-			expect(Object.keys(uploads).length).toBe(3)
-
-			for (let i = 0; i < files.length; i++) {
-				expect(mockedActions.createTemporaryMessage.mock.calls[i][1].text).toBe('{file}')
-				expect(mockedActions.createTemporaryMessage.mock.calls[i][1].uploadId).toBe('upload-id1')
-				expect(mockedActions.createTemporaryMessage.mock.calls[i][1].index).toBeDefined()
-				expect(mockedActions.createTemporaryMessage.mock.calls[i][1].file).toBe(files[i])
-				expect(mockedActions.createTemporaryMessage.mock.calls[i][1].token).toBe('XXTOKENXX')
+			expect(uploads).toHaveLength(files.length)
+
+			for (const index in files) {
+				expect(mockedActions.createTemporaryMessage.mock.calls[index][1]).toMatchObject({
+					text: '{file}',
+					token: 'XXTOKENXX',
+					uploadId: 'upload-id1',
+					index: expect.anything(),
+					file: files[index],
+					localUrl: localUrls[index],
+				})
+			}
+		})
+
+		test('performs upload and sharing of single file', async () => {
+			const file = {
+				name: 'pngimage.png',
+				type: 'image/png',
+				size: 123,
+				lastModified: Date.UTC(2021, 3, 27, 15, 30, 0),
 			}
+			const fileBuffer = await new Blob([file]).arrayBuffer()
+
+			await store.dispatch('initialiseUpload', {
+				uploadId: 'upload-id1',
+				token: 'XXTOKENXX',
+				files: [file],
+			})
+
+			expect(store.getters.currentUploadId).toBe('upload-id1')
 
-			expect(mockedActions.createTemporaryMessage.mock.calls[0][1].localUrl).toBe('local-url:pngimage.png')
-			expect(mockedActions.createTemporaryMessage.mock.calls[1][1].localUrl).toBe('local-url:jpgimage.jpg')
-			expect(mockedActions.createTemporaryMessage.mock.calls[2][1].localUrl).toBe('icon-url:text/plain')
+			const uniqueFileName = '/Talk/' + file.name + 'uniq'
+			findUniquePath.mockResolvedValueOnce(uniqueFileName)
+			client.putFileContents.mockResolvedValue()
+			shareFile.mockResolvedValue()
+
+			await store.dispatch('uploadFiles', { uploadId: 'upload-id1', caption: 'text-caption' })
+
+			expect(findUniquePath).toHaveBeenCalledTimes(1)
+			expect(findUniquePath).toHaveBeenCalledWith(client, '/files/current-user', '/Talk/' + file.name)
+
+			expect(client.putFileContents).toHaveBeenCalledTimes(1)
+			expect(client.putFileContents).toHaveBeenCalledWith(`/files/current-user${uniqueFileName}`, fileBuffer, expect.anything())
+
+			expect(shareFile).toHaveBeenCalledTimes(1)
+			expect(shareFile).toHaveBeenCalledWith(`/${uniqueFileName}`, 'XXTOKENXX', 'reference-id-1', '{"caption":"text-caption"}')
+
+			expect(mockedActions.addTemporaryMessage).toHaveBeenCalledTimes(1)
+			expect(store.getters.currentUploadId).not.toBeDefined()
 		})
 
-		test('performs upload by uploading then sharing', async () => {
+		test('performs upload and sharing of multiple files', async () => {
 			const file1 = {
 				name: 'pngimage.png',
 				type: 'image/png',
@@ -164,23 +202,25 @@ describe('fileUploadStore', () => {
 				.mockResolvedValueOnce('/Talk/' + files[0].name + 'uniq')
 				.mockResolvedValueOnce('/Talk/' + files[1].name + 'uniq')
 			client.putFileContents.mockResolvedValue()
-			shareFile.mockResolvedValue()
+			shareFile
+				.mockResolvedValueOnce({ data: { ocs: { data: { id: '1' } } } })
+				.mockResolvedValueOnce({ data: { ocs: { data: { id: '2' } } } })
 
-			await store.dispatch('uploadFiles', 'upload-id1')
+			await store.dispatch('uploadFiles', { uploadId: 'upload-id1', caption: 'text-caption' })
 
+			expect(findUniquePath).toHaveBeenCalledTimes(2)
 			expect(client.putFileContents).toHaveBeenCalledTimes(2)
 			expect(shareFile).toHaveBeenCalledTimes(2)
 
-			for (let i = 0; i < files.length; i++) {
-				expect(findUniquePath).toHaveBeenCalledWith(client, '/files/current-user', '/Talk/' + files[i].name)
-				expect(client.putFileContents.mock.calls[i][0]).toBe('/files/current-user/Talk/' + files[i].name + 'uniq')
-				expect(client.putFileContents.mock.calls[i][1]).toStrictEqual(fileBuffers[i])
-
-				expect(shareFile.mock.calls[i][0]).toBe('//Talk/' + files[i].name + 'uniq')
-				expect(shareFile.mock.calls[i][1]).toBe('XXTOKENXX')
-				expect(shareFile.mock.calls[i][2]).toBe('reference-id-' + (i + 1))
+			for (const index in files) {
+				expect(findUniquePath).toHaveBeenCalledWith(client, '/files/current-user', '/Talk/' + files[index].name)
+				expect(client.putFileContents).toHaveBeenCalledWith(`/files/current-user/Talk/${files[index].name}uniq`, fileBuffers[index], expect.anything())
 			}
 
+			expect(shareFile).toHaveBeenCalledTimes(2)
+			expect(shareFile).toHaveBeenNthCalledWith(1, '//Talk/' + files[0].name + 'uniq', 'XXTOKENXX', 'reference-id-1', '{}')
+			expect(shareFile).toHaveBeenNthCalledWith(2, '//Talk/' + files[1].name + 'uniq', 'XXTOKENXX', 'reference-id-2', '{"caption":"text-caption"}')
+
 			expect(mockedActions.addTemporaryMessage).toHaveBeenCalledTimes(2)
 			expect(store.getters.currentUploadId).not.toBeDefined()
 		})
@@ -209,7 +249,7 @@ describe('fileUploadStore', () => {
 				},
 			})
 
-			await store.dispatch('uploadFiles', 'upload-id1')
+			await store.dispatch('uploadFiles', { uploadId: 'upload-id1' })
 
 			expect(client.putFileContents).toHaveBeenCalledTimes(1)
 			expect(shareFile).not.toHaveBeenCalled()
@@ -247,7 +287,7 @@ describe('fileUploadStore', () => {
 				},
 			})
 
-			await store.dispatch('uploadFiles', 'upload-id1')
+			await store.dispatch('uploadFiles', { uploadId: 'upload-id1' })
 
 			expect(client.putFileContents).toHaveBeenCalledTimes(1)
 			expect(shareFile).toHaveBeenCalledTimes(1)
@@ -286,9 +326,9 @@ describe('fileUploadStore', () => {
 			await store.dispatch('removeFileFromSelection', 2)
 
 			const uploads = store.getters.getInitialisedUploads('upload-id1')
-			expect(Object.keys(uploads).length).toBe(1)
+			expect(uploads).toHaveLength(1)
 
-			expect(Object.values(uploads)[0].file).toBe(files[0])
+			expect(uploads[0][1].file).toBe(files[0])
 		})
 
 		test('discard an entire upload', async () => {
@@ -316,7 +356,7 @@ describe('fileUploadStore', () => {
 			await store.dispatch('discardUpload', 'upload-id1')
 
 			const uploads = store.getters.getInitialisedUploads('upload-id1')
-			expect(uploads).toStrictEqual({})
+			expect(uploads).toStrictEqual([])
 
 			expect(store.getters.currentUploadId).not.toBeDefined()
 		})