Merge pull request #16657 from overleaf/dp-mongoose-callback-project-update-handler

Promisify ProjectUpdateHandler and ProjectUpdateHandlerTests

GitOrigin-RevId: 312cbe71d431cf50932ab7d5501529d87f7827f2

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

@@ -204,11 +204,11 @@ const ProjectEntityUpdateHandler = {
             return callback(null, { rev })
           }
           // Don't need to block for marking as updated
-          ProjectUpdateHandler.markAsUpdated(
-            projectId,
-            lastUpdatedAt,
-            lastUpdatedBy
-          )
+          ProjectUpdateHandler.promises
+            .markAsUpdated(projectId, lastUpdatedAt, lastUpdatedBy)
+            .catch(error => {
+              logger.error({ error }, 'failed to mark project as updated')
+            })
           TpdsUpdateSender.addDoc(
             {
               projectId,
@@ -557,7 +557,11 @@ const ProjectEntityUpdateHandler = {
               if (error != null) {
                 return callback(error)
               }
-              ProjectUpdateHandler.markAsUpdated(projectId, new Date(), userId)
+              ProjectUpdateHandler.promises
+                .markAsUpdated(projectId, new Date(), userId)
+                .catch(error => {
+                  logger.error({ error }, 'failed to mark project as updated')
+                })
               callback(null, fileRef, folderId)
             }
           )
@@ -616,7 +620,11 @@ const ProjectEntityUpdateHandler = {
             if (err != null) {
               return callback(err)
             }
-            ProjectUpdateHandler.markAsUpdated(projectId, new Date(), userId)
+            ProjectUpdateHandler.promises
+              .markAsUpdated(projectId, new Date(), userId)
+              .catch(error => {
+                logger.error({ error }, 'failed to mark project as updated')
+              })
 
             DocumentUpdaterHandler.updateProjectStructure(
               projectId,

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

@@ -1,24 +1,9 @@
-/* eslint-disable
-    n/handle-callback-err,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const { Project } = require('../../models/Project')
-const logger = require('@overleaf/logger')
-const { promisifyAll } = require('@overleaf/promise-utils')
+const { callbackify } = require('util')
 
 const ProjectUpdateHandler = {
-  markAsUpdated(projectId, lastUpdatedAt, lastUpdatedBy, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
-    if (lastUpdatedAt == null) {
+  async markAsUpdated(projectId, lastUpdatedAt, lastUpdatedBy) {
+    if (!lastUpdatedAt) {
       lastUpdatedAt = new Date()
     }
 
@@ -31,59 +16,32 @@ const ProjectUpdateHandler = {
       lastUpdated: lastUpdatedAt || new Date().getTime(),
       lastUpdatedBy,
     }
-    Project.updateOne(conditions, update, {}, callback)
+    await Project.updateOne(conditions, update, {}).exec()
   },
 
-  // like markAsUpdated but allows lastUpdatedAt to be reset to earlier time
-  resetUpdated(projectId, lastUpdatedAt, lastUpdatedBy, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
-    if (lastUpdatedAt == null) {
-      lastUpdatedAt = new Date()
-    }
-
-    const conditions = {
-      _id: projectId,
-    }
-
-    const update = {
-      lastUpdated: lastUpdatedAt || new Date().getTime(),
-      lastUpdatedBy,
-    }
-    Project.updateOne(conditions, update, {}, callback)
-  },
-
-  markAsOpened(projectId, callback) {
+  async markAsOpened(projectId) {
     const conditions = { _id: projectId }
     const update = { lastOpened: Date.now() }
-    Project.updateOne(conditions, update, {}, function (err) {
-      if (callback != null) {
-        return callback()
-      }
-    })
+    await Project.updateOne(conditions, update, {}).exec()
   },
 
-  markAsInactive(projectId, callback) {
+  async markAsInactive(projectId) {
     const conditions = { _id: projectId }
     const update = { active: false }
-    Project.updateOne(conditions, update, {}, function (err) {
-      if (callback != null) {
-        return callback()
-      }
-    })
+    await Project.updateOne(conditions, update, {}).exec()
   },
 
-  markAsActive(projectId, callback) {
+  async markAsActive(projectId) {
     const conditions = { _id: projectId }
     const update = { active: true }
-    Project.updateOne(conditions, update, {}, function (err) {
-      if (callback != null) {
-        return callback()
-      }
-    })
+    await Project.updateOne(conditions, update, {}).exec()
   },
 }
 
-ProjectUpdateHandler.promises = promisifyAll(ProjectUpdateHandler)
-module.exports = ProjectUpdateHandler
+module.exports = {
+  markAsUpdated: callbackify(ProjectUpdateHandler.markAsUpdated),
+  markAsOpened: callbackify(ProjectUpdateHandler.markAsOpened),
+  markAsInactive: callbackify(ProjectUpdateHandler.markAsInactive),
+  markAsActive: callbackify(ProjectUpdateHandler.markAsActive),
+  promises: ProjectUpdateHandler,
+}

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

@@ -103,7 +103,9 @@ describe('ProjectEntityUpdateHandler', function () {
       findElementByPath: sinon.stub(),
     }
     this.ProjectUpdater = {
-      markAsUpdated: sinon.stub(),
+      promises: {
+        markAsUpdated: sinon.stub().resolves(),
+      },
     }
     this.ProjectEntityHandler = {
       getDoc: sinon.stub(),
@@ -244,7 +246,7 @@ describe('ProjectEntityUpdateHandler', function () {
 
       it('should mark the project as updated', function () {
         sinon.assert.calledWith(
-          this.ProjectUpdater.markAsUpdated,
+          this.ProjectUpdater.promises.markAsUpdated,
           projectId,
           this.lastUpdatedAt,
           this.lastUpdatedBy
@@ -283,7 +285,7 @@ describe('ProjectEntityUpdateHandler', function () {
       })
 
       it('should not mark the project as updated', function () {
-        this.ProjectUpdater.markAsUpdated.called.should.equal(false)
+        this.ProjectUpdater.promises.markAsUpdated.called.should.equal(false)
       })
 
       it('should not send the doc the to the TPDS', function () {
@@ -326,7 +328,7 @@ describe('ProjectEntityUpdateHandler', function () {
       })
 
       it('should not mark the project as updated', function () {
-        this.ProjectUpdater.markAsUpdated.called.should.equal(false)
+        this.ProjectUpdater.promises.markAsUpdated.called.should.equal(false)
       })
 
       it('should not send the doc the to the TPDS', function () {
@@ -373,7 +375,7 @@ describe('ProjectEntityUpdateHandler', function () {
       })
 
       it('should not mark the project as updated', function () {
-        this.ProjectUpdater.markAsUpdated.called.should.equal(false)
+        this.ProjectUpdater.promises.markAsUpdated.called.should.equal(false)
       })
 
       it('should not send the doc the to the TPDS', function () {
@@ -658,7 +660,7 @@ describe('ProjectEntityUpdateHandler', function () {
       })
 
       it('should mark the project as updated', function () {
-        const args = this.ProjectUpdater.markAsUpdated.args[0]
+        const args = this.ProjectUpdater.promises.markAsUpdated.args[0]
         args[0].should.equal(projectId)
         args[1].should.exist
         args[2].should.equal(userId)
@@ -1066,7 +1068,7 @@ describe('ProjectEntityUpdateHandler', function () {
       })
 
       it('should mark the project as updated', function () {
-        const args = this.ProjectUpdater.markAsUpdated.args[0]
+        const args = this.ProjectUpdater.promises.markAsUpdated.args[0]
         args[0].should.equal(projectId)
         args[1].should.exist
         args[2].should.equal(userId)

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

@@ -1,16 +1,3 @@
-/* eslint-disable
-    n/handle-callback-err,
-    max-len,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const sinon = require('sinon')
 const modulePath =
   '../../../../app/src/Features/Project/ProjectUpdateHandler.js'
@@ -27,98 +14,89 @@ describe('ProjectUpdateHandler', function () {
   })
 
   beforeEach(function () {
-    let Project
-    this.ProjectModel = Project = class Project {}
-    this.ProjectModel.updateOne = sinon.stub().callsArg(3)
-    return (this.handler = SandboxedModule.require(modulePath, {
+    this.ProjectModel = class Project {}
+    this.ProjectModel.updateOne = sinon.stub().returns({
+      exec: sinon.stub(),
+    })
+    this.handler = SandboxedModule.require(modulePath, {
       requires: {
         '../../models/Project': { Project: this.ProjectModel },
       },
-    }))
+    })
   })
 
   describe('marking a project as recently updated', function () {
     beforeEach(function () {
       this.project_id = 'project_id'
       this.lastUpdatedAt = 987654321
-      return (this.lastUpdatedBy = 'fake-last-updater-id')
+      this.lastUpdatedBy = 'fake-last-updater-id'
     })
 
-    it('should send an update to mongo', function (done) {
-      return this.handler.markAsUpdated(
+    it('should send an update to mongo', async function () {
+      await this.handler.promises.markAsUpdated(
         this.project_id,
         this.lastUpdatedAt,
-        this.lastUpdatedBy,
-        err => {
-          sinon.assert.calledWith(
-            this.ProjectModel.updateOne,
-            {
-              _id: this.project_id,
-              lastUpdated: { $lt: this.lastUpdatedAt },
-            },
-            {
-              lastUpdated: this.lastUpdatedAt,
-              lastUpdatedBy: this.lastUpdatedBy,
-            }
-          )
-          return done()
+        this.lastUpdatedBy
+      )
+
+      sinon.assert.calledWith(
+        this.ProjectModel.updateOne,
+        {
+          _id: this.project_id,
+          lastUpdated: { $lt: this.lastUpdatedAt },
+        },
+        {
+          lastUpdated: this.lastUpdatedAt,
+          lastUpdatedBy: this.lastUpdatedBy,
         }
       )
     })
 
-    it('should set smart fallbacks', function (done) {
-      return this.handler.markAsUpdated(this.project_id, null, null, err => {
-        sinon.assert.calledWithMatch(
-          this.ProjectModel.updateOne,
-          {
-            _id: this.project_id,
-            lastUpdated: { $lt: this.fakeTime },
-          },
-          {
-            lastUpdated: this.fakeTime,
-            lastUpdatedBy: null,
-          }
-        )
-        return done()
-      })
+    it('should set smart fallbacks', async function () {
+      await this.handler.promises.markAsUpdated(this.project_id, null, null)
+      sinon.assert.calledWithMatch(
+        this.ProjectModel.updateOne,
+        {
+          _id: this.project_id,
+          lastUpdated: { $lt: this.fakeTime },
+        },
+        {
+          lastUpdated: this.fakeTime,
+          lastUpdatedBy: null,
+        }
+      )
     })
   })
 
   describe('markAsOpened', function () {
-    it('should send an update to mongo', function (done) {
+    it('should send an update to mongo', async function () {
       const projectId = 'project_id'
-      return this.handler.markAsOpened(projectId, err => {
-        const args = this.ProjectModel.updateOne.args[0]
-        args[0]._id.should.equal(projectId)
-        const date = args[1].lastOpened + ''
-        const now = Date.now() + ''
-        date.substring(0, 5).should.equal(now.substring(0, 5))
-        return done()
-      })
+      await this.handler.promises.markAsOpened(projectId)
+      const args = this.ProjectModel.updateOne.args[0]
+      args[0]._id.should.equal(projectId)
+      const date = args[1].lastOpened + ''
+      const now = Date.now() + ''
+      date.substring(0, 5).should.equal(now.substring(0, 5))
     })
   })
 
   describe('markAsInactive', function () {
-    it('should send an update to mongo', function (done) {
+    it('should send an update to mongo', async function () {
       const projectId = 'project_id'
-      return this.handler.markAsInactive(projectId, err => {
-        const args = this.ProjectModel.updateOne.args[0]
-        args[0]._id.should.equal(projectId)
-        args[1].active.should.equal(false)
-        return done()
-      })
+      await this.handler.promises.markAsInactive(projectId)
+      const args = this.ProjectModel.updateOne.args[0]
+      args[0]._id.should.equal(projectId)
+      args[1].active.should.equal(false)
     })
   })
 
   describe('markAsActive', function () {
-    it('should send an update to mongo', function (done) {
+    it('should send an update to mongo', async function () {
       const projectId = 'project_id'
-      return this.handler.markAsActive(projectId, err => {
-        const args = this.ProjectModel.updateOne.args[0]
-        args[0]._id.should.equal(projectId)
-        args[1].active.should.equal(true)
-        return done()
-      })
+      await this.handler.promises.markAsActive(projectId)
+      const args = this.ProjectModel.updateOne.args[0]
+      args[0]._id.should.equal(projectId)
+      args[1].active.should.equal(true)
     })
   })
 })