From 8cd6e7ccfc3edc84745096bd06d0ee1746afc2f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Wed, 12 Aug 2020 16:17:15 +0200 Subject: [PATCH] Merge pull request #3064 from overleaf/ta-manage-multiple-groups Allow Users to Manage Multiple Groups GitOrigin-RevId: c918da0114cdd1d679223f69d81159b4c2608874 --- .../SubscriptionGroupController.js | 39 +++---------------- .../Subscription/SubscriptionLocator.js | 7 ---- .../Subscription/SubscriptionRouter.js | 5 --- .../dashboard/_group_memberships.pug | 2 +- .../subscriptions/successful_subscription.pug | 2 +- .../js/main/subscription-dashboard.js | 9 +++-- .../web/test/acceptance/src/ModelTests.js | 21 +--------- .../SubscriptionGroupControllerTests.js | 29 ++++++++++++-- .../Subscription/SubscriptionLocatorTests.js | 16 -------- 9 files changed, 41 insertions(+), 89 deletions(-) diff --git a/services/web/app/src/Features/Subscription/SubscriptionGroupController.js b/services/web/app/src/Features/Subscription/SubscriptionGroupController.js index ee31bdfff8..201488faf6 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionGroupController.js +++ b/services/web/app/src/Features/Subscription/SubscriptionGroupController.js @@ -43,9 +43,12 @@ module.exports = { }, removeSelfFromGroup(req, res, next) { - const adminUserId = req.query.admin_user_id + const subscriptionId = req.query.subscriptionId const userToRemove_id = AuthenticationController.getLoggedInUserId(req) - return getManagedSubscription(adminUserId, function(error, subscription) { + return SubscriptionLocator.getSubscription(subscriptionId, function( + error, + subscription + ) { if (error != null) { return next(error) } @@ -56,7 +59,7 @@ module.exports = { function(err) { if (err != null) { logger.err( - { err, userToRemove_id, adminUserId }, + { err, userToRemove_id, subscriptionId }, 'error removing self from group' ) return res.sendStatus(500) @@ -65,35 +68,5 @@ module.exports = { } ) }) - }, - - // legacy route - redirectToSubscriptionGroupAdminPage(req, res, next) { - const user_id = AuthenticationController.getLoggedInUserId(req) - return getManagedSubscription(user_id, function(error, subscription) { - if (error != null) { - return next(error) - } - if (!(subscription != null ? subscription.groupPlan : undefined)) { - return res.redirect('/user/subscription') - } - return res.redirect(`/manage/groups/${subscription._id}/members`) - }) } } - -var getManagedSubscription = (managerId, callback) => - SubscriptionLocator.findManagedSubscription(managerId, function( - err, - subscription - ) { - if (err) { - return callback(err) - } else if (!subscription) { - return callback( - new Error(`No subscription found managed by user ${managerId}`) - ) - } - - return callback(null, subscription) - }) diff --git a/services/web/app/src/Features/Subscription/SubscriptionLocator.js b/services/web/app/src/Features/Subscription/SubscriptionLocator.js index 085a78ff45..4e22794ec8 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionLocator.js +++ b/services/web/app/src/Features/Subscription/SubscriptionLocator.js @@ -42,10 +42,6 @@ const SubscriptionLocator = { ) }, - findManagedSubscription(managerId, callback) { - return Subscription.findOne({ manager_ids: managerId }, callback) - }, - getManagedGroupSubscriptions(user_or_id, callback) { if (callback == null) { callback = function(error, managedSubscriptions) {} @@ -121,9 +117,6 @@ SubscriptionLocator.promises = { getUserIndividualSubscription: promisify( SubscriptionLocator.getUserIndividualSubscription ), - findManagedSubscription: promisify( - SubscriptionLocator.findManagedSubscription - ), getManagedGroupSubscriptions: promisify( SubscriptionLocator.getManagedGroupSubscriptions ), diff --git a/services/web/app/src/Features/Subscription/SubscriptionRouter.js b/services/web/app/src/Features/Subscription/SubscriptionRouter.js index f4ea226071..848335ceca 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionRouter.js +++ b/services/web/app/src/Features/Subscription/SubscriptionRouter.js @@ -47,11 +47,6 @@ module.exports = { SubscriptionController.canceledSubscription ) - webRouter.get( - '/subscription/group', - AuthenticationController.requireLogin(), - SubscriptionGroupController.redirectToSubscriptionGroupAdminPage - ) webRouter.delete( '/subscription/group/user', AuthenticationController.requireLogin(), diff --git a/services/web/app/views/subscriptions/dashboard/_group_memberships.pug b/services/web/app/views/subscriptions/dashboard/_group_memberships.pug index 5c86fefbd7..197a2ed68c 100644 --- a/services/web/app/views/subscriptions/dashboard/_group_memberships.pug +++ b/services/web/app/views/subscriptions/dashboard/_group_memberships.pug @@ -11,7 +11,7 @@ div(ng-controller="GroupMembershipController") //- Team notice is sanitized in SubscriptionViewModelBuilder em(ng-non-bindable) !{groupSubscription.teamNotice} span - button.btn.btn-danger.text-capitalise(ng-click="removeSelfFromGroup('"+groupSubscription.admin_id._id+"')") #{translate("leave_group")} + button.btn.btn-danger.text-capitalise(ng-click="removeSelfFromGroup('"+groupSubscription._id+"')") #{translate("leave_group")} hr script(type='text/ng-template', id='LeaveGroupModalTemplate') diff --git a/services/web/app/views/subscriptions/successful_subscription.pug b/services/web/app/views/subscriptions/successful_subscription.pug index 558e630454..03ab8cecf2 100644 --- a/services/web/app/views/subscriptions/successful_subscription.pug +++ b/services/web/app/views/subscriptions/successful_subscription.pug @@ -17,7 +17,7 @@ block content a(href="/user/subscription") #{translate("manage_subscription")}. p - if (personalSubscription.groupPlan == true) - a.btn.btn-success.btn-large(href="/subscription/group") #{translate("add_your_first_group_member_now")} + a.btn.btn-success.btn-large(href=`/manage/groups/${personalSubscription._id}/members`) #{translate("add_your_first_group_member_now")} p.letter-from-founders p #{translate("thanks_for_subscribing_you_help_sl", {planName:personalSubscription.plan.name})} p #{translate("need_anything_contact_us_at")} diff --git a/services/web/frontend/js/main/subscription-dashboard.js b/services/web/frontend/js/main/subscription-dashboard.js index f43503dbc9..0f1b1b5543 100644 --- a/services/web/frontend/js/main/subscription-dashboard.js +++ b/services/web/frontend/js/main/subscription-dashboard.js @@ -148,7 +148,10 @@ App.controller('LeaveGroupModalController', function( return $http({ url: '/subscription/group/user', method: 'DELETE', - params: { admin_user_id: $scope.admin_id, _csrf: window.csrfToken } + params: { + subscriptionId: $scope.subscriptionId, + _csrf: window.csrfToken + } }) .then(() => location.reload()) .catch(() => console.log('something went wrong changing plan')) @@ -158,8 +161,8 @@ App.controller('LeaveGroupModalController', function( }) App.controller('GroupMembershipController', function($scope, $modal) { - $scope.removeSelfFromGroup = function(admin_id) { - $scope.admin_id = admin_id + $scope.removeSelfFromGroup = function(subscriptionId) { + $scope.subscriptionId = subscriptionId return $modal.open({ templateUrl: 'LeaveGroupModalTemplate', controller: 'LeaveGroupModalController', diff --git a/services/web/test/acceptance/src/ModelTests.js b/services/web/test/acceptance/src/ModelTests.js index 162e93c0b4..f625182da7 100644 --- a/services/web/test/acceptance/src/ModelTests.js +++ b/services/web/test/acceptance/src/ModelTests.js @@ -23,11 +23,10 @@ describe('mongoose', function() { }) describe('Subsription', function() { - let user, otherUser + let user beforeEach(async function() { user = await User.create({ email: 'wombat@potato.net' }) - otherUser = await User.create({ email: 'giraffe@turnip.org' }) }) it('allows the creation of a subscription', async function() { @@ -41,23 +40,5 @@ describe('mongoose', function() { it('does not allow the creation of a subscription without a manager', async function() { await expect(Subscription.create({ admin_id: user._id })).to.be.rejected }) - - it('does not allow a user to manage more than one group', async function() { - await expect( - Subscription.create({ admin_id: user._id, manager_ids: [user._id] }) - ).to.be.fulfilled - await expect( - Subscription.create({ - admin_id: otherUser._id, - manager_ids: [otherUser._id] - }) - ).to.be.fulfilled - await expect( - Subscription.update( - { admin_id: user._id }, - { $push: { manager_ids: otherUser._id } } - ) - ).to.be.rejected - }) }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js b/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js index 3832e066a3..72284f023b 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js @@ -44,9 +44,7 @@ describe('SubscriptionGroupController', function() { this.GroupHandler = { removeUserFromGroup: sinon.stub().callsArgWith(2) } this.SubscriptionLocator = { - findManagedSubscription: sinon - .stub() - .callsArgWith(1, null, this.subscription) + getSubscription: sinon.stub().callsArgWith(1, null, this.subscription) } this.AuthenticationController = { @@ -91,4 +89,29 @@ describe('SubscriptionGroupController', function() { return this.Controller.removeUserFromGroup(this.req, res) }) }) + + describe('removeSelfFromGroup', function() { + it('gets subscription and remove user', function(done) { + const userIdToRemove = '31231' + this.req.query = { subscriptionId: this.subscriptionId } + const memberUserIdToremove = 123456789 + this.req.session.user._id = memberUserIdToremove + + const res = { + sendStatus: () => { + sinon.assert.calledWith( + this.SubscriptionLocator.getSubscription, + this.subscriptionId + ) + sinon.assert.calledWith( + this.GroupHandler.removeUserFromGroup, + this.subscriptionId, + memberUserIdToremove + ) + return done() + } + } + return this.Controller.removeSelfFromGroup(this.req, res) + }) + }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionLocatorTests.js b/services/web/test/unit/src/Subscription/SubscriptionLocatorTests.js index fab49e7671..7b646123e1 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionLocatorTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionLocatorTests.js @@ -88,21 +88,5 @@ describe('Subscription Locator Tests', function() { } ) }) - - describe('finding managed subscription', function() { - it('should query the database', function(done) { - this.Subscription.findOne.callsArgWith(1, null, this.subscription) - return this.SubscriptionLocator.findManagedSubscription( - this.user._id, - (err, subscription) => { - this.Subscription.findOne - .calledWith({ manager_ids: this.user._id }) - .should.equal(true) - subscription.should.equal(this.subscription) - return done() - } - ) - }) - }) }) })