Merge pull request #5860 from overleaf/bg-fix-docupdater-delete-doc

fix docupdater delete operation

GitOrigin-RevId: daab03b7659688d53c54ac80756394796d3c6348

services/document-updater/app/js/ProjectManager.js

@@ -238,17 +238,48 @@ function updateProjectWithLocks(
         )
         break
       case 'rename-doc':
-        DocumentManager.renameDocWithLock(
+        if (!update.newPathname) {
-          projectId,
+          // an empty newPathname signifies a delete, so there is no need to
-          update.id,
+          // update the pathname in redis
-          userId,
+          ProjectHistoryRedisManager.queueRenameEntity(
-          update,
+            projectId,
-          projectHistoryId,
+            projectHistoryId,
-          (error, count) => {
+            'doc',
-            projectOpsLength = count
+            update.id,
-            cb(error)
+            userId,
-          }
+            update,
-        )
+            (error, count) => {
+              projectOpsLength = count
+              cb(error)
+            }
+          )
+        } else {
+          // rename the doc in redis before queuing the update
+          DocumentManager.renameDocWithLock(
+            projectId,
+            update.id,
+            userId,
+            update,
+            projectHistoryId,
+            error => {
+              if (error) {
+                return cb(error)
+              }
+              ProjectHistoryRedisManager.queueRenameEntity(
+                projectId,
+                projectHistoryId,
+                'doc',
+                update.id,
+                userId,
+                update,
+                (error, count) => {
+                  projectOpsLength = count
+                  cb(error)
+                }
+              )
+            }
+          )
+        }
         break
       case 'add-file':
         ProjectHistoryRedisManager.queueAddEntity(

services/document-updater/app/js/RedisManager.js

@@ -593,36 +593,14 @@ module.exports = RedisManager = {
         if (error != null) {
           return callback(error)
         }
-
         if (lines != null && version != null) {
           return rclient.set(
             keys.pathname({ doc_id }),
             update.newPathname,
-            function (error) {
-              if (error != null) {
-                return callback(error)
-              }
-              return ProjectHistoryRedisManager.queueRenameEntity(
-                project_id,
-                projectHistoryId,
-                'doc',
-                doc_id,
-                user_id,
-                update,
-                callback
-              )
-            }
-          )
-        } else {
-          return ProjectHistoryRedisManager.queueRenameEntity(
-            project_id,
-            projectHistoryId,
-            'doc',
-            doc_id,
-            user_id,
-            update,
             callback
           )
+        } else {
+          return callback()
         }
       }
     )

services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js

@@ -69,6 +69,54 @@ describe("Applying updates to a project's structure", function () {
     })
   })
 
+  describe('deleting a file', function () {
+    before(function (done) {
+      this.project_id = DocUpdaterClient.randomId()
+      this.fileUpdate = {
+        type: 'rename-file',
+        id: DocUpdaterClient.randomId(),
+        pathname: '/file-path',
+        newPathname: '',
+      }
+      this.updates = [this.fileUpdate]
+      DocUpdaterClient.sendProjectUpdate(
+        this.project_id,
+        this.user_id,
+        this.updates,
+        this.version,
+        error => {
+          if (error) {
+            return done(error)
+          }
+          setTimeout(done, 200)
+        }
+      )
+    })
+
+    it('should push the applied file renames to the project history api', function (done) {
+      rclientProjectHistory.lrange(
+        ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }),
+        0,
+        -1,
+        (error, updates) => {
+          if (error) {
+            return done(error)
+          }
+
+          const update = JSON.parse(updates[0])
+          update.file.should.equal(this.fileUpdate.id)
+          update.pathname.should.equal('/file-path')
+          update.new_pathname.should.equal('')
+          update.meta.user_id.should.equal(this.user_id)
+          update.meta.ts.should.be.a('string')
+          update.version.should.equal(`${this.version}.0`)
+
+          done()
+        }
+      )
+    })
+  })
+
   describe('renaming a document', function () {
     before(function () {
       this.update = {
@@ -288,6 +336,126 @@ describe("Applying updates to a project's structure", function () {
     })
   })
 
+  describe('deleting a document', function () {
+    before(function () {
+      this.update = {
+        type: 'rename-doc',
+        id: DocUpdaterClient.randomId(),
+        pathname: '/doc-path',
+        newPathname: '',
+      }
+      this.updates = [this.update]
+    })
+
+    describe('when the document is not loaded', function () {
+      before(function (done) {
+        this.project_id = DocUpdaterClient.randomId()
+        DocUpdaterClient.sendProjectUpdate(
+          this.project_id,
+          this.user_id,
+          this.updates,
+          this.version,
+          error => {
+            if (error) {
+              return done(error)
+            }
+            setTimeout(done, 200)
+          }
+        )
+      })
+
+      it('should push the applied doc update to the project history api', function (done) {
+        rclientProjectHistory.lrange(
+          ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }),
+          0,
+          -1,
+          (error, updates) => {
+            if (error) {
+              return done(error)
+            }
+
+            const update = JSON.parse(updates[0])
+            update.doc.should.equal(this.update.id)
+            update.pathname.should.equal('/doc-path')
+            update.new_pathname.should.equal('')
+            update.meta.user_id.should.equal(this.user_id)
+            update.meta.ts.should.be.a('string')
+            update.version.should.equal(`${this.version}.0`)
+
+            done()
+          }
+        )
+      })
+    })
+
+    describe('when the document is loaded', function () {
+      before(function (done) {
+        this.project_id = DocUpdaterClient.randomId()
+        MockWebApi.insertDoc(this.project_id, this.update.id, {})
+        DocUpdaterClient.preloadDoc(this.project_id, this.update.id, error => {
+          if (error) {
+            return done(error)
+          }
+          sinon.spy(MockWebApi, 'getDocument')
+          DocUpdaterClient.sendProjectUpdate(
+            this.project_id,
+            this.user_id,
+            this.updates,
+            this.version,
+            error => {
+              if (error) {
+                return done(error)
+              }
+              setTimeout(done, 200)
+            }
+          )
+        })
+      })
+
+      after(function () {
+        MockWebApi.getDocument.restore()
+      })
+
+      it('should not modify the doc', function (done) {
+        DocUpdaterClient.getDoc(
+          this.project_id,
+          this.update.id,
+          (error, res, doc) => {
+            if (error) {
+              return done(error)
+            }
+
+            doc.pathname.should.equal('/a/b/c.tex') // default pathname from MockWebApi
+            done()
+          }
+        )
+      })
+
+      it('should push the applied doc update to the project history api', function (done) {
+        rclientProjectHistory.lrange(
+          ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }),
+          0,
+          -1,
+          (error, updates) => {
+            if (error) {
+              return done(error)
+            }
+
+            const update = JSON.parse(updates[0])
+            update.doc.should.equal(this.update.id)
+            update.pathname.should.equal('/doc-path')
+            update.new_pathname.should.equal('')
+            update.meta.user_id.should.equal(this.user_id)
+            update.meta.ts.should.be.a('string')
+            update.version.should.equal(`${this.version}.0`)
+
+            done()
+          }
+        )
+      })
+    })
+  })
+
   describe('adding a file', function () {
     before(function (done) {
       this.project_id = DocUpdaterClient.randomId()

services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js

@@ -1097,20 +1097,6 @@ describe('RedisManager', function () {
           .calledWith(`Pathname:${this.doc_id}`, this.newPathname)
           .should.equal(true)
       })
-
-      return it('should queue an update', function () {
-        return this.ProjectHistoryRedisManager.queueRenameEntity
-          .calledWithExactly(
-            this.project_id,
-            this.projectHistoryId,
-            'doc',
-            this.doc_id,
-            this.userId,
-            this.update,
-            this.callback
-          )
-          .should.equal(true)
-      })
     })
 
     describe('the document is not cached in redis', function () {
@@ -1134,20 +1120,6 @@ describe('RedisManager', function () {
       it('does not update the cached pathname', function () {
         return this.rclient.set.called.should.equal(false)
       })
-
-      return it('should queue an update', function () {
-        return this.ProjectHistoryRedisManager.queueRenameEntity
-          .calledWithExactly(
-            this.project_id,
-            this.projectHistoryId,
-            'doc',
-            this.doc_id,
-            this.userId,
-            this.update,
-            this.callback
-          )
-          .should.equal(true)
-      })
     })
 
     return describe('getDocVersion', function () {