FEATURE: Log username changes by staff

Also fix the tests for changing username

app/controllers/users_controller.rb

@@ -88,7 +88,8 @@ class UsersController < ApplicationController
     user = fetch_user_from_params
     guardian.ensure_can_edit_username!(user)
 
-    result = user.change_username(params[:new_username])
+    # TODO proper error surfacing (result is a Model#save call)
+    result = user.change_username(params[:new_username], current_user)
     raise Discourse::InvalidParameters.new(:new_username) unless result
 
     render json: {

app/models/user.rb

@@ -185,7 +185,11 @@ class User < ActiveRecord::Base
     Jobs.enqueue(:send_system_message, user_id: id, message_type: message_type)
   end
 
-  def change_username(new_username)
+  def change_username(new_username, actor=nil)
+    if actor && actor != self
+      StaffActionLogger.new(actor).log_username_change(self, self.username, new_username)
+    end
+
     self.username = new_username
     save
   end

app/models/user_history.rb

@@ -34,7 +34,8 @@ class UserHistory < ActiveRecord::Base
                           :delete_post,
                           :delete_topic,
                           :impersonate,
-                          :roll_up)
+                          :roll_up,
+                          :change_username)
   end
 
   # Staff actions is a subset of all actions, used to audit actions taken by staff users.
@@ -52,7 +53,8 @@ class UserHistory < ActiveRecord::Base
                         :delete_post,
                         :delete_topic,
                         :impersonate,
-                        :roll_up]
+                        :roll_up,
+                        :change_username]
   end
 
   def self.staff_action_ids

app/services/staff_action_logger.rb

@@ -100,6 +100,16 @@ class StaffActionLogger
     }))
   end
 
+  def log_username_change(user, old_username, new_username, opts={})
+    raise Discourse::InvalidParameters.new('user is nil') unless user
+    UserHistory.create( params(opts).merge({
+      action: UserHistory.actions[:change_username],
+      target_user_id: user.id,
+      previous_value: old_username,
+      new_value: new_username
+    }))
+  end
+
   def log_user_suspend(user, reason, opts={})
     raise Discourse::InvalidParameters.new('user is nil') unless user
     UserHistory.create( params(opts).merge({

config/locales/client.en.yml

@@ -1863,6 +1863,7 @@ en:
           actions:
             delete_user: "delete user"
             change_trust_level: "change trust level"
+            change_username: "change username"
             change_site_setting: "change site setting"
             change_site_customization: "change site customization"
             delete_site_customization: "delete site customization"

spec/controllers/users_controller_spec.rb

@@ -612,28 +612,59 @@ describe UsersController do
     end
 
     context 'while logged in' do
-      let!(:user) { log_in }
-      let(:new_username) { "#{user.username}1234" }
+      let(:old_username) { "OrigUsrname" }
+      let(:new_username) { "#{old_username}1234" }
+      let(:user) { Fabricate(:user, username: old_username) }
+
+      before do
+        user.username = old_username
+        log_in_user(user)
+      end
 
       it 'raises an error without a new_username param' do
         expect { xhr :put, :username, username: user.username }.to raise_error(ActionController::ParameterMissing)
+        user.reload.username.should == old_username
       end
 
       it 'raises an error when you don\'t have permission to change the username' do
         Guardian.any_instance.expects(:can_edit_username?).with(user).returns(false)
         xhr :put, :username, username: user.username, new_username: new_username
         expect(response).to be_forbidden
+        user.reload.username.should == old_username
       end
 
+      # Bad behavior, this should give a real JSON error, not an InvalidParameters
       it 'raises an error when change_username fails' do
-        User.any_instance.expects(:change_username).with(new_username).returns(false)
+        User.any_instance.expects(:save).returns(false)
         expect { xhr :put, :username, username: user.username, new_username: new_username }.to raise_error(Discourse::InvalidParameters)
+        user.reload.username.should == old_username
       end
 
-      it 'should succeed when the change_username returns true' do
-        User.any_instance.expects(:change_username).with(new_username).returns(true)
+      it 'should succeed in normal circumstances' do
         xhr :put, :username, username: user.username, new_username: new_username
-        expect(response).to be_success
+        response.should be_success
+        user.reload.username.should == new_username
+      end
+
+      pending 'should fail if the user is old', 'ensure_can_edit_username! is not throwing' do
+        # Older than the change period and >1 post
+        user.created_at = Time.now - (SiteSetting.username_change_period + 1).days
+        user.stubs(:post_count).returns(200)
+        Guardian.new(user).can_edit_username?(user).should == false
+
+        xhr :put, :username, username: user.username, new_username: new_username
+
+        response.should be_forbidden
+        user.reload.username.should == old_username
+      end
+
+      it 'should create a staff action log when a staff member changes the username' do
+        acting_user = Fabricate(:admin)
+        log_in_user(acting_user)
+        xhr :put, :username, username: user.username, new_username: new_username
+        response.should be_success
+        UserHistory.where(action: UserHistory.actions[:change_username], target_user_id: user.id, acting_user_id: acting_user.id).should be_present
+        user.reload.username.should == new_username
       end
 
       it 'should return a JSON response with the updated username' do