From e91a63197895dbe01a220617fceff8a4e016ca6f Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Mon, 23 Oct 2017 11:16:14 +0800
Subject: [PATCH] REFACTOR: DRY up code and add better test coverage.

---
 app/services/user_updater.rb       | 16 ++++++++--------
 spec/services/user_updater_spec.rb | 23 +++++++++++++++++++----
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb
index d7f764e7b04..e2d3c703a7a 100644
--- a/app/services/user_updater.rb
+++ b/app/services/user_updater.rb
@@ -110,15 +110,15 @@ class UserUpdater
         update_muted_users(attributes[:muted_usernames])
       end
 
-      saved = (!save_options || user.user_option.save) && user_profile.save && user.save
+      if (saved = (!save_options || user.user_option.save) && user_profile.save && user.save) &&
+         (attributes[:name].present? && old_user_name.casecmp(attributes.fetch(:name)) != 0) ||
+         (attributes[:name].blank? && old_user_name.present?)
 
-      if saved
-        # log name changes
-        if attributes[:name].present? && old_user_name.downcase != attributes.fetch(:name).downcase
-          StaffActionLogger.new(@actor).log_name_change(user.id, old_user_name, attributes.fetch(:name))
-        elsif attributes[:name].blank? && old_user_name.present?
-          StaffActionLogger.new(@actor).log_name_change(user.id, old_user_name, "")
-        end
+        StaffActionLogger.new(@actor).log_name_change(
+          user.id,
+          old_user_name,
+          attributes.fetch(:name) { '' }
+        )
       end
     end
 
diff --git a/spec/services/user_updater_spec.rb b/spec/services/user_updater_spec.rb
index b7c595c7bee..b3c9a11ca94 100644
--- a/spec/services/user_updater_spec.rb
+++ b/spec/services/user_updater_spec.rb
@@ -200,10 +200,25 @@ describe UserUpdater do
     it "logs the action" do
       user_without_name = Fabricate(:user, name: nil)
       user = Fabricate(:user, name: 'Billy Bob')
-      expect { UserUpdater.new(acting_user, user).update(name: 'Jim Tom') }.to change { UserHistory.count }.by(1)
-      expect { UserUpdater.new(acting_user, user).update(name: 'Jim Tom') }.to change { UserHistory.count }.by(0) # make sure it does not log a dupe
-      expect { UserUpdater.new(acting_user, user_without_name).update(bio_raw: 'foo bar') }.to change { UserHistory.count }.by(0) # make sure user without name (name = nil) does not raise an error
-      expect { UserUpdater.new(acting_user, user_without_name).update(name: 'Jim Tom') }.to change { UserHistory.count }.by(1)
+      expect do
+        UserUpdater.new(acting_user, user).update(name: 'Jim Tom')
+      end.to change { UserHistory.count }.by(1)
+
+      expect do
+        UserUpdater.new(acting_user, user).update(name: 'JiM TOm')
+      end.to_not change { UserHistory.count }
+
+      expect do
+        UserUpdater.new(acting_user, user_without_name).update(bio_raw: 'foo bar')
+      end.to_not change { UserHistory.count }
+
+      expect do
+        UserUpdater.new(acting_user, user_without_name).update(name: 'Jim Tom')
+      end.to change { UserHistory.count }.by(1)
+
+      expect do
+        UserUpdater.new(acting_user, user).update(name: '')
+      end.to change { UserHistory.count }.by(1)
     end
   end
 end