-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed #15005 - Improvements on user merge #15016
Conversation
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
PR Summary
|
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks amazingly thorough - and thanks, also for the new Console script test! This is the type of thing that gives me confidence that we can make changes to this functionality and know that it's very unlikely to break. Nice work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests! 😄
Two things that should probably be changed and then a ton of nitpicks 😬
Other than that this looks good 😄
}); | ||
} | ||
|
||
public function logUserUpdate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: this would read better in the past tense like the other methods you added. Maybe userUpdated()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually disagree on this. Since it's in the ActionLog factory, I sorta feel like it's more explicit this way, but it's not a hill I'll die on.
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Okay, this should tweak the way the merging works in both the UI and the merge user script to do the following:
The cli and the UI do slightly different things. The UI lets you select two or more users and select which one you want to merge the other(s) into. This is not so much a bulk thing, since if you accidentally duped 10k users, that would take a while, but it's handy for smaller numbers of mistakes, like if a person's name changed in SCIM/LDAP but it wasn't changed in Snipe-IT, so it ended up creating a new user.
The cli version is more for people who messed up their LDAP setup by not renaming all of their users to have usernames that are an email address before initiating LDAP or SCIM, thus duplicating their users. Sometimes people didn't realize that even happened, so both versions of the user could have things checked out to them, so we have to merge the two into one and delete the other.
That artisan command aims to look for all users that don't have an email formatted like an email address and tries to find the user that does have a user formatted as an email address to merge into.
This should also fix #15005