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