Merge pull request #16617 from overleaf/dp-mongoose-callback

Convert ProjectHistoryHandler from callbacks to promises

GitOrigin-RevId: 6b1841269f6976deb4da663f614c423de7b962b7

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

@@ -2,81 +2,61 @@ const { Project } = require('../../models/Project')
 const ProjectDetailsHandler = require('./ProjectDetailsHandler')
 const HistoryManager = require('../History/HistoryManager')
 const ProjectEntityUpdateHandler = require('./ProjectEntityUpdateHandler')
-const { promisifyAll } = require('@overleaf/promise-utils')
+const { callbackify } = require('util')
 
 const ProjectHistoryHandler = {
-  setHistoryId(projectId, historyId, callback) {
+  async setHistoryId(projectId, historyId) {
     // reject invalid history ids
     if (historyId == null) {
-      return callback(new Error('missing history id'))
+      throw new Error('missing history id')
     }
+
     // use $exists:false to prevent overwriting any existing history id, atomically
-    Project.updateOne(
+    const result = await Project.updateOne(
       { _id: projectId, 'overleaf.history.id': { $exists: false } },
-      { 'overleaf.history.id': historyId },
-      function (err, result) {
-        if (err) {
-          return callback(err)
-        }
-        if (result.matchedCount === 0) {
-          return callback(new Error('history exists'))
-        }
-        callback()
-      }
+      { 'overleaf.history.id': historyId }
     )
+
+    if (result.matchedCount === 0) {
+      throw new Error('history exists')
+    }
   },
 
-  getHistoryId(projectId, callback) {
-    ProjectDetailsHandler.getDetails(projectId, function (err, project) {
-      if (err) {
-        return callback(err)
-      } // n.b. getDetails returns an error if the project doesn't exist
-      callback(null, project?.overleaf?.history?.id)
-    })
+  async getHistoryId(projectId) {
+    const project = await ProjectDetailsHandler.promises.getDetails(projectId)
+    return project?.overleaf?.history?.id
   },
 
-  ensureHistoryExistsForProject(projectId, callback) {
+  async ensureHistoryExistsForProject(projectId) {
     // We can only set a history id for a project that doesn't have one. The
     // history id is cached in the project history service, and changing an
     // existing value corrupts the history, leaving it in an irrecoverable
     // state. Setting a history id when one wasn't present before is ok,
     // because undefined history ids aren't cached.
-    ProjectHistoryHandler.getHistoryId(projectId, function (err, historyId) {
-      if (err) {
-        return callback(err)
-      }
-      if (historyId != null) {
-        return callback()
-      } // history already exists, success
-      HistoryManager.initializeProject(projectId, function (err, historyId) {
-        if (err) {
-          return callback(err)
-        }
-        if (historyId == null) {
-          return callback(new Error('failed to initialize history id'))
-        }
-        ProjectHistoryHandler.setHistoryId(
-          projectId,
-          historyId,
-          function (err) {
-            if (err) {
-              return callback(err)
-            }
-            ProjectEntityUpdateHandler.resyncProjectHistory(
-              projectId,
-              function (err) {
-                if (err) {
-                  return callback(err)
-                }
-                HistoryManager.flushProject(projectId, callback)
-              }
-            )
-          }
-        )
-      })
-    })
+    let historyId = await ProjectHistoryHandler.getHistoryId(projectId)
+
+    if (historyId != null) {
+      return
+    }
+
+    historyId = await HistoryManager.promises.initializeProject(projectId)
+    if (historyId == null) {
+      throw new Error('failed to initialize history id')
+    }
+
+    await ProjectHistoryHandler.setHistoryId(projectId, historyId)
+
+    await ProjectEntityUpdateHandler.promises.resyncProjectHistory(projectId)
+
+    await HistoryManager.promises.flushProject(projectId)
   },
 }
 
-ProjectHistoryHandler.promises = promisifyAll(ProjectHistoryHandler)
-module.exports = ProjectHistoryHandler
+module.exports = {
+  setHistoryId: callbackify(ProjectHistoryHandler.setHistoryId),
+  getHistoryId: callbackify(ProjectHistoryHandler.getHistoryId),
+  ensureHistoryExistsForProject: callbackify(
+    ProjectHistoryHandler.ensureHistoryExistsForProject
+  ),
+  promises: ProjectHistoryHandler,
+}

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

@@ -48,50 +48,55 @@ describe('ProjectHistoryHandler', function () {
         '../../models/Project': {
           Project: this.ProjectModel,
         },
-        './ProjectDetailsHandler': (this.ProjectDetailsHandler = {}),
-        '../History/HistoryManager': (this.HistoryManager = {}),
-        './ProjectEntityUpdateHandler': (this.ProjectEntityUpdateHandler = {}),
+        './ProjectDetailsHandler': (this.ProjectDetailsHandler = {
+          promises: {},
+        }),
+        '../History/HistoryManager': (this.HistoryManager = {
+          promises: {},
+        }),
+        './ProjectEntityUpdateHandler': (this.ProjectEntityUpdateHandler = {
+          promises: {},
+        }),
       },
     }))
   })
 
   describe('starting history for an existing project', function () {
-    beforeEach(function () {
-      this.HistoryManager.initializeProject = sinon
+    beforeEach(async function () {
+      this.HistoryManager.promises.initializeProject = sinon
         .stub()
-        .yields(null, this.historyId)
-      this.HistoryManager.flushProject = sinon.stub().callsArg(1)
-      return (this.ProjectEntityUpdateHandler.resyncProjectHistory = sinon
-        .stub()
-        .callsArg(1))
+        .resolves(this.historyId)
+      this.HistoryManager.promises.flushProject = sinon.stub()
+
+      return (this.ProjectEntityUpdateHandler.promises.resyncProjectHistory =
+        sinon.stub())
     })
 
     describe('when the history does not already exist', function () {
-      beforeEach(function () {
-        this.ProjectDetailsHandler.getDetails = sinon
+      beforeEach(async function () {
+        this.ProjectDetailsHandler.promises.getDetails = sinon
           .stub()
           .withArgs(projectId)
-          .callsArgWith(1, null, this.project)
-        this.ProjectModel.updateOne = sinon
-          .stub()
-          .callsArgWith(2, null, { matchedCount: 1 })
-        return this.ProjectHistoryHandler.ensureHistoryExistsForProject(
-          projectId,
-          this.callback
+          .resolves(this.project)
+        this.ProjectModel.updateOne = sinon.stub().resolves({ matchedCount: 1 })
+        return this.ProjectHistoryHandler.promises.ensureHistoryExistsForProject(
+          projectId
         )
       })
 
-      it('should get any existing history id for the project', function () {
-        return this.ProjectDetailsHandler.getDetails
+      it('should get any existing history id for the project', async function () {
+        return this.ProjectDetailsHandler.promises.getDetails
           .calledWith(projectId)
           .should.equal(true)
       })
 
-      it('should initialize a new history in the v1 history service', function () {
-        return this.HistoryManager.initializeProject.called.should.equal(true)
+      it('should initialize a new history in the v1 history service', async function () {
+        return this.HistoryManager.promises.initializeProject.called.should.equal(
+          true
+        )
       })
 
-      it('should set the new history id on the project', function () {
+      it('should set the new history id on the project', async function () {
         return this.ProjectModel.updateOne
           .calledWith(
             { _id: projectId, 'overleaf.history.id': { $exists: false } },
@@ -100,63 +105,58 @@ describe('ProjectHistoryHandler', function () {
           .should.equal(true)
       })
 
-      it('should resync the project history', function () {
-        return this.ProjectEntityUpdateHandler.resyncProjectHistory
+      it('should resync the project history', async function () {
+        return this.ProjectEntityUpdateHandler.promises.resyncProjectHistory
           .calledWith(projectId)
           .should.equal(true)
       })
 
-      it('should flush the project history', function () {
-        return this.HistoryManager.flushProject
+      it('should flush the project history', async function () {
+        return this.HistoryManager.promises.flushProject
           .calledWith(projectId)
           .should.equal(true)
       })
-
-      it('should call the callback without an error', function () {
-        return this.callback.called.should.equal(true)
-      })
     })
 
     describe('when the history already exists', function () {
       beforeEach(function () {
         this.project.overleaf = { history: { id: 1234 } }
-        this.ProjectDetailsHandler.getDetails = sinon
+        this.ProjectDetailsHandler.promises.getDetails = sinon
           .stub()
           .withArgs(projectId)
-          .callsArgWith(1, null, this.project)
-        this.ProjectModel.updateOne = sinon.stub()
-        return this.ProjectHistoryHandler.ensureHistoryExistsForProject(
-          projectId,
-          this.callback
+          .resolves(this.project)
+        this.ProjectModel.updateOne = sinon.stub().resolves({ matchedCount: 1 })
+        return this.ProjectHistoryHandler.promises.ensureHistoryExistsForProject(
+          projectId
         )
       })
 
-      it('should get any existing history id for the project', function () {
-        return this.ProjectDetailsHandler.getDetails
+      it('should get any existing history id for the project', async function () {
+        return this.ProjectDetailsHandler.promises.getDetails
           .calledWith(projectId)
           .should.equal(true)
       })
 
-      it('should not initialize a new history in the v1 history service', function () {
-        return this.HistoryManager.initializeProject.called.should.equal(false)
+      it('should not initialize a new history in the v1 history service', async function () {
+        return this.HistoryManager.promises.initializeProject.called.should.equal(
+          false
+        )
       })
 
-      it('should not set the new history id on the project', function () {
+      it('should not set the new history id on the project', async function () {
         return this.ProjectModel.updateOne.called.should.equal(false)
       })
 
-      it('should not resync the project history', function () {
-        return this.ProjectEntityUpdateHandler.resyncProjectHistory.called.should.equal(
+      it('should not resync the project history', async function () {
+        return this.ProjectEntityUpdateHandler.promises.resyncProjectHistory.called.should.equal(
           false
         )
       })
 
-      it('should not flush the project history', function () {
-        return this.HistoryManager.flushProject.called.should.equal(false)
-      })
-
-      it('should call the callback', function () {
-        return this.callback.calledWith().should.equal(true)
+      it('should not flush the project history', async function () {
+        return this.HistoryManager.promises.flushProject.called.should.equal(
+          false
+        )
       })
     })
   })