DEV: simplify username suggester (PR #14531)

This PR doesn’t change any behavior, but just removes code that wasn’t in use. This is a pretty dangerous place to change, since it gets called during user’s registration. At the same time the refactoring is very straightforward, it’s clear that this code wasn’t doing any work (it still needs to be double-checked during review though). Also, the test coverage of UserNameSuggester is good.

Here is the explanation of what I’ve done (or you can just look at the commit history in this PR). We have this code:

If we inline username_suggester_attributes, we’ll get this:

    if SiteSetting.auth_overrides_username? && username.present? && username != user.username
      user.username = UserNameSuggester.suggest(username || name || email, user.username)
      ...
    end

But we check if username.present? in the if condition, so it’s always present inside the if block, so it’s clear that || name || email never gets called, we can remove it:

    if SiteSetting.auth_overrides_username? && username.present? && username != user.username
      user.username = UserNameSuggester.suggest(username, user.username)
      ...
    end

Now, we pass user.username as a second parameter to UserNameSuggester, the name of that parameter is allowed_username. If the suggester has found a suggestion that’s equal to allowed_username it stops looking for another available usernames and just returns this suggestion. We need it to avoid the situation when the suggester tries to suggest a username of the current user (without this parameter, the suggester would think that this name isn’t free, when in fact it’s fine to use this username).

But, we check if username != user.username in the condition of if block, so there is no point in passing user.username to the username suggester, we already know that this isn’t the username of the current user.

    if SiteSetting.auth_overrides_username? && username.present? && username != user.username
      user.username = UserNameSuggester.suggest(username)
      ...
    end

There is only one more place when we pass the second parameter to UserNameSuggester and that place can be simplified using the same steps.

After that, we can just remove the second parameter from UserNameSuggester.suggest which simplifies the code significantly and also makes the signature logical. The signature was confusing before:

Why pass allowed_username to UserNameSuggester? I mean, if we already know the allowed username, why call UserNameSuggester at all? We can just use that name. After this refactoring, we’ll have just:

def self.suggest(name_or_email)
  ..
end

GitHub

Looks good to me! Thanks for the explanation.

@eviltrout thank you for review!

Did you change all the places where we call this method using the 2nd arg?

Why this change?

Yes, there were only two such places in Core. There are no such calls from plugins.

I’ve discovered this after finishing this fix and opening PR, a test failure helped to find it.

We fix the username if it is too long or contains disallowed characters. So if we want to check if username already exists, we should be checking fixed versions. This part was actually the important job that we were doing inside the username suggester when passing the second argument:

      # we're kind of checking if a user already was registered with this username, 
      # but it won't work, for example, if sso sends us username with invalid characters:
      elsif user.username != username
       # we pass user.username as the second parameter, 
       # just to compare it with a fixed version of the first parameter inside the suggester
       # in fact we comparing usernames the second time inside the suggested
        user.username = UserNameSuggester.suggest(username, user.username)
      end

It’s better to check it once, explicitly, and don’t call the suggester at all if a user already has this username.

I think the change makes sense, but – and that’s not your fault – this piece of code needs some changes. Maybe in another PR?

  • We need to use UsernameChanger.change() instead of simply updating user.username in order to fix Updating mentions when username is changed via SSO
  • user.username.downcase should be replaced with user.username_lower
  • username.downcase should be replaced with User.normalize_username(username) to correctly handle Unicode
  • It might be worth checking that this and this bug reports are fixed, so that we can close them.

Again, work for another PR: This might have the same problem as the SSO code and would require the usage of UsernameChanger.change()? On first glance, it doesn’t seem to handle case changes. Maybe code can be reused between SSO and here?

@gschlager thank you for comments, these all make sense! I’m going to look at this before merging it, maybe will do a part in this PR.

It might be worth checking that this and this bug reports are fixed, so that we can close them.

I’ve checked them, both was fixed before this PR. We already have a test case for the first one, and I’ve added cases for the second one here.

@gschlager also I eventually decided to address all your other comments on subsequent PRs, I’m working on them now.