Merge pull request #975 from jd-erreape/username_refactor

[WIP] Refactored user_name suggestion methods into a module

app/controllers/users/omniauth_callbacks_controller.rb

@@ -1,6 +1,7 @@
 # -*- encoding : utf-8 -*-
 require_dependency 'email'
 require_dependency 'enum'
+require_dependency 'user_name_suggester'
 
 class Users::OmniauthCallbacksController < ApplicationController
   skip_before_filter :redirect_to_login_if_required
@@ -87,7 +88,7 @@ class Users::OmniauthCallbacksController < ApplicationController
     fb_uid = auth_token["uid"]
 
 
-    username = User.suggest_username(name)
+    username = UserNameSuggester.suggest(name)
 
     session[:authentication] = {
       facebook: {
@@ -232,7 +233,7 @@ class Users::OmniauthCallbacksController < ApplicationController
       @data = {
         email: email,
         name: User.suggest_name(name),
-        username: User.suggest_username(username),
+        username: UserNameSuggester.suggest(username),
         email_valid: true ,
         auth_provider: data[:provider] || params[:provider].try(:capitalize)
       }
@@ -306,7 +307,7 @@ class Users::OmniauthCallbacksController < ApplicationController
         email: email,
         email_valid: true,
         name: User.suggest_name(email),
-        username: User.suggest_username(email),
+        username: UserNameSuggester.suggest(email),
         auth_provider: params[:provider].try(:capitalize)
       }
 

app/controllers/users_controller.rb

@@ -1,4 +1,5 @@
 require_dependency 'discourse_hub'
+require_dependency 'user_name_suggester'
 
 class UsersController < ApplicationController
 
@@ -102,7 +103,7 @@ class UsersController < ApplicationController
       if User.username_available?(params[:username])
         render json: {available: true}
       else
-        render json: {available: false, suggestion: User.suggest_username(params[:username])}
+        render json: {available: false, suggestion: UserNameSuggester.suggest(params[:username])}
       end
     else
 
@@ -133,7 +134,7 @@ class UsersController < ApplicationController
         end
       elsif available_globally && !available_locally
         # Already registered on this site with the matching nickname and email address. Why are you signing up again?
-        render json: {available: false, suggestion: User.suggest_username(params[:username])}
+        render json: {available: false, suggestion: UserNameSuggester.suggest(params[:username])}
       else
         # Not available anywhere.
         render json: {available: false, suggestion: suggestion_from_discourse_hub}
@@ -202,7 +203,7 @@ class UsersController < ApplicationController
       message: I18n.t(
         "login.errors",
         errors:I18n.t(
-          "login.not_available", suggestion: User.suggest_username(params[:username])
+          "login.not_available", suggestion: UserNameSuggester.suggest(params[:username])
         )
       )
     }

app/models/user.rb

@@ -5,6 +5,7 @@ require_dependency 'pbkdf2'
 require_dependency 'summarize'
 require_dependency 'discourse'
 require_dependency 'post_destroyer'
+require_dependency 'user_name_suggester'
 
 class User < ActiveRecord::Base
   has_many :posts
@@ -57,10 +58,10 @@ class User < ActiveRecord::Base
   # This is just used to pass some information into the serializer
   attr_accessor :notification_channel_position
 
-  scope :admins, ->{ where(admin: true) }
-  scope :moderators, ->{ where(moderator: true) }
-  scope :staff, ->{ where("moderator or admin ") }
-  scope :blocked, ->{ where(blocked: true) } # no index
+  scope :admins, -> { where(admin: true) }
+  scope :moderators, -> { where(moderator: true) }
+  scope :staff, -> { where("moderator or admin ") }
+  scope :blocked, -> { where(blocked: true) } # no index
 
   module NewTopicDuration
     ALWAYS = -1
@@ -71,46 +72,13 @@ class User < ActiveRecord::Base
     3..15
   end
 
-  def self.sanitize_username!(name)
-    name.gsub!(/^[^[:alnum:]]+|\W+$/, "")
-    name.gsub!(/\W+/, "_")
-  end
-
-
-  def self.find_available_username_based_on(name)
-    sanitize_username!(name)
-    name = rightsize_username(name)
-    i = 1
-    attempt = name
-    until username_available?(attempt)
-      suffix = i.to_s
-      max_length = User.username_length.end - suffix.length - 1
-      attempt = "#{name[0..max_length]}#{suffix}"
-      i += 1
-    end
-    attempt
+  def self.username_available?(username)
+    lower = username.downcase
+    User.where(username_lower: lower).blank?
   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
-
-    find_available_username_based_on(name)
-  end
-
-  def self.rightsize_username(name)
-    name.ljust(username_length.begin, '1')[0,username_length.end]
-  end
-
   def self.new_from_params(params)
     user = User.new
     user.name = params[:name]
@@ -121,7 +89,7 @@ class User < ActiveRecord::Base
   end
 
   def self.create_for_email(email, opts={})
-    username = suggest_username(email)
+    username = UserNameSuggester.suggest(email)
 
     if SiteSetting.call_discourse_hub?
       begin
@@ -147,18 +115,6 @@ class User < ActiveRecord::Base
     user
   end
 
-  def self.username_available?(username)
-    lower = username.downcase
-    User.where(username_lower: lower).blank?
-  end
-
-  def self.username_valid?(username)
-    u = User.new(username: username)
-    u.username_format_validator
-    u.errors[:username].blank?
-  end
-
-
   def self.suggest_name(email)
     return "" unless email
     name = email.split(/[@\+]/)[0]
@@ -184,7 +140,7 @@ class User < ActiveRecord::Base
   def save_and_refresh_staff_groups!
     transaction do
       self.save!
-      Group.refresh_automatic_groups!(:admins,:moderators,:staff)
+      Group.refresh_automatic_groups!(:admins, :moderators, :staff)
     end
   end
 
@@ -285,15 +241,15 @@ class User < ActiveRecord::Base
 
   def saw_notification_id(notification_id)
     User.update_all ["seen_notification_id = ?", notification_id],
-      ["seen_notification_id < ?", notification_id]
+                    ["seen_notification_id < ?", notification_id]
   end
 
   def publish_notifications_state
     MessageBus.publish("/notification/#{id}",
-        { unread_notifications: unread_notifications,
-          unread_private_messages: unread_private_messages },
-        user_ids: [id] # only publish the notification to this user
-      )
+                       {unread_notifications: unread_notifications,
+                        unread_private_messages: unread_private_messages},
+                       user_ids: [id] # only publish the notification to this user
+    )
   end
 
   # A selection of people to autocomplete on @mention
@@ -362,7 +318,7 @@ class User < ActiveRecord::Base
         previous_visit_at = last_seen_at
         update_column(:previous_visit_at, previous_visit_at)
       end
-      update_column(:last_seen_at,  now)
+      update_column(:last_seen_at, now)
     end
   end
 
@@ -506,12 +462,12 @@ class User < ActiveRecord::Base
   def treat_as_new_topic_start_date
     duration = new_topic_duration_minutes || SiteSetting.new_topic_duration_minutes
     case duration
-    when User::NewTopicDuration::ALWAYS
-      created_at
-    when User::NewTopicDuration::LAST_VISIT
-      previous_visit_at || created_at
-    else
-      duration.minutes.ago
+      when User::NewTopicDuration::ALWAYS
+        created_at
+      when User::NewTopicDuration::LAST_VISIT
+        previous_visit_at || created_at
+      else
+        duration.minutes.ago
     end
   end
 
@@ -549,7 +505,7 @@ class User < ActiveRecord::Base
 
   def update_topic_reply_count
     self.topic_reply_count =
-      Topic
+        Topic
         .where(['id in (
               SELECT topic_id FROM posts p
               JOIN topics t2 ON t2.id = p.topic_id
@@ -562,7 +518,7 @@ class User < ActiveRecord::Base
 
   def secure_category_ids
     cats = self.staff? ? Category.select(:id).where(secure: true) : secure_categories.select('categories.id')
-    cats.map{|c| c.id}.sort
+    cats.map { |c| c.id }.sort
   end
 
   # Flag all posts from a user as spam
@@ -580,84 +536,84 @@ class User < ActiveRecord::Base
 
   protected
 
-    def cook
-      if bio_raw.present?
-        self.bio_cooked = PrettyText.cook(bio_raw) if bio_raw_changed?
-      else
-        self.bio_cooked = nil
+  def cook
+    if bio_raw.present?
+      self.bio_cooked = PrettyText.cook(bio_raw) if bio_raw_changed?
+    else
+      self.bio_cooked = nil
+    end
+  end
+
+  def update_tracked_topics
+    return unless auto_track_topics_after_msecs_changed?
+
+    where_conditions = {notifications_reason_id: nil, user_id: id}
+    if auto_track_topics_after_msecs < 0
+      TopicUser.update_all({notification_level: TopicUser.notification_levels[:regular]}, where_conditions)
+    else
+      TopicUser.update_all(["notification_level = CASE WHEN total_msecs_viewed < ? THEN ? ELSE ? END",
+                            auto_track_topics_after_msecs, TopicUser.notification_levels[:regular], TopicUser.notification_levels[:tracking]], where_conditions)
+    end
+  end
+
+
+  def create_email_token
+    email_tokens.create(email: email)
+  end
+
+  def ensure_password_is_hashed
+    if @raw_password
+      self.salt = SecureRandom.hex(16)
+      self.password_hash = hash_password(@raw_password, salt)
+    end
+  end
+
+  def hash_password(password, salt)
+    Pbkdf2.hash_password(password, salt, Rails.configuration.pbkdf2_iterations)
+  end
+
+  def add_trust_level
+    # there is a possiblity we did not load trust level column, skip it
+    return unless has_attribute? :trust_level
+    self.trust_level ||= SiteSetting.default_trust_level
+  end
+
+  def update_username_lower
+    self.username_lower = username.downcase
+  end
+
+  def username_validator
+    username_format_validator || begin
+      lower = username.downcase
+      if username_changed? && User.where(username_lower: lower).exists?
+        errors.add(:username, I18n.t(:'user.username.unique'))
       end
     end
+  end
 
-    def update_tracked_topics
-      return unless auto_track_topics_after_msecs_changed?
-
-      where_conditions = {notifications_reason_id: nil, user_id: id}
-      if auto_track_topics_after_msecs < 0
-        TopicUser.update_all({notification_level: TopicUser.notification_levels[:regular]}, where_conditions)
-      else
-        TopicUser.update_all(["notification_level = CASE WHEN total_msecs_viewed < ? THEN ? ELSE ? END",
-                              auto_track_topics_after_msecs, TopicUser.notification_levels[:regular], TopicUser.notification_levels[:tracking]], where_conditions)
+  def email_validator
+    if (setting = SiteSetting.email_domains_whitelist).present?
+      unless email_in_restriction_setting?(setting)
+        errors.add(:email, I18n.t(:'user.email.not_allowed'))
+      end
+    elsif (setting = SiteSetting.email_domains_blacklist).present?
+      if email_in_restriction_setting?(setting)
+        errors.add(:email, I18n.t(:'user.email.not_allowed'))
       end
     end
+  end
 
+  def email_in_restriction_setting?(setting)
+    domains = setting.gsub('.', '\.')
+    regexp = Regexp.new("@(#{domains})", true)
+    self.email =~ regexp
+  end
 
-    def create_email_token
-      email_tokens.create(email: email)
-    end
-
-    def ensure_password_is_hashed
-      if @raw_password
-        self.salt = SecureRandom.hex(16)
-        self.password_hash = hash_password(@raw_password, salt)
-      end
-    end
-
-    def hash_password(password, salt)
-      Pbkdf2.hash_password(password, salt, Rails.configuration.pbkdf2_iterations)
-    end
-
-    def add_trust_level
-      # there is a possiblity we did not load trust level column, skip it
-      return unless has_attribute? :trust_level
-      self.trust_level ||= SiteSetting.default_trust_level
-    end
-
-    def update_username_lower
-      self.username_lower = username.downcase
-    end
-
-    def username_validator
-      username_format_validator || begin
-        lower = username.downcase
-        if username_changed? && User.where(username_lower: lower).exists?
-          errors.add(:username, I18n.t(:'user.username.unique'))
-        end
-      end
-    end
-
-    def email_validator
-      if (setting = SiteSetting.email_domains_whitelist).present?
-        unless email_in_restriction_setting?(setting)
-          errors.add(:email, I18n.t(:'user.email.not_allowed'))
-        end
-      elsif (setting = SiteSetting.email_domains_blacklist).present?
-        if email_in_restriction_setting?(setting)
-          errors.add(:email, I18n.t(:'user.email.not_allowed'))
-        end
-      end
-    end
-
-    def email_in_restriction_setting?(setting)
-      domains = setting.gsub('.', '\.')
-      regexp = Regexp.new("@(#{domains})", true)
-      self.email =~ regexp
-    end
-
-    def password_validator
-      if (@raw_password && @raw_password.length < 6) || (@password_required && !@raw_password)
-        errors.add(:password, "must be 6 letters or longer")
-      end
+  def password_validator
+    if (@raw_password && @raw_password.length < 6) || (@password_required && !@raw_password)
+      errors.add(:password, "must be 6 letters or longer")
     end
+  end
 
 
 end

lib/user_name_suggester.rb

@@ -0,0 +1,44 @@
+module UserNameSuggester
+
+  def self.suggest(name)
+    return unless name.present?
+    name = parse_name_from_email(name)
+    find_available_username_based_on(name)
+  end
+
+  private
+
+  def self.parse_name_from_email(name)
+    if name =~ User::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
+    name
+  end
+
+  def self.find_available_username_based_on(name)
+    sanitize_username!(name)
+    name = rightsize_username(name)
+    i = 1
+    attempt = name
+    until User.username_available?(attempt)
+      suffix = i.to_s
+      max_length = User.username_length.end - suffix.length - 1
+      attempt = "#{name[0..max_length]}#{suffix}"
+      i += 1
+    end
+    attempt
+  end
+
+  def self.sanitize_username!(name)
+    name.gsub!(/^[^[:alnum:]]+|\W+$/, "")
+    name.gsub!(/\W+/, "_")
+  end
+
+  def self.rightsize_username(name)
+    name.ljust(User.username_length.begin, '1')[0, User.username_length.end]
+  end
+
+end

spec/components/user_name_suggester_spec.rb

@@ -0,0 +1,70 @@
+require 'spec_helper'
+require 'user_name_suggester'
+
+describe UserNameSuggester do
+
+  describe 'name heuristics' do
+    it 'is able to guess a decent username from an email' do
+      UserNameSuggester.suggest('bob@bob.com').should == 'bob'
+    end
+  end
+
+  describe '.suggest' do
+
+    it "doesn't raise an error on nil username" do
+      UserNameSuggester.suggest(nil).should be_nil
+    end
+
+    it 'corrects weird characters' do
+      UserNameSuggester.suggest("Darth%^Vader").should == 'Darth_Vader'
+    end
+
+    it 'adds 1 to an existing username' do
+      user = Fabricate(:user)
+      UserNameSuggester.suggest(user.username).should == "#{user.username}1"
+    end
+
+    it "adds numbers if it's too short" do
+      UserNameSuggester.suggest('a').should == 'a11'
+    end
+
+    it "has a special case for me and i emails" do
+      UserNameSuggester.suggest('me@eviltrout.com').should == 'eviltrout'
+      UserNameSuggester.suggest('i@eviltrout.com').should == 'eviltrout'
+    end
+
+    it "shortens very long suggestions" do
+      UserNameSuggester.suggest("myreallylongnameisrobinwardesquire").should == 'myreallylongnam'
+    end
+
+    it "makes room for the digit added if the username is too long" do
+      User.create(username: 'myreallylongnam', email: 'fake@discourse.org')
+      UserNameSuggester.suggest("myreallylongnam").should == 'myreallylongna1'
+    end
+
+    it "removes leading character if it is not alphanumeric" do
+      UserNameSuggester.suggest("_myname").should == 'myname'
+    end
+
+    it "removes trailing characters if they are invalid" do
+      UserNameSuggester.suggest("myname!^$=").should == 'myname'
+    end
+
+    it "replace dots" do
+      UserNameSuggester.suggest("my.name").should == 'my_name'
+    end
+
+    it "remove leading dots" do
+      UserNameSuggester.suggest(".myname").should == 'myname'
+    end
+
+    it "remove trailing dots" do
+      UserNameSuggester.suggest("myname.").should == 'myname'
+    end
+
+    it 'should handle typical facebook usernames' do
+      UserNameSuggester.suggest('roger.nelson.3344913').should == 'roger_nelson_33'
+    end
+  end
+
+end

spec/models/user_spec.rb

@@ -409,10 +409,6 @@ describe User do
   end
 
   describe 'name heuristics' do
-    it 'is able to guess a decent username from an email' do
-      User.suggest_username('bob@bob.com').should == 'bob'
-    end
-
     it 'is able to guess a decent name from an email' do
       User.suggest_name('sam.saffron@gmail.com').should == 'Sam Saffron'
     end
@@ -474,64 +470,6 @@ describe User do
     end
   end
 
-  describe '.suggest_username' do
-
-    it "doesn't raise an error on nil username" do
-      User.suggest_username(nil).should be_nil
-    end
-
-    it 'corrects weird characters' do
-      User.suggest_username("Darth%^Vader").should == "Darth_Vader"
-    end
-
-    it 'adds 1 to an existing username' do
-      user = Fabricate(:user)
-      User.suggest_username(user.username).should == "#{user.username}1"
-    end
-
-    it "adds numbers if it's too short" do
-      User.suggest_username('a').should == 'a11'
-    end
-
-    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
-      User.suggest_username("myreallylongnameisrobinwardesquire").should == 'myreallylongnam'
-    end
-
-    it "makes room for the digit added if the username is too long" do
-      User.create(username: 'myreallylongnam', email: 'fake@discourse.org')
-      User.suggest_username("myreallylongnam").should == 'myreallylongna1'
-    end
-
-    it "removes leading character if it is not alphanumeric" do
-      User.suggest_username("_myname").should == 'myname'
-    end
-
-    it "removes trailing characters if they are invalid" do
-      User.suggest_username("myname!^$=").should == 'myname'
-    end
-
-    it "replace dots" do
-      User.suggest_username("my.name").should == 'my_name'
-    end
-
-    it "remove leading dots" do
-      User.suggest_username(".myname").should == 'myname'
-    end
-
-    it "remove trailing dots" do
-      User.suggest_username("myname.").should == 'myname'
-    end
-
-    it 'should handle typical facebook usernames' do
-      User.suggest_username('roger.nelson.3344913').should == 'roger_nelson_33'
-    end
-  end
-
   describe 'email_validator' do
     it 'should allow good emails' do
       user = Fabricate.build(:user, email: 'good@gmail.com')