From 5847d37987c2048a45f9ec63f64f5b6d35c80767 Mon Sep 17 00:00:00 2001 From: Paul Neubauer Date: Mon, 13 Jun 2022 15:30:40 +0200 Subject: [PATCH 1/8] Copy / move file name exists count duplicate --- .../src/helpers/resource/copyMove.ts | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/packages/web-app-files/src/helpers/resource/copyMove.ts b/packages/web-app-files/src/helpers/resource/copyMove.ts index b8d9055e9b9..ee4c59326b0 100644 --- a/packages/web-app-files/src/helpers/resource/copyMove.ts +++ b/packages/web-app-files/src/helpers/resource/copyMove.ts @@ -1,5 +1,8 @@ import { Resource } from './index' import { join } from 'path' +import { + buildResource, +} from '../resources' export enum ResolveStrategy { SKIP, @@ -64,10 +67,11 @@ export const resolveFileExists = ( createModal(modal) }) } + export const resolveAllConflicts = async ( resourcesToMove, targetFolder, - client, + targetFolderItems, createModal, hideModal, $gettext, @@ -75,8 +79,6 @@ export const resolveAllConflicts = async ( resolveFileExistsMethod, copy = false ) => { - // if we implement MERGE, we need to use 'infinity' instead of 1 - const targetFolderItems = await client.files.list(targetFolder.webDavPath, 1) const targetPath = targetFolder.path const index = targetFolder.webDavPath.lastIndexOf(targetPath) const webDavPrefix = @@ -215,6 +217,15 @@ export const copy = async ( true ) } + +const resolveFileNameDuplicate = (name, existingFiles, iteration=1) => { + console.log(existingFiles) + const potentialName = `${name} (${iteration})` + const hasConflict = existingFiles.some((f) => f.name === potentialName) + if(!hasConflict) return potentialName + return resolveFileNameDuplicate(name, existingFiles, iteration+1) +} + export const copyMoveResource = async ( resourcesToMove, targetFolder, @@ -228,10 +239,13 @@ export const copyMoveResource = async ( copy = false ) => { const errors = [] + // if we implement MERGE, we need to use 'infinity' instead of 1 + const targetFolderItems = await client.files.list(targetFolder.webDavPath, 1) + const targetFolderResources = targetFolderItems.map((i) => buildResource(i)) const resolvedConflicts = await resolveAllConflicts( resourcesToMove, targetFolder, - client, + targetFolderItems, createModal, hideModal, $gettext, @@ -241,7 +255,9 @@ export const copyMoveResource = async ( ) const movedResources = [] - for (const resource of resourcesToMove) { + for (let resource of resourcesToMove) { + // because javascripts way of cloning is awesome + resource = JSON.parse(JSON.stringify(resource)) const hasConflict = resolvedConflicts.some((e) => e.resource.id === resource.id) let targetName = resource.name let overwriteTarget = false @@ -254,7 +270,7 @@ export const copyMoveResource = async ( overwriteTarget = true } if (resolveStrategy === ResolveStrategy.KEEP_BOTH) { - targetName = $gettextInterpolate($gettext('%{name} copy'), { name: resource.name }, true) + targetName = resolveFileNameDuplicate(resource.name, [...movedResources, ...targetFolderResources]) resource.name = targetName } } From e6cb2ff3ca513f5f8c9d07778fd7b525b72678fc Mon Sep 17 00:00:00 2001 From: Paul Neubauer Date: Mon, 13 Jun 2022 15:44:26 +0200 Subject: [PATCH 2/8] Add unittests, add to changelog --- .../enhancement-copy-move-conflict-dialog | 3 +- .../src/helpers/resource/copyMove.ts | 16 ++--- .../unit/helpers/resource/copyMove.spec.ts | 60 ++++++++++--------- 3 files changed, 42 insertions(+), 37 deletions(-) diff --git a/changelog/unreleased/enhancement-copy-move-conflict-dialog b/changelog/unreleased/enhancement-copy-move-conflict-dialog index c7a28c992b8..adeb38c121e 100644 --- a/changelog/unreleased/enhancement-copy-move-conflict-dialog +++ b/changelog/unreleased/enhancement-copy-move-conflict-dialog @@ -2,5 +2,6 @@ Enhancement: Copy/Move conflict dialog We've added a conflict dialog for moving resources via drag&drop in the files list +https://github.com/owncloud/web/pull/7119 https://github.com/owncloud/web/pull/6994 -https://github.com/owncloud/web/issues/6996 \ No newline at end of file +https://github.com/owncloud/web/issues/6996 diff --git a/packages/web-app-files/src/helpers/resource/copyMove.ts b/packages/web-app-files/src/helpers/resource/copyMove.ts index ee4c59326b0..072a0f75ef7 100644 --- a/packages/web-app-files/src/helpers/resource/copyMove.ts +++ b/packages/web-app-files/src/helpers/resource/copyMove.ts @@ -1,8 +1,6 @@ import { Resource } from './index' import { join } from 'path' -import { - buildResource, -} from '../resources' +import { buildResource } from '../resources' export enum ResolveStrategy { SKIP, @@ -218,12 +216,11 @@ export const copy = async ( ) } -const resolveFileNameDuplicate = (name, existingFiles, iteration=1) => { - console.log(existingFiles) +export const resolveFileNameDuplicate = (name, existingFiles, iteration = 1) => { const potentialName = `${name} (${iteration})` const hasConflict = existingFiles.some((f) => f.name === potentialName) - if(!hasConflict) return potentialName - return resolveFileNameDuplicate(name, existingFiles, iteration+1) + if (!hasConflict) return potentialName + return resolveFileNameDuplicate(name, existingFiles, iteration + 1) } export const copyMoveResource = async ( @@ -270,7 +267,10 @@ export const copyMoveResource = async ( overwriteTarget = true } if (resolveStrategy === ResolveStrategy.KEEP_BOTH) { - targetName = resolveFileNameDuplicate(resource.name, [...movedResources, ...targetFolderResources]) + targetName = resolveFileNameDuplicate(resource.name, [ + ...movedResources, + ...targetFolderResources + ]) resource.name = targetName } } diff --git a/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts b/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts index 46f6f93c8f1..7670be9373b 100644 --- a/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts +++ b/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts @@ -23,6 +23,22 @@ describe('copyMove', () => { webDavPath: '/target' } }) + + it('should name duplicate file correctly', async () => { + const resultA = Resource.resolveFileNameDuplicate('a', [...resourcesToMove]) + const resultA1 = Resource.resolveFileNameDuplicate('a', [ + ...resourcesToMove, + { + id: 'a1', + name: 'a (1)', + webDavPath: '/a' + } + ]) + const resultA11 = Resource.resolveFileNameDuplicate('a (1)', [...resourcesToMove]) + expect(resultA).toEqual('a (1)') + expect(resultA1).toEqual('a (2)') + expect(resultA11).toEqual('a (1) (1)') + }) it('should copy files if no conflicts exist', async () => { const client = { files: { @@ -70,27 +86,21 @@ describe('copyMove', () => { expect(client.files.move).toHaveBeenCalledWith('/b', '/target/b', false) }) it('should not show message if no conflict exists', async () => { - const client = { - files: { - list: async () => { - return [ - { - id: 'c', - path: 'target/c', - webDavPath: '/target/c', - name: '/target/c' - } - ] - } + const targetFolderItems = [ + { + id: 'c', + path: 'target/c', + webDavPath: '/target/c', + name: '/target/c' } - } + ] const resolveFileExistsMethod = jest .fn() .mockImplementation(() => Promise.resolve({ strategy: 0 } as Resource.ResolveConflict)) await Resource.resolveAllConflicts( resourcesToMove, targetFolder, - client, + targetFolderItems, jest.fn(), jest.fn(), jest.fn(), @@ -100,27 +110,21 @@ describe('copyMove', () => { expect(resolveFileExistsMethod).not.toHaveBeenCalled() }) it('should show message if conflict exists', async () => { - const client = { - files: { - list: async () => { - return [ - { - id: 'a', - path: 'target/a', - webDavPath: '/target/a', - name: '/target/a' - } - ] - } + const targetFolderItems = [ + { + id: 'a', + path: 'target/a', + webDavPath: '/target/a', + name: '/target/a' } - } + ] const resolveFileExistsMethod = jest .fn() .mockImplementation(() => Promise.resolve({ strategy: 0 } as Resource.ResolveConflict)) await Resource.resolveAllConflicts( resourcesToMove, targetFolder, - client, + targetFolderItems, jest.fn(), jest.fn(), jest.fn(), From fa169360984a784fce94ab7a4fdcc567f014efc2 Mon Sep 17 00:00:00 2001 From: Paul Neubauer Date: Tue, 14 Jun 2022 10:20:32 +0200 Subject: [PATCH 3/8] Make file extensions work, update unittests --- .../src/helpers/resource/copyMove.ts | 19 +++++++++----- .../unit/helpers/resource/copyMove.spec.ts | 25 ++++++++----------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/packages/web-app-files/src/helpers/resource/copyMove.ts b/packages/web-app-files/src/helpers/resource/copyMove.ts index 072a0f75ef7..60b13b4c951 100644 --- a/packages/web-app-files/src/helpers/resource/copyMove.ts +++ b/packages/web-app-files/src/helpers/resource/copyMove.ts @@ -216,11 +216,18 @@ export const copy = async ( ) } -export const resolveFileNameDuplicate = (name, existingFiles, iteration = 1) => { - const potentialName = `${name} (${iteration})` +export const resolveFileNameDuplicate = (name, extension, existingFiles, iteration = 1) => { + let potentialName + if (extension.length === 0) { + potentialName = `${name} (${iteration})` + } else { + const extensionIndexInName = name.lastIndexOf(`.${extension}`) + const nameWithoutExtension = name.substring(0, extensionIndexInName) + potentialName = `${nameWithoutExtension} (${iteration}).${extension}` + } const hasConflict = existingFiles.some((f) => f.name === potentialName) if (!hasConflict) return potentialName - return resolveFileNameDuplicate(name, existingFiles, iteration + 1) + return resolveFileNameDuplicate(name, extension, existingFiles, iteration + 1) } export const copyMoveResource = async ( @@ -253,8 +260,8 @@ export const copyMoveResource = async ( const movedResources = [] for (let resource of resourcesToMove) { - // because javascripts way of cloning is awesome - resource = JSON.parse(JSON.stringify(resource)) + // shallow copy of resources to prevent modifing existing rows + resource = { ...resource } const hasConflict = resolvedConflicts.some((e) => e.resource.id === resource.id) let targetName = resource.name let overwriteTarget = false @@ -267,7 +274,7 @@ export const copyMoveResource = async ( overwriteTarget = true } if (resolveStrategy === ResolveStrategy.KEEP_BOTH) { - targetName = resolveFileNameDuplicate(resource.name, [ + targetName = resolveFileNameDuplicate(resource.name, resource.extension, [ ...movedResources, ...targetFolderResources ]) diff --git a/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts b/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts index 7670be9373b..f097eb2c340 100644 --- a/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts +++ b/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts @@ -23,21 +23,16 @@ describe('copyMove', () => { webDavPath: '/target' } }) - - it('should name duplicate file correctly', async () => { - const resultA = Resource.resolveFileNameDuplicate('a', [...resourcesToMove]) - const resultA1 = Resource.resolveFileNameDuplicate('a', [ - ...resourcesToMove, - { - id: 'a1', - name: 'a (1)', - webDavPath: '/a' - } - ]) - const resultA11 = Resource.resolveFileNameDuplicate('a (1)', [...resourcesToMove]) - expect(resultA).toEqual('a (1)') - expect(resultA1).toEqual('a (2)') - expect(resultA11).toEqual('a (1) (1)') + it.each([ + { name: 'a', extension: '', expectName: 'a (1)' }, + { name: 'a', extension: '', expectName: 'a (2)', existing: [{ name: 'a (1)' }] }, + { name: 'a (1)', extension: '', expectName: 'a (1) (1)' }, + { name: 'b.png', extension: 'png', expectName: 'b (1).png' }, + { name: 'b.png', extension: 'png', expectName: 'b (2).png', existing: [{ name: 'b (1).png' }] } + ])('should name duplicate file correctly', (dataSet) => { + const existing = dataSet.existing ? [...resourcesToMove, ...dataSet.existing] : resourcesToMove + const result = Resource.resolveFileNameDuplicate(dataSet.name, dataSet.extension, existing) + expect(result).toEqual(dataSet.expectName) }) it('should copy files if no conflicts exist', async () => { const client = { From 5629606a30f5d2dd4912cd4caed9c847e4c7c7b5 Mon Sep 17 00:00:00 2001 From: Paul Neubauer Date: Tue, 14 Jun 2022 10:26:36 +0200 Subject: [PATCH 4/8] Update complex file extensions --- packages/web-app-files/src/helpers/extensions/fileExtensions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/web-app-files/src/helpers/extensions/fileExtensions.ts b/packages/web-app-files/src/helpers/extensions/fileExtensions.ts index 325ef72ed1a..961deb0a1d1 100644 --- a/packages/web-app-files/src/helpers/extensions/fileExtensions.ts +++ b/packages/web-app-files/src/helpers/extensions/fileExtensions.ts @@ -1,3 +1,3 @@ export default { - complex: ['tar.bz2', 'tar.gz'] + complex: ['tar.bz2', 'tar.gz', '.tar.xz'] } From 0dfcea02ad88a3f54965be4237d2db15ea7809ab Mon Sep 17 00:00:00 2001 From: Paul Neubauer Date: Tue, 14 Jun 2022 10:35:55 +0200 Subject: [PATCH 5/8] Use extractNameWithoutExtension function --- packages/web-app-files/src/helpers/resource/copyMove.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/web-app-files/src/helpers/resource/copyMove.ts b/packages/web-app-files/src/helpers/resource/copyMove.ts index 60b13b4c951..fb8bc2e483d 100644 --- a/packages/web-app-files/src/helpers/resource/copyMove.ts +++ b/packages/web-app-files/src/helpers/resource/copyMove.ts @@ -1,4 +1,4 @@ -import { Resource } from './index' +import { Resource, extractNameWithoutExtension } from './index' import { join } from 'path' import { buildResource } from '../resources' @@ -221,8 +221,7 @@ export const resolveFileNameDuplicate = (name, extension, existingFiles, iterati if (extension.length === 0) { potentialName = `${name} (${iteration})` } else { - const extensionIndexInName = name.lastIndexOf(`.${extension}`) - const nameWithoutExtension = name.substring(0, extensionIndexInName) + const nameWithoutExtension = extractNameWithoutExtension({ name, extension } as Resource) potentialName = `${nameWithoutExtension} (${iteration}).${extension}` } const hasConflict = existingFiles.some((f) => f.name === potentialName) From a2a61f85f1f641ae33797a1e31ea680e7265204a Mon Sep 17 00:00:00 2001 From: Paul Neubauer Date: Tue, 14 Jun 2022 11:29:05 +0200 Subject: [PATCH 6/8] Bugfix repeat copy, replace existing results in row duplication --- packages/web-app-files/src/helpers/resource/copyMove.ts | 7 +++---- packages/web-app-files/src/store/mutations.js | 7 +++++-- .../tests/unit/helpers/resource/copyMove.spec.ts | 4 ++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/packages/web-app-files/src/helpers/resource/copyMove.ts b/packages/web-app-files/src/helpers/resource/copyMove.ts index fb8bc2e483d..cb6ffe4dfd5 100644 --- a/packages/web-app-files/src/helpers/resource/copyMove.ts +++ b/packages/web-app-files/src/helpers/resource/copyMove.ts @@ -280,13 +280,11 @@ export const copyMoveResource = async ( resource.name = targetName } } - resource.path = join(targetFolder.path, resource.name) try { - if (copy) { + if (copy && !overwriteTarget) { await client.files.copy( resource.webDavPath, - join(targetFolder.webDavPath, targetName), - overwriteTarget + join(targetFolder.webDavPath, targetName) ) } else { await client.files.move( @@ -295,6 +293,7 @@ export const copyMoveResource = async ( overwriteTarget ) } + resource.path = join(targetFolder.path, resource.name) resource.webDavPath = join(targetFolder.webDavPath, resource.name) movedResources.push(resource) } catch (error) { diff --git a/packages/web-app-files/src/store/mutations.js b/packages/web-app-files/src/store/mutations.js index 24a90ea63a6..22d414a5f55 100644 --- a/packages/web-app-files/src/store/mutations.js +++ b/packages/web-app-files/src/store/mutations.js @@ -324,8 +324,11 @@ export default { // eslint-disable-next-line camelcase function $_upsertResource(state, resource, allowInsert) { const files = [...state.files] - const index = files.findIndex((r) => r.id === resource.id) - const found = index > -1 + let index = files.findIndex((r) => r.id === resource.id) + if(resource.webDavPath && resource.webDavPath.length) { + index = index > -1 ? index : files.findIndex((r) => r.webDavPath === resource.webDavPath) + } + const found = index > -1 state.filesSearched = null diff --git a/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts b/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts index f097eb2c340..2d9709d5c32 100644 --- a/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts +++ b/packages/web-app-files/tests/unit/helpers/resource/copyMove.spec.ts @@ -54,8 +54,8 @@ describe('copyMove', () => { jest.fn(), jest.fn() ) - expect(client.files.copy).toHaveBeenCalledWith('/a', '/target/a', false) - expect(client.files.copy).toHaveBeenCalledWith('/b', '/target/b', false) + expect(client.files.copy).toHaveBeenCalledWith('/a', '/target/a') + expect(client.files.copy).toHaveBeenCalledWith('/b', '/target/b') }) it('should move files if no conflicts exist', async () => { const client = { From ef37c7ae7a8bbf4797c26f035aab9c54abc98487 Mon Sep 17 00:00:00 2001 From: Paul Neubauer Date: Tue, 14 Jun 2022 12:10:15 +0200 Subject: [PATCH 7/8] Address PR issue --- packages/web-app-files/src/store/mutations.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/web-app-files/src/store/mutations.js b/packages/web-app-files/src/store/mutations.js index 22d414a5f55..ed37f4df9b7 100644 --- a/packages/web-app-files/src/store/mutations.js +++ b/packages/web-app-files/src/store/mutations.js @@ -325,10 +325,10 @@ export default { function $_upsertResource(state, resource, allowInsert) { const files = [...state.files] let index = files.findIndex((r) => r.id === resource.id) - if(resource.webDavPath && resource.webDavPath.length) { - index = index > -1 ? index : files.findIndex((r) => r.webDavPath === resource.webDavPath) + if(resource.webDavPath && resource.webDavPath.length && index === -1) { + index = files.findIndex((r) => r.webDavPath === resource.webDavPath) } - const found = index > -1 + const found = index > -1 state.filesSearched = null From 9b29e6ea0ae9a18a6b0caff232a71f6feacea329 Mon Sep 17 00:00:00 2001 From: Paul Neubauer Date: Tue, 14 Jun 2022 12:25:33 +0200 Subject: [PATCH 8/8] Make linter happy --- packages/web-app-files/src/helpers/resource/copyMove.ts | 5 +---- packages/web-app-files/src/store/mutations.js | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/web-app-files/src/helpers/resource/copyMove.ts b/packages/web-app-files/src/helpers/resource/copyMove.ts index cb6ffe4dfd5..c7b046d1040 100644 --- a/packages/web-app-files/src/helpers/resource/copyMove.ts +++ b/packages/web-app-files/src/helpers/resource/copyMove.ts @@ -282,10 +282,7 @@ export const copyMoveResource = async ( } try { if (copy && !overwriteTarget) { - await client.files.copy( - resource.webDavPath, - join(targetFolder.webDavPath, targetName) - ) + await client.files.copy(resource.webDavPath, join(targetFolder.webDavPath, targetName)) } else { await client.files.move( resource.webDavPath, diff --git a/packages/web-app-files/src/store/mutations.js b/packages/web-app-files/src/store/mutations.js index ed37f4df9b7..95149ffce5b 100644 --- a/packages/web-app-files/src/store/mutations.js +++ b/packages/web-app-files/src/store/mutations.js @@ -325,7 +325,7 @@ export default { function $_upsertResource(state, resource, allowInsert) { const files = [...state.files] let index = files.findIndex((r) => r.id === resource.id) - if(resource.webDavPath && resource.webDavPath.length && index === -1) { + if (resource.webDavPath && resource.webDavPath.length && index === -1) { index = files.findIndex((r) => r.webDavPath === resource.webDavPath) } const found = index > -1