Skip to content

Resolve "Refactor by_login and by_id_and_login in user model"

upstream MR

What does this MR do and why?

This MR is related to issue #1127 (closed)

  • Remove unscoped in by_login in user model

    The reasons are as follows:

    1. The program currently only calls the by_login method through the user model, and the user model does not set the default_scope.Related screenshots : 1

    2. unscoped commit message(72f428c7) and MR(https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34) did not find unscoped explanation for necessity

    3. They can still run through local tests and all test cases after removing unscoped.

    So,unscoped is redundant in by_login.

  • Merge by_id_and_login into by_login

    The reasons are as follows:

    1. by_login and by_id_and_login are both methods of user query and the code logic repetition rate is high

    2. by_id_and_login commit(6f841a95) is used to 'Prevent interrupted 2FA sign-in from signing-in incorrect user',which can be merged into by_login for implementation

    3. The by_id_and_login method determines whether login belongs to username or email during sql query time.Moving the check to Ruby makes this method an additional 1.5 times faster compared to doing the check in the SQL query according to 72f428c7

    4. They can still run through local tests and all test cases after merging by_id_and_login into by_login

    So,we should merge by_id_and_login into by_login

Reference of source code

  1. https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/models/user.rb

    This reference is related to targe: Remove unscoped in by_login in user model

    From the user.rb file, it can be found that the default_scope is not set in the user model. Combined with screenshot 1, it can be concluded that the parent class of the user model and the included modules do not have default_scope set.

    As far as the user model itself is concerned, the unscoped in the by_login method makes redundant

  2. The following is the usages of by_login throughout the program:

    These references are related to targe: Remove unscoped in by_login in user model

    As can be seen from the above, by_login is called directly through the user model rather than any of its subclasses, and default_scope is not set when calling by_login. So currently the unscoep of by_login is redundant for the whole program

  3. https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/models/user_spec.rb#L2907

    This reference is related to targe: Merge by_id_and_login into by_login

    The model spec of by_login before merging

    The original by_login model spec covers 740~747 and 751, 752 in by_login in this MR.

    The new by_login model spec needs to cover 748~750 in by_login in this MR.

Screenshots or screen recordings

screenshot-1:Here is a screenshot showing that the user model does not have default_scope set

image-20220704171429201

Closes #1127 (closed)

奇廷 陈 编辑于

合并请求报告

加载中