Merge pull request #2985 from overleaf/msm-oerror-remove-unprocessable-entity-error

Replace UnprocessableEntityError with calls to unprocessableEntity() handler

GitOrigin-RevId: 4bba389c8cdf87a40137d49db571fa81aaac4239

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

@@ -12,6 +12,7 @@ const PrivilegeLevels = require('../Authorization/PrivilegeLevels')
 const TokenAccessHandler = require('../TokenAccess/TokenAccessHandler')
 const AuthenticationController = require('../Authentication/AuthenticationController')
 const Errors = require('../Errors/Errors')
+const HttpErrorHandler = require('../Errors/HttpErrorHandler')
 const ProjectEntityUpdateHandler = require('../Project/ProjectEntityUpdateHandler')
 const { expressify } = require('../../util/promises')
 
@@ -272,11 +273,11 @@ async function convertDocToFile(req, res, next) {
         info: { public: { message: 'Document not found' } }
       })
     } else if (err instanceof Errors.DocHasRangesError) {
-      throw new HttpErrors.UnprocessableEntityError({
+      return HttpErrorHandler.unprocessableEntity(
-        info: {
+        req,
-          public: { message: 'Document has comments or tracked changes' }
+        res,
-        }
+        'Document has comments or tracked changes'
-      })
+      )
     } else {
       throw err
     }

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

@@ -1,5 +1,16 @@
 const logger = require('logger-sharelatex')
 
+function renderJSONError(res, message, info) {
+  const fullInfo = { message, ...info }
+  if (info.message) {
+    logger.warn(
+      info,
+      `http error info shouldn't contain a 'message' field, will be overridden`
+    )
+  }
+  res.json(fullInfo)
+}
+
 module.exports = {
   forbidden(req, res, message = 'restricted', info = {}) {
     res.status(403)
@@ -7,16 +18,24 @@ module.exports = {
       case 'html':
         return res.render('user/restricted', { title: 'restricted' })
       case 'json':
-        const fullInfo = { message, ...info }
+        return renderJSONError(res, message, info)
-        if (info.message) {
-          logger.warn(
-            info,
-            `http error info shouldn't contain a 'message' field, will be overridden`
-          )
-        }
-        return res.json(fullInfo)
       default:
         return res.send('restricted')
     }
+  },
+
+  unprocessableEntity(req, res, message = 'unprocessable entity', info = {}) {
+    res.status(422)
+    switch (req.accepts(['html', 'json'])) {
+      case 'html':
+        return res.render('general/400', {
+          title: 'Client Error',
+          message: message
+        })
+      case 'json':
+        return renderJSONError(res, message, info)
+      default:
+        return res.send('unprocessable entity')
+    }
   }
 }

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

@@ -30,7 +30,9 @@ const planFeatures = require('./planFeatures')
 const GroupPlansData = require('./GroupPlansData')
 const V1SubscriptionManager = require('./V1SubscriptionManager')
 const Errors = require('../Errors/Errors')
+const HttpErrorHandler = require('../Errors/HttpErrorHandler')
 const SubscriptionErrors = require('./Errors')
+const OError = require('@overleaf/o-error')
 const HttpErrors = require('@overleaf/o-error/http')
 
 module.exports = SubscriptionController = {
@@ -82,11 +84,7 @@ module.exports = SubscriptionController = {
     const user = AuthenticationController.getSessionUser(req)
     const plan = PlansLocator.findLocalPlanInSettings(req.query.planCode)
     if (!plan) {
-      return next(
+      return HttpErrorHandler.unprocessableEntity(req, res, 'Plan not found')
-        new HttpErrors.UnprocessableEntityError({
-          info: { public: { message: 'Plan not found' } }
-        })
-      )
     }
     return LimitationsManager.userHasV1OrV2Subscription(user, function(
       err,
@@ -224,13 +222,16 @@ module.exports = SubscriptionController = {
             return res.sendStatus(201)
           }
 
-          if (err instanceof SubscriptionErrors.RecurlyTransactionError) {
+          if (
-            return next(
+            err instanceof SubscriptionErrors.RecurlyTransactionError ||
-              new HttpErrors.UnprocessableEntityError({}).withCause(err)
+            err instanceof Errors.InvalidError
-            )
+          ) {
-          } else if (err instanceof Errors.InvalidError) {
+            logger.warn(err)
-            return next(
+            return HttpErrorHandler.unprocessableEntity(
-              new HttpErrors.UnprocessableEntityError({}).withCause(err)
+              req,
+              res,
+              err.message,
+              OError.getFullInfo(err).public
             )
           }
 

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

@@ -13,6 +13,7 @@ const UserSessionsManager = require('./UserSessionsManager')
 const UserUpdater = require('./UserUpdater')
 const SudoModeHandler = require('../SudoMode/SudoModeHandler')
 const Errors = require('../Errors/Errors')
+const HttpErrorHandler = require('../Errors/HttpErrorHandler')
 const OError = require('@overleaf/o-error')
 const HttpErrors = require('@overleaf/o-error/http')
 const EmailHandler = require('../Email/EmailHandler')
@@ -108,10 +109,12 @@ const UserController = {
                 errorData.info.public = {
                   error: 'SubscriptionAdminDeletionError'
                 }
-                return next(
+                logger.warn(new OError(errorData).withCause(err))
-                  new HttpErrors.UnprocessableEntityError(errorData).withCause(
+                return HttpErrorHandler.unprocessableEntity(
-                    err
+                  req,
-                  )
+                  res,
+                  errorData.message,
+                  errorData.info.public
                 )
               } else {
                 return next(new OError(errorData).withCause(err))

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

@@ -122,6 +122,9 @@ describe('EditorHttpController', function() {
         convertDocToFile: sinon.stub().resolves(this.file)
       }
     }
+    this.HttpErrorHandler = {
+      unprocessableEntity: sinon.stub()
+    }
     this.EditorHttpController = SandboxedModule.require(MODULE_PATH, {
       globals: {
         console: console
@@ -144,6 +147,7 @@ describe('EditorHttpController', function() {
         '../../infrastructure/FileWriter': this.FileWriter,
         '../Project/ProjectEntityUpdateHandler': this
           .ProjectEntityUpdateHandler,
+        '../Errors/HttpErrorHandler': this.HttpErrorHandler,
         '../Errors/Errors': Errors,
         '@overleaf/o-error/http': HttpErrors
       }
@@ -540,14 +544,19 @@ describe('EditorHttpController', function() {
     })
 
     describe('when the doc has ranges', function() {
-      it('should return a 422', function(done) {
+      it('should return a 422 - Unprocessable Entity', function(done) {
         this.ProjectEntityUpdateHandler.promises.convertDocToFile.rejects(
           new Errors.DocHasRangesError({})
         )
-        this.EditorHttpController.convertDocToFile(this.req, this.res, err => {
+        this.HttpErrorHandler.unprocessableEntity = sinon.spy(
-          expect(err).to.be.instanceof(HttpErrors.UnprocessableEntityError)
+          (req, res, message) => {
-          done()
+            expect(req).to.exist
-        })
+            expect(res).to.exist
+            expect(message).to.equal('Document has comments or tracked changes')
+            done()
+          }
+        )
+        this.EditorHttpController.convertDocToFile(this.req, this.res)
       })
     })
   })

services/web/test/unit/src/Errors/HttpErrorHandlerTests.js

@@ -45,4 +45,42 @@ describe('HttpErrorHandler', function() {
       })
     })
   })
+
+  describe('unprocessableEntity', function() {
+    it('returns 422', function() {
+      this.HttpErrorHandler.unprocessableEntity(this.req, this.res)
+      expect(this.res.statusCode).to.equal(422)
+    })
+
+    it('should print a message when no content-type is included', function() {
+      this.HttpErrorHandler.unprocessableEntity(this.req, this.res)
+      expect(this.res.body).to.equal('unprocessable entity')
+    })
+
+    it("should render a template including the error message when content-type is 'html'", function() {
+      this.req.accepts = () => 'html'
+      this.HttpErrorHandler.unprocessableEntity(this.req, this.res, 'an error')
+      expect(this.res.renderedTemplate).to.equal('general/400')
+      expect(this.res.renderedVariables).to.deep.equal({
+        title: 'Client Error',
+        message: 'an error'
+      })
+    })
+
+    it("should return a json object when content-type is 'json'", function() {
+      this.req.accepts = () => 'json'
+      this.HttpErrorHandler.unprocessableEntity(
+        this.req,
+        this.res,
+        'an error',
+        {
+          foo: 'bar'
+        }
+      )
+      expect(JSON.parse(this.res.body)).to.deep.equal({
+        message: 'an error',
+        foo: 'bar'
+      })
+    })
+  })
 })

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

@@ -121,6 +121,9 @@ describe('SubscriptionController', function() {
         './FeaturesUpdater': (this.FeaturesUpdater = {}),
         './GroupPlansData': (this.GroupPlansData = {}),
         './V1SubscriptionManager': (this.V1SubscriptionManager = {}),
+        '../Errors/HttpErrorHandler': (this.HttpErrorHandler = {
+          unprocessableEntity: sinon.stub()
+        }),
         '../Errors/Errors': Errors,
         './Errors': SubscriptionErrors,
         '@overleaf/o-error/http': HttpErrors
@@ -240,18 +243,21 @@ describe('SubscriptionController', function() {
     })
 
     describe('with an invalid plan code', function() {
-      it('should return 422 error', function(done) {
+      it('should return 422 error - Unprocessable Entity', function(done) {
         this.LimitationsManager.userHasV1OrV2Subscription.callsArgWith(
           1,
           null,
           false
         )
         this.PlansLocator.findLocalPlanInSettings.returns(null)
-        this.next = error => {
+        this.HttpErrorHandler.unprocessableEntity = sinon.spy(
-          expect(error).to.exist
+          (req, res, message) => {
-          expect(error.statusCode).to.equal(422)
+            expect(req).to.exist
-          done()
+            expect(res).to.exist
-        }
+            expect(message).to.deep.equal('Plan not found')
+            done()
+          }
+        )
         return this.SubscriptionController.paymentPage(
           this.req,
           this.res,
@@ -470,6 +476,39 @@ describe('SubscriptionController', function() {
       })
       return done()
     })
+
+    it('should handle recurly errors', function(done) {
+      this.LimitationsManager.userHasV1OrV2Subscription.yields(null, false)
+      this.SubscriptionHandler.createSubscription.yields(
+        new SubscriptionErrors.RecurlyTransactionError({})
+      )
+
+      this.HttpErrorHandler.unprocessableEntity = sinon.spy(
+        (req, res, info) => {
+          expect(req).to.exist
+          expect(res).to.exist
+          expect(info).to.deep.equal('Unknown transaction error')
+          done()
+        }
+      )
+
+      return this.SubscriptionController.createSubscription(this.req, this.res)
+    })
+
+    it('should handle invalid error', function(done) {
+      this.LimitationsManager.userHasV1OrV2Subscription.yields(null, false)
+      this.SubscriptionHandler.createSubscription.yields(
+        new Errors.InvalidError({})
+      )
+
+      this.HttpErrorHandler.unprocessableEntity = sinon.spy((req, res) => {
+        expect(req).to.exist
+        expect(res).to.exist
+        done()
+      })
+
+      return this.SubscriptionController.createSubscription(this.req, this.res)
+    })
   })
 
   describe('updateSubscription via post', function() {

services/web/test/unit/src/User/UserControllerTests.js

@@ -70,6 +70,9 @@ describe('UserController', function() {
       revokeAllUserSessions: sinon.stub().callsArgWith(2, null)
     }
     this.SudoModeHandler = { clearSudoMode: sinon.stub() }
+    this.HttpErrorHandler = {
+      unprocessableEntity: sinon.stub()
+    }
     this.UserController = SandboxedModule.require(modulePath, {
       globals: {
         console: console
@@ -95,6 +98,7 @@ describe('UserController', function() {
         './UserHandler': this.UserHandler,
         './UserSessionsManager': this.UserSessionsManager,
         '../SudoMode/SudoModeHandler': this.SudoModeHandler,
+        '../Errors/HttpErrorHandler': this.HttpErrorHandler,
         'settings-sharelatex': this.settings,
         'logger-sharelatex': {
           log() {},
@@ -233,17 +237,19 @@ describe('UserController', function() {
           .yields(new Errors.SubscriptionAdminDeletionError())
       })
 
-      it('should return a json error', function(done) {
+      it('should return a HTTP Unprocessable Entity error', function(done) {
-        this.UserController.tryDeleteUser(this.req, null, error => {
+        this.HttpErrorHandler.unprocessableEntity = sinon.spy(
-          expect(error).to.be.instanceof(HttpErrors.UnprocessableEntityError)
+          (req, res, message, info) => {
-          expect(
+            expect(req).to.exist
-            OError.hasCauseInstanceOf(
+            expect(res).to.exist
-              error,
+            expect(message).to.equal('error while deleting user account')
-              Errors.SubscriptionAdminDeletionError
+            expect(info).to.deep.equal({
-            )
+              error: 'SubscriptionAdminDeletionError'
-          ).to.be.true
+            })
-          done()
+            done()
-        })
+          }
+        )
+        this.UserController.tryDeleteUser(this.req, this.res)
       })
     })