diff --git a/app/assets/javascripts/admin/models/staff_action_log.js b/app/assets/javascripts/admin/models/staff_action_log.js index 3b2f2a733..75c1e3174 100644 --- a/app/assets/javascripts/admin/models/staff_action_log.js +++ b/app/assets/javascripts/admin/models/staff_action_log.js @@ -17,6 +17,8 @@ Discourse.StaffActionLog = Discourse.Model.extend({ var formatted = ""; formatted += this.format('email', 'email'); formatted += this.format('admin.logs.ip_address', 'ip_address'); + formatted += this.format('admin.logs.topic_id', 'topic_id'); + formatted += this.format('admin.logs.post_id', 'post_id'); if (!this.get('useCustomModalForDetails')) { formatted += this.format('admin.logs.staff_actions.new_value', 'new_value'); formatted += this.format('admin.logs.staff_actions.previous_value', 'previous_value'); @@ -25,7 +27,7 @@ Discourse.StaffActionLog = Discourse.Model.extend({ if (this.get('details')) formatted += Handlebars.Utils.escapeExpression(this.get('details')) + '
'; } return formatted; - }.property('ip_address', 'email'), + }.property('ip_address', 'email', 'topic_id', 'post_id'), format: function(label, propertyName) { if (this.get(propertyName)) { diff --git a/app/assets/javascripts/discourse/models/_post.js b/app/assets/javascripts/discourse/models/_post.js index b9064f0bc..1027ca98b 100644 --- a/app/assets/javascripts/discourse/models/_post.js +++ b/app/assets/javascripts/discourse/models/_post.js @@ -280,7 +280,10 @@ Discourse.Post = Discourse.Model.extend({ **/ destroy: function(deletedBy) { this.setDeletedState(deletedBy); - return Discourse.ajax("/posts/" + (this.get('id')), { type: 'DELETE' }); + return Discourse.ajax("/posts/" + this.get('id'), { + data: { context: window.location.pathname }, + type: 'DELETE' + }); }, /** diff --git a/app/assets/javascripts/discourse/models/topic.js b/app/assets/javascripts/discourse/models/topic.js index 9eab5f1a9..40c0d9eb8 100644 --- a/app/assets/javascripts/discourse/models/topic.js +++ b/app/assets/javascripts/discourse/models/topic.js @@ -215,7 +215,10 @@ Discourse.Topic = Discourse.Model.extend({ 'details.can_delete': false, 'details.can_recover': true }); - return Discourse.ajax("/t/" + this.get('id'), { type: 'DELETE' }); + return Discourse.ajax("/t/" + this.get('id'), { + data: { context: window.location.pathname }, + type: 'DELETE' + }); }, // Recover this topic if deleted diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index dab903866..1812d5978 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -167,7 +167,7 @@ class PostsController < ApplicationController guardian.ensure_can_delete!(post) - destroyer = PostDestroyer.new(current_user, post) + destroyer = PostDestroyer.new(current_user, post, { context: params[:context] }) destroyer.destroy render nothing: true diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index f9df8031a..e89cc0812 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -211,7 +211,7 @@ class TopicsController < ApplicationController guardian.ensure_can_delete!(topic) first_post = topic.ordered_posts.first - PostDestroyer.new(current_user, first_post).destroy + PostDestroyer.new(current_user, first_post, { context: params[:context] }).destroy render nothing: true end diff --git a/app/models/user_history.rb b/app/models/user_history.rb index db5e42023..a5f8a6116 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -5,6 +5,9 @@ class UserHistory < ActiveRecord::Base belongs_to :acting_user, class_name: 'User' belongs_to :target_user, class_name: 'User' + belongs_to :post + belongs_to :topic + validates_presence_of :action scope :only_staff_actions, ->{ where("action IN (?)", UserHistory.staff_action_ids) } @@ -12,22 +15,24 @@ class UserHistory < ActiveRecord::Base before_save :set_admin_only def self.actions - @actions ||= Enum.new( :delete_user, - :change_trust_level, - :change_site_setting, - :change_site_customization, - :delete_site_customization, - :checked_for_custom_avatar, - :notified_about_avatar, - :notified_about_sequential_replies, - :notified_about_dominating_topic, - :suspend_user, - :unsuspend_user, - :facebook_no_email, - :grant_badge, - :revoke_badge, - :auto_trust_level_change, - :check_email) + @actions ||= Enum.new(:delete_user, + :change_trust_level, + :change_site_setting, + :change_site_customization, + :delete_site_customization, + :checked_for_custom_avatar, + :notified_about_avatar, + :notified_about_sequential_replies, + :notified_about_dominating_topic, + :suspend_user, + :unsuspend_user, + :facebook_no_email, + :grant_badge, + :revoke_badge, + :auto_trust_level_change, + :check_email, + :delete_post, + :delete_topic) end # Staff actions is a subset of all actions, used to audit actions taken by staff users. @@ -41,7 +46,9 @@ class UserHistory < ActiveRecord::Base :unsuspend_user, :grant_badge, :revoke_badge, - :check_email] + :check_email, + :delete_post, + :delete_topic] end def self.staff_action_ids diff --git a/app/serializers/user_history_serializer.rb b/app/serializers/user_history_serializer.rb index 14041d659..b4e29e294 100644 --- a/app/serializers/user_history_serializer.rb +++ b/app/serializers/user_history_serializer.rb @@ -7,7 +7,9 @@ class UserHistorySerializer < ApplicationSerializer :created_at, :subject, :previous_value, - :new_value + :new_value, + :topic_id, + :post_id has_one :acting_user, serializer: BasicUserSerializer, embed: :objects has_one :target_user, serializer: BasicUserSerializer, embed: :objects @@ -31,4 +33,4 @@ class UserHistorySerializer < ApplicationSerializer nil end end -end \ No newline at end of file +end diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index 5da07806f..bc99302b9 100644 --- a/app/services/staff_action_logger.rb +++ b/app/services/staff_action_logger.rb @@ -2,32 +2,74 @@ class StaffActionLogger def initialize(admin) @admin = admin - raise Discourse::InvalidParameters.new('admin is nil') unless @admin and @admin.is_a?(User) + raise Discourse::InvalidParameters.new('admin is nil') unless @admin && @admin.is_a?(User) end def log_user_deletion(deleted_user, opts={}) - raise Discourse::InvalidParameters.new('user is nil') unless deleted_user and deleted_user.is_a?(User) + raise Discourse::InvalidParameters.new('user is nil') unless deleted_user && deleted_user.is_a?(User) UserHistory.create( params(opts).merge({ action: UserHistory.actions[:delete_user], email: deleted_user.email, ip_address: deleted_user.ip_address.to_s, - details: [:id, :username, :name, :created_at, :trust_level, :last_seen_at, :last_emailed_at].map { |x| "#{x}: #{deleted_user.send(x)}" }.join(', ') + details: [:id, :username, :name, :created_at, :trust_level, :last_seen_at, :last_emailed_at].map { |x| "#{x}: #{deleted_user.send(x)}" }.join("\n") + })) + end + + def log_post_deletion(deleted_post, opts={}) + raise Discourse::InvalidParameters.new("post is nil") unless deleted_post && deleted_post.is_a?(Post) + + topic = deleted_post.topic || Topic.with_deleted.find(deleted_post.topic_id) + + details = [ + "id: #{deleted_post.id}", + "created_at: #{deleted_post.created_at}", + "user: #{deleted_post.user.username} (#{deleted_post.user.name})", + "topic: #{topic.title}", + "post_number: #{deleted_post.post_number}", + "raw: #{deleted_post.raw}" + ] + + UserHistory.create(params(opts).merge({ + action: UserHistory.actions[:delete_post], + post_id: deleted_post.id, + details: details.join("\n") + })) + end + + def log_topic_deletion(deleted_topic, opts={}) + raise Discourse::InvalidParameters.new("topic is nil") unless deleted_topic && deleted_topic.is_a?(Topic) + + details = [ + "id: #{deleted_topic.id}", + "created_at: #{deleted_topic.created_at}", + "user: #{deleted_topic.user.username} (#{deleted_topic.user.name})", + "title: #{deleted_topic.title}" + ] + + if first_post = deleted_topic.ordered_posts.first + details << "raw: #{first_post.raw}" + end + + UserHistory.create(params(opts).merge({ + action: UserHistory.actions[:delete_topic], + topic_id: deleted_topic.id, + details: details.join("\n") })) 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('user is nil') unless user && user.is_a?(User) raise Discourse::InvalidParameters.new('old trust level is invalid') unless TrustLevel.valid? old_trust_level raise Discourse::InvalidParameters.new('new trust level is invalid') unless TrustLevel.valid? new_trust_level UserHistory.create!( params(opts).merge({ action: UserHistory.actions[:change_trust_level], target_user_id: user.id, - details: "old trust level: #{old_trust_level}, new trust level: #{new_trust_level}" + details: "old trust level: #{old_trust_level}\nnew 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) + raise Discourse::InvalidParameters.new('setting_name is invalid') unless setting_name.present? && SiteSetting.respond_to?(setting_name) UserHistory.create( params(opts).merge({ action: UserHistory.actions[:change_site_setting], subject: setting_name, diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 68f36c3b1..b9ca35722 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1768,6 +1768,8 @@ en: last_match_at: "Last Matched" match_count: "Matches" ip_address: "IP" + topic_id: "Topic ID" + post_id: "Post ID" delete: 'Delete' edit: 'Edit' save: 'Save' @@ -1802,6 +1804,8 @@ en: grant_badge: "grant badge" revoke_badge: "revoke badge" check_email: "check email" + delete_topic: "delete topic" + delete_post: "delete post" 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." diff --git a/db/migrate/20141001101041_add_post_id_to_user_histories.rb b/db/migrate/20141001101041_add_post_id_to_user_histories.rb new file mode 100644 index 000000000..049ad95a7 --- /dev/null +++ b/db/migrate/20141001101041_add_post_id_to_user_histories.rb @@ -0,0 +1,5 @@ +class AddPostIdToUserHistories < ActiveRecord::Migration + def change + add_column :user_histories, :post_id, :integer + end +end diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index 783242c44..698b587e7 100644 --- a/lib/post_destroyer.rb +++ b/lib/post_destroyer.rb @@ -33,10 +33,11 @@ class PostDestroyer end end - def initialize(user, post) + def initialize(user, post, opts={}) @user = user @post = post @topic = post.topic if post + @opts = opts end def destroy @@ -79,7 +80,12 @@ class PostDestroyer @post.update_flagged_posts_count remove_associated_replies remove_associated_notifications - @post.topic.trash!(@user) if @post.topic && @post.post_number == 1 + if @post.topic && @post.post_number == 1 + StaffActionLogger.new(@user).log_topic_deletion(@post.topic, @opts.slice(:context)) + @post.topic.trash!(@user) + else + StaffActionLogger.new(@user).log_post_deletion(@post, @opts.slice(:context)) + end update_associated_category_latest_topic update_user_counts end diff --git a/spec/components/post_destroyer_spec.rb b/spec/components/post_destroyer_spec.rb index b25baff23..3ffa058bb 100644 --- a/spec/components/post_destroyer_spec.rb +++ b/spec/components/post_destroyer_spec.rb @@ -170,6 +170,12 @@ describe PostDestroyer do author.reload }.to change { author.post_count }.by(-1) end + + it "creates a new user history entry" do + expect { + PostDestroyer.new(moderator, post).destroy + }.to change { UserHistory.count}.by(1) + end end context "as an admin" do @@ -258,6 +264,12 @@ describe PostDestroyer do post.deleted_at.should be_present post.deleted_by.should == admin end + + it "creates a new user history entry" do + expect { + PostDestroyer.new(admin, post).destroy + }.to change { UserHistory.count}.by(1) + end end end diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index 75c26d5bb..4c3faf7ee 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/spec/controllers/posts_controller_spec.rb @@ -143,7 +143,7 @@ describe PostsController do it "uses a PostDestroyer" do destroyer = mock - PostDestroyer.expects(:new).with(user, post).returns(destroyer) + PostDestroyer.expects(:new).returns(destroyer) destroyer.expects(:destroy) xhr :delete, :destroy, id: post.id end diff --git a/spec/services/staff_action_logger_spec.rb b/spec/services/staff_action_logger_spec.rb index e56c05dda..937e59544 100644 --- a/spec/services/staff_action_logger_spec.rb +++ b/spec/services/staff_action_logger_spec.rb @@ -33,6 +33,42 @@ describe StaffActionLogger do end end + describe 'log_post_deletion' do + let(:deleted_post) { Fabricate(:post) } + + subject(:log_post_deletion) { described_class.new(admin).log_post_deletion(deleted_post) } + + it 'raises an error when post is nil' do + expect { logger.log_post_deletion(nil) }.to raise_error(Discourse::InvalidParameters) + end + + it 'raises an error when post is not a Post' do + expect { logger.log_post_deletion(1) }.to raise_error(Discourse::InvalidParameters) + end + + it 'creates a new UserHistory record' do + expect { log_post_deletion }.to change { UserHistory.count }.by(1) + end + end + + describe 'log_topic_deletion' do + let(:deleted_topic) { Fabricate(:topic) } + + subject(:log_topic_deletion) { described_class.new(admin).log_topic_deletion(deleted_topic) } + + it 'raises an error when topic is nil' do + expect { logger.log_topic_deletion(nil) }.to raise_error(Discourse::InvalidParameters) + end + + it 'raises an error when topic is not a Topic' do + expect { logger.log_topic_deletion(1) }.to raise_error(Discourse::InvalidParameters) + end + + it 'creates a new UserHistory record' do + expect { log_topic_deletion }.to change { UserHistory.count }.by(1) + end + end + describe 'log_trust_level_change' do let(:user) { Fabricate(:user) } let(:old_trust_level) { TrustLevel[0] }