From 9d4df4271adb4bb0dd9cab3ea05f4be9341e0ca5 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Wed, 4 Jul 2018 15:12:54 +0200 Subject: [PATCH 1/2] try adding affiliation on user creation --- .../Features/User/UserAffiliationsManager.coffee | 7 ++++++- .../web/app/coffee/Features/User/UserCreator.coffee | 12 ++++++++++++ .../test/unit/coffee/User/UserCreatorTests.coffee | 12 +++++++++++- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserAffiliationsManager.coffee b/services/web/app/coffee/Features/User/UserAffiliationsManager.coffee index 2a81ae6bd5..77dad7a8ef 100644 --- a/services/web/app/coffee/Features/User/UserAffiliationsManager.coffee +++ b/services/web/app/coffee/Features/User/UserAffiliationsManager.coffee @@ -12,7 +12,12 @@ module.exports = UserAffiliationsManager = }, callback - addAffiliation: (userId, email, { university, department, role }, callback = (error) ->) -> + addAffiliation: (userId, email, affiliationOptions, callback) -> + unless callback? # affiliationOptions is optional + callback = affiliationOptions + affiliationOptions = {} + + { university, department, role } = affiliationOptions makeAffiliationRequest { method: 'POST' path: "/api/v2/users/#{userId.toString()}/affiliations" diff --git a/services/web/app/coffee/Features/User/UserCreator.coffee b/services/web/app/coffee/Features/User/UserCreator.coffee index 4b4dd30ae2..b0676e09e0 100644 --- a/services/web/app/coffee/Features/User/UserCreator.coffee +++ b/services/web/app/coffee/Features/User/UserCreator.coffee @@ -1,6 +1,7 @@ User = require("../../models/User").User logger = require("logger-sharelatex") metrics = require('metrics-sharelatex') +{ addAffiliation } = require("./UserAffiliationsManager") module.exports = UserCreator = @@ -26,6 +27,17 @@ module.exports = UserCreator = user.save (err)-> callback(err, user) + # call addaffiliation after the main callback so it runs in the + # background. There is no guaranty this will run so we must no rely on it + addAffiliation user._id, user.email, (error) -> + if error + logger.log { userId: user._id, email: user.email, error: error }, + "couldn't add affiliation for user on create" + else + logger.log { userId: user._id, email: user.email }, + "added affiliation for user on create" + + metrics.timeAsyncMethod( UserCreator, 'createNewUser', 'mongo.UserCreator', diff --git a/services/web/test/unit/coffee/User/UserCreatorTests.coffee b/services/web/test/unit/coffee/User/UserCreatorTests.coffee index 3764c8a765..1945453b9e 100644 --- a/services/web/test/unit/coffee/User/UserCreatorTests.coffee +++ b/services/web/test/unit/coffee/User/UserCreatorTests.coffee @@ -17,11 +17,12 @@ describe "UserCreator", -> @UserGetter = getUserByMainEmail: sinon.stub() + @addAffiliation = sinon.stub().yields() @UserCreator = SandboxedModule.require modulePath, requires: "../../models/User": User:@UserModel - "./UserGetter":@UserGetter "logger-sharelatex":{log:->} 'metrics-sharelatex': {timeAsyncMethod: ()->} + "./UserAffiliationsManager": addAffiliation: @addAffiliation @email = "bob.oswald@gmail.com" @@ -78,3 +79,12 @@ describe "UserCreator", -> user.emails[0].email.should.equal @email user.emails[0].createdAt.should.be.a 'date' done() + + it "should add affiliation in background", (done)-> + @UserCreator.createNewUser email: @email, (err, user) => + # addaffiliation should not be called before the callback but only after + # a tick of the event loop + sinon.assert.notCalled(@addAffiliation) + process.nextTick () => + sinon.assert.calledWith(@addAffiliation, user._id, user.email) + done() From cfd5c65be480839debe5423e217f31cf026baeb7 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Wed, 4 Jul 2018 15:47:49 +0200 Subject: [PATCH 2/2] add affiliation before confirming email --- .../coffee/Features/User/UserUpdater.coffee | 31 +++++++++++-------- .../unit/coffee/User/UserUpdaterTests.coffee | 19 ++++++++++-- 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index 6adc174be4..948be16d41 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -59,7 +59,7 @@ module.exports = UserUpdater = addAffiliation userId, newEmail, affiliationOptions, (error) => if error? - logger.err error: error, 'problem adding affiliation' + logger.err error: error, 'problem adding affiliation while adding email' return callback(error) update = $push: emails: email: newEmail, createdAt: new Date() @@ -110,18 +110,23 @@ module.exports = UserUpdater = email = EmailHelper.parseEmail(email) return callback(new Error('invalid email')) if !email? logger.log {userId, email}, 'confirming user email' - query = - _id: userId - 'emails.email': email - update = - $set: - 'emails.$.confirmedAt': new Date() - @updateUser query, update, (error, res) -> - return callback(error) if error? - logger.log {res, userId, email}, "tried to confirm email" - if res.n == 0 - return callback(new Errors.NotFoundError('user id and email do no match')) - callback() + addAffiliation userId, email, (error) => + if error? + logger.err error: error, 'problem adding affiliation while confirming email' + return callback(error) + + query = + _id: userId + 'emails.email': email + update = + $set: + 'emails.$.confirmedAt': new Date() + @updateUser query, update, (error, res) -> + return callback(error) if error? + logger.log {res, userId, email}, "tried to confirm email" + if res.n == 0 + return callback(new Errors.NotFoundError('user id and email do no match')) + callback() [ 'updateUser' diff --git a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee index 0c872e5e4a..3820e2a448 100644 --- a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee +++ b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee @@ -19,7 +19,7 @@ describe "UserUpdater", -> getUserByAnyEmail: sinon.stub() ensureUniqueEmailAddress: sinon.stub() @logger = err: sinon.stub(), log: -> - @addAffiliation = sinon.stub().callsArgWith(3, null) + @addAffiliation = sinon.stub().yields() @removeAffiliation = sinon.stub().callsArgWith(2, null) @UserUpdater = SandboxedModule.require modulePath, requires: "logger-sharelatex": @logger @@ -201,9 +201,10 @@ describe "UserUpdater", -> done() describe 'confirmEmail', -> - it 'should update the email record', (done)-> + beforeEach -> @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 1) + it 'should update the email record', (done)-> @UserUpdater.confirmEmail @stubbedUser._id, @newEmail, (err)=> should.not.exist(err) @UserUpdater.updateUser.calledWith( @@ -212,6 +213,13 @@ describe "UserUpdater", -> ).should.equal true done() + it 'add affiliation', (done)-> + @UserUpdater.confirmEmail @stubbedUser._id, @newEmail, (err)=> + should.not.exist(err) + @addAffiliation.calledOnce.should.equal true + sinon.assert.calledWith(@addAffiliation, @stubbedUser._id, @newEmail) + done() + it 'handle error', (done)-> @UserUpdater.updateUser = sinon.stub().callsArgWith(2, new Error('nope')) @@ -230,3 +238,10 @@ describe "UserUpdater", -> @UserUpdater.confirmEmail @stubbedUser._id, '@', (err)=> should.exist(err) done() + + it 'handle affiliation error', (done)-> + @addAffiliation.callsArgWith(2, new Error('nope')) + @UserUpdater.confirmEmail @stubbedUser._id, @newEmail, (err)=> + should.exist(err) + @UserUpdater.updateUser.called.should.equal false + done()