posts: fix double deletion bug.

Fix a bug where, if a user a was deleting a post and they accidentally
clicked the delete button twice, it could create two flags.

app/models/post.rb

@@ -789,7 +789,7 @@ class Post < ApplicationRecord
     end
 
     def delete!(reason, move_favorites: false, user: CurrentUser.user)
-      transaction do
+      with_lock do
         automated = (user == User.system)
 
         flags.pending.update!(status: :succeeded)

app/models/post_flag.rb

@@ -14,7 +14,7 @@ class PostFlag < ApplicationRecord
   validate :validate_creator_is_not_limited, on: :create
   validate :validate_post, on: :create
   validates :creator_id, uniqueness: { scope: :post_id, on: :create, unless: :is_deletion, message: "have already flagged this post" }
-  before_save :update_post
+  after_create :update_post
   after_create :prune_disapprovals
   attr_accessor :is_deletion
 
@@ -76,7 +76,7 @@ class PostFlag < ApplicationRecord
   end
 
   def update_post
-    post.update_column(:is_flagged, true) unless post.is_flagged?
+    post.update_column(:is_flagged, true) if pending?
   end
 
   def validate_creator_is_not_limited
@@ -85,7 +85,8 @@ class PostFlag < ApplicationRecord
 
   def validate_post
     errors.add(:post, "is pending and cannot be flagged") if post.is_pending? && !is_deletion
-    errors.add(:post, "is deleted and cannot be flagged") if post.is_deleted? && !is_deletion
+    errors.add(:post, "is deleted and cannot be flagged") if post.is_deleted? && creator != User.system # DanbooruBot is allowed to prune expired appeals
+    errors.add(:post, "is already flagged") if post.is_flagged? && !is_deletion
 
     flag = post.flags.in_cooldown.last
     if !is_deletion && !creator.is_approver? && flag.present?

test/factories/post_flag.rb

@@ -1,7 +1,7 @@
 FactoryBot.define do
   factory(:post_flag) do
     creator
-    post { build(:post, is_flagged: true) }
+    post { build(:post) }
     reason {"xxx"}
   end
 end

test/functional/post_flags_controller_test.rb

@@ -7,7 +7,7 @@ class PostFlagsControllerTest < ActionDispatch::IntegrationTest
       @flagger = create(:gold_user, id: 999, created_at: 2.weeks.ago)
       @uploader = create(:mod_user, name: "chen", created_at: 2.weeks.ago)
       @mod = create(:mod_user, name: "mod")
-      @post = create(:post, id: 101, is_flagged: true, uploader: @uploader)
+      @post = create(:post, id: 101, uploader: @uploader)
       @post_flag = create(:post_flag, reason: "xxx", post: @post, creator: @flagger)
     end
 
@@ -27,7 +27,7 @@ class PostFlagsControllerTest < ActionDispatch::IntegrationTest
 
     context "index action" do
       setup do
-        @other_flag = create(:post_flag, post: build(:post, is_flagged: true, tag_string: "touhou"))
+        @other_flag = create(:post_flag, post: create(:post, tag_string: "touhou"))
         @unrelated_flag = create(:post_flag, reason: "poor quality")
       end
 
@@ -129,12 +129,31 @@ class PostFlagsControllerTest < ActionDispatch::IntegrationTest
 
     context "create action" do
       should "create a new flag" do
-        assert_difference("PostFlag.count", 1) do
+        @post = create(:post)
-          @post = create(:post)
+        post_auth post_flags_path, @flagger, params: { post_flag: { post_id: @post.id, reason: "xxx" }}, as: :javascript
-          post_auth post_flags_path, @flagger, params: { post_flag: { post_id: @post.id, reason: "xxx" }}, as: :javascript
+
-          assert_redirected_to PostFlag.last
+        assert_redirected_to PostFlag.last
-          assert_equal(true, @post.reload.is_flagged?)
+        assert_equal(true, @post.reload.is_flagged?)
-        end
+        assert_equal(1, @post.flags.count)
+      end
+
+      should "not allow flagging a flagged post" do
+        @post = create(:post, is_flagged: true)
+        post_auth post_flags_path, @flagger, params: { post_flag: { post_id: @post.id, reason: "xxx" }}, as: :javascript
+
+        assert_response :success
+        assert_equal(true, @post.reload.is_flagged?)
+        assert_equal(0, @post.flags.count)
+      end
+
+      should "not allow flagging a deleted post" do
+        @post = create(:post, is_deleted: true)
+        post_auth post_flags_path, @flagger, params: { post_flag: { post_id: @post.id, reason: "xxx" }}, as: :javascript
+
+        assert_response :success
+        assert_equal(false, @post.reload.is_flagged?)
+        assert_equal(true, @post.reload.is_deleted?)
+        assert_equal(0, @post.flags.count)
       end
     end
 

test/functional/posts_controller_test.rb

@@ -827,12 +827,34 @@ class PostsControllerTest < ActionDispatch::IntegrationTest
         assert_equal("test", @post.flags.last.reason)
       end
 
-      should "delete the post even if the deleter has flagged the post previously" do
+      should "delete the post if the post is currently flagged" do
-        create(:post_flag, post: @post, creator: @approver)
+        create(:post_flag, post: @post, reason: "blah")
         delete_auth post_path(@post), @approver, params: { commit: "Delete", post: { reason: "test" } }
 
         assert_redirected_to @post
         assert_equal(true, @post.reload.is_deleted?)
+        assert_equal("blah", @post.flags.first.reason)
+        assert_equal("test", @post.flags.last.reason)
+        assert_equal(2, @post.flags.count)
+      end
+
+      should "delete the post even if the deleter has flagged the post previously" do
+        create(:post_flag, post: @post, creator: @approver, created_at: 7.days.ago, status: "rejected", reason: "blah")
+        delete_auth post_path(@post), @approver, params: { commit: "Delete", post: { reason: "test" } }
+
+        assert_redirected_to @post
+        assert_equal(true, @post.reload.is_deleted?)
+        assert_equal("blah", @post.flags.first.reason)
+        assert_equal("test", @post.flags.last.reason)
+        assert_equal(2, @post.flags.count)
+      end
+
+      should "not delete the post if the post is already deleted" do
+        delete_auth post_path(@post), @user, params: { commit: "Delete" }
+
+        assert_response 403
+        assert_equal(false, @post.is_deleted?)
+        assert_equal(0, @post.flags.count)
       end
 
       should "not delete the post if the user is unauthorized" do
@@ -840,6 +862,7 @@ class PostsControllerTest < ActionDispatch::IntegrationTest
 
         assert_response 403
         assert_equal(false, @post.is_deleted?)
+        assert_equal(0, @post.flags.count)
       end
 
       should "render the delete post dialog for an xhr request" do

test/jobs/prune_posts_job_test.rb

@@ -4,7 +4,7 @@ class PrunePostsJobTest < ActiveJob::TestCase
   context "PrunePostsJob" do
     should "prune expired posts" do
       @pending = create(:post, is_pending: true, created_at: 5.days.ago)
-      @flagged = create(:post, is_flagged: true, created_at: 5.days.ago)
+      @flagged = create(:post, created_at: 5.days.ago)
       @appealed = create(:post, is_deleted: true, created_at: 5.days.ago)
 
       @flag = create(:post_flag, post: @flagged, created_at: 4.days.ago)

test/unit/post_pruner_test.rb

@@ -17,7 +17,7 @@ class PostPrunerTest < ActiveSupport::TestCase
 
     context "for a flagged post" do
       should "prune expired flags" do
-        @post = create(:post, created_at: 4.weeks.ago, is_flagged: true)
+        @post = create(:post, created_at: 4.weeks.ago)
         @flag = create(:post_flag, post: @post, created_at: 5.days.ago)
         PostPruner.prune!
 
@@ -31,7 +31,7 @@ class PostPrunerTest < ActiveSupport::TestCase
       end
 
       should "not prune unexpired flags" do
-        @post = create(:post, created_at: 4.weeks.ago, is_flagged: true)
+        @post = create(:post, created_at: 4.weeks.ago)
         @flag = create(:post_flag, post: @post, created_at: 1.day.ago)
         PostPruner.prune!
 
@@ -44,10 +44,12 @@ class PostPrunerTest < ActiveSupport::TestCase
       end
 
       should "leave the status of old flags unchanged" do
-        @post = create(:post, created_at: 4.weeks.ago, is_flagged: true)
+        @post = create(:post, created_at: 4.weeks.ago)
         @flag1 = create(:post_flag, post: @post, created_at: 3.weeks.ago, status: :succeeded)
         @flag2 = create(:post_flag, post: @post, created_at: 2.weeks.ago, status: :rejected)
         @flag3 = create(:post_flag, post: @post, created_at: 1.weeks.ago, status: :pending)
+
+        assert_equal(true, @post.is_flagged?)
         PostPruner.prune!
 
         assert_equal(true, @post.reload.is_deleted?)

test/unit/post_query_builder_test.rb

@@ -880,7 +880,7 @@ class PostQueryBuilderTest < ActiveSupport::TestCase
     end
 
     should "return posts for the status:unmoderated metatag" do
-      flagged = create(:post, is_flagged: true)
+      flagged = create(:post)
       pending = create(:post, is_pending: true)
       disapproved = create(:post, is_pending: true)
       appealed = create(:post, is_deleted: true)