Merge pull request #1076 from sharelatex/ta-ui-error-handling

Management UI Error Handling

GitOrigin-RevId: a0567b5d54af3a8ca31f7e124c0c2d2d8d26b647

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

@@ -12,7 +12,10 @@ module.exports =
 		subscription = req.entity
 		email = EmailHelper.parseEmail(req.body.email)
 		if !email?
-			return res.sendStatus(400)
+			return res.status(422).json error:
+				code: 'invalid_email'
+				message: req.i18n.translate('invalid_email')
+
 
 		TeamInvitesHandler.createInvite teamManagerId, subscription, email, (err, invite) ->
 			return next(err) if err?

services/web/app/coffee/Features/UserMembership/UserMembershipController.coffee

@@ -2,6 +2,7 @@ AuthenticationController = require('../Authentication/AuthenticationController')
 UserMembershipHandler = require('./UserMembershipHandler')
 EntityConfigs = require('./UserMembershipEntityConfigs')
 Errors = require('../Errors/Errors')
+EmailHelper = require("../Helpers/EmailHelper")
 logger = require("logger-sharelatex")
 
 module.exports =
@@ -18,13 +19,25 @@ module.exports =
 
 	add: (req, res, next)->
 		{ entity, entityConfig } = req
-		email = req.body.email
+		email = EmailHelper.parseEmail(req.body.email)
-		return res.sendStatus 422 unless email
+		if !email?
+			return res.status(400).json error:
+				code: 'invalid_email'
+				message: req.i18n.translate('invalid_email')
+
 
 		if entityConfig.readOnly
 			return next(new Errors.NotFoundError("Cannot add users to entity"))
 
 		UserMembershipHandler.addUser entity, entityConfig, email, (error, user)->
+			if error?.alreadyAdded
+				return res.status(400).json error:
+					code: 'user_already_added'
+					message: req.i18n.translate('user_already_added')
+			if error?.userNotFound
+				return res.status(404).json error:
+					code: 'user_not_found'
+					message: req.i18n.translate('user_not_found')
 			return next(error) if error?
 			res.json(user: user)
 
@@ -35,7 +48,17 @@ module.exports =
 		if entityConfig.readOnly
 			return next(new Errors.NotFoundError("Cannot remove users from entity"))
 
+		loggedInUserId = AuthenticationController.getLoggedInUserId(req)
+		if loggedInUserId == userId
+			return res.status(400).json error:
+				code: 'managers_cannot_remove_self'
+				message: req.i18n.translate('managers_cannot_remove_self')
+
 		UserMembershipHandler.removeUser entity, entityConfig, userId, (error, user)->
+			if error?.isAdmin
+				return res.status(400).json error:
+					code: 'managers_cannot_remove_admin'
+					message: req.i18n.translate('managers_cannot_remove_admin')
 			return next(error) if error?
 			res.send()
 

services/web/app/coffee/Features/UserMembership/UserMembershipHandler.coffee

@@ -24,13 +24,19 @@ module.exports =
 	addUser: (entity, entityConfig, email, callback = (error, user) ->) ->
 		attribute = entityConfig.fields.write
 		UserGetter.getUserByAnyEmail email, (error, user) ->
-			error ||= new Errors.NotFoundError("No user found with email #{email}") unless user
 			return callback(error) if error?
+			unless user
+				return callback(userNotFound: true)
+			if entity[attribute].some((managerId) -> managerId.equals(user._id))
+				return callback(alreadyAdded: true)
+
 			addUserToEntity entity, attribute, user, (error) ->
 				callback(error, UserMembershipViewModel.build(user))
 
 	removeUser: (entity, entityConfig, userId, callback = (error) ->) ->
 		attribute = entityConfig.fields.write
+		if entity.admin_id?.equals(userId)
+			return callback(isAdmin: true)
 		removeUserFromEntity entity, attribute, userId, callback
 
 getPopulatedListOfMembers = (entity, attributes, callback = (error, users)->)->

services/web/app/views/user_membership/index.pug

@@ -17,6 +17,12 @@ block content
 							h1 #{translate(translations.title)}
 
 						.row-spaced-small
+							div(ng-if="inputs.removeMembers.error", ng-cloak)
+								div.alert.alert-danger(ng-if="inputs.removeMembers.errorMessage")
+									| #{translate('error')}:
+									| {{ inputs.removeMembers.errorMessage }}
+								div.alert.alert-danger(ng-if="!inputs.removeMembers.errorMessage")
+									| #{translate('generic_something_went_wrong')}
 							ul.list-unstyled.structured-list(
 								select-all-list,
 								ng-cloak
@@ -62,6 +68,12 @@ block content
 						hr
 						div(ng-if="!groupSize || users.length < groupSize", ng-cloak)
 							p.small #{translate("add_more_members")}
+							div(ng-if="inputs.addMembers.error", ng-cloak)
+								div.alert.alert-danger(ng-if="inputs.addMembers.errorMessage")
+									| #{translate('error')}:
+									| {{ inputs.addMembers.errorMessage }}
+								div.alert.alert-danger(ng-if="!inputs.addMembers.errorMessage")
+									| #{translate('generic_something_went_wrong')}
 							form.form
 								.row
 									.col-xs-6
@@ -69,7 +81,7 @@ block content
 											name="email",
 											type="text",
 											placeholder="jane@example.com, joe@example.com",
-											ng-model="inputs.emails",
+											ng-model="inputs.addMembers.content",
 											on-enter="addMembers()"
 										)
 									.col-xs-4

services/web/public/coffee/main/user-membership.coffee

@@ -8,7 +8,13 @@ define [
 		$scope.selectedUsers = []
 
 		$scope.inputs =
-			emails: ""
+			addMembers:
+				content: ''
+				error: false
+				errorMessage: null
+			removeMembers:
+				error: false
+				errorMessage: null
 
 		parseEmails = (emailsString)->
 			regexBySpaceOrComma = /[\s,]+/
@@ -20,7 +26,9 @@ define [
 			return emails
 
 		$scope.addMembers = () ->
-			emails = parseEmails($scope.inputs.emails)
+			$scope.inputs.addMembers.error = false
+			$scope.inputs.addMembers.errorMessage = null
+			emails = parseEmails($scope.inputs.addMembers.content)
 			for email in emails
 				queuedHttp
 					.post(paths.addMember, {
@@ -30,9 +38,15 @@ define [
 					.then (response) ->
 						{ data } = response
 						$scope.users.push data.user if data.user?
-						$scope.inputs.emails = ""
+						$scope.inputs.addMembers.content = ""
+					.catch (response) ->
+						{ data } = response
+						$scope.inputs.addMembers.error = true
+						$scope.inputs.addMembers.errorMessage = data.error?.message
 
 		$scope.removeMembers = () ->
+			$scope.inputs.removeMembers.error = false
+			$scope.inputs.removeMembers.errorMessage = null
 			for user in $scope.selectedUsers
 				do (user) ->
 					if paths.removeInvite and user.invite and !user._id?
@@ -51,7 +65,11 @@ define [
 							index = $scope.users.indexOf(user)
 							return if index == -1
 							$scope.users.splice(index, 1)
-			$scope.selectedUsers = []
+						.catch (response) ->
+							{ data } = response
+							$scope.inputs.removeMembers.error = true
+							$scope.inputs.removeMembers.errorMessage = data.error?.message
+			$scope.updateSelectedUsers
 
 		$scope.updateSelectedUsers = () ->
 			$scope.selectedUsers = $scope.users.filter (user) -> user.selected

services/web/test/unit/coffee/UserMembership/UserMembershipControllerTests.coffee

@@ -27,6 +27,7 @@ describe "UserMembershipController", ->
 
 		@AuthenticationController =
 			getSessionUser: sinon.stub().returns(@user)
+			getLoggedInUserId: sinon.stub().returns(@user._id)
 		@UserMembershipHandler =
 			getEntity: sinon.stub().yields(null, @subscription)
 			getUsers: sinon.stub().yields(null, @users)
@@ -110,6 +111,24 @@ describe "UserMembershipController", ->
 				expect(error).to.be.an.instanceof(Errors.NotFoundError)
 				done()
 
+		it 'handle user already added', (done) ->
+			@UserMembershipHandler.addUser.yields(alreadyAdded: true)
+			@UserMembershipController.add @req, status: () => json: (payload) =>
+				expect(payload.error.code).to.equal 'user_already_added'
+				done()
+
+		it 'handle user not found', (done) ->
+			@UserMembershipHandler.addUser.yields(userNotFound: true)
+			@UserMembershipController.add @req, status: () => json: (payload) =>
+				expect(payload.error.code).to.equal 'user_not_found'
+				done()
+
+		it 'handle invalid email', (done) ->
+			@req.body.email = 'not_valid_email'
+			@UserMembershipController.add @req, status: () => json: (payload) =>
+				expect(payload.error.code).to.equal 'invalid_email'
+				done()
+
 	describe 'remove', ->
 		beforeEach ->
 			@req.params.userId = @newUser._id
@@ -133,6 +152,18 @@ describe "UserMembershipController", ->
 				expect(error).to.be.an.instanceof(Errors.NotFoundError)
 				done()
 
+		it 'prevent self removal', (done) ->
+			@req.params.userId = @user._id
+			@UserMembershipController.remove @req, status: () => json: (payload) =>
+				expect(payload.error.code).to.equal 'managers_cannot_remove_self'
+				done()
+
+		it 'prevent admin removal', (done) ->
+			@UserMembershipHandler.removeUser.yields(isAdmin: true)
+			@UserMembershipController.remove @req, status: () => json: (payload) =>
+				expect(payload.error.code).to.equal 'managers_cannot_remove_admin'
+				done()
+
 	describe "exportCsv", ->
 
 		beforeEach ->

services/web/test/unit/coffee/UserMembership/UserMembershipHandlerTests.coffee

@@ -12,8 +12,8 @@ EntityConfigs = require("../../../../app/js/Features/UserMembership/UserMembersh
 
 describe 'UserMembershipHandler', ->
 	beforeEach ->
-		@user = _id: 'mock-user-id'
+		@user = _id: ObjectId()
-		@newUser = _id: 'mock-new-user-id', email: 'new-user-email@foo.bar'
+		@newUser = _id: ObjectId(), email: 'new-user-email@foo.bar'
 		@fakeEntityId = ObjectId()
 		@subscription =
 			_id: 'mock-subscription-id'
@@ -133,7 +133,14 @@ describe 'UserMembershipHandler', ->
 				@UserGetter.getUserByAnyEmail.yields(null, null)
 				@UserMembershipHandler.addUser @institution, EntityConfigs.institution, @email, (error) =>
 					expect(error).to.exist
-					expect(error).to.be.an.instanceof(Errors.NotFoundError)
+					expect(error.userNotFound).to.equal true
+					done()
+
+			it 'handle user already added', (done) ->
+				@institution.managerIds.push(@newUser._id)
+				@UserMembershipHandler.addUser @institution, EntityConfigs.institution, @email, (error, users) =>
+					expect(error).to.exist
+					expect(error.alreadyAdded).to.equal true
 					done()
 
 			it 'add user to institution', (done) ->
@@ -153,3 +160,10 @@ describe 'UserMembershipHandler', ->
 					lastCall = @institution.update.lastCall
 					assertCalledWith(@institution.update, { $pull: managerIds: @newUser._id })
 					done()
+
+			it 'handle admin', (done) ->
+				@subscription.admin_id = @newUser._id
+				@UserMembershipHandler.removeUser @subscription, EntityConfigs.groupManagers, @newUser._id, (error, user) =>
+					expect(error).to.exist
+					expect(error.isAdmin).to.equal true
+					done()