From 59fc3bfac4539f3769aeee7b62e9d8c41da19015 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 18 Mar 2013 17:52:29 -0400 Subject: [PATCH] PostDestroyer to replace callbacks for destroying --- app/controllers/posts_controller.rb | 6 +- app/models/post.rb | 53 ------------------ lib/post_destroyer.rb | 68 +++++++++++++++++++++++ spec/components/guardian_spec.rb | 3 +- spec/controllers/posts_controller_spec.rb | 6 +- spec/models/post_action_spec.rb | 4 +- spec/models/post_alert_observer_spec.rb | 3 +- spec/models/post_spec.rb | 12 ++-- spec/models/topic_spec.rb | 3 +- 9 files changed, 93 insertions(+), 65 deletions(-) create mode 100644 lib/post_destroyer.rb diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 52af07ea4..492741872 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -1,4 +1,5 @@ require_dependency 'post_creator' +require_dependency 'post_destroyer' class PostsController < ApplicationController @@ -83,7 +84,10 @@ class PostsController < ApplicationController def destroy post = find_post_from_params guardian.ensure_can_delete!(post) - post.delete_by(current_user) + + destroyer = PostDestroyer.new(current_user, post) + destroyer.destroy + render nothing: true end diff --git a/app/models/post.rb b/app/models/post.rb index 62375c750..5c6ae352a 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -143,25 +143,6 @@ class Post < ActiveRecord::Base @raw_mentions = results.uniq.map { |un| un.first.downcase.gsub!(/^@/, '') } end - # The rules for deletion change depending on who is doing it. - def delete_by(deleted_by) - if deleted_by.moderator? - # As a moderator, delete the post. - Post.transaction do - self.destroy - Topic.reset_highest(topic_id) - update_flagged_posts_count - end - elsif deleted_by.id == user_id - # As the poster, make a revision that says deleted. - Post.transaction do - revise(deleted_by, I18n.t('js.post.deleted_by_author'), force_new_version: true) - update_column(:user_deleted, true) - update_flagged_posts_count - end - end - end - def archetype topic.archetype end @@ -336,40 +317,6 @@ class Post < ActiveRecord::Base self.cooked = cook(raw, topic_id: topic_id) unless new_record? end - before_destroy do - - # Update the last post id to the previous post if it exists - last_post = Post.where("topic_id = ? and id <> ?", topic_id, id).order('created_at desc').limit(1).first - if last_post.present? - topic.update_attributes(last_posted_at: last_post.created_at, - last_post_user_id: last_post.user_id, - highest_post_number: last_post.post_number) - - # If the poster doesn't have any other posts in the topic, clear their posted flag - unless Post.exists?(["topic_id = ? and user_id = ? and id <> ?", topic_id, user_id, id]) - TopicUser.update_all 'posted = false', topic_id: topic_id, user_id: user_id - end - end - - # Feature users in the topic - Jobs.enqueue(:feature_topic_users, topic_id: topic_id, except_post_id: id) - - end - - after_destroy do - # Remove any reply records that point to deleted posts - post_ids = PostReply.select(:post_id).where(reply_id: id).map(&:post_id) - PostReply.delete_all reply_id: id - - if post_ids.present? - Post.where(id: post_ids).each { |p| p.update_column :reply_count, p.replies.count } - end - - # Remove any notifications that point to this deleted post - Notification.delete_all topic_id: topic_id, post_number: post_number - end - - def advance_draft_sequence return if topic.blank? # could be deleted DraftSequence.next!(last_editor_id, topic.draft_key) diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb new file mode 100644 index 000000000..5ba314e1d --- /dev/null +++ b/lib/post_destroyer.rb @@ -0,0 +1,68 @@ +# +# How a post is deleted is affected by who is performing the action. +# this class contains the logic to delete it. +# +class PostDestroyer + + def initialize(user, post) + @user, @post = user, post + end + + def destroy + if @user.moderator? + moderator_destroyed + elsif @user.id == @post.user_id + user_destroyed + end + end + + # When a post is properly deleted. Well, it's still soft deleted, but it will no longer + # show up in the topic + def moderator_destroyed + Post.transaction do + + # Update the last post id to the previous post if it exists + last_post = Post.where("topic_id = ? and id <> ?", @post.topic_id, @post.id).order('created_at desc').limit(1).first + if last_post.present? + @post.topic.update_attributes(last_posted_at: last_post.created_at, + last_post_user_id: last_post.user_id, + highest_post_number: last_post.post_number) + + # If the poster doesn't have any other posts in the topic, clear their posted flag + unless Post.exists?(["topic_id = ? and user_id = ? and id <> ?", @post.topic_id, @post.user_id, @post.id]) + TopicUser.update_all 'posted = false', topic_id: @post.topic_id, user_id: @post.user_id + end + end + + # Feature users in the topic + Jobs.enqueue(:feature_topic_users, topic_id: @post.topic_id, except_post_id: @post.id) + + # Actually soft-delete the post :) + @post.destroy + + Topic.reset_highest(@post.topic_id) + @post.update_flagged_posts_count + + # Remove any reply records that point to deleted posts + post_ids = PostReply.select(:post_id).where(reply_id: @post.id).map(&:post_id) + PostReply.delete_all reply_id: @post.id + + if post_ids.present? + Post.where(id: post_ids).each { |p| p.update_column :reply_count, p.replies.count } + end + + # Remove any notifications that point to this deleted post + Notification.delete_all topic_id: @post.topic_id, post_number: @post.post_number + end + end + + # When a user 'deletes' their own post. We just change the text. + def user_destroyed + Post.transaction do + @post.revise(@user, I18n.t('js.post.deleted_by_author'), force_new_version: true) + @post.update_column(:user_deleted, true) + @post.update_flagged_posts_count + end + end + +end \ No newline at end of file diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index d52bba786..90cdf9b52 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -1,5 +1,6 @@ require 'spec_helper' require 'guardian' +require_dependency 'post_destroyer' describe Guardian do @@ -620,7 +621,7 @@ describe Guardian do end it "returns false when trying to delete your own post that has already been deleted" do - post.delete_by(user) + PostDestroyer.new(user, post).destroy post.reload Guardian.new(user).can_delete?(post).should be_false end diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index 5946cc651..c194dd4f2 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/spec/controllers/posts_controller_spec.rb @@ -85,8 +85,10 @@ describe PostsController do response.should be_forbidden end - it "calls delete_by" do - Post.any_instance.expects(:delete_by).with(user) + it "uses a PostDestroyer" do + destroyer = mock + PostDestroyer.expects(:new).with(user, post).returns(destroyer) + destroyer.expects(:destroy) xhr :delete, :destroy, id: post.id end diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index 3ac3e78b0..67b78edb5 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -1,4 +1,5 @@ require 'spec_helper' +require_dependency 'post_destroyer' describe PostAction do @@ -7,6 +8,7 @@ describe PostAction do it { should belong_to :post_action_type } it { should rate_limit } + let(:moderator) { Fabricate(:moderator) } let(:codinghorror) { Fabricate(:coding_horror) } let(:post) { Fabricate(:post) } let(:bookmark) { PostAction.new(user_id: post.user_id, post_action_type_id: PostActionType.types[:bookmark] , post_id: post.id) } @@ -36,7 +38,7 @@ describe PostAction do it "should reset counts when a post is deleted" do post2 = Fabricate(:post, topic_id: post.topic_id) PostAction.act(codinghorror, post2, PostActionType.types[:off_topic]) - post2.destroy + PostDestroyer.new(moderator, post2).destroy PostAction.flagged_posts_count.should == 0 end diff --git a/spec/models/post_alert_observer_spec.rb b/spec/models/post_alert_observer_spec.rb index 796e0757b..05e546cf7 100644 --- a/spec/models/post_alert_observer_spec.rb +++ b/spec/models/post_alert_observer_spec.rb @@ -1,4 +1,5 @@ require 'spec_helper' +require_dependency 'post_destroyer' describe PostAlertObserver do @@ -90,7 +91,7 @@ describe PostAlertObserver do it 'removes notifications' do post = mention_post lambda { - post.destroy + PostDestroyer.new(Fabricate(:moderator), post).destroy }.should change(evil_trout.notifications, :count).by(-1) end end diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index aefe57207..229f22eac 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -1,4 +1,5 @@ require 'spec_helper' +require_dependency 'post_destroyer' describe Post do @@ -486,7 +487,7 @@ describe Post do context "as the creator of the post" do before do - post.delete_by(post.user) + PostDestroyer.new(post.user, post).destroy post.reload end @@ -508,12 +509,12 @@ describe Post do context "as a moderator" do before do - post.delete_by(post.user) + PostDestroyer.new(moderator, post).destroy post.reload end it "deletes the post" do - post.deleted_at.should be_blank + post.deleted_at.should be_present end end @@ -522,12 +523,13 @@ describe Post do describe 'after delete' do + let(:moderator) { Fabricate(:moderator) } let!(:coding_horror) { Fabricate(:coding_horror) } let!(:post) { Fabricate(:post, post_args.merge(raw: "Hello @CodingHorror")) } it "should feature the users again (in case they've changed)" do Jobs.expects(:enqueue).with(:feature_topic_users, has_entries(topic_id: post.topic_id, except_post_id: post.id)) - post.destroy + PostDestroyer.new(moderator, post).destroy end describe 'with a reply' do @@ -545,7 +547,7 @@ describe Post do it 'lowers the reply_count when the reply is deleted' do lambda { - reply.destroy + PostDestroyer.new(moderator, reply).destroy post.reload }.should change(post.post_replies, :count).by(-1) end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index efd96b464..31ed358ca 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1,6 +1,7 @@ # encoding: UTF-8 require 'spec_helper' +require_dependency 'post_destroyer' describe Topic do @@ -808,7 +809,7 @@ describe Topic do context 'after deleting that post' do before do - @new_post.destroy + PostDestroyer.new(Fabricate(:moderator), @new_post).destroy Topic.reset_highest(@topic.id) @topic.reload end