Skip to content
This repository has been archived by the owner on Apr 27, 2023. It is now read-only.

Added delete a user service ,Added remove icon and refresh the user list #13

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gmkumar2005
Copy link

To remove a user #11
Added a delete icon in manage team members. Admin user cannot delete himself.

image

@@ -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

@@ -26,6 +26,15 @@ 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

@lukaszlenart
Copy link
Member

Do we really want to allow remove users?

@tdziurko
Copy link
Member

tdziurko commented Dec 9, 2014

I am still unsure about it. Maybe we should allow only users created by mistake (without follow-ups, etc.) or only those "just" created (within specified time frame)?

Removing users that were active and have some user related data (comments, follow-ups, etc) seems as not the best idea, instead I would prefer "deactivate" action.

@gmkumar2005
Copy link
Author

I wont need a delete if we can resolve below

  1. When the author is matched with user id it is case sensitive. I happen to create user with lower case. Since the author name is in proper case , he is not able to get any followups
  • We should allow change userid
  1. I happen to give wrong email alias to a wrong author
    Simple option seems to be allow to delete the user and create again
    Other option is allow changing the username and alias
  • We should allow remove an aliase
  • What happens when two users want to have the same alias ?

Request your thoughts

@gmkumar2005
Copy link
Author

This pull request has a strange behavior. Not sure how to fix. Also not sure if this could occur in other parts of the application.
After the user is deleted, view is refreshed with updated list of users. This list is not updated in the immediate request. So effectively the user still appears on the manage users popup. The proper list appears in subsequent operations like page refresh, delete another user,

I have verified that /rest/users is called immediately after delete service. Can conclude UI layer is proper
I have added a test case "delete a user and should not show in the list" in dao layer to simulate. But not able to simulate. Can conclude DAO layer is proper
Checked my proxy configurations and cache settings. Seems clean.

What else could be the issues. Any hint will be of great help
Thanks

@jeffjensen
Copy link

Please pull this feature, it's useful.

@mostr
Copy link
Contributor

mostr commented Feb 9, 2015

@gmkumar2005 I'm reviewing this one and cannot see what happens to user's followups when one gets deleted? I guess they should be removed (followups, not comments/likes).

@gmkumar2005
Copy link
Author

This PR just removes a record from user tables. I think followups should remain for below reasons 1) Followups contain valuable conversations and design ideas 2) If the user was deleted by mistake he can be created again and his work is also recovered.
If we allow to delete all the followups and conversations there should be a way to recover

@mostr
Copy link
Contributor

mostr commented Feb 9, 2015

Nope, followups itself (as objects) are just mappings between user and comment/like. There is no point keeping them around when user with given ID gets removed. Even when you recreate this user he/she will get completely new ID and "old" followups will not be reassigned, in fact they will be "orphaned" when user gets removed.

This is not to remove "reactions" (comments/likes), but only followups which are just notifications about reactions

@lukaszlenart
Copy link
Member

So maybe instead removing a user, it should be turn into inactive state? In such situation there be no need to drop any other data.

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
Copy link

Choose a reason for hiding this comment

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

Typo -> 'delete'

@lukaszlenart
Copy link
Member

grey bar

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants