Log site setting changes and show in admin

app/assets/javascripts/admin/models/staff_action_log.js

@@ -15,14 +15,20 @@ Discourse.StaffActionLog = Discourse.Model.extend({
 
   formattedDetails: function() {
     var formatted = "";
-    if (this.get('email')) {
+    formatted += this.format('email', 'email');
-      formatted += "<b>" + I18n.t("email") + ":</b> " + this.get('email') + "<br/>";
+    formatted += this.format('admin.logs.staff_actions.ip_address', 'ip_address');
-    }
+    formatted += this.format('admin.logs.staff_actions.new_value', 'new_value');
-    if (this.get('ip_address')) {
+    formatted += this.format('admin.logs.staff_actions.previous_value', 'previous_value');
-      formatted += "<b>IP:</b> " + this.get('ip_address') + "<br/>";
-    }
     return formatted;
-  }.property('ip_address', 'email')
+  }.property('ip_address', 'email'),
+
+  format: function(label, propertyName) {
+    if (this.get(propertyName)) {
+      return ('<b>' + I18n.t(label) + ':</b> ' + this.get(propertyName) + '<br/>');
+    } else {
+      return '';
+    }
+  }
 });
 
 Discourse.StaffActionLog.reopenClass({

app/assets/javascripts/admin/templates/logs/staff_action_logs.js.handlebars

@@ -26,10 +26,10 @@
   <div class="heading-container">
     <div class="col heading first staff_user">{{i18n admin.logs.staff_actions.staff_user}}</div>
     <div class="col heading action">{{i18n admin.logs.action}}</div>
-    <div class="col heading target_user">{{i18n admin.logs.staff_actions.target_user}}</div>
+    <div class="col heading subject">{{i18n admin.logs.staff_actions.subject}}</div>
     <div class="col heading created_at">{{i18n admin.logs.staff_actions.when}}</div>
-    <div class="col heading context">{{i18n admin.logs.staff_actions.context}}</div>
     <div class="col heading details">{{i18n admin.logs.staff_actions.details}}</div>
+    <div class="col heading context">{{i18n admin.logs.staff_actions.context}}</div>
     <div class="clearfix"></div>
   </div>
 

app/assets/javascripts/admin/templates/logs/staff_action_logs_list_item.js.handlebars

@@ -5,16 +5,16 @@
 <div class="col value action">
   <a {{action filterByAction action_name}}>{{actionName}}</a>
 </div>
-<div class="col value target_user">
+<div class="col value subject">
   {{#if target_user}}
     {{#linkTo 'adminUser' target_user}}{{avatar target_user imageSize="tiny"}}{{/linkTo}}
     <a {{action filterByTargetUser target_user}}>{{target_user.username}}</a>
-  {{else}}
+  {{/if}}
-    &mdash;
+  {{#if subject}}
+    <span {{bindAttr title="subject"}}>{{subject}}</span>
   {{/if}}
 </div>
 <div class="col value created_at">{{unboundAgeWithTooltip created_at}}</div>
-<div class="col value context">{{context}}</div>
 <div class="col value details">
   {{{formattedDetails}}}
   {{#if showFullDetails}}
@@ -24,4 +24,5 @@
     <a {{action toggleFullDetails this}}>{{i18n more}}</a>
   {{/if}}
 </div>
+<div class="col value context">{{context}}</div>
 <div class="clearfix"></div>

app/assets/stylesheets/admin/admin_base.scss

@@ -716,9 +716,12 @@ table {
   .action {
     width: 120px;
   }
-  .staff_user, .target_user {
+  .staff_user {
     width: 100px;
   }
+  .subject {
+    width: 200px;
+  }
   .created_at {
     width: 50px;
   }
@@ -729,7 +732,7 @@ table {
     text-align: center;
   }
   .details {
-    width: 400px;
+    width: 300px;
     a {
       text-decoration: underline;
     }

app/controllers/admin/site_settings_controller.rb

@@ -8,6 +8,7 @@ class Admin::SiteSettingsController < Admin::AdminController
 
   def update
     raise ActionController::ParameterMissing.new(:value) unless params.has_key?(:value)
+    StaffActionLogger.new(current_user).log_site_setting_change(params[:id], SiteSetting.send("#{params[:id]}"), params[:value]) if SiteSetting.respond_to?(params[:id])
     SiteSetting.send("#{params[:id]}=", params[:value])
     render nothing: true
   end

app/models/staff_action_log.rb

@@ -9,7 +9,7 @@ class StaffActionLog < ActiveRecord::Base
   validates_presence_of :action
 
   def self.actions
-    @actions ||= Enum.new(:delete_user, :change_trust_level)
+    @actions ||= Enum.new(:delete_user, :change_trust_level, :change_site_setting)
   end
 
   def self.with_filters(filters)

app/serializers/staff_action_log_serializer.rb

@@ -4,7 +4,10 @@ class StaffActionLogSerializer < ApplicationSerializer
              :context,
              :ip_address,
              :email,
-             :created_at
+             :created_at,
+             :subject,
+             :previous_value,
+             :new_value
 
   has_one :staff_user,  serializer: BasicUserSerializer, embed: :objects
   has_one :target_user, serializer: BasicUserSerializer, embed: :objects

app/services/staff_action_logger.rb

@@ -7,26 +7,39 @@ class StaffActionLogger
 
   def log_user_deletion(deleted_user, opts={})
     raise Discourse::InvalidParameters.new('user is nil') unless deleted_user and deleted_user.is_a?(User)
-    StaffActionLog.create(
+    StaffActionLog.create( params(opts).merge({
       action: StaffActionLog.actions[:delete_user],
-      context: opts[:context], # should be the url from where the staff member deleted the user
-      staff_user_id: @admin.id,
       target_user_id: deleted_user.id,
       email: deleted_user.email,
       ip_address: deleted_user.ip_address,
       details: [:id, :username, :name, :created_at, :trust_level, :last_seen_at, :last_emailed_at].map { |x| "#{x}: #{deleted_user.send(x)}" }.join(', ')
-    )
+    }))
   end
 
   def log_trust_level_change(user, old_trust_level, new_trust_level, opts={})
     raise Discourse::InvalidParameters.new('user is nil') unless user and user.is_a?(User)
     raise Discourse::InvalidParameters.new('old trust level is invalid') unless TrustLevel.levels.values.include? old_trust_level
     raise Discourse::InvalidParameters.new('new trust level is invalid') unless TrustLevel.levels.values.include? new_trust_level
-    StaffActionLog.create!(
+    StaffActionLog.create!( params(opts).merge({
       action: StaffActionLog.actions[:change_trust_level],
-      staff_user_id: @admin.id,
       target_user_id: user.id,
       details: "old trust level: #{old_trust_level}, new trust level: #{new_trust_level}"
-    )
+    }))
+  end
+
+  def log_site_setting_change(setting_name, previous_value, new_value, opts={})
+    raise Discourse::InvalidParameters.new('setting_name is invalid') unless setting_name.present? and SiteSetting.respond_to?(setting_name)
+    StaffActionLog.create( params(opts).merge({
+      action: StaffActionLog.actions[:change_site_setting],
+      subject: setting_name,
+      previous_value: previous_value,
+      new_value: new_value
+    }))
+  end
+
+  private
+
+  def params(opts)
+    {staff_user_id: @admin.id, context: opts[:context]}
   end
 end

config/locales/client.en.yml

@@ -1192,12 +1192,17 @@ en:
           clear_filters: "Show Everything"
           staff_user: "Staff User"
           target_user: "Target User"
+          subject: "Subject"
           when: "When"
           context: "Context"
           details: "Details"
+          ip_address: "IP"
+          previous_value: "Previous"
+          new_value: "New"
           actions:
             delete_user: "delete user"
             change_trust_level: "change trust level"
+            change_site_setting: "change site setting"
         screened_emails:
           title: "Screened Emails"
           description: "When someone tries to create a new account, the following email addresses will be checked and the registration will be blocked, or some other action performed."

db/migrate/20130819192358_add_value_columns_to_staff_action_logs.rb

@@ -0,0 +1,7 @@
+class AddValueColumnsToStaffActionLogs < ActiveRecord::Migration
+  def change
+    add_column :staff_action_logs, :subject, :text
+    add_column :staff_action_logs, :previous_value, :text
+    add_column :staff_action_logs, :new_value, :text
+  end
+end

spec/controllers/admin/site_settings_controller_spec.rb

@@ -38,6 +38,13 @@ describe Admin::SiteSettingsController do
         SiteSetting.expects(:'test_setting=').with('').once
         xhr :put, :update, id: 'test_setting', value: ''
       end
+
+      it 'logs the change' do
+        SiteSetting.stubs(:test_setting).returns('previous')
+        SiteSetting.expects(:'test_setting=').with('hello').once
+        StaffActionLogger.any_instance.expects(:log_site_setting_change).with('test_setting', 'previous', 'hello')
+        xhr :put, :update, id: 'test_setting', value: 'hello'
+      end
     end
 
   end

spec/services/staff_action_logger_spec.rb

@@ -2,6 +2,9 @@ require 'spec_helper'
 
 describe StaffActionLogger do
 
+  let(:admin)  { Fabricate(:admin) }
+  let(:logger) { described_class.new(admin) }
+
   describe 'new' do
     it 'raises an error when user is nil' do
       expect { described_class.new(nil) }.to raise_error(Discourse::InvalidParameters)
@@ -13,17 +16,16 @@ describe StaffActionLogger do
   end
 
   describe 'log_user_deletion' do
-    let(:admin) { Fabricate(:admin) }
     let(:deleted_user) { Fabricate(:user) }
 
     subject(:log_user_deletion) { described_class.new(admin).log_user_deletion(deleted_user) }
 
     it 'raises an error when user is nil' do
-      expect { described_class.new(admin).log_user_deletion(nil) }.to raise_error(Discourse::InvalidParameters)
+      expect { logger.log_user_deletion(nil) }.to raise_error(Discourse::InvalidParameters)
     end
 
     it 'raises an error when user is not a User' do
-      expect { described_class.new(admin).log_user_deletion(1) }.to raise_error(Discourse::InvalidParameters)
+      expect { logger.log_user_deletion(1) }.to raise_error(Discourse::InvalidParameters)
     end
 
     it 'creates a new StaffActionLog record' do
@@ -33,7 +35,6 @@ describe StaffActionLogger do
   end
 
   describe 'log_trust_level_change' do
-    let(:admin) { Fabricate(:admin) }
     let(:user) { Fabricate(:user) }
     let(:old_trust_level) { TrustLevel.levels[:newuser] }
     let(:new_trust_level) { TrustLevel.levels[:basic] }
@@ -41,18 +42,18 @@ describe StaffActionLogger do
     subject(:log_trust_level_change) { described_class.new(admin).log_trust_level_change(user, old_trust_level, new_trust_level) }
 
     it 'raises an error when user or trust level is nil' do
-      expect { described_class.new(admin).log_trust_level_change(nil, old_trust_level, new_trust_level) }.to raise_error(Discourse::InvalidParameters)
+      expect { logger.log_trust_level_change(nil, old_trust_level, new_trust_level) }.to raise_error(Discourse::InvalidParameters)
-      expect { described_class.new(admin).log_trust_level_change(user, nil, new_trust_level) }.to raise_error(Discourse::InvalidParameters)
+      expect { logger.log_trust_level_change(user, nil, new_trust_level) }.to raise_error(Discourse::InvalidParameters)
-      expect { described_class.new(admin).log_trust_level_change(user, old_trust_level, nil) }.to raise_error(Discourse::InvalidParameters)
+      expect { logger.log_trust_level_change(user, old_trust_level, nil) }.to raise_error(Discourse::InvalidParameters)
     end
 
     it 'raises an error when user is not a User' do
-      expect { described_class.new(admin).log_trust_level_change(1, old_trust_level, new_trust_level) }.to raise_error(Discourse::InvalidParameters)
+      expect { logger.log_trust_level_change(1, old_trust_level, new_trust_level) }.to raise_error(Discourse::InvalidParameters)
     end
 
     it 'raises an error when new trust level is not a Trust Level' do
       max_level = TrustLevel.levels.values.max
-      expect { described_class.new(admin).log_trust_level_change(user, old_trust_level, max_level + 1) }.to raise_error(Discourse::InvalidParameters)
+      expect { logger.log_trust_level_change(user, old_trust_level, max_level + 1) }.to raise_error(Discourse::InvalidParameters)
     end
 
     it 'creates a new StaffActionLog record' do
@@ -60,4 +61,16 @@ describe StaffActionLogger do
       StaffActionLog.last.details.should include "new trust level: #{new_trust_level}"
     end
   end
+
+  describe "log_site_setting_change" do
+    it "raises an error when params are invalid" do
+      SiteSetting.stubs(:respond_to?).with('abc').returns(false)
+      expect { logger.log_site_setting_change(nil, '1', '2') }.to raise_error(Discourse::InvalidParameters)
+      expect { logger.log_site_setting_change('abc', '1', '2') }.to raise_error(Discourse::InvalidParameters)
+    end
+
+    it "creates a new StaffActionLog record" do
+      expect { logger.log_site_setting_change('title', 'Discourse', 'My Site') }.to change { StaffActionLog.count }.by(1)
+    end
+  end
 end