Merge pull request #604 from wpp/refactor_suggest_username

Improve suggest_username method in user class

app/models/user.rb

@@ -59,31 +59,20 @@ class User < ActiveRecord::Base
     3..15
   end
 
-  def self.suggest_username(name)
-    return unless name.present?
-
-    # If it's an email
-    if name =~ /([^@]+)@([^\.]+)/
-      name = Regexp.last_match[1]
-
-      # Special case, if it's "me" or "i" @ something, take the something.
-      name = Regexp.last_match[2] if ['i', 'me'].include?(name)
-    end
-
-    name.gsub!(/^[^A-Za-z0-9]+/, "")
-    name.gsub!(/[^A-Za-z0-9_]+$/, "")
+  def self.sanitize_username!(name)
+    name.gsub!(/^[^A-Za-z0-9]+|[^A-Za-z0-9_]+$/, "")
     name.gsub!(/[^A-Za-z0-9_]+/, "_")
+  end
 
-    # Pad the length with 1s
+  def self.pad_missing_chars_with_1s!(name)
     missing_chars = User.username_length.begin - name.length
     name << ('1' * missing_chars) if missing_chars > 0
+  end
 
-    # Trim extra length
-    name = name[0..User.username_length.end-1]
-
+  def self.find_available_username_based_on(name)
     i = 1
     attempt = name
-    while !username_available?(attempt)
+    until username_available?(attempt)
       suffix = i.to_s
       max_length = User.username_length.end - suffix.length - 1
       attempt = "#{name[0..max_length]}#{suffix}"
@@ -92,6 +81,27 @@ class User < ActiveRecord::Base
     attempt
   end
 
+  EMAIL = %r{([^@]+)@([^\.]+)}
+
+  def self.suggest_username(name)
+    return unless name.present?
+
+    if name =~ EMAIL
+      # When 'walter@white.com' take 'walter'
+      name = Regexp.last_match[1]
+
+      # When 'me@eviltrout.com' take 'eviltrout'
+      name = Regexp.last_match[2] if ['i', 'me'].include?(name)
+    end
+
+    sanitize_username!(name)
+    pad_missing_chars_with_1s!(name)
+
+    # Trim extra length
+    name = name[0..User.username_length.end-1]
+    find_available_username_based_on(name)
+  end
+
   def self.create_for_email(email, opts={})
     username = suggest_username(email)
 
@@ -240,7 +250,7 @@ class User < ActiveRecord::Base
   end
 
   def moderator?
-    # this saves us from checking both, admins are always moderators 
+    # this saves us from checking both, admins are always moderators
     #
     # in future we may split this out
     admin || moderator
@@ -409,7 +419,7 @@ class User < ActiveRecord::Base
   end
 
   # a touch faster than automatic
-  def admin? 
+  def admin?
     admin
   end
 
@@ -545,7 +555,7 @@ class User < ActiveRecord::Base
         end
       end
     end
-    
+
     def email_in_restriction_setting?(setting)
       domains = setting.gsub('.', '\.')
       regexp = Regexp.new("@(#{domains})", true)

spec/models/user_spec.rb

@@ -443,7 +443,7 @@ describe User do
     end
 
     it 'corrects weird characters' do
-      User.suggest_username("Darth%^Vadar").should == "Darth_Vadar"
+      User.suggest_username("Darth%^Vader").should == "Darth_Vader"
     end
 
     it 'adds 1 to an existing username' do
@@ -455,8 +455,9 @@ describe User do
       User.suggest_username('a').should == 'a11'
     end
 
-    it "has a special case for me emails" do
+    it "has a special case for me and i emails" do
       User.suggest_username('me@eviltrout.com').should == 'eviltrout'
+      User.suggest_username('i@eviltrout.com').should == 'eviltrout'
     end
 
     it "shortens very long suggestions" do
@@ -511,12 +512,12 @@ describe User do
       Fabricate.build(:user, email: 'notgood@trashmail.net').should_not be_valid
       Fabricate.build(:user, email: 'mailinator.com@gmail.com').should be_valid
     end
-    
+
     it 'should not reject partial matches' do
       SiteSetting.stubs(:email_domains_blacklist).returns('mail.com')
       Fabricate.build(:user, email: 'mailinator@gmail.com').should be_valid
     end
-    
+
     it 'should reject some emails based on the email_domains_blacklist site setting ignoring case' do
       SiteSetting.stubs(:email_domains_blacklist).returns('trashmail.net')
       Fabricate.build(:user, email: 'notgood@TRASHMAIL.NET').should_not be_valid
@@ -539,7 +540,7 @@ describe User do
       u.email = 'nope@mailinator.com'
       u.should_not be_valid
     end
-    
+
     it 'whitelist should reject some emails based on the email_domains_whitelist site setting' do
       SiteSetting.stubs(:email_domains_whitelist).returns('vaynermedia.com')
       Fabricate.build(:user, email: 'notgood@mailinator.com').should_not be_valid
@@ -565,7 +566,7 @@ describe User do
       u.should be_valid
     end
 
-    it 'email whitelist should be used when email is being changed' do      
+    it 'email whitelist should be used when email is being changed' do
       SiteSetting.stubs(:email_domains_whitelist).returns('vaynermedia.com')
       u = Fabricate(:user, email: 'good@vaynermedia.com')
       u.email = 'nope@mailinator.com'
@@ -735,11 +736,12 @@ describe User do
   end
 
   describe '#create_for_email' do
-    let(:subject) { User.create_for_email('test@email.com') }
+    let(:subject) { User.create_for_email('walter.white@email.com') }
     it { should be_present }
-    its(:username) { should == 'test' }
-    its(:name) { should == 'test'}
+    its(:username) { should == 'walter_white' }
+    its(:name) { should == 'walter_white'}
     it { should_not be_active }
+    its(:email) { should == 'walter.white@email.com' }
   end
 
   describe 'email_confirmed?' do