From 2c25cbaf2538060382ce8dace8e5b19587007d89 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 17 Jul 2018 11:12:09 +0100 Subject: [PATCH] Update error handling on backend --- .../Features/User/UserController.coffee | 3 ++- .../Features/User/UserEmailsController.coffee | 25 +++++++++++-------- .../coffee/Features/User/UserGetter.coffee | 3 ++- .../coffee/Features/User/UserUpdater.coffee | 4 ++- .../acceptance/coffee/UserEmailsTests.coffee | 4 +-- .../coffee/helpers/MockV1Api.coffee | 2 +- .../coffee/User/UserControllerTests.coffee | 2 ++ .../unit/coffee/User/UserGetterTests.coffee | 3 +++ 8 files changed, 30 insertions(+), 16 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index 9b102d13dd..823919f999 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/services/web/app/coffee/Features/User/UserController.coffee @@ -13,6 +13,7 @@ UserSessionsManager = require("./UserSessionsManager") UserUpdater = require("./UserUpdater") SudoModeHandler = require('../SudoMode/SudoModeHandler') settings = require "settings-sharelatex" +Errors = require "../Errors/Errors" module.exports = UserController = @@ -100,7 +101,7 @@ module.exports = UserController = UserUpdater.changeEmailAddress user_id, newEmail, (err)-> if err? logger.err err:err, user_id:user_id, newEmail:newEmail, "problem updaing users email address" - if err.message == "alread_exists" + if err instanceof Errors.EmailExistsError message = req.i18n.translate("email_already_registered") else message = req.i18n.translate("problem_changing_email_address") diff --git a/services/web/app/coffee/Features/User/UserEmailsController.coffee b/services/web/app/coffee/Features/User/UserEmailsController.coffee index b3bac95ca7..780960c3e2 100644 --- a/services/web/app/coffee/Features/User/UserEmailsController.coffee +++ b/services/web/app/coffee/Features/User/UserEmailsController.coffee @@ -26,7 +26,8 @@ module.exports = UserEmailsController = role: req.body.role department: req.body.department UserUpdater.addEmailAddress userId, email, affiliationOptions, (error)-> - return next(error) if error? + if error? + return UserEmailsController._handleEmailError error, req, res, next UserEmailsConfirmationHandler.sendConfirmationEmail userId, email, (err) -> return next(error) if error? res.sendStatus 204 @@ -49,15 +50,7 @@ module.exports = UserEmailsController = UserUpdater.updateV1AndSetDefaultEmailAddress userId, email, (error)-> if error? - if error instanceof Errors.UnconfirmedEmailError - return res.sendStatus 409 - else if error instanceof Errors.EmailExistsError - return res.status(409).json { - error: - message: "The email '#{email}' is already in use by another account" - } - else - return next(error) + return UserEmailsController._handleEmailError error, req, res, next else return res.sendStatus 200 @@ -105,3 +98,15 @@ module.exports = UserEmailsController = next(error) else res.sendStatus 200 + + _handleEmailError: (error, req, res, next) -> + if error instanceof Errors.UnconfirmedEmailError + return res.status(409).json { + message: 'email must be confirmed' + } + else if error instanceof Errors.EmailExistsError + return res.status(409).json { + message: req.i18n.translate("email_already_registered") + } + else + return next(error) \ No newline at end of file diff --git a/services/web/app/coffee/Features/User/UserGetter.coffee b/services/web/app/coffee/Features/User/UserGetter.coffee index bdcdd24482..13b81b399c 100644 --- a/services/web/app/coffee/Features/User/UserGetter.coffee +++ b/services/web/app/coffee/Features/User/UserGetter.coffee @@ -4,6 +4,7 @@ logger = require('logger-sharelatex') db = mongojs.db ObjectId = mongojs.ObjectId { getAffiliations } = require("./UserAffiliationsManager") +Errors = require("../Errors/Errors") module.exports = UserGetter = getUser: (query, projection, callback = (error, user) ->) -> @@ -77,7 +78,7 @@ module.exports = UserGetter = # check for duplicate email address. This is also enforced at the DB level ensureUniqueEmailAddress: (newEmail, callback) -> @getUserByAnyEmail newEmail, (error, user) -> - return callback(message: 'alread_exists') if user? + return callback(new Errors.EmailExistsError('alread_exists')) if user? callback(error) decorateFullEmails = (defaultEmail, emailsData, affiliationsData) -> diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index 2c27f5178d..5e9058eb9b 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -136,7 +136,9 @@ module.exports = UserUpdater = user: Settings.apis.v1.user pass: Settings.apis.v1.pass sendImmediately: true - json: { email: newEmail }, + json: + user: + email: newEmail timeout: 5 * 1000 }, (error, response, body) -> if error? diff --git a/services/web/test/acceptance/coffee/UserEmailsTests.coffee b/services/web/test/acceptance/coffee/UserEmailsTests.coffee index 2d4c437fc1..3e8ffbd777 100644 --- a/services/web/test/acceptance/coffee/UserEmailsTests.coffee +++ b/services/web/test/acceptance/coffee/UserEmailsTests.coffee @@ -470,8 +470,8 @@ describe "UserEmails", -> }, (error, response, body) => return done(error) if error? expect(response.statusCode).to.equal 409 - expect(body.error).to.deep.equal { - message: "The email 'exists-in-v1@example.com' is already in use by another account" + expect(body).to.deep.equal { + message: "This email is already registered" } cb() (cb) => diff --git a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee index ef80ac8b23..eeafa6a44b 100644 --- a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee +++ b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee @@ -69,7 +69,7 @@ module.exports = MockV1Api = res.json [] app.put '/api/v1/sharelatex/users/:id/email', (req, res, next) => - { email } = req.body + { email } = req.body?.user if email in @existingEmails return res.sendStatus 409 else diff --git a/services/web/test/unit/coffee/User/UserControllerTests.coffee b/services/web/test/unit/coffee/User/UserControllerTests.coffee index 26f345db39..d589852a37 100644 --- a/services/web/test/unit/coffee/User/UserControllerTests.coffee +++ b/services/web/test/unit/coffee/User/UserControllerTests.coffee @@ -9,6 +9,7 @@ MockResponse = require "../helpers/MockResponse" MockRequest = require "../helpers/MockRequest" ObjectId = require("mongojs").ObjectId assert = require("assert") +Errors = require "../../../../app/js/Features/Errors/Errors" describe "UserController", -> beforeEach -> @@ -81,6 +82,7 @@ describe "UserController", -> log:-> err:-> "metrics-sharelatex": inc:-> + "../Errors/Errors": Errors @res = send: sinon.stub() diff --git a/services/web/test/unit/coffee/User/UserGetterTests.coffee b/services/web/test/unit/coffee/User/UserGetterTests.coffee index 50ab2261f1..c754b9830b 100644 --- a/services/web/test/unit/coffee/User/UserGetterTests.coffee +++ b/services/web/test/unit/coffee/User/UserGetterTests.coffee @@ -5,6 +5,7 @@ path = require('path') sinon = require('sinon') modulePath = path.join __dirname, "../../../../app/js/Features/User/UserGetter" expect = require("chai").expect +Errors = require "../../../../app/js/Features/Errors/Errors" describe "UserGetter", -> @@ -30,6 +31,7 @@ describe "UserGetter", -> 'settings-sharelatex': settings './UserAffiliationsManager': getAffiliations: @getAffiliations + "../Errors/Errors": Errors describe "getUser", -> it "should get user", (done)-> @@ -150,6 +152,7 @@ describe "UserGetter", -> @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @fakeUser) @UserGetter.ensureUniqueEmailAddress @newEmail, (err)=> should.exist(err) + expect(err).to.be.an.instanceof(Errors.EmailExistsError) err.message.should.equal 'alread_exists' done()