Merge pull request #42889 from nextcloud/fix/copy-in-same-dir

apps/files/src/actions/moveOrCopyAction.ts

@@ -22,18 +22,16 @@
 import '@nextcloud/dialogs/style.css'
 import type { Folder, Node, View } from '@nextcloud/files'
 import type { IFilePickerButton } from '@nextcloud/dialogs'
+import type { FileStat, ResponseDataDetailed } from 'webdav'
 import type { MoveCopyResult } from './moveOrCopyActionUtils'
 
 // eslint-disable-next-line n/no-extraneous-import
 import { AxiosError } from 'axios'
 import { basename, join } from 'path'
 import { emit } from '@nextcloud/event-bus'
-import { generateRemoteUrl } from '@nextcloud/router'
-import { getCurrentUser } from '@nextcloud/auth'
 import { getFilePickerBuilder, showError } from '@nextcloud/dialogs'
-import { Permission, FileAction, FileType, NodeStatus } from '@nextcloud/files'
+import { Permission, FileAction, FileType, NodeStatus, davGetClient, davRootPath, davResultToNode, davGetDefaultPropfind } from '@nextcloud/files'
 import { translate as t } from '@nextcloud/l10n'
-import axios from '@nextcloud/axios'
 import Vue from 'vue'
 
 import CopyIconSvg from '@mdi/svg/svg/folder-multiple.svg?raw'
@@ -41,6 +39,7 @@ import FolderMoveSvg from '@mdi/svg/svg/folder-move.svg?raw'
 
 import { MoveCopyAction, canCopy, canMove, getQueue } from './moveOrCopyActionUtils'
 import logger from '../logger'
+import { getUniqueName } from '../utils/fileUtils'
 
 /**
  * Return the action that is possible for the given nodes
@@ -77,42 +76,64 @@ export const handleCopyMoveNodeTo = async (node: Node, destination: Folder, meth
 		throw new Error(t('files', 'Destination is not a folder'))
 	}
 
-	if (node.dirname === destination.path) {
+	// Do not allow to MOVE a node to the same folder it is already located
+	if (method === MoveCopyAction.MOVE && node.dirname === destination.path) {
 		throw new Error(t('files', 'This file/folder is already in that directory'))
 	}
 
 	/**
 	 * Example:
-	 * node: /foo/bar/file.txt -> path = /foo/bar
-	 * destination: /foo
-	 * Allow move of /foo does not start with /foo/bar so allow
+	 * - node: /foo/bar/file.txt -> path = /foo/bar/file.txt, destination: /foo
+	 *   Allow move of /foo does not start with /foo/bar/file.txt so allow
+	 * - node: /foo , destination: /foo/bar
+	 *   Do not allow as it would copy foo within itself
+	 * - node: /foo/bar.txt, destination: /foo
+	 *   Allow copy a file to the same directory
+	 * - node: "/foo/bar", destination: "/foo/bar 1"
+	 *   Allow to move or copy but we need to check with trailing / otherwise it would report false positive
 	 */
-	if (destination.path.startsWith(node.path)) {
+	if (`${destination.path}/`.startsWith(`${node.path}/`)) {
 		throw new Error(t('files', 'You cannot move a file/folder onto itself or into a subfolder of itself'))
 	}
 
-	const relativePath = join(destination.path, node.basename)
-	const destinationUrl = generateRemoteUrl(`dav/files/${getCurrentUser()?.uid}${relativePath}`)
-
 	// Set loading state
 	Vue.set(node, 'status', NodeStatus.LOADING)
 
 	const queue = getQueue()
 	return await queue.add(async () => {
-		try {
-			await axios({
-				method: method === MoveCopyAction.COPY ? 'COPY' : 'MOVE',
-				url: node.encodedSource,
-				headers: {
-					Destination: encodeURI(destinationUrl),
-					Overwrite: overwrite ? undefined : 'F',
-				},
-			})
+		const copySuffix = (index: number) => {
+			if (index === 1) {
+				return t('files', '(copy)') // TRANSLATORS: Mark a file as a copy of another file
+			}
+			return t('files', '(copy %n)', undefined, index) // TRANSLATORS: Meaning it is the n'th copy of a file
+		}
 
-			// If we're moving, update the node
-			// if we're copying, we don't need to update the node
-			// the view will refresh itself
-			if (method === MoveCopyAction.MOVE) {
+		try {
+			const client = davGetClient()
+			const currentPath = join(davRootPath, node.path)
+			const destinationPath = join(davRootPath, destination.path)
+
+			if (method === MoveCopyAction.COPY) {
+				let target = node.basename
+				// If we do not allow overwriting then find an unique name
+				if (!overwrite) {
+					const otherNodes = await client.getDirectoryContents(destinationPath) as FileStat[]
+					target = getUniqueName(node.basename, otherNodes.map((n) => n.basename), copySuffix)
+				}
+				await client.copyFile(currentPath, join(destinationPath, target))
+				// If the node is copied into current directory the view needs to be updated
+				if (node.dirname === destination.path) {
+					const { data } = await client.stat(
+						join(destinationPath, target),
+						{
+							details: true,
+							data: davGetDefaultPropfind(),
+						},
+					) as ResponseDataDetailed<FileStat>
+					emit('files:node:created', davResultToNode(data))
+				}
+			} else {
+				await client.moveFile(currentPath, join(destinationPath, node.basename))
 				// Delete the node as it will be fetched again
 				// when navigating to the destination folder
 				emit('files:node:deleted', node)
@@ -129,6 +150,7 @@ export const handleCopyMoveNodeTo = async (node: Node, destination: Folder, meth
 					throw new Error(error.message)
 				}
 			}
+			logger.debug(error as Error)
 			throw new Error()
 		} finally {
 			Vue.set(node, 'status', undefined)
@@ -165,16 +187,6 @@ const openFilePickerForAction = async (action: MoveCopyAction, dir = '/', nodes:
 			const dirnames = nodes.map(node => node.dirname)
 			const paths = nodes.map(node => node.path)
 
-			if (dirnames.includes(path)) {
-				// This file/folder is already in that directory
-				return buttons
-			}
-
-			if (paths.includes(path)) {
-				// You cannot move a file/folder onto itself
-				return buttons
-			}
-
 			if (action === MoveCopyAction.COPY || action === MoveCopyAction.MOVE_OR_COPY) {
 				buttons.push({
 					label: target ? t('files', 'Copy to {target}', { target }) : t('files', 'Copy'),
@@ -189,6 +201,17 @@ const openFilePickerForAction = async (action: MoveCopyAction, dir = '/', nodes:
 				})
 			}
 
+			// Invalid MOVE targets (but valid copy targets)
+			if (dirnames.includes(path)) {
+				// This file/folder is already in that directory
+				return buttons
+			}
+
+			if (paths.includes(path)) {
+				// You cannot move a file/folder onto itself
+				return buttons
+			}
+
 			if (action === MoveCopyAction.MOVE || action === MoveCopyAction.MOVE_OR_COPY) {
 				buttons.push({
 					label: target ? t('files', 'Move to {target}', { target }) : t('files', 'Move'),
@@ -207,7 +230,8 @@ const openFilePickerForAction = async (action: MoveCopyAction, dir = '/', nodes:
 		})
 
 		const picker = filePicker.build()
-		picker.pick().catch(() => {
+		picker.pick().catch((error) => {
+			logger.debug(error as Error)
 			reject(new Error(t('files', 'Cancelled move or copy operation')))
 		})
 	})
@@ -236,7 +260,13 @@ export const action = new FileAction({
 
 	async exec(node: Node, view: View, dir: string) {
 		const action = getActionForNodes([node])
-		const result = await openFilePickerForAction(action, dir, [node])
+		let result
+		try {
+			result = await openFilePickerForAction(action, dir, [node])
+		} catch (e) {
+			logger.error(e as Error)
+			return false
+		}
 		try {
 			await handleCopyMoveNodeTo(node, result.destination, result.action)
 			return true

apps/files/src/init-templates.ts

@@ -21,6 +21,7 @@
  *
  */
 import type { Entry } from '@nextcloud/files'
+import type { TemplateFile } from './types'
 
 import { Folder, Node, Permission, addNewFileMenuEntry, removeNewFileMenuEntry } from '@nextcloud/files'
 import { generateOcsUrl } from '@nextcloud/router'
@@ -35,7 +36,7 @@ import Vue from 'vue'
 import PlusSvg from '@mdi/svg/svg/plus.svg?raw'
 
 import TemplatePickerView from './views/TemplatePicker.vue'
-import { getUniqueName } from './newMenu/newFolder'
+import { getUniqueName } from './utils/fileUtils.ts'
 import { getCurrentUser } from '@nextcloud/auth'
 
 // Set up logger
@@ -58,7 +59,7 @@ TemplatePickerRoot.id = 'template-picker'
 document.body.appendChild(TemplatePickerRoot)
 
 // Retrieve and init templates
-let templates = loadState('files', 'templates', [])
+let templates = loadState<TemplateFile[]>('files', 'templates', [])
 let templatesPath = loadState('files', 'templates_path', false)
 logger.debug('Templates providers', { templates })
 logger.debug('Templates folder', { templatesPath })

apps/files/src/init.ts

@@ -19,8 +19,7 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  */
-import MenuIcon from '@mdi/svg/svg/sun-compass.svg?raw'
-import { FileAction, addNewFileMenuEntry, registerDavProperty, registerFileAction } from '@nextcloud/files'
+import { addNewFileMenuEntry, registerDavProperty, registerFileAction } from '@nextcloud/files'
 
 import { action as deleteAction } from './actions/deleteAction'
 import { action as downloadAction } from './actions/downloadAction'

apps/files/src/newMenu/newFolder.ts

@@ -21,7 +21,7 @@
  */
 import type { Entry, Node } from '@nextcloud/files'
 
-import { basename, extname } from 'path'
+import { basename } from 'path'
 import { emit } from '@nextcloud/event-bus'
 import { getCurrentUser } from '@nextcloud/auth'
 import { Permission, Folder } from '@nextcloud/files'
@@ -31,6 +31,7 @@ import axios from '@nextcloud/axios'
 
 import FolderPlusSvg from '@mdi/svg/svg/folder-plus.svg?raw'
 
+import { getUniqueName } from '../utils/fileUtils.ts'
 import logger from '../logger'
 
 type createFolderResponse = {
@@ -55,17 +56,6 @@ const createNewFolder = async (root: Folder, name: string): Promise<createFolder
 	}
 }
 
-// TODO: move to @nextcloud/files
-export const getUniqueName = (name: string, names: string[]): string => {
-	let newName = name
-	let i = 1
-	while (names.includes(newName)) {
-		const ext = extname(name)
-		newName = `${basename(name, ext)} (${i++})${ext}`
-	}
-	return newName
-}
-
 export const entry = {
 	id: 'newFolder',
 	displayName: t('files', 'New folder'),

apps/files/src/types.ts

@@ -111,3 +111,12 @@ export interface UploaderStore {
 export interface DragAndDropStore {
 	dragging: FileId[]
 }
+
+export interface TemplateFile {
+	app: string
+	label: string
+	extension: string
+	iconClass?: string
+	mimetypes: string[]
+	ratio?: number
+}

apps/files/src/utils/fileUtils.ts

@@ -21,6 +21,25 @@
  */
 import { FileType, type Node } from '@nextcloud/files'
 import { translate as t, translatePlural as n } from '@nextcloud/l10n'
+import { basename, extname } from 'path'
+
+// TODO: move to @nextcloud/files
+/**
+ * Create an unique file name
+ * @param name The initial name to use
+ * @param otherNames Other names that are already used
+ * @param suffix A function that takes an index an returns a suffix to add, defaults to '(index)'
+ * @return Either the initial name, if unique, or the name with the suffix so that the name is unique
+ */
+export const getUniqueName = (name: string, otherNames: string[], suffix = (n: number) => `(${n})`): string => {
+	let newName = name
+	let i = 1
+	while (otherNames.includes(newName)) {
+		const ext = extname(name)
+		newName = `${basename(name, ext)} ${suffix(i++)}${ext}`
+	}
+	return newName
+}
 
 export const encodeFilePath = function(path) {
 	const pathSections = (path.startsWith('/') ? path : `/${path}`).split('/')

cypress/e2e/files/files_copy-move.cy.ts

@@ -100,6 +100,37 @@ describe('Files: Move or copy files', { testIsolation: true }, () => {
 		getRowForFile('new-folder').should('not.exist')
 	})
 
+	// This was a bug previously
+	it('Can move a file to folder with similar name', () => {
+		cy.uploadContent(currentUser, new Blob(), 'text/plain', '/original')
+			.mkdir(currentUser, '/original folder')
+		cy.login(currentUser)
+		cy.visit('/apps/files')
+
+		// intercept the copy so we can wait for it
+		cy.intercept('MOVE', /\/remote.php\/dav\/files\//).as('moveFile')
+
+		getRowForFile('original').should('be.visible')
+		triggerActionForFile('original', 'move-copy')
+
+		// select new folder
+		cy.get('.file-picker [data-filename="original folder"]').should('be.visible').click()
+		// click copy
+		cy.get('.file-picker').contains('button', 'Move to original folder').should('be.visible').click()
+
+		cy.wait('@moveFile')
+		// wait until visible again
+		getRowForFile('original folder').should('be.visible')
+
+		// original should be moved -> not exist anymore
+		getRowForFile('original').should('not.exist')
+		getRowForFile('original folder').should('be.visible').find('[data-cy-files-list-row-name-link]').click()
+
+		cy.url().should('contain', 'dir=/original%20folder')
+		getRowForFile('original').should('be.visible')
+		getRowForFile('original folder').should('not.exist')
+	})
+
 	it('Can move a file to its parent folder', () => {
 		cy.mkdir(currentUser, '/new-folder')
 		cy.uploadContent(currentUser, new Blob(), 'text/plain', '/new-folder/original.txt')
@@ -131,4 +162,43 @@ describe('Files: Move or copy files', { testIsolation: true }, () => {
 		getRowForFile('new-folder').should('be.visible')
 		getRowForFile('original.txt').should('be.visible')
 	})
+
+	it('Can copy a file to same folder', () => {
+		cy.uploadContent(currentUser, new Blob(), 'text/plain', '/original.txt')
+		cy.login(currentUser)
+		cy.visit('/apps/files')
+
+		// intercept the copy so we can wait for it
+		cy.intercept('COPY', /\/remote.php\/dav\/files\//).as('copyFile')
+
+		getRowForFile('original.txt').should('be.visible')
+		triggerActionForFile('original.txt', 'move-copy')
+
+		// click copy
+		cy.get('.file-picker').contains('button', 'Copy').should('be.visible').click()
+
+		cy.wait('@copyFile')
+		getRowForFile('original.txt').should('be.visible')
+		getRowForFile('original (copy).txt').should('be.visible')
+	})
+
+	it('Can copy a file multiple times to same folder', () => {
+		cy.uploadContent(currentUser, new Blob(), 'text/plain', '/original.txt')
+		cy.uploadContent(currentUser, new Blob(), 'text/plain', '/original (copy).txt')
+		cy.login(currentUser)
+		cy.visit('/apps/files')
+
+		// intercept the copy so we can wait for it
+		cy.intercept('COPY', /\/remote.php\/dav\/files\//).as('copyFile')
+
+		getRowForFile('original.txt').should('be.visible')
+		triggerActionForFile('original.txt', 'move-copy')
+
+		// click copy
+		cy.get('.file-picker').contains('button', 'Copy').should('be.visible').click()
+
+		cy.wait('@copyFile')
+		getRowForFile('original.txt').should('be.visible')
+		getRowForFile('original (copy 2).txt').should('be.visible')
+	})
 })

dist/files-init.js.LICENSE.txt

@@ -43,6 +43,28 @@
  *
  */
 
+/**
+ * @copyright Copyright (c) 2019 John Molakvoæ <skjnldsv@protonmail.com>
+ *
+ * @author John Molakvoæ <skjnldsv@protonmail.com>
+ *
+ * @license AGPL-3.0-or-later
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
 /**
  * @copyright Copyright (c) 2021 John Molakvoæ <skjnldsv@protonmail.com>
  *