Make getting a user by the username case insensitive
What does this MR do?
This merge request makes sure that when creating a new user object searching by username, it does so in a case sensitive manor.
What are the relevant issue numbers?
Closes #38304 (closed)
Closes #27229 (closed)
Possibly solves #47601 (moved)
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated(After searching, nothing in the docs suggest usernames are sensitive or insensitive), -
Tests added for this feature/bug -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides
Merge request reports
Activity
marked the checklist item Changelog entry added, if necessary as completed
added 1 commit
- a177cf55 - Update users_finder_spec.rb to include case insensitive user search
marked the checklist item Tests added for this feature/bug as completed
- Resolved by William George
- Resolved by William George
added 1 commit
- 34bc613c - Update `User.find_by(username:` to `User.find_by_username(` as the latter is a…
mentioned in issue #47601 (moved)
/cc @toon
assigned to @toon
added Community contribution label
mentioned in issue #38304 (closed)
Per https://docs.gitlab.com/ee/development/reusing_abstractions.html#abstractions there are a few changes that would have to be made. Specifically, we basically need to add a finder instead of using
User.find_by_username
directly in a bunch of places.Based on https://gitlab.com/gitlab-org/gitlab-ce/issues/38304#note_101421351 and the changes in this MR, I'm thinking we could do the following:
- We add a new finder that supports retrieving users via a username, or ID. The initialize argument takes a String that is either the ID, or a username. This finder has three 4 methods:
-
execute
, which does theif
check that's currently used in the API helpers, delegating to the below two methods -
find_by_id
, which just ends up callingUser.find_by_id(...)
. This is a wee bit redundant in its current state, but again it allows us to make changes in the future without having to update 1500 occurrences ofUser.find_by_id
. -
find_by_username
, which just ends up callingUser.find_by_username(...)
-
find_by_username!
, which does the same asfind_by_username
except it will raise if a row is not found. We can then use this method in SnippetsController, removing the need forreturn render_404 ...
. Seeapp/finders/autocomplete/project_finder.rb
for an example on how to do this.
-
- We replace occurrences of
User.find_by_username(...)
withThisNewUserFinder.new(...).find_by_username
. This is a bit more typing, but it allows us to keep the logic used for finding the username (case insensitive vs case sensitive, perhaps additional future options, etc) in one place - The
find_user
method in the API helpers is changed so it simply doesThisNewUserFinder.new(id).execute
- We add a new finder that supports retrieving users via a username, or ID. The initialize argument takes a String that is either the ID, or a username. This finder has three 4 methods:
@awgeorge1 I hope you can continue with that information? If not let me know. Or when you're done with it, I'll be happy to review.
assigned to @awgeorge1
added Manage [DEPRECATED] label
added 1 commit
- 4737865c - Update UserFinder to accept `username` or `id`
Crap, just read @yorickpeterse's last paragraph on the linked comment
Before reading the comment, I choose to add it to the existing UserFinder class, as it was only called in one place, and the fact that the new
UserFinder
I would have made includes the same ID look up functionally I thought it made sense.Failing that - I can change it back, and have a
CaseInsensitveUserFinder
class? Your call.Sorry
added 1 commit
- 25ec327a - Update UserFinder to accept `username` or `id`
@awgeorge1 I thought a bit more about this. Since this use case is so simple, I think we can just put it in
UserFinder
like we did now. This also has the added benefit that we keep theparams
Hash out of the finder, which I always disliked.@yorickpeterse, awesome, so we should be good, as long as I can fix the failing tests?
- Resolved by William George
- Resolved by William George
@awgeorge1 Yes.
@yorickpeterse - Tests passing, thanks for your help.