From 96d23ddd8ddf4ad9278c477de0ab2ab08c506ddf Mon Sep 17 00:00:00 2001 From: Juan de Dios Herrero Date: Thu, 6 Jun 2013 16:40:10 +0200 Subject: [PATCH] Refactored user_name suggestion methods into a module to reduce the complexity of User model --- .../users/omniauth_callbacks_controller.rb | 7 +- app/controllers/users_controller.rb | 7 +- app/models/user.rb | 232 +++++++----------- lib/user_name_suggester.rb | 44 ++++ spec/components/user_name_suggester_spec.rb | 70 ++++++ spec/models/user_spec.rb | 62 ----- 6 files changed, 216 insertions(+), 206 deletions(-) create mode 100644 lib/user_name_suggester.rb create mode 100644 spec/components/user_name_suggester_spec.rb diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 52bdf4cf6b7..c99eac4c6d6 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/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) } diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 4538c2e2c45..00247e21702 100644 --- a/app/controllers/users_controller.rb +++ b/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]) ) ) } diff --git a/app/models/user.rb b/app/models/user.rb index 64d8c779bcc..348c5b72d61 100644 --- a/app/models/user.rb +++ b/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 attr_accessible :name, :username, :password, :email, :bio_raw, :website @@ -59,10 +60,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 @@ -73,46 +74,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] @@ -123,7 +91,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 @@ -149,18 +117,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] @@ -186,7 +142,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 @@ -287,15 +243,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 @@ -364,7 +320,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 @@ -508,12 +464,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 @@ -551,7 +507,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 @@ -564,7 +520,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 @@ -582,84 +538,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 diff --git a/lib/user_name_suggester.rb b/lib/user_name_suggester.rb new file mode 100644 index 00000000000..17f97885c42 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/spec/components/user_name_suggester_spec.rb b/spec/components/user_name_suggester_spec.rb new file mode 100644 index 00000000000..0922a0b0f68 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e7b4aa53437..9925adadd8b 100644 --- a/spec/models/user_spec.rb +++ b/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')