Added DeleteUserUseCaseSpec and test cases. Enhanced error message on ui

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
     }
   }
-
+  
 }

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)
   }
+
 }

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,

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)

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()
+    }
   }
 }
 

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 = _
 
@@ -60,7 +61,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
   }
 

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)
+  }
+
+}

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)
-  }
-
 }

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))
+    }
+  }
+
+}
+

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,16 +39,19 @@ 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() {
             $scope.flash.add('info', 'User details changed');
         }
 	 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);
                 });

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) {