From 4e8185d3692de54479ffe34e0bf9c0e0c71ccdae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez=20Capel?= Date: Wed, 11 Jul 2018 09:31:57 +0100 Subject: [PATCH] Use the id in methods that modify a subscription This will make it easier to allow a user to manage multiple groups. --- .../Subscription/LimitationsManager.coffee | 8 +- .../SubscriptionGroupController.coffee | 73 +++++++++++-------- .../SubscriptionGroupHandler.coffee | 22 +++--- .../Subscription/SubscriptionLocator.coffee | 10 +++ .../Subscription/SubscriptionUpdater.coffee | 14 ++-- .../LimitationsManagerTests.coffee | 8 +- .../SubscriptionGroupControllerTests.coffee | 20 +++-- .../SubscriptionGroupHandlerTests.coffee | 8 +- .../SubscriptionUpdaterTests.coffee | 18 +++-- 9 files changed, 103 insertions(+), 78 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee b/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee index 5f5de9f549..078676086b 100644 --- a/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee +++ b/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee @@ -89,13 +89,13 @@ module.exports = LimitationsManager = return currentTotal >= subscription.membersLimit - hasGroupMembersLimitReached: (user_id, callback = (err, limitReached, subscription)->)-> - SubscriptionLocator.getUsersSubscription user_id, (err, subscription)-> + hasGroupMembersLimitReached: (subscriptionId, callback = (err, limitReached, subscription)->)-> + SubscriptionLocator.getSubscription subscriptionId, (err, subscription)-> if err? - logger.err err:err, user_id:user_id, "error getting users subscription" + logger.err err:err, subscriptionId: subscriptionId, "error getting subscription" return callback(err) if !subscription? - logger.err user_id:user_id, "no subscription found for user" + logger.err subscriptionId: subscriptionId, "no subscription found" return callback("no subscription found") limitReached = LimitationsManager.teamHasReachedMemberLimit(subscription) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionGroupController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionGroupController.coffee index 8e84f1fb0b..cab61c5425 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionGroupController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionGroupController.coffee @@ -9,46 +9,56 @@ async = require("async") module.exports = - addUserToGroup: (req, res)-> + addUserToGroup: (req, res, next)-> adminUserId = AuthenticationController.getLoggedInUserId(req) newEmail = req.body?.email?.toLowerCase()?.trim() - logger.log adminUserId:adminUserId, newEmail:newEmail, "adding user to group subscription" - SubscriptionGroupHandler.addUserToGroup adminUserId, newEmail, (err, user)-> - if err? - logger.err err:err, newEmail:newEmail, adminUserId:adminUserId, "error adding user from group" - return res.sendStatus 500 - result = - user:user - if err and err.limitReached - result.limitReached = true - res.json(result) - removeUserFromGroup: (req, res)-> + SubscriptionLocator.getManagedSubscription adminUserId, (error, subscription) -> + return next(error) if error? + + logger.log adminUserId:adminUserId, newEmail:newEmail, "adding user to group subscription" + + SubscriptionGroupHandler.addUserToGroup subscription._id, newEmail, (err, user)-> + if err? + logger.err err:err, newEmail:newEmail, adminUserId:adminUserId, "error adding user from group" + return res.sendStatus 500 + result = + user:user + if err and err.limitReached + result.limitReached = true + res.json(result) + + removeUserFromGroup: (req, res, next)-> adminUserId = AuthenticationController.getLoggedInUserId(req) userToRemove_id = req.params.user_id - logger.log adminUserId:adminUserId, userToRemove_id:userToRemove_id, "removing user from group subscription" - SubscriptionGroupHandler.removeUserFromGroup adminUserId, userToRemove_id, (err)-> - if err? - logger.err err:err, adminUserId:adminUserId, userToRemove_id:userToRemove_id, "error removing user from group" - return res.sendStatus 500 - res.send() + SubscriptionLocator.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)-> + if err? + logger.err err:err, adminUserId:adminUserId, userToRemove_id:userToRemove_id, "error removing user from group" + return res.sendStatus 500 + res.send() - removeSelfFromGroup: (req, res)-> + removeSelfFromGroup: (req, res, next)-> adminUserId = req.query.admin_user_id userToRemove_id = AuthenticationController.getLoggedInUserId(req) - logger.log adminUserId:adminUserId, userToRemove_id:userToRemove_id, "removing user from group subscription after self request" - SubscriptionGroupHandler.removeUserFromGroup adminUserId, userToRemove_id, (err)-> - if err? - logger.err err:err, userToRemove_id:userToRemove_id, adminUserId:adminUserId, "error removing self from group" - return res.sendStatus 500 - res.send() + SubscriptionLocator.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)-> + if err? + logger.err err:err, userToRemove_id:userToRemove_id, adminUserId:adminUserId, "error removing self from group" + return res.sendStatus 500 + res.send() - renderSubscriptionGroupAdminPage: (req, res)-> + renderSubscriptionGroupAdminPage: (req, res, next)-> user_id = AuthenticationController.getLoggedInUserId(req) - SubscriptionLocator.getUsersSubscription user_id, (err, subscription)-> + SubscriptionLocator.getManagedSubscription user_id, (error, subscription)-> + return next(error) if error? if !subscription?.groupPlan return res.redirect("/user/subscription") - SubscriptionGroupHandler.getPopulatedListOfMembers user_id, (err, users)-> + SubscriptionGroupHandler.getPopulatedListOfMembers subscription._id, (err, users)-> res.render "subscriptions/group_admin", title: 'group_admin' users: users @@ -57,10 +67,9 @@ module.exports = exportGroupCsv: (req, res)-> user_id = AuthenticationController.getLoggedInUserId(req) logger.log user_id: user_id, "exporting group csv" - SubscriptionLocator.getUsersSubscription user_id, (err, subscription)-> - if !subscription.groupPlan - return res.redirect("/") - SubscriptionGroupHandler.getPopulatedListOfMembers user_id, (err, users)-> + SubscriptionLocator.getManagedSubscription user_id, (err, subscription)-> + return next(error) if error? + SubscriptionGroupHandler.getPopulatedListOfMembers subscription._id, (err, users)-> groupCsv = "" for user in users groupCsv += user.email + "\n" diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee index 2f3ac4a803..55bd67ccc2 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee @@ -14,19 +14,19 @@ NotificationsBuilder = require("../Notifications/NotificationsBuilder") module.exports = SubscriptionGroupHandler = - addUserToGroup: (adminUserId, newEmail, callback)-> - logger.log adminUserId:adminUserId, newEmail:newEmail, "adding user to group" - LimitationsManager.hasGroupMembersLimitReached adminUserId, (err, limitReached, subscription)-> + addUserToGroup: (subscriptionId, newEmail, callback)-> + logger.log subscriptionId:subscriptionId, newEmail:newEmail, "adding user to group" + LimitationsManager.hasGroupMembersLimitReached subscriptionId, (err, limitReached, subscription)-> if err? - logger.err err:err, adminUserId:adminUserId, newEmail:newEmail, "error checking if limit reached for group plan" + logger.err err:err, subscriptionId:subscriptionId, newEmail:newEmail, "error checking if limit reached for group plan" return callback(err) if limitReached - logger.err adminUserId:adminUserId, newEmail:newEmail, "group subscription limit reached not adding user to group" + logger.err subscriptionId:subscriptionId, newEmail:newEmail, "group subscription limit reached not adding user to group" return callback(limitReached:limitReached) UserGetter.getUserByAnyEmail newEmail, (err, user)-> return callback(err) if err? if user? - SubscriptionUpdater.addUserToGroup adminUserId, user._id, (err)-> + SubscriptionUpdater.addUserToGroup subscriptionId, user._id, (err)-> if err? logger.err err:err, "error adding user to group" return callback(err) @@ -34,13 +34,13 @@ module.exports = SubscriptionGroupHandler = userViewModel = buildUserViewModel(user) callback(err, userViewModel) else - TeamInvitesHandler.createInvite adminUserId, newEmail, (err) -> + TeamInvitesHandler.createInvite subscriptionId, newEmail, (err) -> return callback(err) if err? userViewModel = buildEmailInviteViewModel(newEmail) callback(err, userViewModel) - removeUserFromGroup: (adminUser_id, userToRemove_id, callback)-> - SubscriptionUpdater.removeUserFromGroup adminUser_id, userToRemove_id, callback + removeUserFromGroup: (subscriptionId, userToRemove_id, callback)-> + SubscriptionUpdater.removeUserFromGroup subscriptionId, userToRemove_id, callback replaceUserReferencesInGroups: (oldId, newId, callback) -> Subscription.update {admin_id: oldId}, {admin_id: newId}, (error) -> @@ -56,8 +56,8 @@ module.exports = SubscriptionGroupHandler = return callback(error) if error? Subscription.update query, removeOldUserUpdate, { multi: true }, callback - getPopulatedListOfMembers: (adminUser_id, callback)-> - SubscriptionLocator.getUsersSubscription adminUser_id, (err, subscription)-> + getPopulatedListOfMembers: (subscriptionId, callback)-> + SubscriptionLocator.getSubscription subscriptionId, (err, subscription)-> users = [] for email in subscription.invited_emails or [] diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee index ef6693f9b8..a228af7ffc 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee @@ -14,6 +14,16 @@ 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) + getMemberSubscriptions: (user_or_id, callback) -> if user_or_id? and user_or_id._id? user_id = user_or_id._id diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee index b5fce978a4..aea2042b06 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee @@ -25,13 +25,13 @@ module.exports = SubscriptionUpdater = return callback(err) if err? SubscriptionUpdater._updateSubscriptionFromRecurly recurlySubscription, subscription, callback - addUserToGroup: (adminUserId, userId, callback)-> - @addUsersToGroup(adminUserId, [userId], callback) + addUserToGroup: (subscriptionId, userId, callback)-> + @addUsersToGroup(subscriptionId, [userId], callback) - addUsersToGroup: (adminUserId, memberIds, callback)-> - logger.log adminUserId: adminUserId, memberIds: memberIds, "adding members into mongo subscription" + addUsersToGroup: (subscriptionId, memberIds, callback)-> + logger.log subscriptionId: subscriptionId, memberIds: memberIds, "adding members into mongo subscription" searchOps = - admin_id: adminUserId + _id: subscriptionId insertOperation = { $push: { member_ids: { $each: memberIds } } } @@ -46,9 +46,9 @@ module.exports = SubscriptionUpdater = async.map userIds, FeaturesUpdater.refreshFeatures, callback - removeUserFromGroup: (adminUser_id, user_id, callback)-> + removeUserFromGroup: (subscriptionId, user_id, callback)-> searchOps = - admin_id: adminUser_id + _id: subscriptionId removeOperation = "$pull": {member_ids:user_id} Subscription.update searchOps, removeOperation, (err)-> diff --git a/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee b/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee index 87a8ac8afc..10b8a897eb 100644 --- a/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee @@ -23,6 +23,7 @@ describe "LimitationsManager", -> @SubscriptionLocator = getUsersSubscription: sinon.stub() + getSubscription: sinon.stub() @LimitationsManager = SandboxedModule.require modulePath, requires: '../Project/ProjectGetter': @ProjectGetter @@ -310,21 +311,21 @@ describe "LimitationsManager", -> ] it "should return true if the limit is hit (including members and invites)", (done)-> - @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, @subscription) + @SubscriptionLocator.getSubscription.callsArgWith(1, null, @subscription) @LimitationsManager.hasGroupMembersLimitReached @user_id, (err, limitReached)-> limitReached.should.equal true done() it "should return false if the limit is not hit (including members and invites)", (done)-> @subscription.membersLimit = 4 - @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, @subscription) + @SubscriptionLocator.getSubscription.callsArgWith(1, null, @subscription) @LimitationsManager.hasGroupMembersLimitReached @user_id, (err, limitReached)-> limitReached.should.equal false done() it "should return true if the limit has been exceded (including members and invites)", (done)-> @subscription.membersLimit = 2 - @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, @subscription) + @SubscriptionLocator.getSubscription.callsArgWith(1, null, @subscription) @LimitationsManager.hasGroupMembersLimitReached @user_id, (err, limitReached)-> limitReached.should.equal true done() @@ -379,4 +380,3 @@ describe "LimitationsManager", -> @V1SubscriptionManager.getSubscriptionsFromV1.calledWith(@user_id).should.equal true result.should.equal false done() - diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionGroupControllerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionGroupControllerTests.coffee index d9ad55080f..5d8cb397c0 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionGroupControllerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionGroupControllerTests.coffee @@ -10,7 +10,7 @@ describe "SubscriptionGroupController", -> beforeEach -> @user = {_id:"!@312431",email:"user@email.com"} @adminUserId = "123jlkj" - @subscription_id = "123434325412" + @subscriptionId = "123434325412" @user_email = "bob@gmail.com" @req = session: @@ -18,15 +18,19 @@ describe "SubscriptionGroupController", -> _id: @adminUserId email:@user_email params: - subscription_id:@subscription_id + subscriptionId:@subscriptionId query:{} - @subscription = {} + @subscription = { + _id: @subscriptionId + } @GroupHandler = addUserToGroup: sinon.stub().callsArgWith(2, null, @user) removeUserFromGroup: sinon.stub().callsArgWith(2) isUserPartOfGroup: sinon.stub() getPopulatedListOfMembers: sinon.stub().callsArgWith(1, null, [@user]) - @SubscriptionLocator = getUsersSubscription: sinon.stub().callsArgWith(1, null, @subscription) + @SubscriptionLocator = + getManagedSubscription: sinon.stub().callsArgWith(1, null, @subscription) + @AuthenticationController = getLoggedInUserId: (req) -> req.session.user._id getSessionUser: (req) -> req.session.user @@ -55,25 +59,25 @@ describe "SubscriptionGroupController", -> describe "addUserToGroup", -> - it "should use the admin id for the logged in user and take the email address from the body", (done)-> + it "should use the subscription id for the logged in user and take the email address from the body", (done)-> newEmail = " boB@gmaiL.com " @req.body = email: newEmail res = json : (data)=> - @GroupHandler.addUserToGroup.calledWith(@adminUserId, "bob@gmail.com").should.equal true + @GroupHandler.addUserToGroup.calledWith(@subscriptionId, "bob@gmail.com").should.equal true data.user.should.deep.equal @user done() @Controller.addUserToGroup @req, res describe "removeUserFromGroup", -> - it "should use the admin id for the logged in user and take the user id from the params", (done)-> + it "should use the subscription id for the logged in user and take the user id from the params", (done)-> userIdToRemove = "31231" @req.params = user_id: userIdToRemove res = send : => - @GroupHandler.removeUserFromGroup.calledWith(@adminUserId, userIdToRemove).should.equal true + @GroupHandler.removeUserFromGroup.calledWith(@subscriptionId, userIdToRemove).should.equal true done() @Controller.removeUserFromGroup @req, res diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee index e5a5695cc1..db8aac0375 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee @@ -81,7 +81,7 @@ describe "SubscriptionGroupHandler", -> beforeEach -> @LimitationsManager.hasGroupMembersLimitReached.callsArgWith(1, null, false, @subscription) @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @user) - + it "should find the user", (done)-> @Handler.addUserToGroup @adminUser_id, @newEmail, (err)=> @UserGetter.getUserByAnyEmail.calledWith(@newEmail).should.equal true @@ -156,13 +156,13 @@ describe "SubscriptionGroupHandler", -> describe "getPopulatedListOfMembers", -> beforeEach -> @subscription = {} - @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, @subscription) + @SubscriptionLocator.getSubscription.callsArgWith(1, null, @subscription) @UserGetter.getUser.callsArgWith(1, null, {_id:"31232"}) it "should locate the subscription", (done)-> @UserGetter.getUser.callsArgWith(1, null, {_id:"31232"}) - @Handler.getPopulatedListOfMembers @adminUser_id, (err, users)=> - @SubscriptionLocator.getUsersSubscription.calledWith(@adminUser_id).should.equal true + @Handler.getPopulatedListOfMembers @subscriptionId, (err, users)=> + @SubscriptionLocator.getSubscription.calledWith(@subscriptionId).should.equal true done() it "should get the users by id", (done)-> diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee index 376d526db0..f88dac383f 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee @@ -18,6 +18,7 @@ describe "SubscriptionUpdater", -> @otherUserId = "5208dd34438842e2db000005" @allUserIds = ["13213", "dsadas", "djsaiud89"] @subscription = subscription = + _id: "111111111111111111111111" admin_id: @adminUser._id member_ids: @allUserIds save: sinon.stub().callsArgWith(0) @@ -26,6 +27,7 @@ describe "SubscriptionUpdater", -> @user_id = @adminuser_id @groupSubscription = + _id: "222222222222222222222222" admin_id: @adminUser._id member_ids: @allUserIds save: sinon.stub().callsArgWith(0) @@ -158,9 +160,9 @@ describe "SubscriptionUpdater", -> @SubscriptionUpdater.addUsersToGroup = sinon.stub().yields(null) it "delegates to addUsersToGroup", (done)-> - @SubscriptionUpdater.addUserToGroup @adminUser._id, @otherUserId, => + @SubscriptionUpdater.addUserToGroup @subscription._id, @otherUserId, => @SubscriptionUpdater.addUsersToGroup - .calledWith(@adminUser._id, [@otherUserId]).should.equal true + .calledWith(@subscription._id, [@otherUserId]).should.equal true done() describe "addUsersToGroup", -> @@ -168,16 +170,16 @@ describe "SubscriptionUpdater", -> @FeaturesUpdater.refreshFeatures = sinon.stub().callsArgWith(1) it "should add the user ids to the group as a set", (done)-> - @SubscriptionUpdater.addUsersToGroup @adminUser._id, [@otherUserId], => + @SubscriptionUpdater.addUsersToGroup @subscription._id, [@otherUserId], => searchOps = - admin_id: @adminUser._id + _id: @subscription._id insertOperation = { $push: { member_ids: { $each: [@otherUserId] } } } @findAndModifyStub.calledWith(searchOps, insertOperation).should.equal true done() it "should update the users features", (done)-> - @SubscriptionUpdater.addUserToGroup @adminUser._id, @otherUserId, => + @SubscriptionUpdater.addUserToGroup @subscription._id, @otherUserId, => @FeaturesUpdater.refreshFeatures.calledWith(@otherUserId).should.equal true done() @@ -186,16 +188,16 @@ describe "SubscriptionUpdater", -> @FeaturesUpdater.refreshFeatures = sinon.stub().callsArgWith(1) it "should pull the users id from the group", (done)-> - @SubscriptionUpdater.removeUserFromGroup @adminUser._id, @otherUserId, => + @SubscriptionUpdater.removeUserFromGroup @subscription._id, @otherUserId, => searchOps = - admin_id:@adminUser._id + _id: @subscription._id removeOperation = "$pull": {member_ids:@otherUserId} @updateStub.calledWith(searchOps, removeOperation).should.equal true done() it "should update the users features", (done)-> - @SubscriptionUpdater.removeUserFromGroup @adminUser._id, @otherUserId, => + @SubscriptionUpdater.removeUserFromGroup @subscription._id, @otherUserId, => @FeaturesUpdater.refreshFeatures.calledWith(@otherUserId).should.equal true done()