Merge pull request #2661 from overleaf/em-convert-doc-to-file-ranges

Do not convert a doc to file when it has ranges

GitOrigin-RevId: 52f0151e54c426178f80c34c6afac908bbf7b90d

services/web/app/src/Features/Editor/EditorHttpController.js

@@ -241,6 +241,12 @@ async function convertDocToFile(req, res, next) {
       throw new HttpErrors.NotFoundError({
         info: { public: { message: 'Document not found' } }
       })
+    } else if (err instanceof Errors.DocHasRangesError) {
+      throw new HttpErrors.UnprocessableEntityError({
+        info: {
+          public: { message: 'Document has comments or tracked changes' }
+        }
+      })
     } else {
       throw err
     }

services/web/app/src/Features/Errors/Errors.js

@@ -138,6 +138,12 @@ class UserNotCollaboratorError extends OError {
   }
 }
 
+class DocHasRangesError extends OError {
+  constructor(options) {
+    super({ message: 'document has ranges', ...options })
+  }
+}
+
 module.exports = {
   OError,
   BackwardCompatibleError,
@@ -164,5 +170,6 @@ module.exports = {
   SubscriptionAdminDeletionError,
   ProjectNotFoundError,
   UserNotFoundError,
-  UserNotCollaboratorError
+  UserNotCollaboratorError,
+  DocHasRangesError
 }

services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js

@@ -1521,7 +1521,7 @@ const ProjectEntityUpdateHandler = {
   convertDocToFile: wrapWithLock({
     beforeLock(next) {
       return function(projectId, docId, userId, callback) {
-        DocumentUpdaterHandler.deleteDoc(projectId, docId, err => {
+        DocumentUpdaterHandler.flushDocToMongo(projectId, docId, err => {
           if (err) {
             return callback(err)
           }
@@ -1532,47 +1532,59 @@ const ProjectEntityUpdateHandler = {
               if (err) {
                 return callback(err)
               }
-              DocstoreManager.getDoc(projectId, docId, (err, docLines) => {
-                if (err) {
-                  return callback(err)
-                }
-                FileWriter.writeLinesToDisk(
-                  projectId,
-                  docLines,
-                  (err, fsPath) => {
+              DocstoreManager.getDoc(
+                projectId,
+                docId,
+                (err, docLines, rev, version, ranges) => {
+                  if (err) {
+                    return callback(err)
+                  }
+                  if (!_.isEmpty(ranges)) {
+                    return callback(new Errors.DocHasRangesError({}))
+                  }
+                  DocumentUpdaterHandler.deleteDoc(projectId, docId, err => {
                     if (err) {
                       return callback(err)
                     }
-                    FileStoreHandler.uploadFileFromDisk(
+                    FileWriter.writeLinesToDisk(
                       projectId,
-                      { name: doc.name },
-                      fsPath,
-                      (err, fileStoreUrl, fileRef) => {
+                      docLines,
+                      (err, fsPath) => {
                         if (err) {
                           return callback(err)
                         }
-                        fs.unlink(fsPath, err => {
-                          if (err) {
-                            logger.warn(
-                              { err, path: fsPath },
-                              'failed to clean up temporary file'
-                            )
+                        FileStoreHandler.uploadFileFromDisk(
+                          projectId,
+                          { name: doc.name },
+                          fsPath,
+                          (err, fileStoreUrl, fileRef) => {
+                            if (err) {
+                              return callback(err)
+                            }
+                            fs.unlink(fsPath, err => {
+                              if (err) {
+                                logger.warn(
+                                  { err, path: fsPath },
+                                  'failed to clean up temporary file'
+                                )
+                              }
+                              next(
+                                projectId,
+                                doc,
+                                docPath,
+                                fileRef,
+                                fileStoreUrl,
+                                userId,
+                                callback
+                              )
+                            })
                           }
-                          next(
-                            projectId,
-                            doc,
-                            docPath,
-                            fileRef,
-                            fileStoreUrl,
-                            userId,
-                            callback
-                          )
-                        })
+                        )
                       }
                     )
-                  }
-                )
-              })
+                  })
+                }
+              )
             }
           )
         })

services/web/test/unit/src/Editor/EditorHttpControllerTests.js

@@ -3,6 +3,7 @@ const sinon = require('sinon')
 const { expect } = require('chai')
 const { ObjectId } = require('mongodb')
 const Errors = require('../../../../app/src/Features/Errors/Errors')
+const HttpErrors = require('@overleaf/o-error/http')
 
 const MODULE_PATH = '../../../../app/src/Features/Editor/EditorHttpController'
 
@@ -143,7 +144,8 @@ describe('EditorHttpController', function() {
         '../../infrastructure/FileWriter': this.FileWriter,
         '../Project/ProjectEntityUpdateHandler': this
           .ProjectEntityUpdateHandler,
-        '../Errors/Errors': Errors
+        '../Errors/Errors': Errors,
+        '@overleaf/o-error/http': HttpErrors
       }
     })
   })
@@ -519,19 +521,33 @@ describe('EditorHttpController', function() {
       this.EditorHttpController.convertDocToFile(this.req, this.res)
     })
 
-    it('should convert the doc to a file', function() {
-      expect(
-        this.ProjectEntityUpdateHandler.promises.convertDocToFile
-      ).to.have.been.calledWith(
-        this.project._id.toString(),
-        this.doc._id.toString(),
-        this.user._id.toString()
-      )
+    describe('when successful', function() {
+      it('should convert the doc to a file', function() {
+        expect(
+          this.ProjectEntityUpdateHandler.promises.convertDocToFile
+        ).to.have.been.calledWith(
+          this.project._id.toString(),
+          this.doc._id.toString(),
+          this.user._id.toString()
+        )
+      })
+
+      it('should return the file id in the response', function() {
+        expect(this.res.json).to.have.been.calledWith({
+          fileId: this.file._id.toString()
+        })
+      })
     })
 
-    it('should return the file id in the response', function() {
-      expect(this.res.json).to.have.been.calledWith({
-        fileId: this.file._id.toString()
+    describe('when the doc has ranges', function() {
+      it('should return a 422', function(done) {
+        this.ProjectEntityUpdateHandler.promises.convertDocToFile.rejects(
+          new Errors.DocHasRangesError({})
+        )
+        this.EditorHttpController.convertDocToFile(this.req, this.res, err => {
+          expect(err).to.be.instanceof(HttpErrors.UnprocessableEntityError)
+          done()
+        })
       })
     })
   })

services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js

@@ -2067,7 +2067,7 @@ describe('ProjectEntityUpdateHandler', function() {
   })
 
   describe('convertDocToFile', function() {
-    beforeEach(function(done) {
+    beforeEach(function() {
       this.docPath = '/folder/doc.tex'
       this.docLines = ['line one', 'line two']
       this.tmpFilePath = '/tmp/file'
@@ -2100,74 +2100,103 @@ describe('ProjectEntityUpdateHandler', function() {
         null,
         this.project
       )
-      this.ProjectEntityUpdateHandler.convertDocToFile(
-        this.project._id,
-        this.doc._id,
-        this.user._id,
-        done
-      )
     })
 
-    it('deletes the document in doc updater', function() {
-      expect(this.DocumentUpdaterHandler.deleteDoc).to.have.been.calledWith(
-        this.project._id,
-        this.doc._id
-      )
-    })
+    describe('successfully', function() {
+      beforeEach(function(done) {
+        this.ProjectEntityUpdateHandler.convertDocToFile(
+          this.project._id,
+          this.doc._id,
+          this.user._id,
+          done
+        )
+      })
 
-    it('uploads the file to filestore', function() {
-      expect(this.FileStoreHandler.uploadFileFromDisk).to.have.been.calledWith(
-        this.project._id,
-        { name: this.doc.name },
-        this.tmpFilePath
-      )
-    })
+      it('deletes the document in doc updater', function() {
+        expect(this.DocumentUpdaterHandler.deleteDoc).to.have.been.calledWith(
+          this.project._id,
+          this.doc._id
+        )
+      })
 
-    it('cleans up the temporary file', function() {
-      expect(this.fs.unlink).to.have.been.calledWith(this.tmpFilePath)
-    })
+      it('uploads the file to filestore', function() {
+        expect(
+          this.FileStoreHandler.uploadFileFromDisk
+        ).to.have.been.calledWith(
+          this.project._id,
+          { name: this.doc.name },
+          this.tmpFilePath
+        )
+      })
 
-    it('replaces the doc with the file', function() {
-      expect(
-        this.ProjectEntityMongoUpdateHandler.replaceDocWithFile
-      ).to.have.been.calledWith(this.project._id, this.doc._id, this.file)
-    })
+      it('cleans up the temporary file', function() {
+        expect(this.fs.unlink).to.have.been.calledWith(this.tmpFilePath)
+      })
 
-    it('notifies document updater of changes', function() {
-      expect(
-        this.DocumentUpdaterHandler.updateProjectStructure
-      ).to.have.been.calledWith(
-        this.project._id,
-        this.project.overleaf.history.id,
-        this.user._id,
-        {
-          oldDocs: [{ doc: this.doc, path: this.path }],
-          newFiles: [
-            { file: this.file, path: this.path, url: this.fileStoreUrl }
-          ],
-          newProject: this.project
-        }
-      )
-    })
+      it('replaces the doc with the file', function() {
+        expect(
+          this.ProjectEntityMongoUpdateHandler.replaceDocWithFile
+        ).to.have.been.calledWith(this.project._id, this.doc._id, this.file)
+      })
 
-    it('should notify real-time of the doc deletion', function() {
-      expect(this.EditorRealTimeController.emitToRoom).to.have.been.calledWith(
-        this.project._id,
-        'removeEntity',
-        this.doc._id,
-        'convertDocToFile'
-      )
+      it('notifies document updater of changes', function() {
+        expect(
+          this.DocumentUpdaterHandler.updateProjectStructure
+        ).to.have.been.calledWith(
+          this.project._id,
+          this.project.overleaf.history.id,
+          this.user._id,
+          {
+            oldDocs: [{ doc: this.doc, path: this.path }],
+            newFiles: [
+              { file: this.file, path: this.path, url: this.fileStoreUrl }
+            ],
+            newProject: this.project
+          }
+        )
+      })
+
+      it('should notify real-time of the doc deletion', function() {
+        expect(
+          this.EditorRealTimeController.emitToRoom
+        ).to.have.been.calledWith(
+          this.project._id,
+          'removeEntity',
+          this.doc._id,
+          'convertDocToFile'
+        )
+      })
+
+      it('should notify real-time of the file creation', function() {
+        expect(
+          this.EditorRealTimeController.emitToRoom
+        ).to.have.been.calledWith(
+          this.project._id,
+          'reciveNewFile',
+          this.folder._id,
+          this.file,
+          'convertDocToFile',
+          null
+        )
+      })
     })
 
-    it('should notify real-time of the file creation', function() {
-      expect(this.EditorRealTimeController.emitToRoom).to.have.been.calledWith(
-        this.project._id,
-        'reciveNewFile',
-        this.folder._id,
-        this.file,
-        'convertDocToFile',
-        null
-      )
+    describe('when the doc has ranges', function() {
+      it('should throw a DocHasRangesError', function(done) {
+        this.ranges = { comments: [{ id: 123 }] }
+        this.DocstoreManager.getDoc
+          .withArgs(this.project._id, this.doc._id)
+          .yields(null, this.docLines, 'rev', 'version', this.ranges)
+        this.ProjectEntityUpdateHandler.convertDocToFile(
+          this.project._id,
+          this.doc._id,
+          this.user._id,
+          err => {
+            expect(err).to.be.instanceof(Errors.DocHasRangesError)
+            done()
+          }
+        )
+      })
     })
   })
 })