diff --git a/app/models/post.rb b/app/models/post.rb index 34327ab45..39967db1c 100644 --- a/app/models/post.rb +++ b/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) diff --git a/app/models/post_flag.rb b/app/models/post_flag.rb index 236d387e9..15f72bd86 100644 --- a/app/models/post_flag.rb +++ b/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? diff --git a/test/factories/post_flag.rb b/test/factories/post_flag.rb index 634d9bb4d..250be12ad 100644 --- a/test/factories/post_flag.rb +++ b/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 diff --git a/test/functional/post_flags_controller_test.rb b/test/functional/post_flags_controller_test.rb index 09c69343b..7ff7b425b 100644 --- a/test/functional/post_flags_controller_test.rb +++ b/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_auth post_flags_path, @flagger, params: { post_flag: { post_id: @post.id, reason: "xxx" }}, as: :javascript - assert_redirected_to PostFlag.last - assert_equal(true, @post.reload.is_flagged?) - end + @post = create(:post) + post_auth post_flags_path, @flagger, params: { post_flag: { post_id: @post.id, reason: "xxx" }}, as: :javascript + + assert_redirected_to PostFlag.last + assert_equal(true, @post.reload.is_flagged?) + 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 diff --git a/test/functional/posts_controller_test.rb b/test/functional/posts_controller_test.rb index 273ffcd4a..e8750cf44 100644 --- a/test/functional/posts_controller_test.rb +++ b/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 - create(:post_flag, post: @post, creator: @approver) + should "delete the post if the post is currently flagged" do + 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 diff --git a/test/jobs/prune_posts_job_test.rb b/test/jobs/prune_posts_job_test.rb index ce568c924..c3d54bda3 100644 --- a/test/jobs/prune_posts_job_test.rb +++ b/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) diff --git a/test/unit/post_pruner_test.rb b/test/unit/post_pruner_test.rb index dd288eca0..976ff19f1 100644 --- a/test/unit/post_pruner_test.rb +++ b/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?) diff --git a/test/unit/post_query_builder_test.rb b/test/unit/post_query_builder_test.rb index bdb09aa0f..1c9da0748 100644 --- a/test/unit/post_query_builder_test.rb +++ b/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)