From f71a02489e03e22b7dc9c45e969d7ac18aebba72 Mon Sep 17 00:00:00 2001 From: kiran kumar Date: Fri, 5 Dec 2014 21:29:50 +0530 Subject: [PATCH 1/4] Delete a user service changes --- .../codebrag/dao/user/SQLUserDAO.scala | 13 +++++++++++-- .../softwaremill/codebrag/dao/user/UserDAO.scala | 3 +++ .../softwaremill/codebrag/rest/UsersServlet.scala | 6 +++++- .../usecases/user/ModifyUserDetailsUseCase.scala | 4 ++++ .../app/scripts/users/manageUsersPopupCtrl.js | 14 ++++++++++++++ codebrag-ui/app/scripts/users/userMgmtService.js | 7 ++++++- codebrag-ui/app/views/popups/manageUsers.html | 8 ++++++-- 7 files changed, 49 insertions(+), 6 deletions(-) diff --git a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala index aa11fa3e..bf5caff4 100644 --- a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala +++ b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala @@ -31,7 +31,7 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema { } def findById(userId: ObjectId) = findOneWhere(_.id === userId) - + def findByEmail(email: String) = findOneWhere(_.emailLowerCase === email.toLowerCase) def findByLowerCasedLogin(login: String) = db.withTransaction { implicit session => @@ -125,6 +125,8 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema { userQuery.firstOption.map(queryUserAliases).map(untuple) } + + private def queryUserAliases(tuple: (UserTuple, SQLAuth, SQLSettings, SQLLastNotif))(implicit session: Session) = { val userId = tuple._1._1 val aliases = userAliases.where(_.userId === userId).list() @@ -135,4 +137,11 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema { val aliases = userAliases.where(_.userId === tuple._1).list() (tuple._1, tuple._2, tuple._3, tuple._4, UserAliases(aliases.map(_.toUserAlias))) } -} \ No newline at end of file + + def delete(userId: ObjectId) { + db.withTransaction { implicit session => + users.filter(_.id === userId).delete + } + } + +} diff --git a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala index 2f228520..6b2b30b6 100644 --- a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala +++ b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala @@ -40,4 +40,7 @@ trait UserDAO { def countAll(): Long def countAllActive(): Long + + def delete(userId: ObjectId) + } diff --git a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala index 2ad75fa5..c22a1cf5 100644 --- a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala +++ b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala @@ -34,8 +34,12 @@ class UsersServlet( case _ => scalatra.Ok() } } - +delete("/:id") { + haltIfNotAuthenticated() + modifyUserUseCase.delete(new ObjectId(params("id"))) + } } + object UsersServlet { val MappingPath = "users" diff --git a/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala b/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala index fbe9e468..27f29f3b 100644 --- a/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala +++ b/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala @@ -51,4 +51,8 @@ class ModifyUserDetailsUseCase(protected val userDao: UserDAO) extends Logging { validate(checkUserActive, changeOwnFlagsCheck) } + def delete(userId: ObjectId) = { + userDao.delete(userId) + } + } diff --git a/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js b/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js index 747172dc..f688ebff 100644 --- a/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js +++ b/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js @@ -26,6 +26,12 @@ angular.module('codebrag.userMgmt') }); }; + $scope.deleteUser = function(userId) { + $scope.flash.clear(); + var userData = { userId: userId }; + userMgmtService.deleteUser(userId).then(modifySuccess, deleteFailed('active', userId)) + }; + $scope.askForNewPassword = function(user) { $scope.flash.clear(); var modal = popupsService.openSetUserPasswordPopup(user); @@ -37,6 +43,14 @@ angular.module('codebrag.userMgmt') function modifySuccess() { $scope.flash.add('info', 'User details changed'); } + function deleteFailed(flag, userId) { + return function(errorsMap) { + $scope.flash.add('error', 'Could not change user details'); + flattenErrorsMap(errorsMap).forEach(function(error) { + $scope.flash.add('error', error); + }); + } + } function modifyFailed(flag, user) { return function(errorsMap) { diff --git a/codebrag-ui/app/scripts/users/userMgmtService.js b/codebrag-ui/app/scripts/users/userMgmtService.js index 21465f08..0a8e7f07 100644 --- a/codebrag-ui/app/scripts/users/userMgmtService.js +++ b/codebrag-ui/app/scripts/users/userMgmtService.js @@ -19,6 +19,11 @@ angular.module('codebrag.userMgmt') return $http.put(modifyUserUrl, userData).then(null, modifyUserFailed); }; + this.deleteUser = function(userId) { + var modifyUserUrl = [usersApiUrl, '/', userId].join(''); + return $http.delete(modifyUserUrl).then(null, modifyUserFailed); + }; + function modifyUserFailed(response) { return $q.reject(response.data); } @@ -34,4 +39,4 @@ angular.module('codebrag.userMgmt') $modal.open(config) } - }); \ No newline at end of file + }); diff --git a/codebrag-ui/app/views/popups/manageUsers.html b/codebrag-ui/app/views/popups/manageUsers.html index af2df3aa..32d596e1 100644 --- a/codebrag-ui/app/views/popups/manageUsers.html +++ b/codebrag-ui/app/views/popups/manageUsers.html @@ -21,10 +21,13 @@

Manage team members
Admin?
- +
     
+ +
  
+ @@ -32,6 +35,7 @@

Manage team members + @@ -45,4 +49,4 @@

Manage team members - \ No newline at end of file + From a6bd633b95c718c1bc3589f6f9f142e25399ba44 Mon Sep 17 00:00:00 2001 From: kiran kumar Date: Mon, 8 Dec 2014 18:05:21 +0530 Subject: [PATCH 2/4] Added remove icon and refresh the user list after remove --- .../softwaremill/codebrag/dao/user/SQLUserDAO.scala | 4 +--- codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js | 10 ++++++++-- codebrag-ui/app/scripts/users/userMgmtService.js | 4 ++-- codebrag-ui/app/views/popups/manageUsers.html | 2 +- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala index bf5caff4..3d05cd6a 100644 --- a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala +++ b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala @@ -124,9 +124,7 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema { } yield (u, a, s, l) userQuery.firstOption.map(queryUserAliases).map(untuple) } - - - + private def queryUserAliases(tuple: (UserTuple, SQLAuth, SQLSettings, SQLLastNotif))(implicit session: Session) = { val userId = tuple._1._1 val aliases = userAliases.where(_.userId === userId).list() diff --git a/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js b/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js index f688ebff..16d9491e 100644 --- a/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js +++ b/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js @@ -26,10 +26,13 @@ angular.module('codebrag.userMgmt') }); }; - $scope.deleteUser = function(userId) { + $scope.deleteUser = function(userId) { $scope.flash.clear(); var userData = { userId: userId }; - userMgmtService.deleteUser(userId).then(modifySuccess, deleteFailed('active', userId)) + userMgmtService.deleteUser(userId).then(deleteSuccess, deleteFailed('active', userId)) + userMgmtService.loadUsers().then(function(users) { + $scope.users = users; + }); }; $scope.askForNewPassword = function(user) { @@ -39,6 +42,9 @@ angular.module('codebrag.userMgmt') $scope.flash.add('info', 'User password changed'); }); }; + function deleteSuccess() { + $scope.flash.add('error', 'User is removed'); + } function modifySuccess() { $scope.flash.add('info', 'User details changed'); diff --git a/codebrag-ui/app/scripts/users/userMgmtService.js b/codebrag-ui/app/scripts/users/userMgmtService.js index 0a8e7f07..9a6facb4 100644 --- a/codebrag-ui/app/scripts/users/userMgmtService.js +++ b/codebrag-ui/app/scripts/users/userMgmtService.js @@ -20,8 +20,8 @@ angular.module('codebrag.userMgmt') }; this.deleteUser = function(userId) { - var modifyUserUrl = [usersApiUrl, '/', userId].join(''); - return $http.delete(modifyUserUrl).then(null, modifyUserFailed); + var modifyUserUrl = [usersApiUrl, '/', userId].join(''); + return $http.delete(modifyUserUrl).then(null, modifyUserFailed); }; function modifyUserFailed(response) { diff --git a/codebrag-ui/app/views/popups/manageUsers.html b/codebrag-ui/app/views/popups/manageUsers.html index 32d596e1..47a296b8 100644 --- a/codebrag-ui/app/views/popups/manageUsers.html +++ b/codebrag-ui/app/views/popups/manageUsers.html @@ -35,7 +35,7 @@

Manage team members - + From 75b9f3c4a0605873374716c690312a80f179b3ed Mon Sep 17 00:00:00 2001 From: kiran kumar Date: Fri, 5 Dec 2014 21:29:50 +0530 Subject: [PATCH 3/4] Added delete a user service ,Added remove icon and refresh the user list after remove --- .../codebrag/dao/user/SQLUserDAO.scala | 13 +++++++++--- .../codebrag/dao/user/UserDAO.scala | 3 +++ .../codebrag/rest/UsersServlet.scala | 6 +++++- .../user/ModifyUserDetailsUseCase.scala | 4 ++++ .../app/scripts/users/manageUsersPopupCtrl.js | 20 +++++++++++++++++++ .../app/scripts/users/userMgmtService.js | 7 ++++++- codebrag-ui/app/views/popups/manageUsers.html | 8 ++++++-- 7 files changed, 54 insertions(+), 7 deletions(-) diff --git a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala index aa11fa3e..3d05cd6a 100644 --- a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala +++ b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala @@ -31,7 +31,7 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema { } def findById(userId: ObjectId) = findOneWhere(_.id === userId) - + def findByEmail(email: String) = findOneWhere(_.emailLowerCase === email.toLowerCase) def findByLowerCasedLogin(login: String) = db.withTransaction { implicit session => @@ -124,7 +124,7 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema { } yield (u, a, s, l) userQuery.firstOption.map(queryUserAliases).map(untuple) } - + private def queryUserAliases(tuple: (UserTuple, SQLAuth, SQLSettings, SQLLastNotif))(implicit session: Session) = { val userId = tuple._1._1 val aliases = userAliases.where(_.userId === userId).list() @@ -135,4 +135,11 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema { val aliases = userAliases.where(_.userId === tuple._1).list() (tuple._1, tuple._2, tuple._3, tuple._4, UserAliases(aliases.map(_.toUserAlias))) } -} \ No newline at end of file + + def delete(userId: ObjectId) { + db.withTransaction { implicit session => + users.filter(_.id === userId).delete + } + } + +} diff --git a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala index 2f228520..6b2b30b6 100644 --- a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala +++ b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala @@ -40,4 +40,7 @@ trait UserDAO { def countAll(): Long def countAllActive(): Long + + def delete(userId: ObjectId) + } diff --git a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala index 2ad75fa5..c22a1cf5 100644 --- a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala +++ b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala @@ -34,8 +34,12 @@ class UsersServlet( case _ => scalatra.Ok() } } - +delete("/:id") { + haltIfNotAuthenticated() + modifyUserUseCase.delete(new ObjectId(params("id"))) + } } + object UsersServlet { val MappingPath = "users" diff --git a/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala b/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala index fbe9e468..27f29f3b 100644 --- a/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala +++ b/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala @@ -51,4 +51,8 @@ class ModifyUserDetailsUseCase(protected val userDao: UserDAO) extends Logging { validate(checkUserActive, changeOwnFlagsCheck) } + def delete(userId: ObjectId) = { + userDao.delete(userId) + } + } diff --git a/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js b/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js index 747172dc..16d9491e 100644 --- a/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js +++ b/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js @@ -26,6 +26,15 @@ angular.module('codebrag.userMgmt') }); }; + $scope.deleteUser = function(userId) { + $scope.flash.clear(); + var userData = { userId: userId }; + userMgmtService.deleteUser(userId).then(deleteSuccess, deleteFailed('active', userId)) + userMgmtService.loadUsers().then(function(users) { + $scope.users = users; + }); + }; + $scope.askForNewPassword = function(user) { $scope.flash.clear(); var modal = popupsService.openSetUserPasswordPopup(user); @@ -33,10 +42,21 @@ angular.module('codebrag.userMgmt') $scope.flash.add('info', 'User password changed'); }); }; + function deleteSuccess() { + $scope.flash.add('error', 'User is removed'); + } function modifySuccess() { $scope.flash.add('info', 'User details changed'); } + function deleteFailed(flag, userId) { + return function(errorsMap) { + $scope.flash.add('error', 'Could not change user details'); + flattenErrorsMap(errorsMap).forEach(function(error) { + $scope.flash.add('error', error); + }); + } + } function modifyFailed(flag, user) { return function(errorsMap) { diff --git a/codebrag-ui/app/scripts/users/userMgmtService.js b/codebrag-ui/app/scripts/users/userMgmtService.js index 21465f08..9a6facb4 100644 --- a/codebrag-ui/app/scripts/users/userMgmtService.js +++ b/codebrag-ui/app/scripts/users/userMgmtService.js @@ -19,6 +19,11 @@ angular.module('codebrag.userMgmt') return $http.put(modifyUserUrl, userData).then(null, modifyUserFailed); }; + this.deleteUser = function(userId) { + var modifyUserUrl = [usersApiUrl, '/', userId].join(''); + return $http.delete(modifyUserUrl).then(null, modifyUserFailed); + }; + function modifyUserFailed(response) { return $q.reject(response.data); } @@ -34,4 +39,4 @@ angular.module('codebrag.userMgmt') $modal.open(config) } - }); \ No newline at end of file + }); diff --git a/codebrag-ui/app/views/popups/manageUsers.html b/codebrag-ui/app/views/popups/manageUsers.html index af2df3aa..47a296b8 100644 --- a/codebrag-ui/app/views/popups/manageUsers.html +++ b/codebrag-ui/app/views/popups/manageUsers.html @@ -21,10 +21,13 @@

Manage team members
Admin?
- +
     
+ +
  
+ @@ -32,6 +35,7 @@

Manage team members + @@ -45,4 +49,4 @@

Manage team members - \ No newline at end of file + From 4721493eda5029ff1e822294117da73427d8df3d Mon Sep 17 00:00:00 2001 From: kiran kumar Date: Wed, 10 Dec 2014 11:39:43 +0530 Subject: [PATCH 4/4] Added DeleteUserUseCaseSpec and test cases. Enhanced error message on ui --- .../codebrag/dao/user/SQLUserDAO.scala | 2 +- .../codebrag/dao/user/UserDAOSpec.scala | 32 ++++++ .../src/main/scala/ScalatraBootstrap.scala | 2 +- .../com/softwaremill/codebrag/Beans.scala | 1 + .../codebrag/rest/UsersServlet.scala | 13 ++- .../codebrag/rest/UsersServletSpec.scala | 6 +- .../usecases/user/DeleteUserUseCase.scala | 37 +++++++ .../user/ModifyUserDetailsUseCase.scala | 4 - .../usecases/user/DeleteUserUseCaseSpec.scala | 88 +++++++++++++++++ .../app/scripts/users/manageUsersPopupCtrl.js | 18 ++-- .../app/scripts/users/userMgmtService.js | 6 +- codebrag-ui/app/views/popups/manageUsers.html | 99 ++++++++++--------- 12 files changed, 237 insertions(+), 71 deletions(-) create mode 100644 codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCase.scala create mode 100644 codebrag-service/src/test/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCaseSpec.scala diff --git a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala index 3d05cd6a..8491bb0d 100644 --- a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala +++ b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala @@ -141,5 +141,5 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema { users.filter(_.id === userId).delete } } - + } diff --git a/codebrag-dao/src/test/scala/com/softwaremill/codebrag/dao/user/UserDAOSpec.scala b/codebrag-dao/src/test/scala/com/softwaremill/codebrag/dao/user/UserDAOSpec.scala index 016f17d7..6f95914b 100644 --- a/codebrag-dao/src/test/scala/com/softwaremill/codebrag/dao/user/UserDAOSpec.scala +++ b/codebrag-dao/src/test/scala/com/softwaremill/codebrag/dao/user/UserDAOSpec.scala @@ -370,7 +370,38 @@ class SQLUserDAOSpec extends FlatSpecWithSQL with ClearSQLDataAfterTest with Bef foundUser.get.notifications.commits.get.getMillis should equal(commitDate.getMillis) foundUser.get.notifications.followups.get.getMillis should equal(followupDate.getMillis) } + it should "deleta a user " taggedAs RequiresDb in { + // given + val user = UserAssembler.randomUser.withBasicAuth("user", "pass").withAdmin(set = false).withActive(set = false).get + userDAO.add(user) + val newAuth = Authentication.basic(user.authentication.username, "newpass") + // when + val modifiedUser = user.copy(authentication = newAuth, admin = true, active = true) + userDAO.delete(modifiedUser.id) + val deletedUser = userDAO.findById(user.id) + + // then + assert(deletedUser === None , "Deletion was attempted but found " + deletedUser) + } + it should "deleta a user and should not show in the list " taggedAs RequiresDb in { + // given + val user = UserAssembler.randomUser.withBasicAuth("user", "pass").withAdmin(set = false).withActive(set = false).get + userDAO.add(user) + val newAuth = Authentication.basic(user.authentication.username, "newpass") + + // when + val tobeDeletedUser = user.copy(authentication = newAuth, admin = true, active = true) + val userCountBeforeDelete = userDAO.findAll().length + userDAO.delete(tobeDeletedUser.id) + val deletedUser = userDAO.findById(user.id) + val userCountAfterDelete = userDAO.findAll().length + // then + assert(deletedUser === None , "Deletion was attempted but found " + deletedUser) + userCountAfterDelete should be(userCountBeforeDelete -1) + + } + "rememberNotifications" should "store dates properly" taggedAs RequiresDb in { // given val user = UserAssembler.randomUser.get @@ -479,4 +510,5 @@ class SQLUserDAOSpec extends FlatSpecWithSQL with ClearSQLDataAfterTest with Bef partial should have size (2) partial.map(_.name).toSet should be (users.map(_.name).toSet) } + } \ No newline at end of file diff --git a/codebrag-rest/src/main/scala/ScalatraBootstrap.scala b/codebrag-rest/src/main/scala/ScalatraBootstrap.scala index f7c91ef6..d45a3419 100644 --- a/codebrag-rest/src/main/scala/ScalatraBootstrap.scala +++ b/codebrag-rest/src/main/scala/ScalatraBootstrap.scala @@ -69,7 +69,7 @@ class ScalatraBootstrap extends LifeCycle with Logging { RepositoryUpdateScheduler.scheduleUpdates(actorSystem, repositories, commitImportService) context.mount(new RegistrationServlet(registerService, registerNewUserUseCase, listReposAfterRegistration, listRepoBranchesAfterRegistration, watchBranchAfterRegistration, unwatchBranchAfterRegistration), Prefix + RegistrationServlet.MappingPath) context.mount(new SessionServlet(authenticator, loginUserUseCase, userFinder), Prefix + SessionServlet.MappingPath) - context.mount(new UsersServlet(authenticator, userFinder, modifyUserDetailsUseCase, config), Prefix + UsersServlet.MappingPath) + context.mount(new UsersServlet(authenticator, userFinder, modifyUserDetailsUseCase, deleteUserUseCase, config), Prefix + UsersServlet.MappingPath) context.mount(new UserAliasesEndpoint(authenticator, addUserAliasUseCase, deleteUserAliasUseCase), Prefix + UserAliasesEndpoint.MappingPath) context.mount(new UsersSettingsServlet(authenticator, userDao, changeUserSettingsUseCase), Prefix + "users/settings") context.mount(new CommitsServlet(authenticator, toReviewCommitsFinder, allCommitsFinder, reactionFinder, addCommentUseCase, diff --git a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/Beans.scala b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/Beans.scala index e9cf4221..5c2caeec 100644 --- a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/Beans.scala +++ b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/Beans.scala @@ -79,6 +79,7 @@ trait Beans extends ActorSystemSupport with CommitsModule with Daos { lazy val generateInvitationCodeUseCase = new GenerateInvitationCodeUseCase(invitationsService, userDao) lazy val sendInvitationEmailUseCase = new SendInvitationEmailUseCase(invitationsService, userDao) lazy val modifyUserDetailsUseCase = new ModifyUserDetailsUseCase(userDao) + lazy val deleteUserUseCase = new DeleteUserUseCase(userDao) lazy val updateUserBrowsingContextUseCase = new UpdateUserBrowsingContextUseCase(userRepoDetailsDao) lazy val addUserAliasUseCase = new AddUserAliasUseCase(userAliasDao, userDao) lazy val deleteUserAliasUseCase = new DeleteUserAliasUseCase(userAliasDao) diff --git a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala index c22a1cf5..10fe15dc 100644 --- a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala +++ b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala @@ -6,12 +6,13 @@ import org.bson.types.ObjectId import org.scalatra import com.softwaremill.codebrag.finders.user.UserFinder import com.softwaremill.codebrag.finders.user.ManagedUsersListView -import com.softwaremill.codebrag.usecases.user.{RegisterNewUserUseCase, ModifyUserDetailsUseCase, ModifyUserDetailsForm} +import com.softwaremill.codebrag.usecases.user.{RegisterNewUserUseCase, ModifyUserDetailsUseCase, ModifyUserDetailsForm,DeleteUserUseCase,DeleteUserForm} class UsersServlet( val authenticator: Authenticator, userFinder: UserFinder, modifyUserUseCase: ModifyUserDetailsUseCase, + deleteUserUseCase: DeleteUserUseCase, config: CodebragConfig) extends JsonServletWithAuthentication { get("/") { @@ -34,9 +35,13 @@ class UsersServlet( case _ => scalatra.Ok() } } -delete("/:id") { - haltIfNotAuthenticated() - modifyUserUseCase.delete(new ObjectId(params("id"))) +delete("/:userId") { + haltIfNotAuthenticated() + val targetUserId = new ObjectId(params("userId")) + deleteUserUseCase.execute(user.id, DeleteUserForm(targetUserId)) match { + case Left(errors) => scalatra.BadRequest(errors) + case _ => scalatra.Ok() + } } } diff --git a/codebrag-rest/src/test/scala/com/softwaremill/codebrag/rest/UsersServletSpec.scala b/codebrag-rest/src/test/scala/com/softwaremill/codebrag/rest/UsersServletSpec.scala index cebd1c7a..cf7ba27f 100644 --- a/codebrag-rest/src/test/scala/com/softwaremill/codebrag/rest/UsersServletSpec.scala +++ b/codebrag-rest/src/test/scala/com/softwaremill/codebrag/rest/UsersServletSpec.scala @@ -13,11 +13,12 @@ import com.softwaremill.codebrag.finders.user.ManagedUserView import com.softwaremill.codebrag.finders.user.ManagedUsersListView import com.softwaremill.codebrag.domain.builder.UserAssembler import com.softwaremill.codebrag.domain.User -import com.softwaremill.codebrag.usecases.user.ModifyUserDetailsUseCase +import com.softwaremill.codebrag.usecases.user.{ ModifyUserDetailsUseCase, DeleteUserUseCase } class UsersServletSpec extends AuthenticatableServletSpec { val modifyUserUseCase = mock[ModifyUserDetailsUseCase] + val deleteUserUseCase = mock[DeleteUserUseCase] var userFinder: UserFinder = _ var config: CodebragConfig = _ @@ -50,7 +51,6 @@ class UsersServletSpec extends AuthenticatableServletSpec { } } - def configWithDemo(mode: Boolean) = { val p = new Properties() p.setProperty("codebrag.demo", mode.toString) @@ -60,7 +60,7 @@ class UsersServletSpec extends AuthenticatableServletSpec { } class TestableUsersServlet(fakeAuthenticator: Authenticator, fakeScentry: Scentry[User]) - extends UsersServlet(fakeAuthenticator, userFinder, modifyUserUseCase, config) { + extends UsersServlet(fakeAuthenticator, userFinder, modifyUserUseCase, deleteUserUseCase, config) { override def scentry(implicit request: javax.servlet.http.HttpServletRequest) = fakeScentry } diff --git a/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCase.scala b/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCase.scala new file mode 100644 index 00000000..fa0a8bfb --- /dev/null +++ b/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCase.scala @@ -0,0 +1,37 @@ +package com.softwaremill.codebrag.usecases.user + +import com.softwaremill.codebrag.dao.user.UserDAO +import com.softwaremill.codebrag.usecases.assertions.UserAssertions +import org.bson.types.ObjectId +import com.softwaremill.codebrag.domain.{Authentication, User} +import com.typesafe.scalalogging.slf4j.Logging +import com.softwaremill.scalaval.Validation._ + +case class DeleteUserForm(userId: ObjectId) { + +} + +class DeleteUserUseCase(protected val userDao: UserDAO) extends Logging { + + import UserAssertions._ + + def execute(executorId: ObjectId, form: DeleteUserForm): Either[Errors, Unit] = { + assertUserWithId(executorId, mustBeActive, mustBeAdmin)(userDao) + val targetUser = loadUser(form.userId) + validateUserDetails(executorId, targetUser, form).whenOk[Unit] { + logger.debug(s"Validation passed, attempting to delete user $targetUser") + userDao.delete(form.userId) + } + } + + private def loadUser(userId: ObjectId) = userDao.findById(userId).getOrElse(throw new IllegalStateException(s"User $userId not found")) + + private def validateUserDetails(executorId: ObjectId, user: User, form: DeleteUserForm) = { + val changeOwnFlagsCheck = rule("userId") { + val isDeleteFlags = user.admin || user.active + (!isDeleteFlags || (isDeleteFlags && executorId != user.id), "Cannot delete own user") + } + validate(changeOwnFlagsCheck) + } + +} diff --git a/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala b/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala index 27f29f3b..fbe9e468 100644 --- a/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala +++ b/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/ModifyUserDetailsUseCase.scala @@ -51,8 +51,4 @@ class ModifyUserDetailsUseCase(protected val userDao: UserDAO) extends Logging { validate(checkUserActive, changeOwnFlagsCheck) } - def delete(userId: ObjectId) = { - userDao.delete(userId) - } - } diff --git a/codebrag-service/src/test/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCaseSpec.scala b/codebrag-service/src/test/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCaseSpec.scala new file mode 100644 index 00000000..6e44d268 --- /dev/null +++ b/codebrag-service/src/test/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCaseSpec.scala @@ -0,0 +1,88 @@ +package com.softwaremill.codebrag.usecases.user + +import org.scalatest.{BeforeAndAfter, FlatSpec} +import org.scalatest.matchers.ShouldMatchers +import org.scalatest.mock.MockitoSugar +import com.softwaremill.codebrag.dao.user.UserDAO +import org.mockito.Matchers._ +import org.mockito.Mockito._ +import com.softwaremill.codebrag.domain.builder.UserAssembler +import com.softwaremill.codebrag.domain.{Authentication, User} +import com.softwaremill.codebrag.usecases.assertions.{ActiveUserStatusRequiredException, AdminRoleRequiredException} +import org.bson.types.ObjectId + +class DeleteUserUseCaseSpec extends FlatSpec with ShouldMatchers with BeforeAndAfter with MockitoSugar { + + val userDao = mock[UserDAO] + val useCase = new DeleteUserUseCase(userDao) + + val InactiveUser = UserAssembler.randomUser.withActive(set = false).get + val ActiveUser = UserAssembler.randomUser.withActive().get + + val ValidExecutor = UserAssembler.randomUser.withAdmin().withActive().get + val NonAdminExecutor = UserAssembler.randomUser.withActive().withAdmin(set = false).get + val InactiveExecutor = UserAssembler.randomUser.withActive(set = false).withAdmin().get + + after { + reset(userDao) + } + + it should "not delete user when executing user is neither admin nor active" in { + // given + setupReturningUserFromDB(NonAdminExecutor, InactiveExecutor) + val form = DeleteUserForm(ActiveUser.id) + + // when + intercept[AdminRoleRequiredException] { + useCase.execute(NonAdminExecutor.id, form) + } + intercept[ActiveUserStatusRequiredException] { + useCase.execute(InactiveExecutor.id, form) + } + + // then + verify(userDao, never()).delete(any[ObjectId]) + } + + it should "not allow to delete yourself" in { + // given + setupReturningUserFromDB(ValidExecutor) + + // when + val ownChangeForm = DeleteUserForm(ValidExecutor.id) + val Left(result) = useCase.execute(ValidExecutor.id, ownChangeForm) + + // then + result should be(Map("userId" -> List("Cannot delete own user"))) + verify(userDao, never()).delete(any[ObjectId]) + } + + + it should "delete user when validation passes" in { + // given + stubCurrentlyActiveUsersCountTo(0) + setupReturningUserFromDB(ValidExecutor, ActiveUser) + + // when + val newAuth = Authentication.basic(ActiveUser.authentication.username, "secret") + val form = new DeleteUserForm(ActiveUser.id) + val result = useCase.execute(ValidExecutor.id, form) + + // then + result should be('right) + val expectedUser = form + verify(userDao).delete(expectedUser.userId) + } + + private def stubCurrentlyActiveUsersCountTo(activeUsersCount: Int) { + when(userDao.countAllActive()).thenReturn(activeUsersCount) + } + + private def setupReturningUserFromDB(users: User*) { + users.foreach { user => + when(userDao.findById(user.id)).thenReturn(Some(user)) + } + } + +} + \ No newline at end of file diff --git a/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js b/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js index 16d9491e..945a40e4 100644 --- a/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js +++ b/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js @@ -26,13 +26,10 @@ angular.module('codebrag.userMgmt') }); }; - $scope.deleteUser = function(userId) { + $scope.deleteUser = function(user) { $scope.flash.clear(); - var userData = { userId: userId }; - userMgmtService.deleteUser(userId).then(deleteSuccess, deleteFailed('active', userId)) - userMgmtService.loadUsers().then(function(users) { - $scope.users = users; - }); + var userData = { userId: user.userId }; + userMgmtService.deleteUser(userData).then(deleteSuccess(user), deleteFailed('active', user.userId)) }; $scope.askForNewPassword = function(user) { @@ -42,8 +39,11 @@ angular.module('codebrag.userMgmt') $scope.flash.add('info', 'User password changed'); }); }; - function deleteSuccess() { - $scope.flash.add('error', 'User is removed'); + function deleteSuccess(user) { + $scope.flash.add('error', 'User ' + user.email + ' is deleted'); + userMgmtService.loadUsers().then(function(users) { + $scope.users = users; + }); } function modifySuccess() { @@ -51,7 +51,7 @@ angular.module('codebrag.userMgmt') } function deleteFailed(flag, userId) { return function(errorsMap) { - $scope.flash.add('error', 'Could not change user details'); + $scope.flash.add('error', ' Could not delete user '); flattenErrorsMap(errorsMap).forEach(function(error) { $scope.flash.add('error', error); }); diff --git a/codebrag-ui/app/scripts/users/userMgmtService.js b/codebrag-ui/app/scripts/users/userMgmtService.js index 9a6facb4..41b3d211 100644 --- a/codebrag-ui/app/scripts/users/userMgmtService.js +++ b/codebrag-ui/app/scripts/users/userMgmtService.js @@ -19,9 +19,9 @@ angular.module('codebrag.userMgmt') return $http.put(modifyUserUrl, userData).then(null, modifyUserFailed); }; - this.deleteUser = function(userId) { - var modifyUserUrl = [usersApiUrl, '/', userId].join(''); - return $http.delete(modifyUserUrl).then(null, modifyUserFailed); + this.deleteUser = function(userData) { + var deleteUserUrl = [usersApiUrl, '/', userData.userId].join(''); + return $http.delete(deleteUserUrl).then(null, modifyUserFailed); }; function modifyUserFailed(response) { diff --git a/codebrag-ui/app/views/popups/manageUsers.html b/codebrag-ui/app/views/popups/manageUsers.html index 47a296b8..c813660c 100644 --- a/codebrag-ui/app/views/popups/manageUsers.html +++ b/codebrag-ui/app/views/popups/manageUsers.html @@ -1,52 +1,59 @@
-

Manage team members +

+ Manage team members -
-   - {{ f.message }} - {{ f.message }} -
-

- -
-
- - - - - - - - - - - - - - - - - -
-
Team member email
- 		-
Active?
- 		-
Admin?
- 		-
     
- 		-
  
-
{{ user.email}}
-
-
+
+   {{ + f.message }} {{ f.message }} +
+

-
- -
+
+
+ + + + + + + + + + + + + + + + + +
+
Team member email
+ 		+
Active?
+ 		+
Admin?
+ 		+
     
+ 		+
  
+
{{ user.email}}
+
+
- - - +
+ +
+ + +