diff --git a/core/remote/change.js b/core/remote/change.js index 9b7bc7d65..5a33f994e 100644 --- a/core/remote/change.js +++ b/core/remote/change.js @@ -233,6 +233,8 @@ const isTrash = (a /*: RemoteChange */) /*: boolean %checks */ => a.type === 'DirTrashing' || a.type === 'FileTrashing' const isRestore = (a /*: RemoteChange */) /*: boolean %checks */ => a.type === 'DirRestoration' || a.type === 'FileRestoration' +const isIgnored = (a /*: RemoteChange */) /*: boolean %checks */ => + a.type === 'IgnoredChange' function includeDescendant( parent /*: RemoteDirMove */, @@ -260,6 +262,10 @@ const aFirst = -1 const bFirst = 1 const sorter = (a, b) => { + if (isIgnored(a) && !isIgnored(b)) return bFirst + if (isIgnored(b) && !isIgnored(a)) return aFirst + if (isIgnored(a) && isIgnored(b)) return 0 + if (areParentChild(createdId(a), deletedId(b))) return aFirst if (areParentChild(createdId(b), deletedId(a))) return bFirst @@ -280,7 +286,12 @@ const sorter = (a, b) => { // if there isnt 2 add paths, sort by del path if (lower(deletedId(b), deletedId(a))) return aFirst - return bFirst + if (lower(deletedId(a), deletedId(b))) return bFirst + + if (lower(createdId(a), deletedId(b))) return bFirst + if (lower(deletedId(a), createdId(b))) return aFirst + + return 0 } function sort(changes /*: Array */) /*: void */ { diff --git a/test/integration/move.js b/test/integration/move.js index c7828cd13..ae5d7d2e6 100644 --- a/test/integration/move.js +++ b/test/integration/move.js @@ -177,9 +177,8 @@ describe('Move', () => { it('local', async () => { const oldFolder = await pouch.byRemoteIdMaybeAsync(dir._id) - // FIXME: Why is this a file? And why does it break with a directory? const doc = builders - .metafile() + .metadir() .path('parent/dst/dir') .build() @@ -228,17 +227,23 @@ describe('Move', () => { await cozy.files.updateAttributesById(dir._id, { dir_id: dst._id }) await helpers.remote.pullChanges() - /* FIXME: Nondeterministic - should(helpers.putDocs('path', '_deleted', 'trashed')).deepEqual([ - {path: 'parent/src/dir/subdir/file', _deleted: true}, - {path: 'parent/src/dir/subdir', _deleted: true}, - {path: 'parent/src/dir/empty-subdir', _deleted: true}, - {path: 'parent/src/dir', _deleted: true}, - {path: 'parent/dst/dir'}, - {path: 'parent/dst/dir/subdir'}, - {path: 'parent/dst/dir/empty-subdir'} + should( + helpers.putDocs('path', '_deleted', 'trashed', 'childMove') + ).deepEqual([ + { path: 'parent/src/dir', _deleted: true }, + { path: 'parent/dst/dir' }, + { + path: 'parent/src/dir/empty-subdir', + _deleted: true, + childMove: true + }, + { path: 'parent/dst/dir/empty-subdir' }, + { path: 'parent/src/dir/subdir', _deleted: true, childMove: true }, + { path: 'parent/dst/dir/subdir' }, + { path: 'parent/src/dir/subdir/file', _deleted: true, childMove: true }, + { path: 'parent/dst/dir/subdir/file' } ]) - */ + await helpers.syncAll() should(await helpers.local.tree()).deepEqual([ 'parent/', @@ -257,91 +262,47 @@ describe('Move', () => { }) it('from remote client', async () => { - // FIXME: Ensure events occur in the same order as resulting from the - // local dir test - await helpers._remote.addFolderAsync( - _.defaults( - { - path: path.normalize('parent/dst/dir'), - updated_at: '2017-06-20T12:58:49.681Z' - }, - await pouch.byRemoteIdAsync(dir._id) - ) - ) - await helpers._remote.addFolderAsync( + const oldFolderMetadata = await pouch.byRemoteIdAsync(dir._id) + await helpers._remote.moveFolderAsync( _.defaults( { - path: path.normalize('parent/dst/dir/empty-subdir'), - updated_at: '2017-06-20T12:58:49.817Z' + path: path.normalize('parent/dst/dir') }, - await pouch.byRemoteIdAsync(emptySubdir._id) - ) - ) - await helpers._remote.addFolderAsync( - _.defaults( - { - path: path.normalize('parent/dst/dir/subdir'), - updated_at: '2017-06-20T12:58:49.873Z' - }, - await pouch.byRemoteIdAsync(subdir._id) - ) - ) - const oldFileMetadata = await pouch.byRemoteIdAsync(file._id) - await helpers._remote.moveFileAsync( - _.defaults( - { - path: path.normalize('parent/dst/dir/subdir/file') - // FIXME: Why does moveFileAsync({updated_at: ...}) fail? - // updated_at: '2017-06-20T12:58:49.921Z' - }, - oldFileMetadata + oldFolderMetadata ), - oldFileMetadata - ) - const oldSubdirMetadata = await pouch.byRemoteIdAsync(subdir._id) - await helpers._remote.deleteFolderAsync(oldSubdirMetadata) - const oldEmptySubdirMetadata = await pouch.byRemoteIdAsync( - emptySubdir._id + oldFolderMetadata ) - await helpers._remote.deleteFolderAsync(oldEmptySubdirMetadata) - const oldDirMetadata = await pouch.byRemoteIdAsync(dir._id) - await helpers._remote.deleteFolderAsync(oldDirMetadata) await helpers.remote.pullChanges() - /* FIXME: Nondeterministic - should(helpers.putDocs('path', '_deleted', 'trashed')).deepEqual([ - // file 1/2 - {path: path.normalize('parent/src/dir/subdir/file'), _deleted: true}, - // file 2/2 - {path: path.normalize('parent/dst/dir/subdir/file')}, - // dir 2/2 - {path: path.normalize('parent/dst/dir')}, - // empty-subdir 2/2 - {path: path.normalize('parent/dst/dir/empty-subdir')}, - // subdir 2/3 - {path: path.normalize('parent/dst/dir/subdir')}, - // subdir 1/3 - {path: path.normalize('parent/src/dir/subdir'), _deleted: true}, - {path: path.normalize('parent/src/dir/subdir'), trashed: true}, - // empty-subdir 1/2 - {path: path.normalize('parent/src/dir/empty-subdir'), _deleted: true}, - {path: path.normalize('parent/src/dir/empty-subdir'), trashed: true}, - // dir 1/2 - {path: path.normalize('parent/src/dir'), _deleted: true}, - {path: path.normalize('parent/src/dir'), trashed: true} + should( + helpers.putDocs('path', '_deleted', 'trashed', 'childMove') + ).deepEqual([ + { path: path.normalize('parent/src/dir'), _deleted: true }, + { path: path.normalize('parent/dst/dir') }, + { + path: path.normalize('parent/src/dir/empty-subdir'), + _deleted: true, + childMove: true + }, + { path: path.normalize('parent/dst/dir/empty-subdir') }, + { + path: path.normalize('parent/src/dir/subdir'), + _deleted: true, + childMove: true + }, + { path: path.normalize('parent/dst/dir/subdir') }, + { + path: path.normalize('parent/src/dir/subdir/file'), + _deleted: true, + childMove: true + }, + { path: path.normalize('parent/dst/dir/subdir/file') } ]) - */ await helpers.syncAll() - should( - (await helpers.local.tree()) - // FIXME: Sometimes a copy of the file ends up in the OS trash. - // Issue was already occurring from time to time, even before we start - // to permanently delete empty dirs. - .filter(path => path !== '/Trash/file') - ).deepEqual([ + should(await helpers.local.tree()).deepEqual([ 'parent/', 'parent/dst/', 'parent/dst/dir/', diff --git a/test/unit/remote/change.js b/test/unit/remote/change.js index 947a1a274..93bd9d93b 100644 --- a/test/unit/remote/change.js +++ b/test/unit/remote/change.js @@ -6,7 +6,7 @@ describe('remote change sort', () => { it('sort correctly move inside move', () => { const parent = { doc: { path: 'parent/dst/dir' }, - type: 'FolderMove', + type: 'DirMove', was: { path: 'parent/src/dir' } } const child = { @@ -41,5 +41,129 @@ describe('remote change sort', () => { remoteChange.sort(changes) changes.should.deepEqual([deleted, created]) }) + + it('even with other changes', () => { + const netflixBillAddition = { + doc: { + path: 'Administratif/Netflix/email_2/2019-05-06_12,34.pdf' + }, + was: null, + type: 'FileAddition' + } + const edfContract1ConflictCreation = { + doc: { + path: + 'Administratif/EDF/email_1/Address 1/attestation de contrat-conflict-2019-05-06T12_34_56.012Z.pdf' + }, + was: { + path: 'Administratif/EDF/email_1/Address 1/attestation de contrat.pdf' + }, + type: 'FileMove' + } + const edfContract2ConflictCreation = { + doc: { + path: + 'Administratif/EDF/email_1/Address 3/attestation de contrat-conflict-2019-05-06T12_34_56.345Z.pdf' + }, + was: { + path: 'Administratif/EDF/email_1/Address 3/attestation de contrat.pdf' + }, + type: 'FileMove' + } + const edfContract3Deletion = { + doc: { + path: 'Administratif/EDF/email_2/Address 2/attestation de contrat.pdf' + }, + was: { + path: 'Administratif/EDF/email_2/Address 2/attestation de contrat.pdf' + }, + type: 'FileDeletion' + } + const edfContract3Addition = { + doc: { + path: 'Administratif/EDF/email_2/Address 2/attestation de contrat.pdf' + }, + was: null, + type: 'FileAddition' + } + const edfContract2Addition = { + doc: { + path: 'Administratif/EDF/email_1/Address 3/attestation de contrat.pdf' + }, + was: null, + type: 'FileAddition' + } + const edfContract1Addition = { + doc: { + path: 'Administratif/EDF/email_1/Address 1/attestation de contrat.pdf' + }, + was: null, + type: 'FileAddition' + } + const digipostBouyguesBill = { + doc: { + path: + 'Administratif/Digiposte/email_2/Bouygues Telecom - Factures/Facture_2019-05-06.pdf' + }, + was: null, + type: 'FileAddition' + } + const alanInsuranceCardDeletion = { + doc: { + path: 'Administratif/Alan/email_2/Carte_Mutuelle.pdf' + }, + was: { + path: 'Administratif/Alan/email_2/Carte_Mutuelle.pdf' + }, + type: 'FileDeletion' + } + const alanInsuranceCardAddition = { + doc: { + path: 'Administratif/Alan/email_2/Carte_Mutuelle.pdf' + }, + was: null, + type: 'FileAddition' + } + const photoAddition = { + doc: { + path: 'Photos/Sauvegardées depuis mon mobile/20190506_123456.jpg' + }, + was: null, + type: 'FileAddition' + } + + const changes = [ + netflixBillAddition, + edfContract1ConflictCreation, + edfContract2ConflictCreation, + edfContract3Deletion, + edfContract3Addition, + edfContract2Addition, + edfContract1Addition, + digipostBouyguesBill, + alanInsuranceCardDeletion, + alanInsuranceCardAddition, + photoAddition + ] + remoteChange.sort(changes) + + // Sort order: + // - alphabetical order + // - conflicts before anything else on same id + // - deletion before addition on same id + changes.should.deepEqual([ + edfContract3Deletion, + digipostBouyguesBill, + edfContract1ConflictCreation, + edfContract1Addition, + edfContract2ConflictCreation, + edfContract2Addition, + edfContract3Addition, + alanInsuranceCardDeletion, + alanInsuranceCardAddition, + netflixBillAddition, + photoAddition + ]) + }) }) }) diff --git a/test/unit/remote/watcher.js b/test/unit/remote/watcher.js index d1581694f..3ecdc98d3 100644 --- a/test/unit/remote/watcher.js +++ b/test/unit/remote/watcher.js @@ -176,8 +176,24 @@ describe('RemoteWatcher', function() { apply.callCount.should.equal(2) // Changes are sorted before applying (first one got the original // RemoteDeletion, while second one was given Metadata since it is valid) - should(apply.args[0][0].doc).deepEqual(remoteDocs[1]) - should(apply.args[1][0].doc).deepEqual(validMetadata(remoteDocs[0])) + should(apply.args).deepEqual([ + [ + { + sideName: 'remote', + type: 'FileAddition', + doc: validMetadata(remoteDocs[0]) + } + ], + [ + { + sideName: 'remote', + type: 'IgnoredChange', + detail: + 'file or directory was created, trashed, and removed remotely', + doc: remoteDocs[1] + } + ] + ]) }) context('when apply() rejects some file/dir', function() { @@ -198,14 +214,24 @@ describe('RemoteWatcher', function() { it('still tries to pull other files/dirs', async function() { await this.watcher.pullMany(remoteDocs).catch(() => {}) should(apply).have.been.calledTwice() - should(apply.args[0][0]).have.properties({ - type: 'IgnoredChange', - doc: remoteDocs[1] - }) - should(apply.args[1][0]).have.properties({ - type: 'FileAddition', - doc: validMetadata(remoteDocs[0]) - }) + should(apply.args).deepEqual([ + [ + { + sideName: 'remote', + type: 'FileAddition', + doc: validMetadata(remoteDocs[0]) + } + ], + [ + { + sideName: 'remote', + type: 'IgnoredChange', + detail: + 'file or directory was created, trashed, and removed remotely', + doc: remoteDocs[1] + } + ] + ]) }) it('releases the Pouch lock', async function() { @@ -338,15 +364,15 @@ describe('RemoteWatcher', function() { const remoteDocs = [srcFileMoved, dstFileTrashed] const changes = this.watcher.analyse(remoteDocs, olds) should(relevantChangesProps(changes)).deepEqual([ - { - type: 'IgnoredChange', - doc: { path: '.cozy_trash/file' }, - was: { path: 'dst/file' } - }, { type: 'FileMove', doc: { path: 'dst/file', overwrite: true }, was: { path: 'src/file' } + }, + { + type: 'IgnoredChange', + doc: { path: '.cozy_trash/file' }, + was: { path: 'dst/file' } } ]) }) @@ -552,15 +578,15 @@ describe('RemoteWatcher', function() { const remoteDocs = [srcMoved, dstTrashed] const changes = this.watcher.analyse(remoteDocs, olds) should(relevantChangesProps(changes)).deepEqual([ - { - type: 'IgnoredChange', - doc: { path: '.cozy_trash/dir' }, - was: { path: 'dst/dir' } - }, { type: 'DirMove', doc: { path: 'dst/dir', overwrite: true }, was: { path: 'src/dir' } + }, + { + type: 'IgnoredChange', + doc: { path: '.cozy_trash/dir' }, + was: { path: 'dst/dir' } } ]) })