FIX: do not log duplicate username changes

app/services/username_changer.rb

@@ -11,7 +11,7 @@ class UsernameChanger
   end
 
   def change
-    if @actor
+    if @actor && @user.username != @new_username
       StaffActionLogger.new(@actor).log_username_change(@user, @user.username, @new_username)
     end
 

spec/services/user_updater_spec.rb

@@ -149,7 +149,7 @@ describe UserUpdater do
         guardian = stub
         guardian.stubs(:can_grant_title?).with(user).returns(false)
         Guardian.stubs(:new).with(acting_user).returns(guardian)
-        updater = described_class.new(acting_user, user)
+        updater = UserUpdater.new(acting_user, user)
 
         updater.update(title: 'Minion')
 
@@ -160,7 +160,7 @@ describe UserUpdater do
     context 'when website includes http' do
       it 'does not add http before updating' do
         user = Fabricate(:user)
-        updater = described_class.new(acting_user, user)
+        updater = UserUpdater.new(acting_user, user)
 
         updater.update(website: 'http://example.com')
 
@@ -171,7 +171,7 @@ describe UserUpdater do
     context 'when website does not include http' do
       it 'adds http before updating' do
         user = Fabricate(:user)
-        updater = described_class.new(acting_user, user)
+        updater = UserUpdater.new(acting_user, user)
 
         updater.update(website: 'example.com')
 
@@ -184,7 +184,7 @@ describe UserUpdater do
         user = Fabricate(:user)
         user.custom_fields = {'import_username' => 'my_old_username'}
         user.save
-        updater = described_class.new(acting_user, user)
+        updater = UserUpdater.new(acting_user, user)
 
         updater.update(website: 'example.com', custom_fields: '')
         expect(user.reload.custom_fields).to eq({'import_username' => 'my_old_username'})
@@ -193,7 +193,8 @@ describe UserUpdater do
 
     it "logs the action" do
       user = Fabricate(:user, name: 'Billy Bob')
-      expect { described_class.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(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
     end
   end
 end

spec/services/username_changer_spec.rb

@@ -9,7 +9,7 @@ describe UsernameChanger do
       let(:new_username) { "#{user.username}1234" }
 
       before do
-        @result = described_class.change(user, new_username)
+        @result = UsernameChanger.change(user, new_username)
       end
 
       it 'returns true' do
@@ -33,7 +33,7 @@ describe UsernameChanger do
       let(:username_lower_before_change) { user.username_lower }
 
       before do
-        @result = described_class.change(user, wrong_username)
+        @result = UsernameChanger.change(user, wrong_username)
       end
 
       it 'returns false' do
@@ -55,16 +55,17 @@ describe UsernameChanger do
       let!(:myself) { Fabricate(:user, username: 'hansolo') }
 
       it 'should return true' do
-        expect(described_class.change(myself, "HanSolo")).to eq(true)
+        expect(UsernameChanger.change(myself, "HanSolo")).to eq(true)
       end
 
       it 'should change the username' do
-        described_class.change(myself, "HanSolo")
+        UsernameChanger.change(myself, "HanSolo")
         expect(myself.reload.username).to eq('HanSolo')
       end
 
       it "logs the action" do
-        expect { described_class.change(myself, "HanSolo", myself) }.to change { UserHistory.count }.by(1)
+        expect { UsernameChanger.change(myself, "HanSolo", myself) }.to change { UserHistory.count }.by(1)
+        expect { UsernameChanger.change(myself, "HanSolo", myself) }.to change { UserHistory.count }.by(0) # make sure it does not log a dupe
       end
     end
 
@@ -75,17 +76,17 @@ describe UsernameChanger do
       end
 
       it 'should allow a shorter username than default' do
-        result = described_class.change(user, 'a' * @custom_min)
+        result = UsernameChanger.change(user, 'a' * @custom_min)
         expect(result).not_to eq(false)
       end
 
       it 'should not allow a shorter username than limit' do
-        result = described_class.change(user, 'a' * (@custom_min - 1))
+        result = UsernameChanger.change(user, 'a' * (@custom_min - 1))
         expect(result).to eq(false)
       end
 
       it 'should not allow a longer username than limit' do
-        result = described_class.change(user, 'a' * (User.username_length.end + 1))
+        result = UsernameChanger.change(user, 'a' * (User.username_length.end + 1))
         expect(result).to eq(false)
       end
     end