Skip to content
This repository was archived by the owner on Apr 27, 2023. It is now read-only.
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema {
}

def findById(userId: ObjectId) = findOneWhere(_.id === userId)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this empty line

def findByEmail(email: String) = findOneWhere(_.emailLowerCase === email.toLowerCase)

def findByLowerCasedLogin(login: String) = db.withTransaction { implicit session =>
Expand Down Expand Up @@ -124,7 +124,7 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema {
} yield (u, a, s, l)
userQuery.firstOption.map(queryUserAliases).map(untuple)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this empty line

private def queryUserAliases(tuple: (UserTuple, SQLAuth, SQLSettings, SQLLastNotif))(implicit session: Session) = {
val userId = tuple._1._1
val aliases = userAliases.where(_.userId === userId).list()
Expand All @@ -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)))
}
}

def delete(userId: ObjectId) {
db.withTransaction { implicit session =>
users.filter(_.id === userId).delete
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,7 @@ trait UserDAO {
def countAll(): Long

def countAllActive(): Long

def delete(userId: ObjectId)

}
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,12 @@ class UsersServlet(
case _ => scalatra.Ok()
}
}

delete("/:id") {
haltIfNotAuthenticated()
modifyUserUseCase.delete(new ObjectId(params("id")))
}
}


object UsersServlet {
val MappingPath = "users"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,8 @@ class ModifyUserDetailsUseCase(protected val userDao: UserDAO) extends Logging {
validate(checkUserActive, changeOwnFlagsCheck)
}

def delete(userId: ObjectId) = {
userDao.delete(userId)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct indentation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This delete function should not belong to ModifyUserDetailsUseCase. By design use cases are single-purpose objects. This one is modifying user details, not removing user. delete should have its own use case class with its own validation (if any etc.). I know it may look like overkill, but it helps keeping things consistent and easy to reason about.
Now it's like: "oh I see, this use case modifies user (adds admin rights etc), but why the hell it also removes one?"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing out, I will create a new use case for delete. It is a good place to add the validations like use cannot delete himself and admin cannot be deleted.


}
20 changes: 20 additions & 0 deletions codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,37 @@ angular.module('codebrag.userMgmt')
});
};

$scope.deleteUser = function(userId) {
$scope.flash.clear();
var userData = { userId: userId };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is not used

userMgmtService.deleteUser(userId).then(deleteSuccess, deleteFailed('active', userId))
userMgmtService.loadUsers().then(function(users) {
$scope.users = users;
});
};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please unident the whole section

$scope.askForNewPassword = function(user) {
$scope.flash.clear();
var modal = popupsService.openSetUserPasswordPopup(user);
modal.result.then(function() {
$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) {
Expand Down
7 changes: 6 additions & 1 deletion codebrag-ui/app/scripts/users/userMgmtService.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please un-indent both lines

};

function modifyUserFailed(response) {
return $q.reject(response.data);
}
Expand All @@ -34,4 +39,4 @@ angular.module('codebrag.userMgmt')
$modal.open(config)
}

});
});
8 changes: 6 additions & 2 deletions codebrag-ui/app/views/popups/manageUsers.html
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,21 @@ <h1>Manage team members
</th>
<th class="cell-centered">
<div class="thead">Admin?</div>
</th>
</th>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

???

<th>
<div class="thead">&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</div>
</th>
<th class="cell-centered">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add proper indent here

<div class="thead">&nbsp;&nbsp;</div>
</th>
</thead>
<tbody>
<tr ng-repeat="user in users">
<td class="user-email">{{ user.email}}</td>
<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>
</tr>
</tbody>
</table>
Expand All @@ -45,4 +49,4 @@ <h1>Manage team members
<span class="close-btn" ng-click="$close()">
<i class="icon-remove"></i>
</span>
</div>
</div>