From d3b2a2650fe7c02f999366c36ff18b5212330e21 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Tue, 19 Jun 2018 13:46:15 +0200 Subject: [PATCH 1/5] add/remove affiliations when adding/removing emails --- .../Features/User/UserEmailsController.coffee | 8 +- .../coffee/Features/User/UserUpdater.coffee | 78 ++++++++++++++++--- .../User/UserEmailsControllerTests.coffee | 16 +++- .../unit/coffee/User/UserUpdaterTests.coffee | 66 ++++++++++++++-- 4 files changed, 144 insertions(+), 24 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserEmailsController.coffee b/services/web/app/coffee/Features/User/UserEmailsController.coffee index f877cbe345..a6335521d6 100644 --- a/services/web/app/coffee/Features/User/UserEmailsController.coffee +++ b/services/web/app/coffee/Features/User/UserEmailsController.coffee @@ -20,7 +20,11 @@ module.exports = UserEmailsController = email = EmailHelper.parseEmail(req.body.email) return res.sendStatus 422 unless email? - UserUpdater.addEmailAddress userId, email, (error)-> + affiliationOptions = + university: req.body.university + role: req.body.role + department: req.body.department + UserUpdater.addEmailAddress userId, email, affiliationOptions, (error)-> return next(error) if error? UserEmailsConfirmationHandler.sendConfirmationEmail userId, email, (err) -> return next(error) if error? @@ -65,4 +69,4 @@ module.exports = UserEmailsController = else next(error) else - res.sendStatus 200 \ No newline at end of file + res.sendStatus 200 diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index 4de994b5c2..c73bef7b73 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -7,6 +7,8 @@ ObjectId = mongojs.ObjectId UserGetter = require("./UserGetter") EmailHelper = require "../Helpers/EmailHelper" Errors = require "../Errors/Errors" +settings = require "settings-sharelatex" +request = require "request" module.exports = UserUpdater = updateUser: (query, update, callback = (error) ->) -> @@ -44,29 +46,43 @@ module.exports = UserUpdater = # Add a new email address for the user. Email cannot be already used by this # or any other user - addEmailAddress: (userId, newEmail, callback) -> + addEmailAddress: (userId, newEmail, affiliationOptions, callback) -> + unless callback? # affiliationOptions is optional + callback = affiliationOptions + affiliationOptions = {} + UserGetter.ensureUniqueEmailAddress newEmail, (error) => return callback(error) if error? - update = $push: emails: email: newEmail, createdAt: new Date() - @updateUser userId, update, (error) -> + addAffiliation userId, newEmail, affiliationOptions, (error) => if error? - logger.err error: error, 'problem updating users emails' + logger.err error: error, 'problem adding affiliation' return callback(error) - callback() + + update = $push: emails: email: newEmail, createdAt: new Date() + @updateUser userId, update, (error) -> + if error? + logger.err error: error, 'problem updating users emails' + return callback(error) + callback() # remove one of the user's email addresses. The email cannot be the user's # default email address removeEmailAddress: (userId, email, callback) -> - query = _id: userId, email: $ne: email - update = $pull: emails: email: email - @updateUser query, update, (error, res) -> + removeAffiliation userId, email, (error) => if error? - logger.err error:error, 'problem removing users email' + logger.err error: error, 'problem removing affiliation' return callback(error) - if res.n == 0 - return callback(new Error('Cannot remove default email')) - callback() + + query = _id: userId, email: $ne: email + update = $pull: emails: email: email + @updateUser query, update, (error, res) -> + if error? + logger.err error:error, 'problem removing users email' + return callback(error) + if res.n == 0 + return callback(new Error('Cannot remove default email')) + callback() # set the default email address by setting the `email` attribute. The email @@ -99,6 +115,44 @@ module.exports = UserUpdater = return callback(new Errors.NotFoundError('user id and email do no match')) callback() +addAffiliation = (userId, email, { university, department, role }, callback = (error) ->) -> + makeAffiliationRequest { + method: 'POST' + path: "/api/v2/users/#{userId.toString()}/affiliations" + body: { email, university, department, role } + defaultErrorMessage: "Couldn't create affiliation" + }, callback + +removeAffiliation = (userId, email, callback = (error) ->) -> + email = encodeURIComponent(email) + makeAffiliationRequest { + method: 'DELETE' + path: "/api/v2/users/#{userId.toString()}/affiliations/#{email}" + extraSuccessStatusCodes: [404] # `Not Found` responses are considered successful + defaultErrorMessage: "Couldn't remove affiliation" + }, callback + +makeAffiliationRequest = (requestOptions, callback = (error) ->) -> + requestOptions.extraSuccessStatusCodes ||= [] + request { + method: requestOptions.method + url: "#{settings.apis.v1.url}#{requestOptions.path}" + body: requestOptions.body + auth: { user: settings.apis.v1.user, pass: settings.apis.v1.pass } + json: true, + timeout: 20 * 1000 + }, (error, response, body) -> + return callback(error) if error? + isSuccess = 200 <= response.statusCode < 300 + isSuccess ||= response.statusCode in requestOptions.extraSuccessStatusCodes + unless isSuccess + if body?.errors + errorMessage = "#{response.statusCode}: #{body.errors}" + else + errorMessage = "#{requestOptions.defaultErrorMessage}: #{response.statusCode}" + return callback(new Error(errorMessage)) + + callback(null) [ 'updateUser' diff --git a/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee b/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee index d0e8076276..d55b7fd963 100644 --- a/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee +++ b/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee @@ -53,10 +53,14 @@ describe "UserEmailsController", -> describe 'Add', -> beforeEach -> @newEmail = 'new_email@baz.com' - @req.body.email = @newEmail + @req.body = + email: @newEmail + university: { name: 'University Name' } + department: 'Department' + role: 'Role' @EmailHelper.parseEmail.returns @newEmail - @UserUpdater.addEmailAddress.callsArgWith 2, null @UserEmailsConfirmationHandler.sendConfirmationEmail = sinon.stub().yields() + @UserUpdater.addEmailAddress.callsArgWith 3, null it 'adds new email', (done) -> @UserEmailsController.add @req, @@ -64,6 +68,13 @@ describe "UserEmailsController", -> code.should.equal 204 assertCalledWith @EmailHelper.parseEmail, @newEmail assertCalledWith @UserUpdater.addEmailAddress, @user._id, @newEmail + + affiliationOptions = @UserUpdater.addEmailAddress.lastCall.args[2] + Object.keys(affiliationOptions).length.should.equal 3 + affiliationOptions.university.should.equal @req.body.university + affiliationOptions.department.should.equal @req.body.department + affiliationOptions.role.should.equal @req.body.role + done() it 'sends an email confirmation', (done) -> @@ -75,7 +86,6 @@ describe "UserEmailsController", -> it 'handles email parse error', (done) -> @EmailHelper.parseEmail.returns null - @UserEmailsController.add @req, sendStatus: (code) => code.should.equal 422 diff --git a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee index 6b9a1ecc85..77f2c38c85 100644 --- a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee +++ b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee @@ -11,7 +11,6 @@ describe "UserUpdater", -> beforeEach -> tk.freeze(Date.now()) - @settings = {} @mongojs = db:{} ObjectId:(id)-> return id @@ -20,12 +19,15 @@ describe "UserUpdater", -> getUserByAnyEmail: sinon.stub() ensureUniqueEmailAddress: sinon.stub() @logger = err: sinon.stub(), log: -> + settings = apis: { v1: { url: '', user: '', pass: '' } } + @request = sinon.stub() @UserUpdater = SandboxedModule.require modulePath, requires: - "settings-sharelatex":@settings "logger-sharelatex": @logger "./UserGetter": @UserGetter "../../infrastructure/mongojs":@mongojs "metrics-sharelatex": timeAsyncMethod: sinon.stub() + 'settings-sharelatex': settings + 'request': @request @stubbedUser = _id: "3131231" @@ -66,10 +68,10 @@ describe "UserUpdater", -> describe 'addEmailAddress', -> beforeEach -> @UserGetter.ensureUniqueEmailAddress = sinon.stub().callsArgWith(1) + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null) + @request.callsArgWith(1, null, { statusCode: 201 }) it 'add email', (done)-> - @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null) - @UserUpdater.addEmailAddress @stubbedUser._id, @newEmail, (err)=> @UserGetter.ensureUniqueEmailAddress.called.should.equal true should.not.exist(err) @@ -79,6 +81,27 @@ describe "UserUpdater", -> ).should.equal true done() + it 'add affiliation', (done)-> + affiliationOptions = + university: { id: 1 } + role: 'Prof' + department: 'Math' + @UserUpdater.addEmailAddress @stubbedUser._id, @newEmail, affiliationOptions, (err)=> + should.not.exist(err) + @request.calledOnce.should.equal true + requestOptions = @request.lastCall.args[0] + expectedUrl = "/api/v2/users/#{@stubbedUser._id}/affiliations" + requestOptions.url.should.equal expectedUrl + requestOptions.method.should.equal 'POST' + + body = requestOptions.body + Object.keys(body).length.should.equal 4 + body.email.should.equal @newEmail + body.university.should.equal affiliationOptions.university + body.department.should.equal affiliationOptions.department + body.role.should.equal affiliationOptions.role + done() + it 'handle error', (done)-> @UserUpdater.updateUser = sinon.stub().callsArgWith(2, new Error('nope')) @@ -87,10 +110,21 @@ describe "UserUpdater", -> should.exist(err) done() - describe 'removeEmailAddress', -> - it 'remove email', (done)-> - @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 1) + it 'handle affiliation error', (done)-> + body = errors: 'affiliation error message' + @request.callsArgWith(1, null, { statusCode: 422 }, body) + @UserUpdater.addEmailAddress @stubbedUser._id, @newEmail, (err)=> + err.message.should.have.string 422 + err.message.should.have.string body.errors + @UserUpdater.updateUser.called.should.equal false + done() + describe 'removeEmailAddress', -> + beforeEach -> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 1) + @request.callsArgWith(1, null, { statusCode: 404 }) + + it 'remove email', (done)-> @UserUpdater.removeEmailAddress @stubbedUser._id, @newEmail, (err)=> should.not.exist(err) @UserUpdater.updateUser.calledWith( @@ -99,6 +133,17 @@ describe "UserUpdater", -> ).should.equal true done() + it 'remove affiliation', (done)-> + @UserUpdater.removeEmailAddress @stubbedUser._id, @newEmail, (err)=> + should.not.exist(err) + @request.calledOnce.should.equal true + requestOptions = @request.lastCall.args[0] + expectedUrl = "/api/v2/users/#{@stubbedUser._id}/affiliations/" + expectedUrl += encodeURIComponent(@newEmail) + requestOptions.url.should.equal expectedUrl + requestOptions.method.should.equal 'DELETE' + done() + it 'handle error', (done)-> @UserUpdater.updateUser = sinon.stub().callsArgWith(2, new Error('nope')) @@ -113,6 +158,13 @@ describe "UserUpdater", -> should.exist(err) done() + it 'handle affiliation error', (done)-> + @request.callsArgWith(1, null, { statusCode: 500 }) + @UserUpdater.removeEmailAddress @stubbedUser._id, @newEmail, (err)=> + err.message.should.exist + @UserUpdater.updateUser.called.should.equal false + done() + describe 'setDefaultEmailAddress', -> it 'set default', (done)-> @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 1) From e41391fb4a46a4b53957c985806314275cd0d230 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Thu, 21 Jun 2018 13:31:55 +0200 Subject: [PATCH 2/5] ignore affiliations if v1 is not configured --- services/web/app/coffee/Features/User/UserUpdater.coffee | 3 ++- services/web/test/unit/coffee/User/UserUpdaterTests.coffee | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index c73bef7b73..eb983cb76b 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -132,7 +132,8 @@ removeAffiliation = (userId, email, callback = (error) ->) -> defaultErrorMessage: "Couldn't remove affiliation" }, callback -makeAffiliationRequest = (requestOptions, callback = (error) ->) -> +makeAffiliationRequest = (requestOptions, callback = (error) ->) -> + return callback(null) unless settings?.apis?.v1?.url # service is not configured requestOptions.extraSuccessStatusCodes ||= [] request { method: requestOptions.method diff --git a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee index 77f2c38c85..a2348b54e2 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: -> - settings = apis: { v1: { url: '', user: '', pass: '' } } + settings = apis: { v1: { url: 'v1.url', user: '', pass: '' } } @request = sinon.stub() @UserUpdater = SandboxedModule.require modulePath, requires: "logger-sharelatex": @logger @@ -90,7 +90,7 @@ describe "UserUpdater", -> should.not.exist(err) @request.calledOnce.should.equal true requestOptions = @request.lastCall.args[0] - expectedUrl = "/api/v2/users/#{@stubbedUser._id}/affiliations" + expectedUrl = "v1.url/api/v2/users/#{@stubbedUser._id}/affiliations" requestOptions.url.should.equal expectedUrl requestOptions.method.should.equal 'POST' @@ -138,7 +138,7 @@ describe "UserUpdater", -> should.not.exist(err) @request.calledOnce.should.equal true requestOptions = @request.lastCall.args[0] - expectedUrl = "/api/v2/users/#{@stubbedUser._id}/affiliations/" + expectedUrl = "v1.url/api/v2/users/#{@stubbedUser._id}/affiliations/" expectedUrl += encodeURIComponent(@newEmail) requestOptions.url.should.equal expectedUrl requestOptions.method.should.equal 'DELETE' From c81f9c24ed71f908bb721d526b470b9f749e8f01 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Thu, 21 Jun 2018 17:59:35 +0200 Subject: [PATCH 3/5] mock affiliations API in acceptance tests --- services/web/test/acceptance/coffee/UserEmailsTests.coffee | 1 + services/web/test/acceptance/coffee/helpers/MockV1Api.coffee | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/services/web/test/acceptance/coffee/UserEmailsTests.coffee b/services/web/test/acceptance/coffee/UserEmailsTests.coffee index 01f1dde531..137932e878 100644 --- a/services/web/test/acceptance/coffee/UserEmailsTests.coffee +++ b/services/web/test/acceptance/coffee/UserEmailsTests.coffee @@ -4,6 +4,7 @@ User = require "./helpers/User" request = require "./helpers/request" settings = require "settings-sharelatex" {db, ObjectId} = require("../../../app/js/infrastructure/mongojs") +MockV1Api = require "./helpers/MockV1Api" describe "UserEmails", -> beforeEach (done) -> diff --git a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee index 389ecd1762..65a77eb0e4 100644 --- a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee +++ b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee @@ -42,6 +42,11 @@ module.exports = MockV1Api = @exportParams = Object.assign({}, req.body) res.json exportId: @exportId + app.post "/api/v2/users/:userId/affiliations", (req, res, next) => + res.sendStatus 201 + + app.delete "/api/v2/users/:userId/affiliations/:email", (req, res, next) => + res.sendStatus 204 app.listen 5000, (error) -> throw error if error? From c6de896bb5ef3b87b87c5c4adc44082180d7a48e Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Thu, 21 Jun 2018 12:30:12 +0200 Subject: [PATCH 4/5] decorate emails list with affiliation data --- .../coffee/Features/User/UserGetter.coffee | 39 ++++++++++++++++--- .../coffee/helpers/MockV1Api.coffee | 3 ++ .../unit/coffee/User/UserGetterTests.coffee | 34 ++++++++++++++++ 3 files changed, 71 insertions(+), 5 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserGetter.coffee b/services/web/app/coffee/Features/User/UserGetter.coffee index 59e45e1795..21969656a0 100644 --- a/services/web/app/coffee/Features/User/UserGetter.coffee +++ b/services/web/app/coffee/Features/User/UserGetter.coffee @@ -3,6 +3,8 @@ metrics = require('metrics-sharelatex') logger = require('logger-sharelatex') db = mongojs.db ObjectId = mongojs.ObjectId +settings = require "settings-sharelatex" +request = require "request" module.exports = UserGetter = getUser: (query, projection, callback = (error, user) ->) -> @@ -30,11 +32,9 @@ module.exports = UserGetter = return callback error if error? return callback new Error('User not Found') unless user - fullEmails = user.emails.map (emailData) -> - emailData.default = emailData.email == user.email - emailData - - callback null, fullEmails + getAffiliations userId, (error, affiliationsData) -> + return callback error if error? + callback null, decorateFullEmails(user.email, user.emails, affiliationsData) getUserByMainEmail: (email, projection, callback = (error, user) ->) -> email = email.trim() @@ -81,6 +81,35 @@ module.exports = UserGetter = return callback(message: 'alread_exists') if user? callback(error) +decorateFullEmails = (defaultEmail, emailsData, affiliationsData) -> + emailsData.map (emailData) -> + emailData.default = emailData.email == defaultEmail + + affiliation = affiliationsData.find (aff) -> aff.email == emailData.email + if affiliation? + { institution, inferred, role, department } = affiliation + emailData.affiliation = { institution, inferred, role, department } + else + emailsData.affiliation = null + + emailData + +getAffiliations = (userId, callback = (error) ->) -> + return callback(null, []) unless settings?.apis?.v1?.url # service is not configured + request { + method: 'GET' + url: "#{settings.apis.v1.url}/api/v2/users/#{userId.toString()}/affiliations" + auth: { user: settings.apis.v1.user, pass: settings.apis.v1.pass } + json: true, + timeout: 20 * 1000 + }, (error, response, body) -> + return callback(error) if error? + unless 200 <= response.statusCode < 300 + errorMessage = "Couldn't get affiliations: #{response.statusCode}" + return callback(new Error(errorMessage)) + + callback(null, body) + [ 'getUser', 'getUserEmail', diff --git a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee index 65a77eb0e4..91e3f0b25c 100644 --- a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee +++ b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee @@ -42,6 +42,9 @@ module.exports = MockV1Api = @exportParams = Object.assign({}, req.body) res.json exportId: @exportId + app.get "/api/v2/users/:userId/affiliations", (req, res, next) => + res.json [] + app.post "/api/v2/users/:userId/affiliations", (req, res, next) => res.sendStatus 201 diff --git a/services/web/test/unit/coffee/User/UserGetterTests.coffee b/services/web/test/unit/coffee/User/UserGetterTests.coffee index f2b7895b57..6b80b5a2a7 100644 --- a/services/web/test/unit/coffee/User/UserGetterTests.coffee +++ b/services/web/test/unit/coffee/User/UserGetterTests.coffee @@ -20,11 +20,15 @@ describe "UserGetter", -> @Mongo = db: users: findOne: @findOne ObjectId: (id) -> return id + settings = apis: { v1: { url: 'v1.url', user: '', pass: '' } } + @request = sinon.stub() @UserGetter = SandboxedModule.require modulePath, requires: "logger-sharelatex": log:-> "../../infrastructure/mongojs": @Mongo "metrics-sharelatex": timeAsyncMethod: sinon.stub() + 'settings-sharelatex': settings + 'request': @request describe "getUser", -> it "should get user", (done)-> @@ -42,6 +46,9 @@ describe "UserGetter", -> done() describe "getUserFullEmails", - + beforeEach -> + @request.callsArgWith(1, null, { statusCode: 200 }, []) + it "should get user", (done)-> @UserGetter.getUser = sinon.stub().callsArgWith(2, null, @fakeUser) projection = email: 1, emails: 1 @@ -59,6 +66,33 @@ describe "UserGetter", -> ] done() + it "should merge affiliation data", (done)-> + @UserGetter.getUser = sinon.stub().callsArgWith(2, null, @fakeUser) + affiliationsData = [ + { + email: 'email1@foo.bar' + role: 'Prof' + department: 'Maths' + inferred: false + institution: { name: 'University Name', isUniversity: true } + } + ] + @request.callsArgWith(1, null, { statusCode: 200 }, affiliationsData) + @UserGetter.getUserFullEmails @fakeUser._id, (error, fullEmails) => + assert.deepEqual fullEmails, [ + { + email: 'email1@foo.bar' + default: false + affiliation: + institution: affiliationsData[0].institution + inferred: affiliationsData[0].inferred + department: affiliationsData[0].department + role: affiliationsData[0].role + } + { email: 'email2@foo.bar', default: true } + ] + done() + describe "getUserbyMainEmail", -> it "query user by main email", (done)-> email = 'hello@world.com' From 7a3fcf6d2c840395c26f6f3acc1c93a11585652d Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Mon, 25 Jun 2018 13:46:46 +0200 Subject: [PATCH 5/5] change error message on email update failure --- services/web/app/coffee/Features/User/UserUpdater.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index eb983cb76b..8bb6015597 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -81,7 +81,7 @@ module.exports = UserUpdater = logger.err error:error, 'problem removing users email' return callback(error) if res.n == 0 - return callback(new Error('Cannot remove default email')) + return callback(new Error('Cannot remove email')) callback()