fix(files): Allow downloading multiple nodes not from same base

When downloading files in e.g. the *favorites* or *recent* view,
then the nodes are not always share the same parent folder
and we can not use the current directory as it is probably just a
virtual one.

So we calculate the longest common base and use that as the directory
for the download endpoint.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

Author: susnux

apps/files/src/actions/downloadAction.spec.ts

@@ -4,7 +4,14 @@
  */
 import { action } from './downloadAction'
 import { expect } from '@jest/globals'
-import { File, Folder, Permission, View, FileAction, DefaultType } from '@nextcloud/files'
+import {
+	File,
+	Folder,
+	Permission,
+	View,
+	FileAction,
+	DefaultType,
+} from '@nextcloud/files'
 
 const view = {
 	id: 'files',
@@ -104,7 +111,9 @@ describe('Download action execute tests', () => {
 		// Silent action
 		expect(exec).toBe(null)
 		expect(link.download).toEqual('')
-		expect(link.href).toEqual('https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt')
+		expect(link.href).toEqual(
+			'https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt',
+		)
 		expect(link.click).toHaveBeenCalledTimes(1)
 	})
 
@@ -122,7 +131,9 @@ describe('Download action execute tests', () => {
 		// Silent action
 		expect(exec).toStrictEqual([null])
 		expect(link.download).toEqual('')
-		expect(link.href).toEqual('https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt')
+		expect(link.href).toEqual(
+			'https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt',
+		)
 		expect(link.click).toHaveBeenCalledTimes(1)
 	})
 
@@ -139,7 +150,11 @@ describe('Download action execute tests', () => {
 		// Silent action
 		expect(exec).toBe(null)
 		expect(link.download).toEqual('')
-		expect(link.href.startsWith('/index.php/apps/files/ajax/download.php?dir=%2F&files=%5B%22FooBar%22%5D&downloadStartSecret=')).toBe(true)
+		expect(
+			link.href.startsWith(
+				'/index.php/apps/files/ajax/download.php?dir=%2F&files=%5B%22FooBar%22%5D&downloadStartSecret=',
+			),
+		).toBe(true)
 		expect(link.click).toHaveBeenCalledTimes(1)
 	})
 
@@ -164,7 +179,76 @@ describe('Download action execute tests', () => {
 		// Silent action
 		expect(exec).toStrictEqual([null, null])
 		expect(link.download).toEqual('')
-		expect(link.href.startsWith('/index.php/apps/files/ajax/download.php?dir=%2FDir&files=%5B%22foo.txt%22%2C%22bar.txt%22%5D&downloadStartSecret=')).toBe(true)
 		expect(link.click).toHaveBeenCalledTimes(1)
+
+		expect(link.href).toMatch(
+			'/index.php/apps/files/ajax/download.php?dir=%2F&files=%5B%22foo.txt%22%2C%22bar.txt%22%5D&downloadStartSecret=',
+		)
+	})
+
+	test('Download multiple nodes from different sources', async () => {
+		const files = [
+			new File({
+				id: 1,
+				source: 'https://cloud.domain.com/remote.php/dav/files/admin/Folder 1/foo.txt',
+				owner: 'admin',
+				mime: 'text/plain',
+				permissions: Permission.READ,
+			}),
+			new File({
+				id: 2,
+				source: 'https://cloud.domain.com/remote.php/dav/files/admin/Folder 2/bar.txt',
+				owner: 'admin',
+				mime: 'text/plain',
+				permissions: Permission.READ,
+			}),
+			new File({
+				id: 3,
+				source: 'https://cloud.domain.com/remote.php/dav/files/admin/Folder 2/baz.txt',
+				owner: 'admin',
+				mime: 'text/plain',
+				permissions: Permission.READ,
+			}),
+		]
+
+		const exec = await action.execBatch!(files, view, '/Dir')
+
+		// Silent action
+		expect(exec).toStrictEqual([null, null, null])
+		expect(link.download).toEqual('')
+		expect(link.click).toHaveBeenCalledTimes(1)
+
+		expect(link.href).toMatch(
+			'/index.php/apps/files/ajax/download.php?dir=%2F&files=%5B%22foo.txt%22%2C%22bar.txt%22%2C%22baz.txt%22%5D&downloadStartSecret=',
+		)
+	})
+
+	test('Download node and parent folder', async () => {
+		const files = [
+			new File({
+				id: 1,
+				source: 'https://cloud.domain.com/remote.php/dav/files/admin/Folder 1/foo.txt',
+				owner: 'admin',
+				mime: 'text/plain',
+				permissions: Permission.READ,
+			}),
+			new Folder({
+				id: 2,
+				source: 'https://cloud.domain.com/remote.php/dav/files/admin/Folder 1',
+				owner: 'admin',
+				permissions: Permission.READ,
+			}),
+		]
+
+		const exec = await action.execBatch!(files, view, '/Dir')
+
+		// Silent action
+		expect(exec).toStrictEqual([null, null])
+		expect(link.download).toEqual('')
+		expect(link.click).toHaveBeenCalledTimes(1)
+
+		expect(link.href).toMatch(
+			'/index.php/apps/files/ajax/download.php?dir=%2F&files=%5B%22foo.txt%22%2C%22Folder%201%22%5D&downloadStartSecret=',
+		)
 	})
 })

apps/files/src/actions/downloadAction.ts

@@ -17,12 +17,57 @@ const triggerDownload = function(url: string) {
 	hiddenElement.click()
 }
 
-const downloadNodes = function(dir: string, nodes: Node[]) {
+/**
+ * Find the longest common path prefix of both input paths
+ * @param first The first path
+ * @param second The second path
+ */
+function longestCommonPath(first: string, second: string): string {
+	const firstSegments = first.split('/').filter(Boolean)
+	const secondSegments = second.split('/').filter(Boolean)
+	let base = '/'
+	for (const [index, segment] of firstSegments.entries()) {
+		if (index >= second.length) {
+			break
+		}
+		if (segment !== secondSegments[index]) {
+			break
+		}
+		const sep = base === '/' ? '' : '/'
+		base = `${base}${sep}${segment}`
+	}
+	return base
+}
+
+/**
+ * Handle downloading multiple nodes
+ * @param nodes The nodes to download
+ */
+function downloadNodes(nodes: Node[]): void {
+	// Remove nodes that are already included in parent folders
+	// Example: Download A/foo.txt and A will only return A as A/foo.txt is already included
+	const filteredNodes = nodes.filter((node) => {
+		const parent = nodes.find((other) => (
+			other.type === FileType.Folder
+				&& node.path.startsWith(`${other.path}/`)
+		))
+		return parent === undefined
+	})
+
+	let base = filteredNodes[0].dirname
+	for (const node of filteredNodes.slice(1)) {
+		base = longestCommonPath(base, node.dirname)
+	}
+	base = base || '/'
+
+	// Remove the common prefix
+	const filenames = filteredNodes.map((node) => node.path.slice(base === '/' ? 1 : (base.length + 1)))
+
 	const secret = Math.random().toString(36).substring(2)
-	const url = generateUrl('/apps/files/ajax/download.php?dir={dir}&files={files}&downloadStartSecret={secret}', {
-		dir,
+	const url = generateUrl('/apps/files/ajax/download.php?dir={base}&files={files}&downloadStartSecret={secret}', {
+		base,
 		secret,
-		files: JSON.stringify(nodes.map(node => node.basename)),
+		files: JSON.stringify(filenames),
 	})
 	triggerDownload(url)
 }
@@ -69,7 +114,7 @@ export const action = new FileAction({
 
 	async exec(node: Node, view: View, dir: string) {
 		if (node.type === FileType.Folder) {
-			downloadNodes(dir, [node])
+			downloadNodes([node])
 			return null
 		}
 
@@ -83,7 +128,7 @@ export const action = new FileAction({
 			return [null]
 		}
 
-		downloadNodes(dir, nodes)
+		downloadNodes(nodes)
 		return new Array(nodes.length).fill(null)
 	},
 

apps/files/src/actions/editLocallyAction.spec.ts

@@ -120,7 +120,7 @@ describe('Edit locally action enabled tests', () => {
 describe('Edit locally action execute tests', () => {
 	test('Edit locally opens proper URL', async () => {
 		jest.spyOn(axios, 'post').mockImplementation(async () => ({
-			data: { ocs: { data: { token: 'foobar' } } }
+			data: { ocs: { data: { token: 'foobar' } } },
 		}))
 		const mockedShowError = jest.mocked(showError)
 		const spyDialogBuilder = jest.spyOn(dialogBuilder, 'build')