diff --git a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js index 7ccad7c604..81aeb9e1bd 100644 --- a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js @@ -1130,10 +1130,13 @@ const ProjectEntityUpdateHandler = { path.fileSystem, userId, source, - error => { + (error, subtreeListing) => { if (error != null) { return callback(error) } + const subtreeEntityIds = subtreeListing.map(entry => + entry.entity._id.toString() + ) TpdsUpdateSender.deleteEntity( { projectId, @@ -1141,6 +1144,7 @@ const ProjectEntityUpdateHandler = { projectName: projectBeforeDeletion.name, entityId, entityType, + subtreeEntityIds, }, error => { if (error != null) { @@ -1557,87 +1561,69 @@ const ProjectEntityUpdateHandler = { source, callback ) { + const subtreeListing = _listSubtree(entity, entityType, path) ProjectEntityUpdateHandler._updateProjectStructureWithDeletedEntity( project, newProject, - entity, - entityType, - path, + subtreeListing, userId, source, error => { if (error != null) { return callback(error) } - if (entityType.indexOf('file') !== -1) { - ProjectEntityUpdateHandler._cleanUpFile( - project, - entity, - path, - userId, - callback - ) - } else if (entityType.indexOf('doc') !== -1) { - ProjectEntityUpdateHandler._cleanUpDoc( - project, - entity, - path, - userId, - callback - ) - } else if (entityType.indexOf('folder') !== -1) { - ProjectEntityUpdateHandler._cleanUpFolder( - project, - entity, - path, - userId, - callback - ) - } else { - callback() + const jobs = [] + + for (const entry of subtreeListing) { + if (entry.type === 'doc') { + jobs.push(cb => { + ProjectEntityUpdateHandler._cleanUpDoc( + project, + entry.entity, + entry.path, + userId, + cb + ) + }) + } else if (entry.type === 'file') { + jobs.push(cb => { + ProjectEntityUpdateHandler._cleanUpFile( + project, + entry.entity, + entry.path, + userId, + cb + ) + }) + } } + async.series(jobs, err => { + if (err) { + return callback(err) + } + callback(null, subtreeListing) + }) } ) }, - // Note: the _cleanUpEntity code and _updateProjectStructureWithDeletedEntity - // methods both need to recursively iterate over the entities in folder. - // These are currently using separate implementations of the recursion. In - // future, these could be simplified using a common project entity iterator. _updateProjectStructureWithDeletedEntity( project, newProject, - entity, - entityType, - entityPath, + subtreeListing, userId, source, callback ) { - // compute the changes to the project structure - let changes - if (entityType.indexOf('file') !== -1) { - changes = { oldFiles: [{ file: entity, path: entityPath }] } - } else if (entityType.indexOf('doc') !== -1) { - changes = { oldDocs: [{ doc: entity, path: entityPath }] } - } else if (entityType.indexOf('folder') !== -1) { - changes = { oldDocs: [], oldFiles: [] } - const _recurseFolder = (folder, folderPath) => { - for (const doc of iterablePaths(folder, 'docs')) { - changes.oldDocs.push({ doc, path: Path.join(folderPath, doc.name) }) - } - for (const file of iterablePaths(folder, 'fileRefs')) { - changes.oldFiles.push({ - file, - path: Path.join(folderPath, file.name), - }) - } - for (const childFolder of iterablePaths(folder, 'folders')) { - _recurseFolder(childFolder, Path.join(folderPath, childFolder.name)) - } + const changes = { oldDocs: [], oldFiles: [] } + for (const entry of subtreeListing) { + if (entry.type === 'doc') { + changes.oldDocs.push({ doc: entry.entity, path: entry.path }) + } else if (entry.type === 'file') { + changes.oldFiles.push({ file: entry.entity, path: entry.path }) } - _recurseFolder(entity, entityPath) } + // now send the project structure changes to the docupdater changes.newProject = newProject const projectId = project._id.toString() @@ -1692,50 +1678,6 @@ const ProjectEntityUpdateHandler = { ) }, - _cleanUpFolder(project, folder, folderPath, userId, callback) { - const jobs = [] - folder.docs.forEach(doc => { - const docPath = Path.join(folderPath, doc.name) - jobs.push(callback => - ProjectEntityUpdateHandler._cleanUpDoc( - project, - doc, - docPath, - userId, - callback - ) - ) - }) - - folder.fileRefs.forEach(file => { - const filePath = Path.join(folderPath, file.name) - jobs.push(callback => - ProjectEntityUpdateHandler._cleanUpFile( - project, - file, - filePath, - userId, - callback - ) - ) - }) - - folder.folders.forEach(childFolder => { - folderPath = Path.join(folderPath, childFolder.name) - jobs.push(callback => - ProjectEntityUpdateHandler._cleanUpFolder( - project, - childFolder, - folderPath, - userId, - callback - ) - ) - }) - - async.series(jobs, callback) - }, - convertDocToFile: wrapWithLock({ beforeLock(next) { return function (projectId, docId, userId, source, callback) { @@ -1881,6 +1823,45 @@ const ProjectEntityUpdateHandler = { }), } +/** + * List all descendants of an entity along with their type and path. Include + * the top-level entity as well. + */ +function _listSubtree(entity, entityType, entityPath) { + if (entityType.indexOf('file') !== -1) { + return [{ type: 'file', entity, path: entityPath }] + } else if (entityType.indexOf('doc') !== -1) { + return [{ type: 'doc', entity, path: entityPath }] + } else if (entityType.indexOf('folder') !== -1) { + const listing = [] + const _recurseFolder = (folder, folderPath) => { + listing.push({ type: 'folder', entity: folder, path: folderPath }) + for (const doc of iterablePaths(folder, 'docs')) { + listing.push({ + type: 'doc', + entity: doc, + path: Path.join(folderPath, doc.name), + }) + } + for (const file of iterablePaths(folder, 'fileRefs')) { + listing.push({ + type: 'file', + entity: file, + path: Path.join(folderPath, file.name), + }) + } + for (const childFolder of iterablePaths(folder, 'folders')) { + _recurseFolder(childFolder, Path.join(folderPath, childFolder.name)) + } + } + _recurseFolder(entity, entityPath) + return listing + } else { + // This shouldn't happen, but if it does, fail silently. + return [] + } +} + module.exports = ProjectEntityUpdateHandler module.exports.promises = promisifyAll(ProjectEntityUpdateHandler, { without: ['isPathValidForRootDoc'], diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateSender.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateSender.js index 3263dc72f9..2931d69758 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateSender.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateSender.js @@ -108,7 +108,14 @@ function buildTpdsUrl(userId, projectName, filePath) { async function deleteEntity(params) { metrics.inc('tpds.delete-entity') - const { projectId, path, projectName, entityId, entityType } = params + const { + projectId, + path, + projectName, + entityId, + entityType, + subtreeEntityIds, + } = params const projectUserIds = await getProjectUsersIds(projectId) @@ -123,6 +130,10 @@ async function deleteEntity(params) { sl_entity_type: entityType, }, uri: buildTpdsUrl(userId, projectName, path), + // We're sending a body with the DELETE request. This is unconventional, + // but Express does handle it on the other side. Ideally, this operation + // would be moved to a POST endpoint. + json: { subtreeEntityIds }, title: 'deleteEntity', sl_all_user_ids: JSON.stringify([userId]), } diff --git a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js index f4cbfcaa91..d870d4c6df 100644 --- a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js +++ b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js @@ -1567,7 +1567,9 @@ describe('ProjectEntityUpdateHandler', function () { this.projectBeforeDeletion, this.newProject ) - this.ProjectEntityUpdateHandler._cleanUpEntity = sinon.stub().yields() + this.ProjectEntityUpdateHandler._cleanUpEntity = sinon + .stub() + .yields(null, [{ type: 'doc', entity: this.doc, path: this.path }]) this.ProjectEntityUpdateHandler.deleteEntity( projectId, @@ -1600,15 +1602,14 @@ describe('ProjectEntityUpdateHandler', function () { }) it('it notifies the tpds', function () { - this.TpdsUpdateSender.deleteEntity - .calledWith({ - projectId, - path: this.path, - projectName: this.projectBeforeDeletion.name, - entityId: docId, - entityType: 'doc', - }) - .should.equal(true) + this.TpdsUpdateSender.deleteEntity.should.have.been.calledWith({ + projectId, + path: this.path, + projectName: this.projectBeforeDeletion.name, + entityId: docId, + entityType: 'doc', + subtreeEntityIds: [this.doc._id], + }) }) it('retuns the entity_id', function () { @@ -2358,7 +2359,13 @@ describe('ProjectEntityUpdateHandler', function () { this.path, userId, this.source, - done + (err, subtreeListing) => { + if (err) { + return done(err) + } + this.subtreeListing = subtreeListing + done() + } ) }) @@ -2378,20 +2385,25 @@ describe('ProjectEntityUpdateHandler', function () { this.DocumentUpdaterHandler.deleteDoc.called.should.equal(false) }) - it('should should send the update to the doc updater', function () { + it('should send the update to the doc updater', function () { const oldFiles = [{ file: this.entity, path: this.path }] - this.DocumentUpdaterHandler.updateProjectStructure - .calledWith( - projectId, - projectHistoryId, - userId, - { - oldFiles, - newProject: this.newProject, - }, - this.source - ) - .should.equal(true) + this.DocumentUpdaterHandler.updateProjectStructure.should.have.been.calledWith( + projectId, + projectHistoryId, + userId, + { + oldFiles, + oldDocs: [], + newProject: this.newProject, + }, + this.source + ) + }) + + it('should return a subtree listing containing only the file', function () { + expect(this.subtreeListing).to.deep.equal([ + { type: 'file', entity: this.entity, path: this.path }, + ]) }) }) @@ -2409,7 +2421,13 @@ describe('ProjectEntityUpdateHandler', function () { this.path, userId, this.source, - done + (err, subtreeListing) => { + if (err) { + return done(err) + } + this.subtreeListing = subtreeListing + done() + } ) }) @@ -2419,20 +2437,25 @@ describe('ProjectEntityUpdateHandler', function () { .should.equal(true) }) - it('should should send the update to the doc updater', function () { + it('should send the update to the doc updater', function () { const oldDocs = [{ doc: this.entity, path: this.path }] - this.DocumentUpdaterHandler.updateProjectStructure - .calledWith( - projectId, - projectHistoryId, - userId, - { - oldDocs, - newProject: this.newProject, - }, - this.source - ) - .should.equal(true) + this.DocumentUpdaterHandler.updateProjectStructure.should.have.been.calledWith( + projectId, + projectHistoryId, + userId, + { + oldDocs, + oldFiles: [], + newProject: this.newProject, + }, + this.source + ) + }) + + it('should return a subtree listing containing only the doc', function () { + expect(this.subtreeListing).to.deep.equal([ + { type: 'doc', entity: this.entity, path: this.path }, + ]) }) }) @@ -2465,7 +2488,13 @@ describe('ProjectEntityUpdateHandler', function () { path, userId, this.source, - done + (err, subtreeListing) => { + if (err) { + return done(err) + } + this.subtreeListing = subtreeListing + done() + } ) }) @@ -2520,6 +2549,29 @@ describe('ProjectEntityUpdateHandler', function () { ) .should.equal(true) }) + + it('should return a subtree listing containing all sub-entities', function () { + expect(this.subtreeListing).to.have.deep.members([ + { type: 'folder', entity: this.folder, path: '/folder' }, + { + type: 'folder', + entity: this.folder.folders[0], + path: '/folder/subfolder', + }, + { + type: 'file', + entity: this.file1, + path: '/folder/subfolder/file-name-1', + }, + { + type: 'doc', + entity: this.doc1, + path: '/folder/subfolder/doc-name-1', + }, + { type: 'file', entity: this.file2, path: '/folder/file-name-2' }, + { type: 'doc', entity: this.doc2, path: '/folder/doc-name-2' }, + ]) + }) }) }) diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js index e9c13ccc3d..957e420bce 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js @@ -2,6 +2,7 @@ const { ObjectId } = require('mongodb') const SandboxedModule = require('sandboxed-module') const path = require('path') const sinon = require('sinon') +const { expect } = require('chai') const modulePath = path.join( __dirname, @@ -200,11 +201,13 @@ describe('TpdsUpdateSender', function () { it('deleting entity', async function () { const path = '/path/here/t.tex' + const subtreeEntityIds = ['id1', 'id2'] await this.TpdsUpdateSender.promises.deleteEntity({ projectId, path, projectName, + subtreeEntityIds, }) const { @@ -221,6 +224,7 @@ describe('TpdsUpdateSender', function () { )}${encodeURIComponent(path)}` job0.headers.sl_all_user_ids.should.eql(JSON.stringify([userId])) job0.uri.should.equal(expectedUrl) + expect(job0.json).to.deep.equal({ subtreeEntityIds }) const { group: group1, job: job1 } = this.request.secondCall.args[0].json group1.should.equal(collaberatorRef)