From 6562f3003d8a5f8a66da63ca7800aac93039e56d Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Thu, 16 Jul 2020 08:47:46 +0200 Subject: [PATCH] Merge pull request #2985 from overleaf/msm-oerror-remove-unprocessable-entity-error Replace UnprocessableEntityError with calls to unprocessableEntity() handler GitOrigin-RevId: 4bba389c8cdf87a40137d49db571fa81aaac4239 --- .../Features/Editor/EditorHttpController.js | 11 ++-- .../src/Features/Errors/HttpErrorHandler.js | 35 ++++++++++--- .../Subscription/SubscriptionController.js | 25 ++++----- .../app/src/Features/User/UserController.js | 11 ++-- .../src/Editor/EditorHttpControllerTests.js | 19 +++++-- .../unit/src/Errors/HttpErrorHandlerTests.js | 38 ++++++++++++++ .../SubscriptionControllerTests.js | 51 ++++++++++++++++--- .../test/unit/src/User/UserControllerTests.js | 28 ++++++---- 8 files changed, 167 insertions(+), 51 deletions(-) diff --git a/services/web/app/src/Features/Editor/EditorHttpController.js b/services/web/app/src/Features/Editor/EditorHttpController.js index 7776a775f2..5931cf275d 100644 --- a/services/web/app/src/Features/Editor/EditorHttpController.js +++ b/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({ - info: { - public: { message: 'Document has comments or tracked changes' } - } - }) + return HttpErrorHandler.unprocessableEntity( + req, + res, + 'Document has comments or tracked changes' + ) } else { throw err } diff --git a/services/web/app/src/Features/Errors/HttpErrorHandler.js b/services/web/app/src/Features/Errors/HttpErrorHandler.js index 92b407ffa5..d6b5c76d2b 100644 --- a/services/web/app/src/Features/Errors/HttpErrorHandler.js +++ b/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 } - if (info.message) { - logger.warn( - info, - `http error info shouldn't contain a 'message' field, will be overridden` - ) - } - return res.json(fullInfo) + return renderJSONError(res, message, info) 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') + } } } diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.js b/services/web/app/src/Features/Subscription/SubscriptionController.js index caa0c354fb..9ede57951d 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionController.js +++ b/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( - new HttpErrors.UnprocessableEntityError({ - info: { public: { message: 'Plan not found' } } - }) - ) + return HttpErrorHandler.unprocessableEntity(req, res, '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) { - return next( - new HttpErrors.UnprocessableEntityError({}).withCause(err) - ) - } else if (err instanceof Errors.InvalidError) { - return next( - new HttpErrors.UnprocessableEntityError({}).withCause(err) + if ( + err instanceof SubscriptionErrors.RecurlyTransactionError || + err instanceof Errors.InvalidError + ) { + logger.warn(err) + return HttpErrorHandler.unprocessableEntity( + req, + res, + err.message, + OError.getFullInfo(err).public ) } diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index e6d86071cd..e1926e7a9a 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/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( - new HttpErrors.UnprocessableEntityError(errorData).withCause( - err - ) + logger.warn(new OError(errorData).withCause(err)) + return HttpErrorHandler.unprocessableEntity( + req, + res, + errorData.message, + errorData.info.public ) } else { return next(new OError(errorData).withCause(err)) diff --git a/services/web/test/unit/src/Editor/EditorHttpControllerTests.js b/services/web/test/unit/src/Editor/EditorHttpControllerTests.js index 953683b03c..dd674559cd 100644 --- a/services/web/test/unit/src/Editor/EditorHttpControllerTests.js +++ b/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 => { - expect(err).to.be.instanceof(HttpErrors.UnprocessableEntityError) - done() - }) + this.HttpErrorHandler.unprocessableEntity = sinon.spy( + (req, res, message) => { + 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) }) }) }) diff --git a/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js b/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js index 8337e5870e..88321f3a67 100644 --- a/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js +++ b/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' + }) + }) + }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js index dec8efeddb..4eb28a1e6e 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js +++ b/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 => { - expect(error).to.exist - expect(error.statusCode).to.equal(422) - done() - } + this.HttpErrorHandler.unprocessableEntity = sinon.spy( + (req, res, message) => { + expect(req).to.exist + 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() { diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index 31e54980ea..adf22e2c02 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/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) { - this.UserController.tryDeleteUser(this.req, null, error => { - expect(error).to.be.instanceof(HttpErrors.UnprocessableEntityError) - expect( - OError.hasCauseInstanceOf( - error, - Errors.SubscriptionAdminDeletionError - ) - ).to.be.true - done() - }) + it('should return a HTTP Unprocessable Entity error', function(done) { + this.HttpErrorHandler.unprocessableEntity = sinon.spy( + (req, res, message, info) => { + expect(req).to.exist + expect(res).to.exist + expect(message).to.equal('error while deleting user account') + expect(info).to.deep.equal({ + error: 'SubscriptionAdminDeletionError' + }) + done() + } + ) + this.UserController.tryDeleteUser(this.req, this.res) }) })