From e27b1b98d18f760374f30c07437f8796b090bd07 Mon Sep 17 00:00:00 2001
From: Arpit Jalan <arpit@techapj.com>
Date: Wed, 1 Mar 2017 13:42:17 +0530
Subject: [PATCH] FIX: handle new user when logging name change

---
 app/services/staff_action_logger.rb |  2 +-
 app/services/user_updater.rb        | 22 +++++++++++++---------
 spec/services/user_updater_spec.rb  |  3 +++
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb
index 796b8b36798..71734ab2e20 100644
--- a/app/services/staff_action_logger.rb
+++ b/app/services/staff_action_logger.rb
@@ -146,7 +146,7 @@ class StaffActionLogger
 
   def log_site_text_change(subject, new_text=nil, old_text=nil, opts={})
     raise Discourse::InvalidParameters.new(:subject) unless subject.present?
-    UserHistory.create( params(opts).merge({
+    UserHistory.create!( params(opts).merge({
       action: UserHistory.actions[:change_site_text],
       subject: subject,
       previous_value: old_text,
diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb
index d3b9f680bf3..4d1f6f44984 100644
--- a/app/services/user_updater.rb
+++ b/app/services/user_updater.rb
@@ -53,6 +53,9 @@ class UserUpdater
     user_profile.profile_background = attributes.fetch(:profile_background) { user_profile.profile_background }
     user_profile.card_background = attributes.fetch(:card_background) { user_profile.card_background }
 
+    old_user_name = user.name.present? ? user.name : ""
+    user.name = attributes.fetch(:name) { user.name }
+
     user.locale = attributes.fetch(:locale) { user.locale }
     user.date_of_birth = attributes.fetch(:date_of_birth) { user.date_of_birth }
 
@@ -94,20 +97,21 @@ class UserUpdater
     end
 
     User.transaction do
-      # log name changes
-      if attributes[:name].present? && user.name.downcase != attributes.fetch(:name).downcase
-        StaffActionLogger.new(@actor).log_name_change(user.id, user.name, attributes.fetch(:name))
-      elsif attributes[:name].blank? && user.name.present?
-        StaffActionLogger.new(@actor).log_name_change(user.id, user.name, "")
-      end
-      user.name = attributes.fetch(:name) { user.name }
-
       if attributes.key?(:muted_usernames)
         update_muted_users(attributes[:muted_usernames])
       end
 
       saved = (!save_options || user.user_option.save) && user_profile.save && user.save
-      DiscourseEvent.trigger(:user_updated, user) if saved
+      if saved
+        DiscourseEvent.trigger(:user_updated, user)
+
+        # 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
+      end
       saved
     end
   end
diff --git a/spec/services/user_updater_spec.rb b/spec/services/user_updater_spec.rb
index 544ca55c759..3305f67842d 100644
--- a/spec/services/user_updater_spec.rb
+++ b/spec/services/user_updater_spec.rb
@@ -192,9 +192,12 @@ describe UserUpdater do
     end
 
     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)
     end
   end
 end