From f71a18facd72768646e2c34a11446160dc2db177 Mon Sep 17 00:00:00 2001
From: Vinoth Kannan <vinothkannan@vinkas.com>
Date: Mon, 2 Apr 2018 21:59:11 +0530
Subject: [PATCH] FIX: Username uniqueness check should not happen to current
 user_id

---
 app/models/group.rb              | 14 ++------------
 app/models/user.rb               | 30 ++----------------------------
 app/models/username_validator.rb | 28 ++++++++++++++++++++++++----
 3 files changed, 28 insertions(+), 44 deletions(-)

diff --git a/app/models/group.rb b/app/models/group.rb
index c0b5ff68074..d769c9f68ae 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -42,7 +42,7 @@ class Group < ActiveRecord::Base
     ApplicationSerializer.expire_cache_fragment!("group_names")
   end
 
-  validate :name_format_validator
+  validate :name_format_validator, if: :will_save_change_to_name?
   validates :name, presence: true
   validate :automatic_membership_email_domains_format_validator
   validate :incoming_email_validator
@@ -599,17 +599,7 @@ class Group < ActiveRecord::Base
       self.name.strip!
       self.name.downcase!
 
-      UsernameValidator.perform_validation(self, 'name') || begin
-        if will_save_change_to_name?
-          existing = Group.exec_sql(
-            User::USERNAME_EXISTS_SQL, username: self.name
-          ).values.present?
-
-          if existing
-            errors.add(:name, I18n.t("activerecord.errors.messages.taken"))
-          end
-        end
-      end
+      UsernameValidator.perform_validation(self, 'name')
     end
 
     def automatic_membership_email_domains_format_validator
diff --git a/app/models/user.rb b/app/models/user.rb
index 45bea901600..8c0ad3fe402 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -770,8 +770,8 @@ class User < ActiveRecord::Base
     Guardian.new(self)
   end
 
-  def username_format_validator
-    UsernameValidator.perform_validation(self, 'username')
+  def username_validator
+    UsernameValidator.perform_validation(self, 'username', self.id)
   end
 
   def email_confirmed?
@@ -1121,32 +1121,6 @@ class User < ActiveRecord::Base
     self.username_lower = username.downcase
   end
 
-  USERNAME_EXISTS_SQL = <<~SQL
-  (SELECT 1 FROM users
-  WHERE users.username_lower = :username)
-
-  UNION ALL
-
-  (SELECT 1 FROM groups
-  WHERE lower(groups.name) = :username)
-  SQL
-
-  def username_validator
-    username_format_validator || begin
-      if will_save_change_to_username?
-        lower = username.downcase
-
-        existing = User.exec_sql(
-          USERNAME_EXISTS_SQL, username: lower
-        ).values.present?
-
-        if existing
-          errors.add(:username, I18n.t(:'user.username.unique'))
-        end
-      end
-    end
-  end
-
   def send_approval_email
     if SiteSetting.must_approve_users
       Jobs.enqueue(:critical_user_email,
diff --git a/app/models/username_validator.rb b/app/models/username_validator.rb
index eeb4df859de..34483327d1b 100644
--- a/app/models/username_validator.rb
+++ b/app/models/username_validator.rb
@@ -6,21 +6,23 @@ class UsernameValidator
   #
   # object - Object in which we're performing the validation
   # field_name - name of the field that we're validating
+  # user_id - skip id while validating username uniqueness
   #
   # Example: UsernameValidator.perform_validation(user, 'name')
-  def self.perform_validation(object, field_name)
-    validator = UsernameValidator.new(object.send(field_name))
+  def self.perform_validation(object, field_name, user_id = nil)
+    validator = UsernameValidator.new(object.send(field_name), user_id)
     unless validator.valid_format?
       validator.errors.each { |e| object.errors.add(field_name.to_sym, e) }
     end
   end
 
-  def initialize(username)
+  def initialize(username, user_id = nil)
     @username = username
+    @user_id = user_id
     @errors = []
   end
   attr_accessor :errors
-  attr_reader :username
+  attr_reader :username, :user_id
 
   def user
     @user ||= User.new(user)
@@ -35,6 +37,7 @@ class UsernameValidator
     username_last_char_valid?
     username_no_double_special?
     username_does_not_end_with_confusing_suffix?
+    username_unique?
     errors.empty?
   end
 
@@ -97,4 +100,21 @@ class UsernameValidator
       self.errors << I18n.t(:'user.username.must_not_end_with_confusing_suffix')
     end
   end
+
+  def username_unique?
+    lower = username.downcase
+    sql = <<~SQL
+    (SELECT 1 FROM users
+    WHERE users.username_lower = :username#{" AND users.id != #{user_id}" if user_id.present?})
+
+    UNION ALL
+
+    (SELECT 1 FROM groups
+    WHERE lower(groups.name) = :username)
+    SQL
+
+    if User.exec_sql(sql, username: lower).values.present?
+      self.errors << user_id.present? ? I18n.t(:'user.username.unique') : I18n.t("activerecord.errors.messages.taken")
+    end
+  end
 end