From b7327942afb7d97ce6d3fd2e648c4c599e2f66f8 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 9 Jul 2013 15:20:18 -0400 Subject: [PATCH] Add `deleted_by` to `Trashable` tables --- .../javascripts/admin/models/flagged_post.js | 9 +++---- app/controllers/invites_controller.rb | 2 +- app/controllers/topics_controller.rb | 2 +- app/models/post.rb | 4 +-- app/models/post_action.rb | 8 +++--- app/models/topic.rb | 4 +-- ...130709184941_add_deleted_by_id_to_posts.rb | 8 ++++++ lib/post_destroyer.rb | 8 +++--- lib/trashable.rb | 26 +++++++++++++------ spec/components/guardian_spec.rb | 6 +++-- spec/components/post_destroyer_spec.rb | 3 +++ spec/controllers/invites_controller_spec.rb | 2 +- .../post_actions_controller_spec.rb | 2 +- spec/controllers/posts_controller_spec.rb | 5 ++-- spec/integration/spam_rules_spec.rb | 2 +- spec/models/notification_spec.rb | 2 +- spec/models/topic_spec.rb | 5 ++-- spec/models/user_action_spec.rb | 2 +- 18 files changed, 62 insertions(+), 38 deletions(-) create mode 100644 db/migrate/20130709184941_add_deleted_by_id_to_posts.rb diff --git a/app/assets/javascripts/admin/models/flagged_post.js b/app/assets/javascripts/admin/models/flagged_post.js index 7c3a95cf3..4acc260d4 100644 --- a/app/assets/javascripts/admin/models/flagged_post.js +++ b/app/assets/javascripts/admin/models/flagged_post.js @@ -77,9 +77,7 @@ Discourse.FlaggedPost = Discourse.Post.extend({ return Discourse.ajax('/admin/flags/agree/' + this.id, { type: 'POST', cache: false }); }, - postHidden: function() { - return (this.get('hidden')); - }.property(), + postHidden: Em.computed.alias('hidden'), extraClasses: function() { var classes = []; @@ -92,9 +90,8 @@ Discourse.FlaggedPost = Discourse.Post.extend({ return classes.join(' '); }.property(), - deleted: function() { - return (this.get('deleted_at') || this.get('topic_deleted_at')); - }.property() + deleted: Em.computed.or('deleted_at', 'topic_deleted_at') + }); Discourse.FlaggedPost.reopenClass({ diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 85b3c1f6e..238fe8b17 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -32,7 +32,7 @@ class InvitesController < ApplicationController invite = Invite.where(invited_by_id: current_user.id, email: params[:email]).first raise Discourse::InvalidParameters.new(:email) if invite.blank? - invite.trash! + invite.trash!(current_user) render nothing: true end diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index b66eb3f0a..53c07e6d1 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -169,7 +169,7 @@ class TopicsController < ApplicationController def destroy topic = Topic.where(id: params[:id]).first guardian.ensure_can_delete!(topic) - topic.trash! + topic.trash!(current_user) render nothing: true end diff --git a/app/models/post.rb b/app/models/post.rb index b94b75eac..ece65e770 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -54,9 +54,9 @@ class Post < ActiveRecord::Base @types ||= Enum.new(:regular, :moderator_action) end - def trash! + def trash!(trashed_by=nil) self.topic_links.each(&:destroy) - super + super(trashed_by) end def recover! diff --git a/app/models/post_action.rb b/app/models/post_action.rb index a83cea166..9938cddee 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -63,7 +63,7 @@ class PostAction < ActiveRecord::Base moderator_id == -1 ? PostActionType.auto_action_flag_types.values : PostActionType.flag_types.values end - PostAction.where({ post_id: post.id, post_action_type_id: actions }).update_all({ deleted_at: Time.zone.now, deleted_by: moderator_id }) + PostAction.where({ post_id: post.id, post_action_type_id: actions }).update_all({ deleted_at: Time.zone.now, deleted_by_id: moderator_id }) f = actions.map{|t| ["#{PostActionType.types[t]}_count", 0]} Post.where(id: post.id).with_deleted.update_all(Hash[*f.flatten]) update_flagged_posts_count @@ -140,13 +140,13 @@ class PostAction < ActiveRecord::Base user_id: user.id, post_action_type_id: post_action_type_id).first - action.trash! + action.trash!(user) action.run_callbacks(:save) end end def remove_act!(user) - trash! + trash!(user) run_callbacks(:save) end @@ -390,7 +390,7 @@ end # deleted_at :datetime # created_at :datetime not null # updated_at :datetime not null -# deleted_by :integer +# deleted_by_id :integer # message :text # related_post_id :integer # staff_took_action :boolean default(FALSE), not null diff --git a/app/models/topic.rb b/app/models/topic.rb index e9cb55314..f2702df10 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -21,9 +21,9 @@ class Topic < ActiveRecord::Base versioned if: :new_version_required? - def trash! + def trash!(trashed_by=nil) update_category_topic_count_by(-1) if deleted_at.nil? - super + super(trashed_by) update_flagged_posts_count end diff --git a/db/migrate/20130709184941_add_deleted_by_id_to_posts.rb b/db/migrate/20130709184941_add_deleted_by_id_to_posts.rb new file mode 100644 index 000000000..7f725b198 --- /dev/null +++ b/db/migrate/20130709184941_add_deleted_by_id_to_posts.rb @@ -0,0 +1,8 @@ +class AddDeletedByIdToPosts < ActiveRecord::Migration + def change + add_column :posts, :deleted_by_id, :integer, null: true + add_column :topics, :deleted_by_id, :integer, null: true + add_column :invites, :deleted_by_id, :integer, null: true + rename_column :post_actions, :deleted_by, :deleted_by_id + end +end diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index e15e501e4..7f011f5c3 100644 --- a/lib/post_destroyer.rb +++ b/lib/post_destroyer.rb @@ -37,12 +37,14 @@ class PostDestroyer # Feature users in the topic Jobs.enqueue(:feature_topic_users, topic_id: @post.topic_id, except_post_id: @post.id) - @post.post_actions.map(&:trash!) + @post.post_actions.each do |pa| + pa.trash!(@user) + end f = PostActionType.types.map{|k,v| ["#{k}_count", 0]} Post.with_deleted.where(id: @post.id).update_all(Hash[*f.flatten]) - @post.trash! + @post.trash!(@user) Topic.reset_highest(@post.topic_id) @@ -59,7 +61,7 @@ class PostDestroyer # Remove any notifications that point to this deleted post Notification.delete_all topic_id: @post.topic_id, post_number: @post.post_number - @post.topic.trash! if @post.post_number == 1 + @post.topic.trash!(@user) if @post.post_number == 1 end end diff --git a/lib/trashable.rb b/lib/trashable.rb index 13767b5d2..2d763c33f 100644 --- a/lib/trashable.rb +++ b/lib/trashable.rb @@ -5,6 +5,8 @@ module Trashable default_scope where(with_deleted_scope_sql) # scope unscoped does not work + + belongs_to :deleted_by, class_name: 'User' end @@ -24,23 +26,31 @@ module Trashable end end - def trash! + def trash!(trashed_by=nil) # note, an argument could be made that the column should probably called trashed_at # however, deleted_at is the terminology used in the UI # # we could hijack use a delete! and delete - redirecting the originals elsewhere, but that is # confusing as well. So for now, we go with trash! # - update_column(:deleted_at, DateTime.now) + trash_update(DateTime.now, trashed_by.try(:id)) end def recover! - # see: https://github.com/rails/rails/issues/8436 - # - # Fixed in Rails 4 - # - self.class.unscoped.where(id: self.id).update_all({deleted_at: nil}) - raw_write_attribute :deleted_at, nil + trash_update(nil, nil) end + + private + + def trash_update(deleted_at, deleted_by_id) + # see: https://github.com/rails/rails/issues/8436 + # + # Fixed in Rails 4 + # + self.class.unscoped.where(id: self.id).update_all(deleted_at: deleted_at, deleted_by_id: deleted_by_id) + raw_write_attribute :deleted_at, deleted_at + raw_write_attribute :deleted_by_id, deleted_by_id + end + end diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 049a5966e..b28612b91 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -229,20 +229,22 @@ describe Guardian do end describe 'a Post' do + + let(:another_admin) { Fabricate(:admin) } it 'correctly handles post visibility' do post = Fabricate(:post) topic = post.topic Guardian.new(user).can_see?(post).should be_true - post.trash! + post.trash!(another_admin) post.reload Guardian.new(user).can_see?(post).should be_false Guardian.new(admin).can_see?(post).should be_true post.recover! post.reload - topic.trash! + topic.trash!(another_admin) topic.reload Guardian.new(user).can_see?(post).should be_false Guardian.new(admin).can_see?(post).should be_true diff --git a/spec/components/post_destroyer_spec.rb b/spec/components/post_destroyer_spec.rb index 357edf0e9..6ed1f15d9 100644 --- a/spec/components/post_destroyer_spec.rb +++ b/spec/components/post_destroyer_spec.rb @@ -23,6 +23,7 @@ describe PostDestroyer do it "doesn't delete the post" do post.deleted_at.should be_blank + post.deleted_by.should be_blank post.raw.should == I18n.t('js.post.deleted_by_author') post.version.should == 2 end @@ -35,6 +36,7 @@ describe PostDestroyer do it "deletes the post" do post.deleted_at.should be_present + post.deleted_by.should == moderator end end @@ -45,6 +47,7 @@ describe PostDestroyer do it "deletes the post" do post.deleted_at.should be_present + post.deleted_by.should == admin end end diff --git a/spec/controllers/invites_controller_spec.rb b/spec/controllers/invites_controller_spec.rb index 88e4978fc..79152e2ac 100644 --- a/spec/controllers/invites_controller_spec.rb +++ b/spec/controllers/invites_controller_spec.rb @@ -29,7 +29,7 @@ describe InvitesController do end it "destroys the invite" do - Invite.any_instance.expects(:trash!) + Invite.any_instance.expects(:trash!).with(user) delete :destroy, email: invite.email end diff --git a/spec/controllers/post_actions_controller_spec.rb b/spec/controllers/post_actions_controller_spec.rb index 9c7b8e8df..aa81cfc2d 100644 --- a/spec/controllers/post_actions_controller_spec.rb +++ b/spec/controllers/post_actions_controller_spec.rb @@ -137,7 +137,7 @@ describe PostActionsController do end it "works with a deleted post" do - flagged_post.trash! + flagged_post.trash!(user) xhr :post, :clear_flags, id: flagged_post.id, post_action_type_id: PostActionType.types[:spam] response.should be_success end diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index 434fc772f..cb88c55bc 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/spec/controllers/posts_controller_spec.rb @@ -14,7 +14,8 @@ describe PostsController do describe 'show' do - let(:post) { Fabricate(:post, user: log_in) } + let(:user) { log_in } + let(:post) { Fabricate(:post, user: user) } it 'ensures the user can see the post' do Guardian.any_instance.expects(:can_see?).with(post).returns(false) @@ -30,7 +31,7 @@ describe PostsController do context "deleted post" do before do - post.trash! + post.trash!(user) end it "can't find deleted posts as an anonymous user" do diff --git a/spec/integration/spam_rules_spec.rb b/spec/integration/spam_rules_spec.rb index 1fdba0628..df028ccbf 100644 --- a/spec/integration/spam_rules_spec.rb +++ b/spec/integration/spam_rules_spec.rb @@ -58,7 +58,7 @@ describe PostAction do end context "a post is deleted" do - When { spam_post.trash!; spammer.reload } + When { spam_post.trash!(moderator); spammer.reload } Then { expect(spammer.reload).to be_blocked } end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index d00a824b2..134725890 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -215,7 +215,7 @@ describe Notification do Notification.create!(read: false, user_id: p2.user_id, topic_id: p2.topic_id, post_number: p2.post_number, data: '[]', notification_type: Notification.types[:liked]) - p2.trash! + p2.trash!(p.user) # we may want to make notification "trashable" but for now we nuke pm notifications from deleted topics/posts Notification.ensure_consistency! diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index bb3422056..fef822306 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1296,16 +1296,17 @@ describe Topic do describe 'trash!' do context "its category's topic count" do + let(:moderator) { Fabricate(:moderator) } let(:category) { Fabricate(:category) } it "subtracts 1 if topic is being deleted" do topic = Fabricate(:topic, category: category) - expect { topic.trash! }.to change { category.reload.topic_count }.by(-1) + expect { topic.trash!(moderator) }.to change { category.reload.topic_count }.by(-1) end it "doesn't subtract 1 if topic is already deleted" do topic = Fabricate(:topic, category: category, deleted_at: 1.day.ago) - expect { topic.trash! }.to_not change { category.reload.topic_count } + expect { topic.trash!(moderator) }.to_not change { category.reload.topic_count } end end end diff --git a/spec/models/user_action_spec.rb b/spec/models/user_action_spec.rb index 1b9c26d77..8a1e5b7dc 100644 --- a/spec/models/user_action_spec.rb +++ b/spec/models/user_action_spec.rb @@ -62,7 +62,7 @@ describe UserAction do other_stats.should == expecting - public_topic.trash! + public_topic.trash!(user) stats_for_user.should == [] stream_count.should == 0