Skip to content
Snippets Groups Projects

Make getting a user by the username case insensitive

Merged William George requested to merge awgeorge1/gitlab-ce:master into master

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?

Edited by William George

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • William George resolved all discussions

    resolved all discussions

  • William George added 1 commit

    added 1 commit

    • 34bc613c - Update `User.find_by(username:` to `User.find_by_username(` as the latter is a…

    Compare with previous version

  • William George changed the description

    changed the description

  • William George added 1 commit

    added 1 commit

    • c15e05bb - Update snippets_controller.rb

    Compare with previous version

  • William George added 1 commit

    added 1 commit

    • 6ee50a75 - Fix return correct 409 conflit error code

    Compare with previous version

  • William George resolved all discussions

    resolved all discussions

  • William George added 1 commit

    added 1 commit

    Compare with previous version

  • mentioned in issue #47601 (moved)

  • William George changed the description

    changed the description

  • William George added 1 commit

    added 1 commit

    Compare with previous version

  • William George added 1 commit

    added 1 commit

    Compare with previous version

  • William George unmarked as a Work In Progress

    unmarked as a Work In Progress

  • William George changed the description

    changed the description

  • Author Contributor

    If someone could help me getting this merged, I'd appreciate it. -- Thanks

  • Author Contributor

    /cc @toon

  • assigned to @toon

  • 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:

    1. 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:
      1. execute, which does the if check that's currently used in the API helpers, delegating to the below two methods
      2. find_by_id, which just ends up calling User.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 of User.find_by_id.
      3. find_by_username, which just ends up calling User.find_by_username(...)
      4. find_by_username!, which does the same as find_by_username except it will raise if a row is not found. We can then use this method in SnippetsController, removing the need for return render_404 .... See app/finders/autocomplete/project_finder.rb for an example on how to do this.
    2. We replace occurrences of User.find_by_username(...) with ThisNewUserFinder.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
    3. The find_user method in the API helpers is changed so it simply does ThisNewUserFinder.new(id).execute
  • @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

  • William George added 1 commit

    added 1 commit

    • 4737865c - Update UserFinder to accept `username` or `id`

    Compare with previous version

  • Author Contributor

    Crap, just read @yorickpeterse's last paragraph on the linked comment :face_palm:

    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 :slight_frown:

  • William George added 1 commit

    added 1 commit

    • 25ec327a - Update UserFinder to accept `username` or `id`

    Compare with previous version

  • William George added 1 commit

    added 1 commit

    Compare with previous version

  • Author Contributor

    If somebody can give me a pointer to why the tests are failing with wrong number of arguments (given 1, expected 0) I would greatly appreciate it.

    Thanks.

  • @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 the params Hash out of the finder, which I always disliked.

  • Author Contributor

    @yorickpeterse, awesome, so we should be good, as long as I can fix the failing tests?

  • William George added 1 commit

    added 1 commit

    • 833f7394 - Removed getter/setter as not needed.

    Compare with previous version

  • William George added 1 commit

    added 1 commit

    • f4d251ec - Removed getter/setter as not needed.

    Compare with previous version

  • William George added 1 commit

    added 1 commit

    Compare with previous version

  • Author Contributor

    @yorickpeterse - Tests passing, thanks for your help.

  • Author Contributor

    Just quickly going to add one more test.

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading