Merge pull request #718 from sharelatex/afc-multiple-managers-queries

Use the manager_ids array to query for subscriptions

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

@@ -9,9 +9,6 @@ module.exports = SubscriptionDomainHandler =
 		licence = SubscriptionDomainHandler._findDomainLicence(user.email)
 		return licence
 
-	rejectInvitationToGroup: (user, subscription, callback)->
-		removeUserFromGroup(subscription.admin_id, user._id, callback)
-
 	getDomainLicencePage: (user)->
 		licence = SubscriptionDomainHandler._findDomainLicence(user.email)
 		if licence?.verifyEmail

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

@@ -13,7 +13,7 @@ module.exports =
 		adminUserId = AuthenticationController.getLoggedInUserId(req)
 		newEmail = req.body?.email?.toLowerCase()?.trim()
 
-		SubscriptionLocator.getManagedSubscription adminUserId, (error, subscription) ->
+		getManagedSubscription adminUserId, (error, subscription) ->
 			return next(error) if error?
 
 			logger.log adminUserId:adminUserId, newEmail:newEmail, "adding user to group subscription"
@@ -31,7 +31,7 @@ module.exports =
 	removeUserFromGroup: (req, res, next)->
 		adminUserId = AuthenticationController.getLoggedInUserId(req)
 		userToRemove_id = req.params.user_id
-		SubscriptionLocator.getManagedSubscription adminUserId, (error, subscription) ->
+		getManagedSubscription adminUserId, (error, subscription) ->
 			return next(error) if error?
 			logger.log adminUserId:adminUserId, userToRemove_id:userToRemove_id, "removing user from group subscription"
 			SubscriptionGroupHandler.removeUserFromGroup subscription._id, userToRemove_id, (err)->
@@ -43,7 +43,7 @@ module.exports =
 	removeSelfFromGroup: (req, res, next)->
 		adminUserId = req.query.admin_user_id
 		userToRemove_id = AuthenticationController.getLoggedInUserId(req)
-		SubscriptionLocator.getManagedSubscription adminUserId, (error, subscription) ->
+		getManagedSubscription adminUserId, (error, subscription) ->
 			return next(error) if error?
 			logger.log adminUserId:adminUserId, userToRemove_id:userToRemove_id, "removing user from group subscription after self request"
 			SubscriptionGroupHandler.removeUserFromGroup subscription._id, userToRemove_id, (err)->
@@ -54,7 +54,7 @@ module.exports =
 
 	renderSubscriptionGroupAdminPage: (req, res, next)->
 		user_id = AuthenticationController.getLoggedInUserId(req)
-		SubscriptionLocator.getManagedSubscription user_id, (error, subscription)->
+		getManagedSubscription user_id, (error, subscription)->
 			return next(error) if error?
 			if !subscription?.groupPlan
 				return res.redirect("/user/subscription")
@@ -67,7 +67,7 @@ module.exports =
 	exportGroupCsv: (req, res)->
 		user_id = AuthenticationController.getLoggedInUserId(req)
 		logger.log user_id: user_id, "exporting group csv"
-		SubscriptionLocator.getManagedSubscription user_id, (err, subscription)->
+		getManagedSubscription user_id, (err, subscription)->
 			return next(error) if error?
 			if !subscription.groupPlan
 				return res.redirect("/")
@@ -81,3 +81,13 @@ module.exports =
 				)
 				res.contentType('text/csv')
 				res.send(groupCsv)
+
+
+getManagedSubscription = (managerId, callback) ->
+	SubscriptionLocator.findManagedSubscription managerId, (err, subscription)->
+		if subscription?
+			logger.log managerId: managerId, "got managed subscription"
+		else
+			err ||= new Error("No subscription found managed by user #{managerId}")
+
+		return callback(err, subscription)

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

@@ -46,15 +46,10 @@ module.exports = SubscriptionGroupHandler =
 		Subscription.update {admin_id: oldId}, {admin_id: newId}, (error) ->
 			callback(error) if error?
 
-			# Mongo won't let us pull and addToSet in the same query, so do it in
-			# two. Note we need to add first, since the query is based on the old user.
-			query = { member_ids: oldId }
-			addNewUserUpdate = $addToSet: { member_ids: newId }
-			removeOldUserUpdate =  $pull: { member_ids: oldId }
+			replaceInArray Subscription, "manager_ids", oldId, newId, (error) ->
+				callback(error) if error?
 
-			Subscription.update query, addNewUserUpdate, { multi: true }, (error) ->
-				return callback(error) if error?
-				Subscription.update query, removeOldUserUpdate, { multi: true }, callback
+				replaceInArray Subscription, "member_ids", oldId, newId, callback
 
 	getPopulatedListOfMembers: (subscriptionId, callback)->
 		SubscriptionLocator.getSubscription subscriptionId, (err, subscription)->
@@ -87,6 +82,24 @@ module.exports = SubscriptionGroupHandler =
 			logger.log user_id:user_id, subscription_id:subscription_id, partOfGroup:partOfGroup, "checking if user is part of a group"
 			callback(err, partOfGroup)
 
+replaceInArray = (model, property, oldValue, newValue, callback) ->
+	logger.log "Replacing #{oldValue} with #{newValue} in #{property} of #{model}"
+
+	# Mongo won't let us pull and addToSet in the same query, so do it in
+	# two. Note we need to add first, since the query is based on the old user.
+	query = {}
+	query[property] = oldValue
+
+	setNewValue = {}
+	setNewValue[property] = newValue
+
+	setOldValue = {}
+	setOldValue[property] = oldValue
+
+	model.update query, { $addToSet: setNewValue }, { multi: true }, (error) ->
+		return callback(error) if error?
+		model.update query, { $pull: setOldValue }, { multi: true }, callback
+
 buildUserViewModel = (user)->
 	u =
 		email: user.email

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

@@ -2,7 +2,7 @@ Subscription = require('../../models/Subscription').Subscription
 logger = require("logger-sharelatex")
 ObjectId = require('mongoose').Types.ObjectId
 
-module.exports =
+module.exports = SubscriptionLocator =
 
 	getUsersSubscription: (user_or_id, callback)->
 		if user_or_id? and user_or_id._id?
@@ -14,15 +14,9 @@ module.exports =
 			logger.log user_id:user_id, "got users subscription"
 			callback(err, subscription)
 
-	getManagedSubscription: (managerId, callback)->
-			logger.log managerId: managerId, "getting managed subscription"
-			Subscription.findOne admin_id: managerId, (err, subscription)->
-				if subscription?
-					logger.log managerId: managerId, "got managed subscription"
-				else
-					err ||= new Error("No subscription found managed by user #{managerId}")
-
-				callback(err, subscription)
+	findManagedSubscription: (managerId, callback)->
+		logger.log managerId: managerId, "finding managed subscription"
+		Subscription.findOne manager_ids: managerId, callback
 
 	getMemberSubscriptions: (user_or_id, callback) ->
 		if user_or_id? and user_or_id._id?

services/web/test/acceptance/coffee/FeatureUpdaterTests.coffee

@@ -39,6 +39,7 @@ describe "FeatureUpdater.refreshFeatures", ->
 		beforeEach ->
 			Subscription.create {
 				admin_id: @user._id
+				manager_ids: [@user._id]
 				planCode: 'collaborator'
 				customAccount: true
 			} # returns a promise
@@ -163,6 +164,7 @@ describe "FeatureUpdater.refreshFeatures", ->
 		beforeEach (done) ->
 			Subscription.create {
 				admin_id: @user._id
+				manager_ids: [@user._id]
 				planCode: 'professional'
 				customAccount: true
 			}, (error) =>

services/web/test/unit/coffee/Subscription/SubscriptionGroupControllerTests.coffee

@@ -29,7 +29,7 @@ describe "SubscriptionGroupController", ->
 			isUserPartOfGroup: sinon.stub()
 			getPopulatedListOfMembers: sinon.stub().callsArgWith(1, null, [@user])
 		@SubscriptionLocator =
-			getManagedSubscription: sinon.stub().callsArgWith(1, null, @subscription)
+			findManagedSubscription: sinon.stub().callsArgWith(1, null, @subscription)
 
 		@AuthenticationController =
 			getLoggedInUserId: (req) -> req.session.user._id

services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee

@@ -16,7 +16,8 @@ describe "SubscriptionGroupHandler", ->
 		@subscription_id = "31DSd1123D"
 
 		@subscription =
-			admin_id:@adminUser_id
+			admin_id: @adminUser_id
+			manager_ids: [@adminUser_id]
 			_id:@subscription_id
 
 		@SubscriptionLocator =
@@ -124,34 +125,37 @@ describe "SubscriptionGroupHandler", ->
 				done()
 
 	describe "replaceUserReferencesInGroups", ->
-		beforeEach ->
+		beforeEach (done)->
 			@oldId = "ba5eba11"
 			@newId = "5ca1ab1e"
+			@Handler.replaceUserReferencesInGroups @oldId, @newId, ->
+				done()
 
-		it "replaces the admin_id", (done) ->
-			@Handler.replaceUserReferencesInGroups @oldId, @newId, (err) =>
-
+		it "replaces the admin_id", ->
 				@Subscription.update.calledWith(
 					{ admin_id: @oldId },
 					{ admin_id: @newId }
 				).should.equal true
 
-				done()
-
-		it "replaces the member ids", (done) ->
-			@Handler.replaceUserReferencesInGroups @oldId, @newId, (err) =>
-
+		it "replaces the manager_ids", ->
 				@Subscription.update.calledWith(
-					{ member_ids: @oldId },
-					{ $addToSet: { member_ids: @newId } }
+					{manager_ids:"ba5eba11"},{$addToSet:{manager_ids:"5ca1ab1e"}},{multi:true}
 				).should.equal true
 
 				@Subscription.update.calledWith(
-					{ member_ids: @oldId },
-					{ $pull: { member_ids: @oldId } }
+					{manager_ids:"ba5eba11"},{$pull:{manager_ids:"ba5eba11"}},{multi:true}
 				).should.equal true
 
-				done()
+		it "replaces the member ids", ->
+			@Subscription.update.calledWith(
+				{ member_ids: @oldId },
+				{ $addToSet: { member_ids: @newId } }
+			).should.equal true
+
+			@Subscription.update.calledWith(
+				{ member_ids: @oldId },
+				{ $pull: { member_ids: @oldId } }
+			).should.equal true
 
 	describe "getPopulatedListOfMembers", ->
 		beforeEach ->

services/web/test/unit/coffee/Subscription/SubscriptionLocatorTests.coffee

@@ -3,7 +3,7 @@ should = require('chai').should()
 sinon = require 'sinon'
 modulePath = "../../../../app/js/Features/Subscription/SubscriptionLocator"
 assert = require("chai").assert
-ObjectId = require('mongoose').Types.ObjectId	
+ObjectId = require('mongoose').Types.ObjectId
 
 
 describe "Subscription Locator Tests", ->
@@ -41,3 +41,11 @@ describe "Subscription Locator Tests", ->
 				subscription.should.equal @subscription
 				done()
 
+		describe "finding managed subscription", ->
+
+			it "should query the database", (done)->
+				@Subscription.findOne.callsArgWith(1, null, @subscription)
+				@SubscriptionLocator.findManagedSubscription @user._id, (err, subscription)=>
+					@Subscription.findOne.calledWith({"manager_ids":@user._id}).should.equal true
+					subscription.should.equal @subscription
+					done()