-
Notifications
You must be signed in to change notification settings - Fork 643
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
FindByKey and FindByUsername should not include deleted accounts. #5864
Conversation
…lude deleted accounts.
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.
Review/address feedback, then LGTM.
@@ -352,17 +352,33 @@ public virtual IList<User> FindByUnconfirmedEmailAddress(string unconfirmedEmail | |||
} | |||
} | |||
|
|||
public virtual User FindByUsername(string username) | |||
public virtual User FindByUsername(string username, bool includeDeleteRecord = false) |
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.
Arg names should match the interface (plural). Also, would rename to simply includeDeleted
as Record
is kind of generic data term that we don't use elsewhere in the gallery.
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.
Updated
return UserRepository.GetAll() | ||
.Where(u => !u.IsDeleted) | ||
.Include(u => u.Roles) | ||
.Include(u => u.Credentials) | ||
.SingleOrDefault(u => u.Username == username); |
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.
nit: can reuse code by wrapping only Where clause in the condition.
var users = UserRepository.GetAll();
if (includeDeleted)
{
users = users..Where(u => u.IsDeleted);
}
return users.Include(...).Include(...).SingleOrDefault(...);
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.
Same for below. Also, username should be case insensitive search to match our DB collation.
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 equality comparison is case insensitive by default due to db collation, why would it matter if username has any casing?
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.
Updated (first comment) , second comment @shishirx34 is correct.
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.
LGTM, agree with @chenriksson's comments.
The default behavior for FindByKey and FindByUsername should not include deleted accounts.