refactor affiliation requests

services/web/app/coffee/Features/User/UserAffiliationsManager.coffee

@@ -0,0 +1,64 @@
+logger = require("logger-sharelatex")
+metrics = require("metrics-sharelatex")
+settings = require "settings-sharelatex"
+request = require "request"
+
+module.exports = UserAffiliationsManager =
+	getAffiliations: (userId, callback = (error, body) ->) ->
+		makeAffiliationRequest {
+			method: 'GET'
+			path: "/api/v2/users/#{userId.toString()}/affiliations"
+			defaultErrorMessage: "Couldn't get affiliations"
+		}, 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) ->) ->
+	return callback(null) unless settings?.apis?.v1?.url # service is not configured
+	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, body)
+
+[
+	'getAffiliations',
+	'addAffiliation',
+	'removeAffiliation',
+].map (method) ->
+	metrics.timeAsyncMethod(
+		UserAffiliationsManager, method, 'mongo.UserAffiliationsManager', logger
+	)

services/web/app/coffee/Features/User/UserGetter.coffee

@@ -3,8 +3,7 @@ metrics = require('metrics-sharelatex')
 logger = require('logger-sharelatex')
 db = mongojs.db
 ObjectId = mongojs.ObjectId
-settings = require "settings-sharelatex"
+{ getAffiliations } = require("./UserAffiliationsManager")
-request = require "request"
 
 module.exports = UserGetter =
 	getUser: (query, projection, callback = (error, user) ->) ->
@@ -94,22 +93,6 @@ decorateFullEmails = (defaultEmail, emailsData, affiliationsData) ->
 
 		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',

services/web/app/coffee/Features/User/UserUpdater.coffee

@@ -5,10 +5,9 @@ db = mongojs.db
 async = require("async")
 ObjectId = mongojs.ObjectId
 UserGetter = require("./UserGetter")
+{ addAffiliation, removeAffiliation } = require("./UserAffiliationsManager")
 EmailHelper = require "../Helpers/EmailHelper"
 Errors = require "../Errors/Errors"
-settings = require "settings-sharelatex"
-request = require "request"
 
 module.exports = UserUpdater =
 	updateUser: (query, update, callback = (error) ->) ->
@@ -66,6 +65,7 @@ module.exports = UserUpdater =
 						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) ->
@@ -115,46 +115,6 @@ 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) ->) ->
-	return callback(null) unless settings?.apis?.v1?.url # service is not configured
-	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'
 	'changeEmailAddress'

services/web/test/unit/coffee/User/UserAffiliationsManagerTests.coffee

@@ -0,0 +1,105 @@
+should = require('chai').should()
+SandboxedModule = require('sandboxed-module')
+assert = require('assert')
+path = require('path')
+sinon = require('sinon')
+modulePath = path.join __dirname, "../../../../app/js/Features/User/UserAffiliationsManager"
+expect = require("chai").expect
+
+describe "UserAffiliationsManager", ->
+
+	beforeEach ->
+		@logger = err: sinon.stub(), log: ->
+		settings = apis: { v1: { url: 'v1.url', user: '', pass: '' } }
+		@request = sinon.stub()
+		@UserAffiliationsManager = SandboxedModule.require modulePath, requires:
+			"logger-sharelatex": @logger
+			"metrics-sharelatex": timeAsyncMethod: sinon.stub()
+			'settings-sharelatex': settings
+			'request': @request
+
+		@stubbedUser = 
+			_id: "3131231"
+			name:"bob"
+			email:"hello@world.com"
+		@newEmail = "bob@bob.com"
+
+	describe 'getAffiliations', ->
+		it 'get affiliations', (done)->
+			responseBody = [{ foo: 'bar' }]
+			@request.callsArgWith(1, null, { statusCode: 201 }, responseBody)
+			@UserAffiliationsManager.getAffiliations @stubbedUser._id, (err, body) =>
+				should.not.exist(err)
+				@request.calledOnce.should.equal true
+				requestOptions = @request.lastCall.args[0]
+				expectedUrl = "v1.url/api/v2/users/#{@stubbedUser._id}/affiliations"
+				requestOptions.url.should.equal expectedUrl
+				requestOptions.method.should.equal 'GET'
+				should.not.exist(requestOptions.body)
+				body.should.equal responseBody
+				done()
+
+		it 'handle error', (done)->
+			body = errors: 'affiliation error message'
+			@request.callsArgWith(1, null, { statusCode: 503 }, body)
+			@UserAffiliationsManager.getAffiliations @stubbedUser._id, (err) =>
+				should.exist(err)
+				err.message.should.have.string 503
+				err.message.should.have.string body.errors
+				done()
+
+	describe 'addAffiliation', ->
+		beforeEach ->
+			@request.callsArgWith(1, null, { statusCode: 201 })
+
+		it 'add affiliation', (done)->
+			affiliationOptions =
+				university: { id: 1 }
+				role: 'Prof'
+				department: 'Math'
+			@UserAffiliationsManager.addAffiliation @stubbedUser._id, @newEmail, affiliationOptions, (err)=>
+				should.not.exist(err)
+				@request.calledOnce.should.equal true
+				requestOptions = @request.lastCall.args[0]
+				expectedUrl = "v1.url/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)->
+			body = errors: 'affiliation error message'
+			@request.callsArgWith(1, null, { statusCode: 422 }, body)
+			@UserAffiliationsManager.addAffiliation @stubbedUser._id, @newEmail, {}, (err)=>
+				should.exist(err)
+				err.message.should.have.string 422
+				err.message.should.have.string body.errors
+				done()
+
+	describe 'removeAffiliation', ->
+		beforeEach ->
+			@request.callsArgWith(1, null, { statusCode: 404 })
+
+		it 'remove affiliation', (done)->
+			@UserAffiliationsManager.removeAffiliation @stubbedUser._id, @newEmail, (err)=>
+				should.not.exist(err)
+				@request.calledOnce.should.equal true
+				requestOptions = @request.lastCall.args[0]
+				expectedUrl = "v1.url/api/v2/users/#{@stubbedUser._id}/affiliations/"
+				expectedUrl += encodeURIComponent(@newEmail)
+				requestOptions.url.should.equal expectedUrl
+				requestOptions.method.should.equal 'DELETE'
+				done()
+
+		it 'handle error', (done)->
+			@request.callsArgWith(1, null, { statusCode: 500 })
+			@UserAffiliationsManager.removeAffiliation @stubbedUser._id, @newEmail, (err)=>
+				should.exist(err)
+				err.message.should.exist
+				done()

services/web/test/unit/coffee/User/UserGetterTests.coffee

@@ -21,14 +21,15 @@ describe "UserGetter", ->
 			db: users: findOne: @findOne
 			ObjectId: (id) -> return id
 		settings = apis: { v1: { url: 'v1.url', user: '', pass: '' } }
-		@request = sinon.stub()
+		@getAffiliations = sinon.stub().callsArgWith(1, null, [])
 
 		@UserGetter = SandboxedModule.require modulePath, requires:
 			"logger-sharelatex": log:->
 			"../../infrastructure/mongojs": @Mongo
 			"metrics-sharelatex": timeAsyncMethod: sinon.stub()
 			'settings-sharelatex': settings
-			'request': @request
+			'./UserAffiliationsManager':
+				getAffiliations: @getAffiliations
 
 	describe "getUser", ->
 		it "should get user", (done)->
@@ -46,9 +47,6 @@ 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
@@ -77,7 +75,7 @@ describe "UserGetter", ->
 					institution: { name: 'University Name', isUniversity: true }
 				}
 			]
-			@request.callsArgWith(1, null, { statusCode: 200 }, affiliationsData)
+			@getAffiliations.callsArgWith(1, null, affiliationsData)
 			@UserGetter.getUserFullEmails @fakeUser._id, (error, fullEmails) =>
 				assert.deepEqual fullEmails, [
 					{

services/web/test/unit/coffee/User/UserUpdaterTests.coffee

@@ -19,15 +19,16 @@ describe "UserUpdater", ->
 			getUserByAnyEmail: sinon.stub()
 			ensureUniqueEmailAddress: sinon.stub()
 		@logger = err: sinon.stub(), log: ->
-		settings = apis: { v1: { url: 'v1.url', user: '', pass: '' } }
+		@addAffiliation = sinon.stub().callsArgWith(3, null)
-		@request = sinon.stub()
+		@removeAffiliation = sinon.stub().callsArgWith(2, null)
 		@UserUpdater = SandboxedModule.require modulePath, requires:
 			"logger-sharelatex": @logger
 			"./UserGetter": @UserGetter
+			'./UserAffiliationsManager':
+				addAffiliation: @addAffiliation
+				removeAffiliation: @removeAffiliation
 			"../../infrastructure/mongojs":@mongojs
 			"metrics-sharelatex": timeAsyncMethod: sinon.stub()
-			'settings-sharelatex': settings
-			'request': @request
 
 		@stubbedUser = 
 			_id: "3131231"
@@ -69,7 +70,6 @@ describe "UserUpdater", ->
 		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.addEmailAddress @stubbedUser._id, @newEmail, (err)=>
@@ -88,18 +88,11 @@ describe "UserUpdater", ->
 				department: 'Math'
 			@UserUpdater.addEmailAddress @stubbedUser._id, @newEmail, affiliationOptions, (err)=>
 				should.not.exist(err)
-				@request.calledOnce.should.equal true
+				@addAffiliation.calledOnce.should.equal true
-				requestOptions = @request.lastCall.args[0]
+				args = @addAffiliation.lastCall.args
-				expectedUrl = "v1.url/api/v2/users/#{@stubbedUser._id}/affiliations"
+				args[0].should.equal @stubbedUser._id
-				requestOptions.url.should.equal expectedUrl
+				args[1].should.equal @newEmail
-				requestOptions.method.should.equal 'POST'
+				args[2].should.equal affiliationOptions
-
-				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)->
@@ -112,17 +105,15 @@ describe "UserUpdater", ->
 
 		it 'handle affiliation error', (done)->
 			body = errors: 'affiliation error message'
-			@request.callsArgWith(1, null, { statusCode: 422 }, body)
+			@addAffiliation.callsArgWith(3, new Error('nope'))
 			@UserUpdater.addEmailAddress @stubbedUser._id, @newEmail, (err)=>
-				err.message.should.have.string 422
+				should.exist(err)
-				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)=>
@@ -136,12 +127,10 @@ describe "UserUpdater", ->
 		it 'remove affiliation', (done)->
 			@UserUpdater.removeEmailAddress @stubbedUser._id, @newEmail, (err)=>
 				should.not.exist(err)
-				@request.calledOnce.should.equal true
+				@removeAffiliation.calledOnce.should.equal true
-				requestOptions = @request.lastCall.args[0]
+				args = @removeAffiliation.lastCall.args
-				expectedUrl = "v1.url/api/v2/users/#{@stubbedUser._id}/affiliations/"
+				args[0].should.equal @stubbedUser._id
-				expectedUrl += encodeURIComponent(@newEmail)
+				args[1].should.equal @newEmail
-				requestOptions.url.should.equal expectedUrl
-				requestOptions.method.should.equal 'DELETE'
 				done()
 
 		it 'handle error', (done)->
@@ -159,9 +148,9 @@ describe "UserUpdater", ->
 				done()
 
 		it 'handle affiliation error', (done)->
-			@request.callsArgWith(1, null, { statusCode: 500 })
+			@removeAffiliation.callsArgWith(2, new Error('nope'))
 			@UserUpdater.removeEmailAddress @stubbedUser._id, @newEmail, (err)=>
-				err.message.should.exist
+				should.exist(err)
 				@UserUpdater.updateUser.called.should.equal false
 				done()
 
@@ -216,6 +205,3 @@ describe "UserUpdater", ->
 			@UserUpdater.confirmEmail @stubbedUser._id, @newEmail, (err)=>
 				should.exist(err)
 				done()
-
-
-