diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index 0551362386..bb3ed6be55 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/services/web/app/coffee/Features/User/UserController.coffee @@ -28,9 +28,11 @@ module.exports = updateUserSettings : (req, res)-> logger.log user: req.session.user, "updating account settings" - User.findById req.session.user._id, (err, user)-> + newEmail = req.body.email?.trim() + user_id = req.session.user._id + User.findById user_id, (err, user)-> if err? or !user? - logger.err err:err, user_id:req.session.user._id, "problem updaing user settings" + logger.err err:err, user_id:user_id, "problem updaing user settings" return res.send 500 user.first_name = sanitize.escape(req.body.first_name).trim() user.last_name = sanitize.escape(req.body.last_name).trim() @@ -40,8 +42,17 @@ module.exports = user.ace.autoComplete = req.body.autoComplete == "true" user.ace.spellCheckLanguage = req.body.spellCheckLanguage user.ace.pdfViewer = req.body.pdfViewer - user.save -> - res.send() + user.save (err)-> + if !newEmail? or newEmail.length == 0 or newEmail.indexOf("@") == -1 + return res.send(412) + else if newEmail == user.email + return res.send 200 + else + UserUpdater.changeEmailAddress user_id, newEmail, (err)-> + if err? + logger.err err:err, user_id:user_id, newEmail:newEmail, "problem updaing users email address" + return res.send 500, {message:err?.message} + res.send(200) logout : (req, res)-> metrics.inc "user.logout" @@ -104,13 +115,5 @@ module.exports = text:'Your old password is wrong' changeEmailAddress: (req, res)-> - newEmail = req.body.email - user_id = req.session.user._id - if !newEmail? or newEmail.length == 0 or newEmail.indexOf("@") == -1 - return res.send(412) - UserUpdater.changeEmailAddress user_id, newEmail, (err)-> - if err? - return res.send 500, {message:err?.message} - res.send 200 diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index 71a761a8e2..86ea60240d 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -1,3 +1,4 @@ +logger = require("logger-sharelatex") mongojs = require("../../infrastructure/mongojs") db = mongojs.db ObjectId = mongojs.ObjectId @@ -15,13 +16,15 @@ module.exports = UserUpdater = changeEmailAddress: (user_id, newEmail, callback)-> self = @ - UserLocator.findById user_id, (error, user) -> + logger.log user_id:user_id, newEmail:newEmail, "updaing email address of user" + UserLocator.findByEmail newEmail, (error, user) -> if user? return callback({message:"User with that email already exists."}) self.updateUser user_id.toString(), { $set: { "email": newEmail}, }, (err) -> if err? + logger.err err:err, "problem updating users email" return callback(err) callback() diff --git a/services/web/app/views/user/settings.jade b/services/web/app/views/user/settings.jade index a982adb6cd..4270557d4d 100644 --- a/services/web/app/views/user/settings.jade +++ b/services/web/app/views/user/settings.jade @@ -29,6 +29,10 @@ block content form#userSettings.tab-content.form-horizontal input(type="hidden", name="_csrf", value=csrfToken) .tab-pane#personalSettings.active + .control-group + label(for='firstName').control-label Email + .controls + input#emailAddress(type='email', name='email', value=user.email) .control-group label(for='firstName').control-label First Name .controls diff --git a/services/web/public/coffee/forms.coffee b/services/web/public/coffee/forms.coffee index 1f877af851..83569ca811 100644 --- a/services/web/public/coffee/forms.coffee +++ b/services/web/public/coffee/forms.coffee @@ -172,15 +172,9 @@ require [ $('form#userSettings').validate rules: - newPassword1: - minlength: 1 - newPassword2: - equalTo: "#newPassword1" - messages: - newPassword1: - minlength: "1 character minimum" - newPassword2: - equalTo: "Passwords don't match" + email: + required: true + email: true highlight: (element, errorClass, validClas)-> $(element).parents("div[class='clearfix']").addClass("error") @@ -191,14 +185,19 @@ require [ formData = $(form).serialize() $.ajax url: '/user/settings' - type:'POST' + type: 'POST' data: formData success: (data)-> if data.message displayMessage data else new Message {text:"Your settings have been saved"} + error:(data)-> + message = JSON.parse(data?.responseText)?.message + new Message type:"error", text: message || "Something went wrong processing your request." + + class Message constructor: (message)-> aClose = $('').addClass('close').attr('href','#').text('x') diff --git a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee index 587fa7d804..cfdb1c7f10 100644 --- a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee @@ -77,6 +77,8 @@ describe "UserController", -> @UserController.unsubscribe @req, @res describe "updateUserSettings", -> + beforeEach -> + @newEmail = "hello@world.com" it "should call save", (done)-> @req.body = {} @@ -102,6 +104,38 @@ describe "UserController", -> @UserController.updateUserSettings @req, @res + it "should return an error if the email address is null", (done)-> + @req.body.email = null + @res.send = (code)-> + code.should.equal 412 + done() + @UserController.updateUserSettings @req, @res + + it "should send an error if the email is 0 len", (done)-> + @req.body.email = "" + @res.send = (code)-> + code.should.equal 412 + done() + @UserController.updateUserSettings @req, @res + + it "should send an error if the email does not contain an @", (done)-> + @req.body.email = "bob at something dot com" + @res.send = (code)-> + code.should.equal 412 + done() + @UserController.updateUserSettings @req, @res + + it "should call the user updater with the new email and user _id", (done)-> + @req.body.email = @newEmail + @UserUpdater.changeEmailAddress.callsArgWith(2) + @res.send = (code)=> + code.should.equal 200 + @UserUpdater.changeEmailAddress.calledWith(@user_id, @newEmail).should.equal true + done() + @UserController.updateUserSettings @req, @res + + + describe "logout", -> it "should destroy the session", (done)-> @@ -207,36 +241,3 @@ describe "UserController", -> @UserController.changePassword @req, @res - describe "changeEmailAddress", -> - beforeEach -> - @newEmail = "new@email.com" - - it "should return an error if the email address is null", (done)-> - @req.body.email = null - @res.send = (code)-> - code.should.equal 412 - done() - @UserController.changeEmailAddress @req, @res - - it "should send an error if the email is 0 len", (done)-> - @req.body.email = "" - @res.send = (code)-> - code.should.equal 412 - done() - @UserController.changeEmailAddress @req, @res - - it "should send an error if the email does not contain an @", (done)-> - @req.body.email = "bob at something dot com" - @res.send = (code)-> - code.should.equal 412 - done() - @UserController.changeEmailAddress @req, @res - - it "should call the user updater with the new email and user _id", (done)-> - @req.body.email = @newEmail - @UserUpdater.changeEmailAddress.callsArgWith(2) - @res.send = (code)=> - code.should.equal 200 - @UserUpdater.changeEmailAddress.calledWith(@user_id, @newEmail).should.equal true - done() - @UserController.changeEmailAddress @req, @res diff --git a/services/web/test/UnitTests/coffee/User/UserUpdaterTests.coffee b/services/web/test/UnitTests/coffee/User/UserUpdaterTests.coffee index 02ad96af89..b4a62285b5 100644 --- a/services/web/test/UnitTests/coffee/User/UserUpdaterTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserUpdaterTests.coffee @@ -15,7 +15,7 @@ describe "UserUpdater", -> db:{} ObjectId:(id)-> return id @UserLocator = - findById:sinon.stub() + findByEmail:sinon.stub() @UserUpdater = SandboxedModule.require modulePath, requires: "settings-sharelatex":@settings "logger-sharelatex": log:-> @@ -33,15 +33,15 @@ describe "UserUpdater", -> @UserUpdater.updateUser = sinon.stub().callsArgWith(2) it "should check if the new email already has an account", (done)-> - @UserLocator.findById.callsArgWith(1, null, @stubbedUser) - @UserUpdater.changeEmailAddress @user_id, @newEmail, (err)=> + @UserLocator.findByEmail.callsArgWith(1, null, @stubbedUser) + @UserUpdater.changeEmailAddress @user_id, @stubbedUser.email, (err)=> @UserUpdater.updateUser.called.should.equal false should.exist(err) done() it "should set the users password", (done)-> - @UserLocator.findById.callsArgWith(1, null) + @UserLocator.findByEmail.callsArgWith(1, null) @UserUpdater.changeEmailAddress @user_id, @newEmail, (err)=> @UserUpdater.updateUser.calledWith(@user_id, $set: { "email": @newEmail}).should.equal true done()