Merge pull request #1924 from overleaf/ta-full-error

Don't Callback with String

GitOrigin-RevId: 82e3efb055ef197d95ff9c8a876bee0d6a0327a3

services/web/app/src/Features/Announcements/AnnouncementsHandler.js

@@ -40,7 +40,7 @@ module.exports = AnnouncementsHandler = {
       callback = function(err, announcements) {}
     }
     if (user == null && user._id == null) {
-      return callback('user not supplied')
+      return callback(new Error('user not supplied'))
     }
 
     const timestamp = user._id.toString().substring(0, 8)

services/web/app/src/Features/Blog/BlogHandler.js

@@ -30,7 +30,7 @@ module.exports = BlogHandler = {
         return callback(err)
       }
       if (res.statusCode !== 200) {
-        return callback('blog announcement returned non 200')
+        return callback(new Error('blog announcement returned non 200'))
       }
       logger.log(
         {

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

@@ -159,11 +159,11 @@ module.exports = EditorHttpController = {
       'editor',
       user_id,
       function(error, doc) {
-        if (error === 'project_has_to_many_files') {
+        if (error && error.message === 'project_has_to_many_files') {
           return res
             .status(400)
             .json(req.i18n.translate('project_has_to_many_files'))
-        } else if (error != null) {
+        } else if (error) {
           return next(error)
         } else {
           return res.json(doc)
@@ -185,15 +185,13 @@ module.exports = EditorHttpController = {
       name,
       'editor',
       function(error, doc) {
-        if (error === 'project_has_to_many_files') {
+        if (error && error.message === 'project_has_to_many_files') {
           return res
             .status(400)
             .json(req.i18n.translate('project_has_to_many_files'))
-        } else if (
-          (error != null ? error.message : undefined) === 'invalid element name'
-        ) {
+        } else if (error && error.message === 'invalid element name') {
           return res.status(400).json(req.i18n.translate('invalid_file_name'))
-        } else if (error != null) {
+        } else if (error) {
           return next(error)
         } else {
           return res.json(doc)

services/web/app/src/Features/Email/EmailSender.js

@@ -173,7 +173,7 @@ module.exports = {
           },
           'rate limit hit for sending email, not sending'
         )
-        return callback('rate limit hit sending email')
+        return callback(new Error('rate limit hit sending email'))
       }
       metrics.inc('email')
       options = {

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

@@ -651,7 +651,7 @@ module.exports = ProjectEntityMongoUpdateHandler = self = {
         'project too big, stopping insertions'
       )
       CooldownManager.putProjectOnCooldown(project._id)
-      return callback('project_has_to_many_files')
+      return callback(new Error('project_has_to_many_files'))
     }
 
     return ProjectLocator.findElement(

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

@@ -1130,7 +1130,7 @@ module.exports = ProjectEntityUpdateHandler = self = {
     logger.log({ entity_id, entityType, project_id }, 'deleting project entity')
     if (entityType == null) {
       logger.warn({ err: 'No entityType set', project_id, entity_id })
-      return callback('No entityType set')
+      return callback(new Error('No entityType set'))
     }
     entityType = entityType.toLowerCase()
     return ProjectEntityMongoUpdateHandler.deleteEntity(
@@ -1261,7 +1261,7 @@ module.exports = ProjectEntityUpdateHandler = self = {
     )
     if (entityType == null) {
       logger.warn({ err: 'No entityType set', project_id, entity_id })
-      return callback('No entityType set')
+      return callback(new Error('No entityType set'))
     }
     entityType = entityType.toLowerCase()
     return ProjectEntityMongoUpdateHandler.moveEntity(
@@ -1309,7 +1309,7 @@ module.exports = ProjectEntityUpdateHandler = self = {
     logger.log({ entity_id, project_id }, `renaming ${entityType}`)
     if (entityType == null) {
       logger.warn({ err: 'No entityType set', project_id, entity_id })
-      return callback('No entityType set')
+      return callback(new Error('No entityType set'))
     }
     entityType = entityType.toLowerCase()
 

services/web/app/src/Features/Subscription/LimitationsManager.js

@@ -246,7 +246,7 @@ module.exports = LimitationsManager = {
       }
       if (subscription == null) {
         logger.warn({ subscriptionId }, 'no subscription found')
-        return callback('no subscription found')
+        return callback(new Error('no subscription found'))
       }
 
       const limitReached = LimitationsManager.teamHasReachedMemberLimit(

services/web/app/src/Features/Subscription/RecurlyWrapper.js

@@ -505,7 +505,9 @@ module.exports = RecurlyWrapper = {
                   /accounts\/(.*)/
                 )[1]
               } else {
-                return callback("I don't understand the response from Recurly")
+                return callback(
+                  new Error("I don't understand the response from Recurly")
+                )
               }
 
               return RecurlyWrapper.getAccount(accountId, function(

services/web/app/src/Features/Subscription/SubscriptionHandler.js

@@ -225,7 +225,7 @@ module.exports = {
             return callback(error)
           }
           if (user == null) {
-            return callback('no user found')
+            return callback(new Error('no user found'))
           }
           return SubscriptionUpdater.syncSubscription(
             recurlySubscription,

services/web/app/src/Features/Uploads/FileSystemImportManager.js

@@ -44,7 +44,7 @@ module.exports = FileSystemImportManager = {
           { user_id, project_id, folder_id, name, path },
           'add doc is from symlink, stopping process'
         )
-        return callback('path is symlink')
+        return callback(new Error('path is symlink'))
       }
       return fs.readFile(path, charset, function(error, content) {
         if (error != null) {
@@ -90,7 +90,7 @@ module.exports = FileSystemImportManager = {
           { user_id, project_id, folder_id, name, path },
           'add file is from symlink, stopping insert'
         )
-        return callback('path is symlink')
+        return callback(new Error('path is symlink'))
       }
 
       if (replace) {
@@ -132,7 +132,7 @@ module.exports = FileSystemImportManager = {
           { user_id, project_id, folder_id, path },
           'add folder is from symlink, stopping insert'
         )
-        return callback('path is symlink')
+        return callback(new Error('path is symlink'))
       }
       return EditorController.addFolder(
         project_id,
@@ -181,7 +181,7 @@ module.exports = FileSystemImportManager = {
           { user_id, project_id, parent_folder_id, folderPath },
           'add folder contents is from symlink, stopping insert'
         )
-        return callback('path is symlink')
+        return callback(new Error('path is symlink'))
       }
       return fs.readdir(folderPath, (error, entries) => {
         if (entries == null) {
@@ -231,7 +231,7 @@ module.exports = FileSystemImportManager = {
           { user_id, project_id, folder_id, path },
           'add entry is from symlink, stopping insert'
         )
-        return callback('path is symlink')
+        return callback(new Error('path is symlink'))
       }
 
       return FileTypeManager.isDirectory(path, (error, isDirectory) => {

services/web/app/src/Features/User/UserDeleter.js

@@ -65,7 +65,7 @@ module.exports = UserDeleter = {
     }
     if (user_id == null) {
       logger.warn('user_id is null when trying to delete user')
-      return callback('no user_id')
+      return callback(new Error('no user_id'))
     }
     return User.findById(user_id, function(err, user) {
       if (err != null) {

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

@@ -51,7 +51,7 @@ describe('EditorHttpController', function() {
     this.AuthenticationController.getLoggedInUserId = sinon
       .stub()
       .returns(this.userId)
-    this.req = {}
+    this.req = { i18n: { translate: string => string } }
     this.res = {
       send: sinon.stub(),
       sendStatus: sinon.stub(),
@@ -306,13 +306,27 @@ describe('EditorHttpController', function() {
     })
 
     describe('unsuccesfully', function() {
-      beforeEach(function() {
+      it('handle name too short', function() {
         this.req.body.name = ''
-        return this.EditorHttpController.addDoc(this.req, this.res)
+        this.EditorHttpController.addDoc(this.req, this.res)
+        this.res.sendStatus.calledWith(400).should.equal(true)
       })
 
-      it('should send back a bad request status code', function() {
-        return this.res.sendStatus.calledWith(400).should.equal(true)
+      it('handle too many files', function() {
+        this.EditorController.addDoc.yields(
+          new Error('project_has_to_many_files')
+        )
+        let res = {
+          status: status => {
+            status.should.equal(400)
+            return {
+              json: json => {
+                json.should.equal('project_has_to_many_files')
+              }
+            }
+          }
+        }
+        this.EditorHttpController.addDoc(this.req, res)
       })
     })
   })
@@ -352,13 +366,44 @@ describe('EditorHttpController', function() {
     })
 
     describe('unsuccesfully', function() {
-      beforeEach(function() {
+      it('handle name too short', function() {
         this.req.body.name = ''
-        return this.EditorHttpController.addFolder(this.req, this.res)
+        this.EditorHttpController.addFolder(this.req, this.res)
+        this.res.sendStatus.calledWith(400).should.equal(true)
       })
 
-      it('should send back a bad request status code', function() {
-        return this.res.sendStatus.calledWith(400).should.equal(true)
+      it('handle too many files', function() {
+        this.EditorController.addFolder.yields(
+          new Error('project_has_to_many_files')
+        )
+        let res = {
+          status: status => {
+            status.should.equal(400)
+            return {
+              json: json => {
+                json.should.equal('project_has_to_many_files')
+              }
+            }
+          }
+        }
+        this.EditorHttpController.addFolder(this.req, res)
+      })
+
+      it('handle invalid element name', function() {
+        this.EditorController.addFolder.yields(
+          new Error('invalid element name')
+        )
+        let res = {
+          status: status => {
+            status.should.equal(400)
+            return {
+              json: json => {
+                json.should.equal('invalid_file_name')
+              }
+            }
+          }
+        }
+        this.EditorHttpController.addFolder(this.req, res)
       })
     })
   })

services/web/test/unit/src/Subscription/RecurlyWrapperTests.js

@@ -110,7 +110,7 @@ const mockApiRequest = function(options, callback) {
   if (fixtures[options.url]) {
     return callback(null, { statusCode: 200 }, fixtures[options.url])
   } else {
-    return callback('Not found', { statusCode: 404 })
+    return callback(new Error('Not found'), { statusCode: 404 })
   }
 }