PostDestroyer to replace callbacks for destroying

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
 

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)

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

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

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
 

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
 

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

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

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