Refactor addUserToProject for better access by groups

services/web/app/coffee/Features/Collaborators/CollaboratorsController.coffee

@@ -1,11 +1,9 @@
 ProjectGetter = require "../Project/ProjectGetter"
 CollaboratorsHandler = require "./CollaboratorsHandler"
-CollaboratorsEmailHandler = require "./CollaboratorsEmailHandler"
 ProjectEditorHandler = require "../Project/ProjectEditorHandler"
 EditorRealTimeController = require "../Editor/EditorRealTimeController"
-UserGetter = require "../User/UserGetter"
 LimitationsManager = require "../Subscription/LimitationsManager"
-ContactManager = require "../Contacts/ContactManager"
+UserGetter = require "../User/UserGetter"
 mimelib = require("mimelib")
 
 module.exports = CollaboratorsController =
@@ -20,10 +18,10 @@ module.exports = CollaboratorsController =
 			
 	addUserToProject: (req, res, next) ->
 		project_id = req.params.Project_id
-		LimitationsManager.isCollaboratorLimitReached project_id, (error, limit_reached) =>
+		LimitationsManager.canAddXCollaborators project_id, 1, (error, allowed) =>
 			return next(error) if error?
 
-			if limit_reached
+			if !allowed
 				return res.json { user: false }
 			else
 				{email, privileges} = req.body
@@ -32,18 +30,13 @@ module.exports = CollaboratorsController =
 				if !email? or email == ""
 					return res.status(400).send("invalid email address")
 					
-				CollaboratorsHandler.addEmailToProject project_id, email, privileges, (error, user_id) =>
+				adding_user_id = req.session?.user?._id
+				CollaboratorsHandler.addEmailToProject project_id, adding_user_id, email, privileges, (error, user_id) =>
 					return next(error) if error?
 					UserGetter.getUser user_id, (error, raw_user) ->
 						return next(error) if error?
 						user = ProjectEditorHandler.buildUserModelView(raw_user, privileges)
-						
-						# These things can all be done in the background
-						adding_user_id = req.session?.user?._id
-						CollaboratorsEmailHandler.notifyUserOfProjectShare project_id, user.email
 						EditorRealTimeController.emitToRoom(project_id, 'userAddedToProject', user, privileges)
-						ContactManager.addContact adding_user_id, user_id
-
 						return res.json { user: user }
 
 	removeUserFromProject: (req, res, next) ->

services/web/app/coffee/Features/Collaborators/CollaboratorsHandler.coffee

@@ -3,6 +3,9 @@ Project = require("../../models/Project").Project
 ProjectEntityHandler = require("../Project/ProjectEntityHandler")
 mimelib = require("mimelib")
 logger = require('logger-sharelatex')
+UserGetter = require "../User/UserGetter"
+ContactManager = require "../Contacts/ContactManager"
+CollaboratorsEmailHandler = require "./CollaboratorsEmailHandler"
 
 module.exports = CollaboratorsHandler =
 	removeUserFromProject: (project_id, user_id, callback = (error) ->)->
@@ -15,18 +18,18 @@ module.exports = CollaboratorsHandler =
 				logger.error err: err, "problem removing user from project collaberators"
 			callback(err)
 	
-	addEmailToProject: (project_id, unparsed_email, privilegeLevel, callback = (error, user) ->) ->
+	addEmailToProject: (project_id, adding_user_id, unparsed_email, privilegeLevel, callback = (error, user) ->) ->
 		emails = mimelib.parseAddresses(unparsed_email)
 		email = emails[0]?.address?.toLowerCase()
 		if !email? or email == ""
 			return callback(new Error("no valid email provided: '#{unparsed_email}'"))
 		UserCreator.getUserOrCreateHoldingAccount email, (error, user) ->
 			return callback(error) if error?
-			CollaboratorsHandler.addUserToProject project_id, user._id, privilegeLevel, (error) ->
+			CollaboratorsHandler.addUserIdToProject project_id, adding_user_id, user._id, privilegeLevel, (error) ->
 				return callback(error) if error?
 				return callback null, user._id
 
-	addUserToProject: (project_id, user_id, privilegeLevel, callback = (error) ->)->
+	addUserIdToProject: (project_id, adding_user_id, user_id, privilegeLevel, callback = (error) ->)->
 		if privilegeLevel == 'readAndWrite'
 			level = {"collaberator_refs":user_id}
 			logger.log {privileges: "readAndWrite", user_id, project_id}, "adding user"
@@ -35,6 +38,14 @@ module.exports = CollaboratorsHandler =
 			logger.log {privileges: "readOnly", user_id, project_id}, "adding user"
 		else
 			return callback(new Error("unknown privilegeLevel: #{privilegeLevel}"))
+
+		# Do these in the background
+		UserGetter.getUser user_id, {email: 1}, (error, user) ->
+			if error?
+				logger.error {err: error, project_id, user_id}, "error getting user while adding to project"
+			CollaboratorsEmailHandler.notifyUserOfProjectShare project_id, user.email
+		ContactManager.addContact adding_user_id, user_id
+
 		Project.update { _id: project_id }, { $push:level }, (error) ->
 			return callback(error) if error?
 			# Flush to TPDS in background to add files to collaborator's Dropbox

services/web/app/coffee/Features/Subscription/LimitationsManager.coffee

@@ -19,15 +19,15 @@ module.exports =
 			return callback(error) if error?
 			callback null, (project.collaberator_refs.length + project.readOnly_refs.length)
 
-	isCollaboratorLimitReached: (project_id, callback = (error, limit_reached)->) ->
+	canAddXCollaborators: (project_id, x_collaborators, callback = (error, allowed)->) ->
 		@allowedNumberOfCollaboratorsInProject project_id, (error, allowed_number) =>
 			return callback(error) if error?
 			@currentNumberOfCollaboratorsInProject project_id, (error, current_number) =>
 				return callback(error) if error?
-				if current_number < allowed_number or allowed_number < 0
+				if current_number + x_collaborators <= allowed_number or allowed_number < 0
-					callback null, false
-				else
 					callback null, true
+				else
+					callback null, false
 
 	userHasSubscriptionOrIsGroupMember: (user, callback = (err, hasSubscriptionOrIsMember)->) ->
 		@userHasSubscription user, (err, hasSubscription, subscription)=>

services/web/app/coffee/infrastructure/Modules.coffee

@@ -47,7 +47,6 @@ module.exports = Modules =
 	hooks:
 		_hooks: {}
 		attach: (name, method) ->
-			console.log "attaching hook", name, method
 			@_hooks[name] ?= []
 			@_hooks[name].push method
 			

services/web/app/views/project/editor/share.jade

@@ -146,12 +146,12 @@ script(type="text/ng-template", id="shareTagTemplate")
 script(type="text/ng-template", id="shareAutocompleteTemplate")
 	.autocomplete-template
 		div(ng-if="data.type == 'user'")
-			i.fa.fa-user
+			i.fa.fa-fw.fa-user
 			| 
 			span(ng-bind-html="$highlight(data.email)")
 			div.subdued.small(ng-show="data.name", ng-bind-html="$highlight(data.name)")
 		div(ng-if="data.type == 'group'")
-			i.fa.fa-group
+			i.fa.fa-fw.fa-group
 			| 
 			span(ng-bind-html="$highlight(data.name)")
 			div.subdued.small(ng-show="data.member_count") {{ data.memberCount }} members

services/web/test/UnitTests/coffee/Collaborators/CollaboratorsControllerTests.coffee

@@ -14,12 +14,10 @@ describe "CollaboratorsController", ->
 		@CollaboratorsController = SandboxedModule.require modulePath, requires:
 			"../Project/ProjectGetter": @ProjectGetter = {}
 			"./CollaboratorsHandler": @CollaboratorsHandler = {}
-			"./CollaboratorsEmailHandler": @CollaboratorsEmailHandler = {}
-			"../User/UserGetter": @UserGetter = {}
 			"../Editor/EditorRealTimeController": @EditorRealTimeController = {}
 			'../Subscription/LimitationsManager' : @LimitationsManager = {}
 			'../Project/ProjectEditorHandler' : @ProjectEditorHandler = {}
-			"../Contacts/ContactManager": @ContactManager = {}
+			'../User/UserGetter': @UserGetter = {}
 		@res = new MockResponse()
 		@req = new MockRequest()
 
@@ -72,13 +70,11 @@ describe "CollaboratorsController", ->
 			@user_view = {
 				id: @user_id, first_name: "Joe", last_name: "Example", email: "joe@example.com"
 			}
-			@LimitationsManager.isCollaboratorLimitReached = sinon.stub().callsArgWith(1, null, false)
+			@LimitationsManager.canAddXCollaborators = sinon.stub().callsArgWith(2, null, true)
 			@ProjectEditorHandler.buildUserModelView = sinon.stub().returns(@user_view)
-			@CollaboratorsHandler.addEmailToProject = sinon.stub().callsArgWith(3, null, @user_id)
+			@CollaboratorsHandler.addEmailToProject = sinon.stub().callsArgWith(4, null, @user_id)
 			@UserGetter.getUser = sinon.stub().callsArgWith(1, null, @user)
-			@CollaboratorsEmailHandler.notifyUserOfProjectShare = sinon.stub()
 			@EditorRealTimeController.emitToRoom = sinon.stub()
-			@ContactManager.addContact = sinon.stub()
 			@callback = sinon.stub()
 
 		describe "when the project can accept more collaborators", ->
@@ -87,7 +83,7 @@ describe "CollaboratorsController", ->
 
 			it "should add the user to the project", ->
 				@CollaboratorsHandler.addEmailToProject
-					.calledWith(@project_id, @email.toLowerCase(), @privileges)
+					.calledWith(@project_id, @adding_user_id, @email.toLowerCase(), @privileges)
 					.should.equal true
 
 			it "should emit a userAddedToProject event", ->
@@ -95,16 +91,6 @@ describe "CollaboratorsController", ->
 					.calledWith(@project_id, "userAddedToProject", @user_view, @privileges)
 					.should.equal true
 
-			it "should send an email to the shared-with user", ->
-				@CollaboratorsEmailHandler.notifyUserOfProjectShare
-					.calledWith(@project_id, @email.toLowerCase())
-					.should.equal true
-			
-			it "should add the user as a contact for the adding user", ->
-				@ContactManager.addContact
-					.calledWith(@adding_user_id, @user_id)
-					.should.equal true
-
 			it "should send the user as the response body", ->
 				@res.json
 					.calledWith({
@@ -114,7 +100,7 @@ describe "CollaboratorsController", ->
 
 		describe "when the project cannot accept more collaborators", ->
 			beforeEach ->
-				@LimitationsManager.isCollaboratorLimitReached = sinon.stub().callsArgWith(1, null, true)
+				@LimitationsManager.canAddXCollaborators = sinon.stub().callsArgWith(2, null, false)
 				@CollaboratorsController.addUserToProject @req, @res, @next
 		
 			it "should not add the user to the project", ->

services/web/test/UnitTests/coffee/Collaborators/CollaboratorsHandlerTests.coffee

@@ -11,11 +11,16 @@ describe "CollaboratorsHandler", ->
 		@CollaboratorHandler = SandboxedModule.require modulePath, requires:
 			"logger-sharelatex": @logger = { log: sinon.stub(), err: sinon.stub() }
 			'../User/UserCreator': @UserCreator = {}
+			'../User/UserGetter': @UserGetter = {}
+			"../Contacts/ContactManager": @ContactManager = {}
 			"../../models/Project": Project: @Project = {}
 			"../Project/ProjectEntityHandler": @ProjectEntityHandler = {}
+			"./CollaboratorsEmailHandler": @CollaboratorsEmailHandler = {}
 
 		@project_id = "mock-project-id"
 		@user_id = "mock-user-id"
+		@adding_user_id = "adding-user-id"
+		@email = "joe@sharelatex.com"
 		@callback = sinon.stub()
 
 	describe "removeUserFromProject", ->
@@ -36,10 +41,14 @@ describe "CollaboratorsHandler", ->
 		beforeEach ->
 			@Project.update = sinon.stub().callsArg(2)
 			@ProjectEntityHandler.flushProjectToThirdPartyDataStore = sinon.stub().callsArg(1)
+			@CollaboratorHandler.addEmailToProject = sinon.stub().callsArgWith(4, null, @user_id)
+			@UserGetter.getUser = sinon.stub().callsArgWith(2, null, @user = { _id: @user_id, email: @email })
+			@CollaboratorsEmailHandler.notifyUserOfProjectShare = sinon.stub()
+			@ContactManager.addContact = sinon.stub()
 			
 		describe "as readOnly", ->
 			beforeEach ->
-				@CollaboratorHandler.addUserToProject @project_id, @user_id, "readOnly", @callback
+				@CollaboratorHandler.addUserIdToProject @project_id, @adding_user_id, @user_id, "readOnly", @callback
 
 			it "should add the user to the readOnly_refs", ->
 				@Project.update
@@ -55,9 +64,19 @@ describe "CollaboratorsHandler", ->
 					.calledWith(@project_id)
 					.should.equal true
 
+			it "should send an email to the shared-with user", ->
+				@CollaboratorsEmailHandler.notifyUserOfProjectShare
+					.calledWith(@project_id, @email)
+					.should.equal true
+			
+			it "should add the user as a contact for the adding user", ->
+				@ContactManager.addContact
+					.calledWith(@adding_user_id, @user_id)
+					.should.equal true
+					
 		describe "as readAndWrite", ->
 			beforeEach ->
-				@CollaboratorHandler.addUserToProject @project_id, @user_id, "readAndWrite", @callback
+				@CollaboratorHandler.addUserIdToProject @project_id, @adding_user_id, @user_id, "readAndWrite", @callback
 
 			it "should add the user to the collaberator_refs", ->
 				@Project.update
@@ -75,7 +94,7 @@ describe "CollaboratorsHandler", ->
 		
 		describe "with invalid privilegeLevel", ->
 			beforeEach ->
-				@CollaboratorHandler.addUserToProject @project_id, @user_id, "notValid", @callback
+				@CollaboratorHandler.addUserIdToProject @project_id, @adding_user_id, @user_id, "notValid", @callback
 
 			it "should call the callback with an error", ->
 				@callback.calledWith(new Error()).should.equal true
@@ -83,11 +102,11 @@ describe "CollaboratorsHandler", ->
 	describe "addEmailToProject", ->
 		beforeEach ->
 			@UserCreator.getUserOrCreateHoldingAccount = sinon.stub().callsArgWith(1, null, @user = {_id: @user_id})
-			@CollaboratorHandler.addUserToProject = sinon.stub().callsArg(3)
+			@CollaboratorHandler.addUserIdToProject = sinon.stub().callsArg(4)
 
 		describe "with a valid email", ->
 			beforeEach ->
-				@CollaboratorHandler.addEmailToProject @project_id, (@email = "Joe@example.com"), (@privilegeLevel = "readAndWrite"), @callback
+				@CollaboratorHandler.addEmailToProject @project_id, @adding_user_id, (@email = "Joe@example.com"), (@privilegeLevel = "readAndWrite"), @callback
 			
 			it "should get the user with the lowercased email", ->
 				@UserCreator.getUserOrCreateHoldingAccount
@@ -95,8 +114,8 @@ describe "CollaboratorsHandler", ->
 					.should.equal true
 		
 			it "should add the user to the project by id", ->
-				@CollaboratorHandler.addUserToProject
+				@CollaboratorHandler.addUserIdToProject
-					.calledWith(@project_id, @user_id, @privilegeLevel)
+					.calledWith(@project_id, @adding_user_id, @user_id, @privilegeLevel)
 					.should.equal true
 			
 			it "should return the callback with the user_id", ->
@@ -104,13 +123,13 @@ describe "CollaboratorsHandler", ->
 		
 		describe "with an invalid email", ->
 			beforeEach ->
-				@CollaboratorHandler.addEmailToProject @project_id, "not-and-email", (@privilegeLevel = "readAndWrite"), @callback
+				@CollaboratorHandler.addEmailToProject @project_id, @adding_user_id, "not-and-email", (@privilegeLevel = "readAndWrite"), @callback
 		
 			it "should call the callback with an error", ->
 				@callback.calledWith(new Error()).should.equal true
 			
 			it "should not add any users to the proejct", ->
-				@CollaboratorHandler.addUserToProject.called.should.equal false
+				@CollaboratorHandler.addUserIdToProject.called.should.equal false
 
 
 

services/web/test/UnitTests/coffee/Subscription/LimitationsManagerTests.coffee

@@ -62,7 +62,7 @@ describe "LimitationsManager", ->
 		it "should return the total number of collaborators", ->
 			@callback.calledWith(null, 3).should.equal true
 
-	describe "isCollaboratorLimitReached", ->
+	describe "canAddXCollaborators", ->
 		beforeEach ->
 			sinon.stub @LimitationsManager,
 					   "currentNumberOfCollaboratorsInProject",
@@ -76,7 +76,16 @@ describe "LimitationsManager", ->
 			beforeEach ->
 				@current_number = 1
 				@allowed_number = 2
-				@LimitationsManager.isCollaboratorLimitReached(@project_id, @callback)
+				@LimitationsManager.canAddXCollaborators(@project_id, 1, @callback)
+
+			it "should return true", ->
+				@callback.calledWith(null, true).should.equal true
+
+		describe "when the project has fewer collaborators than allowed but I want to add more than allowed", ->
+			beforeEach ->
+				@current_number = 1
+				@allowed_number = 2
+				@LimitationsManager.canAddXCollaborators(@project_id, 2, @callback)
 
 			it "should return false", ->
 				@callback.calledWith(null, false).should.equal true
@@ -85,19 +94,19 @@ describe "LimitationsManager", ->
 			beforeEach ->
 				@current_number = 3
 				@allowed_number = 2
-				@LimitationsManager.isCollaboratorLimitReached(@project_id, @callback)
+				@LimitationsManager.canAddXCollaborators(@project_id, 1, @callback)
 
-			it "should return true", ->
+			it "should return false", ->
-				@callback.calledWith(null, true).should.equal true
+				@callback.calledWith(null, false).should.equal true
 
 		describe "when the project has infinite collaborators", ->
 			beforeEach ->
 				@current_number = 100
 				@allowed_number = -1
-				@LimitationsManager.isCollaboratorLimitReached(@project_id, @callback)
+				@LimitationsManager.canAddXCollaborators(@project_id, 1, @callback)
 
-			it "should return false", ->
+			it "should return true", ->
-				@callback.calledWith(null, false).should.equal true
+				@callback.calledWith(null, true).should.equal true
 
 
 	describe "userHasSubscription", ->