Enhanced error messages and unit test cases

Author: gmkumar2005

Diff for: 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 for: 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 for: 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 for: 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
   }
 

Diff for: codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCase.scala

@@ -7,24 +7,11 @@ import com.softwaremill.codebrag.domain.{Authentication, User}
 import com.typesafe.scalalogging.slf4j.Logging
 import com.softwaremill.scalaval.Validation._
 
-case class DeleteUserForm(userId: ObjectId, newPassword: Option[String], admin: Option[Boolean], active: Option[Boolean]) {
-
-  def applyTo(user: User) = {
-    val modifiedAuth = newPassword.map { newPass =>
-      buildNewAuth(user.authentication.username, newPass)
-    } getOrElse user.authentication
-    user.copy(
-      admin = admin.getOrElse(user.admin),
-      active = active.getOrElse(user.active),
-      authentication = modifiedAuth
-    )
-  }
-
-  protected def buildNewAuth(username: String, password: String) = Authentication.basic(username, password)
+case class DeleteUserForm(userId: ObjectId) {
 
 }
 
-class DeleteUserDetailsUseCase(protected val userDao: UserDAO) extends Logging {
+class DeleteUserUseCase(protected val userDao: UserDAO) extends Logging {
 
   import UserAssertions._
 
@@ -41,7 +28,7 @@ class DeleteUserDetailsUseCase(protected val userDao: UserDAO) extends Logging {
 
   private def validateUserDetails(executorId: ObjectId, user: User, form: DeleteUserForm) = {   
     val changeOwnFlagsCheck = rule("userId") {
-      val isDeleteFlags = form.admin.isDefined || form.active.isDefined
+      val isDeleteFlags = user.admin || user.active
       (!isDeleteFlags || (isDeleteFlags && executorId != user.id), "Cannot delete own user")
     }
     validate(changeOwnFlagsCheck)

Diff for: codebrag-service/src/test/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCaseSpec.scala

@@ -14,7 +14,7 @@ import org.bson.types.ObjectId
 class DeleteUserUseCaseSpec extends FlatSpec with ShouldMatchers with BeforeAndAfter with MockitoSugar {
 
   val userDao = mock[UserDAO]
-  val useCase = new DeleteUserDetailsUseCase(userDao)
+  val useCase = new DeleteUserUseCase(userDao)
 
   val InactiveUser = UserAssembler.randomUser.withActive(set = false).get
   val ActiveUser = UserAssembler.randomUser.withActive().get
@@ -30,7 +30,7 @@ class DeleteUserUseCaseSpec extends FlatSpec with ShouldMatchers with BeforeAndA
   it should "not delete  user when executing user is neither admin nor active" in {
     // given
     setupReturningUserFromDB(NonAdminExecutor, InactiveExecutor)
-    val form = DeleteUserForm(ActiveUser.id, None, None, None)
+    val form = DeleteUserForm(ActiveUser.id)
 
     // when
     intercept[AdminRoleRequiredException] {
@@ -49,7 +49,7 @@ class DeleteUserUseCaseSpec extends FlatSpec with ShouldMatchers with BeforeAndA
     setupReturningUserFromDB(ValidExecutor)
 
     // when
-    val ownChangeForm = DeleteUserForm(ValidExecutor.id, None, admin = Some(false), active = Some(false))
+    val ownChangeForm = DeleteUserForm(ValidExecutor.id)
     val Left(result) = useCase.execute(ValidExecutor.id, ownChangeForm)
 
     // then
@@ -65,15 +65,13 @@ class DeleteUserUseCaseSpec extends FlatSpec with ShouldMatchers with BeforeAndA
 
     // when
     val newAuth = Authentication.basic(ActiveUser.authentication.username, "secret")
-    val form = new DeleteUserForm(ActiveUser.id, newPassword = Some("secret"), admin = Some(true), active = None) {
-      override def buildNewAuth(username: String, password: String) = newAuth
-    }
+    val form = new DeleteUserForm(ActiveUser.id)
     val result = useCase.execute(ValidExecutor.id, form)
 
     // then
     result should be('right)
-    val expectedUser = form.applyTo(ActiveUser)
-    verify(userDao).delete(expectedUser.id)
+    val expectedUser = form
+    verify(userDao).delete(expectedUser.userId)
   }
 
   private def stubCurrentlyActiveUsersCountTo(activeUsersCount: Int) {
@@ -87,3 +85,4 @@ class DeleteUserUseCaseSpec extends FlatSpec with ShouldMatchers with BeforeAndA
   }
 
 }
+  

Diff for: 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);
                 });

Diff for: 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 for: codebrag-ui/app/views/popups/manageUsers.html

@@ -35,7 +35,7 @@ <h1>Manage team members
                         <td class="cell-centered"><input type="checkbox" ng-model="user.active" ng-click="modifyUser(user, 'active')" ng-disabled="user.locked"/></td>
                         <td class="cell-centered"><input type="checkbox" ng-model="user.admin" ng-click="modifyUser(user, 'admin')" ng-disabled="user.locked"/></td>
                         <td><button class="link" ng-disabled="!user.active" ng-click="askForNewPassword(user)">Set&nbsp;password</button></td>
-						<td><i class="icon-remove" ng-hide="user.admin" ng-disabled="user.locked" ng-click="deleteUser(user.userId)"></i></td>						
+						<td><i class="icon-remove" ng-hide="user.admin" ng-disabled="user.locked" ng-click="deleteUser(user)"></i></td>						
                     </tr>
                 </tbody>
             </table>