Merge pull request #14027 from overleaf/ab-tags-handler-async

[web] Convert TagsHandler to async/await

GitOrigin-RevId: ddde174b77a6d42e61f3f6aa2783762c8605ee91

services/web/app/src/Features/Tags/TagsHandler.js

@@ -1,44 +1,36 @@
 const { Tag } = require('../../models/Tag')
-const { promisifyAll } = require('../../util/promises')
+const { callbackify } = require('../../util/promises')
 
 const MAX_TAG_LENGTH = 50
 
-function getAllTags(userId, callback) {
-  Tag.find({ user_id: userId }, callback)
+async function getAllTags(userId) {
+  return Tag.find({ user_id: userId })
 }
 
-function createTag(userId, name, color, options, callback) {
-  if (typeof options === 'function') {
-    callback = options
-    options = {}
-  }
-  if (!callback) {
-    callback = function () {}
-  }
+async function createTag(userId, name, color, options = {}) {
   if (name.length > MAX_TAG_LENGTH) {
     if (options.truncate) {
       name = name.slice(0, MAX_TAG_LENGTH)
     } else {
-      return callback(new Error('Exceeded max tag length'))
+      throw new Error('Exceeded max tag length')
     }
   }
-  Tag.create({ user_id: userId, name, color }, function (err, tag) {
+  try {
+    return await Tag.create({ user_id: userId, name, color })
+  } catch (error) {
     // on duplicate key error return existing tag
-    if (err && err.code === 11000) {
-      return Tag.findOne({ user_id: userId, name }, callback)
+    if (error && error.code === 11000) {
+      return Tag.findOne({ user_id: userId, name })
     }
-    callback(err, tag)
-  })
+    throw error
+  }
 }
 
-function renameTag(userId, tagId, name, callback) {
-  if (!callback) {
-    callback = function () {}
-  }
+async function renameTag(userId, tagId, name) {
   if (name.length > MAX_TAG_LENGTH) {
-    return callback(new Error('Exceeded max tag length'))
+    throw new Error('Exceeded max tag length')
   }
-  Tag.updateOne(
+  return Tag.updateOne(
     {
       _id: tagId,
       user_id: userId,
@@ -47,16 +39,15 @@ function renameTag(userId, tagId, name, callback) {
       $set: {
         name,
       },
-    },
-    callback
+    }
   )
 }
 
-function editTag(userId, tagId, name, color, callback) {
+async function editTag(userId, tagId, name, color) {
   if (name.length > MAX_TAG_LENGTH) {
-    return callback(new Error('Exceeded max tag length'))
+    throw new Error('Exceeded max tag length')
   }
-  Tag.updateOne(
+  return Tag.updateOne(
     {
       _id: tagId,
       user_id: userId,
@@ -66,113 +57,91 @@ function editTag(userId, tagId, name, color, callback) {
         name,
         color,
       },
-    },
-    callback
-  )
-}
-
-function deleteTag(userId, tagId, callback) {
-  if (!callback) {
-    callback = function () {}
-  }
-  Tag.deleteOne(
-    {
-      _id: tagId,
-      user_id: userId,
-    },
-    callback
+    }
   )
 }
 
-// TODO: unused?
-function updateTagUserIds(oldUserId, newUserId, callback) {
-  if (!callback) {
-    callback = function () {}
-  }
-  const searchOps = { user_id: oldUserId }
-  const updateOperation = { $set: { user_id: newUserId } }
-  Tag.updateMany(searchOps, updateOperation, callback)
+async function deleteTag(userId, tagId) {
+  await Tag.deleteOne({
+    _id: tagId,
+    user_id: userId,
+  })
 }
 
-function removeProjectFromTag(userId, tagId, projectId, callback) {
-  if (!callback) {
-    callback = function () {}
-  }
+async function removeProjectFromTag(userId, tagId, projectId) {
   const searchOps = {
     _id: tagId,
     user_id: userId,
   }
   const deleteOperation = { $pull: { project_ids: projectId } }
-  Tag.updateOne(searchOps, deleteOperation, callback)
+  await Tag.updateOne(searchOps, deleteOperation)
 }
 
-function removeProjectsFromTag(userId, tagId, projectIds, callback) {
-  if (!callback) {
-    callback = function () {}
-  }
+async function removeProjectsFromTag(userId, tagId, projectIds) {
   const searchOps = {
     _id: tagId,
     user_id: userId,
   }
   const deleteOperation = { $pullAll: { project_ids: projectIds } }
-  Tag.updateOne(searchOps, deleteOperation, callback)
+  await Tag.updateOne(searchOps, deleteOperation)
 }
 
-function addProjectToTag(userId, tagId, projectId, callback) {
-  if (!callback) {
-    callback = function () {}
-  }
+async function addProjectToTag(userId, tagId, projectId) {
   const searchOps = {
     _id: tagId,
     user_id: userId,
   }
   const insertOperation = { $addToSet: { project_ids: projectId } }
-  Tag.findOneAndUpdate(searchOps, insertOperation, callback)
+  return Tag.findOneAndUpdate(searchOps, insertOperation)
 }
 
-function addProjectsToTag(userId, tagId, projectIds, callback) {
-  if (!callback) {
-    callback = function () {}
-  }
+async function addProjectsToTag(userId, tagId, projectIds) {
   const searchOps = {
     _id: tagId,
     user_id: userId,
   }
   const insertOperation = { $addToSet: { project_ids: { $each: projectIds } } }
-  Tag.findOneAndUpdate(searchOps, insertOperation, callback)
+  await Tag.findOneAndUpdate(searchOps, insertOperation)
 }
 
-function addProjectToTagName(userId, name, projectId, callback) {
-  if (!callback) {
-    callback = function () {}
-  }
+async function addProjectToTagName(userId, name, projectId) {
   const searchOps = {
     name,
     user_id: userId,
   }
   const insertOperation = { $addToSet: { project_ids: projectId } }
-  Tag.updateOne(searchOps, insertOperation, { upsert: true }, callback)
+  await Tag.updateOne(searchOps, insertOperation, { upsert: true })
 }
 
-function removeProjectFromAllTags(userId, projectId, callback) {
+async function removeProjectFromAllTags(userId, projectId) {
   const searchOps = { user_id: userId }
   const deleteOperation = { $pull: { project_ids: projectId } }
-  Tag.updateMany(searchOps, deleteOperation, callback)
+  await Tag.updateMany(searchOps, deleteOperation)
 }
 
-const TagsHandler = {
-  getAllTags,
-  createTag,
-  renameTag,
-  editTag,
-  deleteTag,
-  updateTagUserIds,
-  addProjectToTag,
-  addProjectsToTag,
-  removeProjectFromTag,
-  removeProjectsFromTag,
-  addProjectToTagName,
-  removeProjectFromAllTags,
+module.exports = {
+  getAllTags: callbackify(getAllTags),
+  createTag: callbackify(createTag),
+  renameTag: callbackify(renameTag),
+  editTag: callbackify(editTag),
+  deleteTag: callbackify(deleteTag),
+  addProjectToTag: callbackify(addProjectToTag),
+  addProjectsToTag: callbackify(addProjectsToTag),
+  removeProjectFromTag: callbackify(removeProjectFromTag),
+  removeProjectsFromTag: callbackify(removeProjectsFromTag),
+  addProjectToTagName: callbackify(addProjectToTagName),
+  removeProjectFromAllTags: callbackify(removeProjectFromAllTags),
+  promises: {
+    getAllTags,
+    createTag,
+    renameTag,
+    editTag,
+    deleteTag,
+    addProjectToTag,
+    addProjectsToTag,
+    removeProjectFromTag,
+    removeProjectsFromTag,
+    addProjectToTagName,
+    removeProjectFromAllTags,
+  },
 }
-TagsHandler.promises = promisifyAll(TagsHandler)
-module.exports = TagsHandler

services/web/test/unit/src/Tags/TagsHandlerTests.js

@@ -16,6 +16,7 @@ describe('TagsHandler', function () {
     this.tag = { user_id: this.userId, name: 'some name', color: '#3399CC' }
     this.tagId = ObjectId().toString()
     this.projectId = ObjectId().toString()
+    this.projectIds = [ObjectId().toString(), ObjectId().toString()]
 
     this.mongodb = { ObjectId }
     this.TagMock = sinon.mock(Tag)
@@ -34,7 +35,7 @@ describe('TagsHandler', function () {
       this.TagMock.expects('find')
         .once()
         .withArgs({ user_id: this.userId })
-        .yields(null, stubbedTags)
+        .resolves(stubbedTags)
       this.TagsHandler.getAllTags(this.userId, (err, result) => {
         expect(err).to.not.exist
         this.TagMock.verify()
@@ -50,7 +51,7 @@ describe('TagsHandler', function () {
         this.TagMock.expects('create')
           .withArgs(this.tag)
           .once()
-          .yields(null, this.tag)
+          .resolves(this.tag)
         this.TagsHandler.createTag(
           this.tag.user_id,
           this.tag.name,
@@ -74,7 +75,7 @@ describe('TagsHandler', function () {
         this.TagMock.expects('create')
           .withArgs(this.tag)
           .once()
-          .yields(null, this.tag)
+          .resolves(this.tag)
         this.TagsHandler.createTag(
           this.tag.user_id,
           // Pass this too-long name
@@ -114,11 +115,11 @@ describe('TagsHandler', function () {
         this.TagMock.expects('create')
           .withArgs(this.tag)
           .once()
-          .yields(this.duplicateKeyError)
+          .throws(this.duplicateKeyError)
         this.TagMock.expects('findOne')
           .withArgs({ user_id: this.tag.user_id, name: this.tag.name })
           .once()
-          .yields(null, this.tag)
+          .resolves(this.tag)
         this.TagsHandler.createTag(
           this.tag.user_id,
           this.tag.name,
@@ -138,8 +139,6 @@ describe('TagsHandler', function () {
 
   describe('addProjectToTag', function () {
     describe('with a valid tag_id', function () {
-      beforeEach(function () {})
-
       it('should call update in mongo', function (done) {
         this.TagMock.expects('findOneAndUpdate')
           .once()
@@ -147,7 +146,7 @@ describe('TagsHandler', function () {
             { _id: this.tagId, user_id: this.userId },
             { $addToSet: { project_ids: this.projectId } }
           )
-          .yields()
+          .resolves()
         this.TagsHandler.addProjectToTag(
           this.userId,
           this.tagId,
@@ -162,6 +161,30 @@ describe('TagsHandler', function () {
     })
   })
 
+  describe('addProjectsToTag', function () {
+    describe('with a valid tag_id', function () {
+      it('should call update in mongo', function (done) {
+        this.TagMock.expects('findOneAndUpdate')
+          .once()
+          .withArgs(
+            { _id: this.tagId, user_id: this.userId },
+            { $addToSet: { project_ids: { $each: this.projectIds } } }
+          )
+          .resolves()
+        this.TagsHandler.addProjectsToTag(
+          this.userId,
+          this.tagId,
+          this.projectIds,
+          err => {
+            expect(err).to.not.exist
+            this.TagMock.verify()
+            done()
+          }
+        )
+      })
+    })
+  })
+
   describe('addProjectToTagName', function () {
     it('should call update in mongo', function (done) {
       this.TagMock.expects('updateOne')
@@ -171,7 +194,7 @@ describe('TagsHandler', function () {
           { $addToSet: { project_ids: this.projectId } },
           { upsert: true }
         )
-        .yields()
+        .resolves()
       this.TagsHandler.addProjectToTagName(
         this.tag.userId,
         this.tag.name,
@@ -185,25 +208,36 @@ describe('TagsHandler', function () {
     })
   })
 
-  describe('updateTagUserIds', function () {
-    it('should call update in mongo', function (done) {
-      this.newUserId = ObjectId().toString()
-      this.TagMock.expects('updateMany')
-        .once()
-        .withArgs(
-          { user_id: this.userId },
-          { $set: { user_id: this.newUserId } }
+  describe('removeProjectFromTag', function () {
+    describe('with a valid tag_id', function () {
+      it('should call update in mongo', function (done) {
+        this.TagMock.expects('updateOne')
+          .once()
+          .withArgs(
+            {
+              _id: this.tagId,
+              user_id: this.userId,
+            },
+            {
+              $pull: { project_ids: this.projectId },
+            }
+          )
+          .resolves()
+        this.TagsHandler.removeProjectFromTag(
+          this.userId,
+          this.tagId,
+          this.projectId,
+          err => {
+            expect(err).to.not.exist
+            this.TagMock.verify()
+            done()
+          }
         )
-        .yields()
-      this.TagsHandler.updateTagUserIds(this.userId, this.newUserId, err => {
-        expect(err).to.not.exist
-        this.TagMock.verify()
-        done()
       })
     })
   })
 
-  describe('removeProjectFromTag', function () {
+  describe('removeProjectsFromTag', function () {
     describe('with a valid tag_id', function () {
       it('should call update in mongo', function (done) {
         this.TagMock.expects('updateOne')
@@ -214,14 +248,14 @@ describe('TagsHandler', function () {
               user_id: this.userId,
             },
             {
-              $pull: { project_ids: this.projectId },
+              $pullAll: { project_ids: this.projectIds },
             }
           )
-          .yields()
-        this.TagsHandler.removeProjectFromTag(
+          .resolves()
+        this.TagsHandler.removeProjectsFromTag(
           this.userId,
           this.tagId,
-          this.projectId,
+          this.projectIds,
           err => {
             expect(err).to.not.exist
             this.TagMock.verify()
@@ -244,7 +278,7 @@ describe('TagsHandler', function () {
             $pull: { project_ids: this.projectId },
           }
         )
-        .yields()
+        .resolves()
       this.TagsHandler.removeProjectFromAllTags(
         this.userId,
         this.projectId,
@@ -263,7 +297,7 @@ describe('TagsHandler', function () {
         this.TagMock.expects('deleteOne')
           .once()
           .withArgs({ _id: this.tagId, user_id: this.userId })
-          .yields()
+          .resolves()
         this.TagsHandler.deleteTag(this.userId, this.tagId, err => {
           expect(err).to.not.exist
           this.TagMock.verify()
@@ -283,7 +317,7 @@ describe('TagsHandler', function () {
             { _id: this.tagId, user_id: this.userId },
             { $set: { name: this.newName } }
           )
-          .yields()
+          .resolves()
         this.TagsHandler.renameTag(
           this.userId,
           this.tagId,