Merge pull request #3063 from evazion/fix-post-delete

Fix flagging cooldown preventing manual deletions.

app/controllers/moderator/post/posts_controller.rb

@@ -14,8 +14,7 @@ module Moderator
       def delete
         @post = ::Post.find(params[:id])
         if params[:commit] == "Delete"
-          @post.flag!(params[:reason], :is_deletion => true)
-          @post.delete!(:reason => params[:reason], :move_favorites => params[:move_favorites].present?)
+          @post.delete!(params[:reason], :move_favorites => params[:move_favorites].present?)
         end
         redirect_to(post_path(@post))
       end

app/logical/post_pruner.rb

@@ -13,11 +13,10 @@ protected
     CurrentUser.scoped(User.system, "127.0.0.1") do
       Post.where("is_deleted = ? and is_pending = ? and created_at < ?", false, true, 3.days.ago).each do |post|
         begin
-          post.flag!("Unapproved in three days")
+          post.delete!("Unapproved in three days")
         rescue PostFlag::Error
           # swallow
         end
-        post.delete!
       end
     end
   end
@@ -27,11 +26,10 @@ protected
       Post.where("is_deleted = ? and is_flagged = ?", false, true).each do |post|
         if post.flags.unresolved.old.any?
           begin
-            post.flag!("Unapproved in three days after returning to moderation queue")
+            post.delete!("Unapproved in three days after returning to moderation queue")
           rescue PostFlag::Error
             # swallow
           end
-          post.delete!
         end
       end
     end

app/models/post.rb

@@ -1355,7 +1355,7 @@ class Post < ActiveRecord::Base
       end
 
       ModAction.log("permanently deleted post ##{id}")
-      delete!(:without_mod_action => true)
+      delete!("Permanently deleted post ##{id}", :without_mod_action => true)
       Post.without_timeout do
         give_favorites_to_parent
         update_children_on_destroy
@@ -1377,13 +1377,15 @@ class Post < ActiveRecord::Base
       ModAction.log("unbanned post ##{id}")
     end
 
-    def delete!(options = {})
+    def delete!(reason, options = {})
       if is_status_locked?
         self.errors.add(:is_status_locked, "; cannot delete post")
         return false
       end
 
       Post.transaction do
+        flag!(reason, is_deletion: true)
+
         self.is_deleted = true
         self.is_pending = false
         self.is_flagged = false
@@ -1398,11 +1400,7 @@ class Post < ActiveRecord::Base
         update_parent_on_save
 
         unless options[:without_mod_action]
-          if options[:reason]
-            ModAction.log("deleted post ##{id}, reason: #{options[:reason]}")
-          else
-            ModAction.log("deleted post ##{id}")
-          end
+          ModAction.log("deleted post ##{id}, reason: #{reason}")
         end
       end
     end

app/models/post_flag.rb

@@ -139,10 +139,10 @@ class PostFlag < ActiveRecord::Base
   end
 
   def validate_creator_is_not_limited
+    return if is_deletion
+
     if CurrentUser.can_approve_posts?
       # do nothing
-    elsif User.system.present? && CurrentUser.id == User.system.id
-      # do nothing
     elsif creator.created_at > 1.week.ago
       errors[:creator] << "cannot flag within the first week of sign up"
     elsif creator.is_gold? && flag_count_for_creator >= 10
@@ -150,11 +150,6 @@ class PostFlag < ActiveRecord::Base
     elsif !creator.is_gold? && flag_count_for_creator >= 1
       errors[:creator] << "can flag 1 post a day"
     end
-  end
-
-  def validate_post
-    errors[:post] << "is locked and cannot be flagged" if post.is_status_locked?
-    errors[:post] << "is deleted" if post.is_deleted?
 
     flag = post.flags.in_cooldown.last
     if flag.present?
@@ -162,6 +157,11 @@ class PostFlag < ActiveRecord::Base
     end
   end
 
+  def validate_post
+    errors[:post] << "is locked and cannot be flagged" if post.is_status_locked?
+    errors[:post] << "is deleted" if post.is_deleted?
+  end
+
   def initialize_creator
     self.creator_id ||= CurrentUser.id
     self.creator_ip_addr ||= CurrentUser.ip_addr

test/unit/post_test.rb

@@ -80,7 +80,7 @@ class PostTest < ActiveSupport::TestCase
         end
 
         should "fail" do
-          @post.delete!
+          @post.delete!("test")
           assert_equal(["Is status locked ; cannot delete post"], @post.errors.full_messages)
           assert_equal(1, Post.where("id = ?", @post.id).count)
         end
@@ -89,18 +89,29 @@ class PostTest < ActiveSupport::TestCase
       context "with the banned_artist tag" do
         should "also ban the post" do
           post = FactoryGirl.create(:post, :tag_string => "banned_artist")
-          post.delete!
+          post.delete!("test")
           post.reload
           assert(post.is_banned?)
         end
       end
 
+      context "that is still in cooldown after being flagged" do
+        should "succeed" do
+          post = FactoryGirl.create(:post)
+          post.flag!("test flag")
+          post.delete!("test deletion")
+
+          assert_equal(true, post.is_deleted)
+          assert_equal(2, post.flags.size)
+        end
+      end
+
       should "update the fast count" do
         Danbooru.config.stubs(:estimate_post_counts).returns(false)
         post = FactoryGirl.create(:post, :tag_string => "aaa")
         assert_equal(1, Post.fast_count)
         assert_equal(1, Post.fast_count("aaa"))
-        post.delete!
+        post.delete!("test")
         assert_equal(1, Post.fast_count)
         assert_equal(1, Post.fast_count("aaa"))
       end
@@ -108,14 +119,14 @@ class PostTest < ActiveSupport::TestCase
       should "toggle the is_deleted flag" do
         post = FactoryGirl.create(:post)
         assert_equal(false, post.is_deleted?)
-        post.delete!
+        post.delete!("test")
         assert_equal(true, post.is_deleted?)
       end
 
       should "not decrement the tag counts" do
         post = FactoryGirl.create(:post, :tag_string => "aaa")
         assert_equal(1, Tag.find_by_name("aaa").post_count)
-        post.delete!
+        post.delete!("test")
         assert_equal(1, Tag.find_by_name("aaa").post_count)
       end
     end
@@ -209,7 +220,7 @@ class PostTest < ActiveSupport::TestCase
           c1 = FactoryGirl.create(:post, :parent_id => p1.id)
           user = FactoryGirl.create(:gold_user)
           c1.add_favorite!(user)
-          c1.delete!
+          c1.delete!("test")
           p1.reload
           assert(Favorite.exists?(:post_id => c1.id, :user_id => user.id))
           assert(!Favorite.exists?(:post_id => p1.id, :user_id => user.id))
@@ -220,7 +231,7 @@ class PostTest < ActiveSupport::TestCase
           c1 = FactoryGirl.create(:post, :parent_id => p1.id)
           user = FactoryGirl.create(:gold_user)
           c1.add_favorite!(user)
-          c1.delete!(:move_favorites => true)
+          c1.delete!("test", :move_favorites => true)
           p1.reload
           assert(!Favorite.exists?(:post_id => c1.id, :user_id => user.id), "Child should not still have favorites")
           assert(Favorite.exists?(:post_id => p1.id, :user_id => user.id), "Parent should have favorites")
@@ -229,7 +240,7 @@ class PostTest < ActiveSupport::TestCase
         should "not update the parent's has_children flag" do
           p1 = FactoryGirl.create(:post)
           c1 = FactoryGirl.create(:post, :parent_id => p1.id)
-          c1.delete!
+          c1.delete!("test")
           p1.reload
           assert(p1.has_children?, "Parent should have children")
         end
@@ -239,7 +250,7 @@ class PostTest < ActiveSupport::TestCase
         should "not remove the has_children flag" do
           p1 = FactoryGirl.create(:post)
           c1 = FactoryGirl.create(:post, :parent_id => p1.id)
-          p1.delete!
+          p1.delete!("test")
           p1.reload
           assert_equal(true, p1.has_children?)
         end
@@ -247,7 +258,7 @@ class PostTest < ActiveSupport::TestCase
         should "not remove the parent of that child" do
           p1 = FactoryGirl.create(:post)
           c1 = FactoryGirl.create(:post, :parent_id => p1.id)
-          p1.delete!
+          p1.delete!("test")
           c1.reload
           assert_not_nil(c1.parent)
         end
@@ -259,7 +270,7 @@ class PostTest < ActiveSupport::TestCase
           c1 = FactoryGirl.create(:post, :parent_id => p1.id)
           c2 = FactoryGirl.create(:post, :parent_id => p1.id)
           c3 = FactoryGirl.create(:post, :parent_id => p1.id)
-          p1.delete!
+          p1.delete!("test")
           c1.reload
           c2.reload
           c3.reload
@@ -275,7 +286,7 @@ class PostTest < ActiveSupport::TestCase
         new_user = FactoryGirl.create(:moderator_user)
         p1 = FactoryGirl.create(:post)
         c1 = FactoryGirl.create(:post, :parent_id => p1.id)
-        c1.delete!
+        c1.delete!("test")
         CurrentUser.scoped(new_user, "127.0.0.1") do
           c1.undelete!
         end
@@ -286,7 +297,7 @@ class PostTest < ActiveSupport::TestCase
       should "preserve the parent's has_children flag" do
         p1 = FactoryGirl.create(:post)
         c1 = FactoryGirl.create(:post, :parent_id => p1.id)
-        c1.delete!
+        c1.delete!("test")
         c1.undelete!
         p1.reload
         assert_not_nil(c1.parent_id)