From 4fd028a5cec7830ebd908bea0bdf6fd8995fdb83 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 20 Nov 2022 18:33:15 -0600 Subject: [PATCH] artists: fix ban/unban actions. Fix the ban! and unban! methods to: * Lock the artist while it is being banned or unbanned. * Perform the edits as a mass update, so that the posts are updated in parallel. * Edit the artist as the banner rather than as the current user. * Soft delete the banned_artist implication when an artist is unbanned instead of hard deleting it. * Ignore the banned_artist implication if it's deleted. --- app/controllers/artists_controller.rb | 4 ++-- app/models/artist.rb | 27 +++++++++------------- test/functional/artists_controller_test.rb | 4 ++-- test/unit/artist_test.rb | 20 ++++++++++++---- test/unit/post_test.rb | 6 +---- 5 files changed, 32 insertions(+), 29 deletions(-) diff --git a/app/controllers/artists_controller.rb b/app/controllers/artists_controller.rb index 15684dcb2..787f673ea 100644 --- a/app/controllers/artists_controller.rb +++ b/app/controllers/artists_controller.rb @@ -19,13 +19,13 @@ class ArtistsController < ApplicationController def ban @artist = authorize Artist.find(params[:id]) - @artist.ban!(banner: CurrentUser.user) + @artist.ban!(CurrentUser.user) redirect_to(artist_path(@artist), :notice => "Artist was banned") end def unban @artist = authorize Artist.find(params[:id]) - @artist.unban! + @artist.unban!(CurrentUser.user) redirect_to(artist_path(@artist), :notice => "Artist was unbanned") end diff --git a/app/models/artist.rb b/app/models/artist.rb index 73f66bee3..75945017d 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -193,33 +193,28 @@ class Artist < ApplicationRecord end module BanMethods - def unban!(current_user = CurrentUser.user) - Post.transaction do - ti = TagImplication.find_by(antecedent_name: name, consequent_name: "banned_artist") - ti&.destroy + def unban!(current_user) + with_lock do + ti = TagImplication.active.find_by(antecedent_name: name, consequent_name: "banned_artist") + ti&.update!(status: "deleted") - Post.raw_tag_match(name).find_each do |post| - post.unban!(current_user) - fixed_tags = post.tag_string.sub(/(?:\A| )banned_artist(?:\Z| )/, " ").strip - post.update(tag_string: fixed_tags) # XXX should use current_user - end + BulkUpdateRequestProcessor.mass_update(name, "-status:banned -banned_artist", user: current_user) - update!(is_banned: false) # XXX should use current_user + CurrentUser.scoped(current_user) { update!(is_banned: false) } ModAction.log("unbanned artist ##{id}", :artist_unban, subject: self, user: current_user) end end - def ban!(banner: CurrentUser.user) - Post.transaction do - Post.raw_tag_match(name).each { |post| post.ban!(banner) } + def ban!(banner) + with_lock do + BulkUpdateRequestProcessor.mass_update(name, "status:banned", user: banner) - # potential race condition but unlikely - unless TagImplication.where(:antecedent_name => name, :consequent_name => "banned_artist").exists? + unless TagImplication.active.exists?(antecedent_name: name, consequent_name: "banned_artist") Tag.find_or_create_by_name("banned_artist", category: "artist", current_user: banner) TagImplication.approve!(antecedent_name: name, consequent_name: "banned_artist", approver: banner) end - update!(is_banned: true) + CurrentUser.scoped(banner) { update!(is_banned: true) } ModAction.log("banned artist ##{id}", :artist_ban, subject: self, user: banner) end end diff --git a/test/functional/artists_controller_test.rb b/test/functional/artists_controller_test.rb index 79dc088b9..f249e0baa 100644 --- a/test/functional/artists_controller_test.rb +++ b/test/functional/artists_controller_test.rb @@ -132,12 +132,12 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest context "unban action" do should "unban an artist" do - as(@admin) { @artist.ban!(banner: @admin) } + @artist.ban!(@admin) put_auth unban_artist_path(@artist.id), @admin assert_redirected_to(@artist) assert_equal(false, @artist.reload.is_banned?) - assert_equal(false, TagImplication.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist")) + assert_equal(true, TagImplication.deleted.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist")) end end diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index 5b13ceaa0..5f900ef43 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -63,7 +63,7 @@ class ArtistTest < ActiveSupport::TestCase @artist = FactoryBot.create(:artist, :name => "aaa") @post = FactoryBot.create(:post, :tag_string => "aaa") @admin = FactoryBot.create(:admin_user) - @artist.ban!(banner: @admin) + @artist.ban!(@admin) perform_enqueued_jobs @post.reload end @@ -72,15 +72,17 @@ class ArtistTest < ActiveSupport::TestCase assert_equal(true, @artist.reload.is_banned?) assert_equal(true, @post.reload.is_banned?) assert_equal(true, @artist.versions.last.is_banned?) + assert_equal(true, TagImplication.active.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist")) - assert_difference("TagImplication.count", -1) do - @artist.unban! - end + @artist.unban!(@admin) assert_equal(false, @artist.reload.is_banned?) assert_equal(false, @post.reload.is_banned?) assert_equal(false, @artist.versions.last.is_banned?) assert_equal("aaa", @post.tag_string) + assert_equal(false, TagImplication.active.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist")) + assert_equal(true, TagImplication.deleted.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist")) + assert_equal(true, ModAction.artist_unban.exists?(subject: @artist)) end should "ban the post" do @@ -106,8 +108,18 @@ class ArtistTest < ActiveSupport::TestCase end should "update the artist history" do + assert_equal(true, @artist.reload.is_banned?) assert_equal(true, @artist.versions.last.is_banned?) end + + should "tag the posts" do + assert_equal(true, @post.reload.is_banned?) + assert_equal(true, @post.has_tag?("banned_artist")) + end + + should "create a mod action" do + assert_equal(true, ModAction.artist_ban.exists?(subject: @artist)) + end end should "normalize its name" do diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index 4eafa3132..4a64a098c 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -384,11 +384,7 @@ class PostTest < ActiveSupport::TestCase context "with a banned artist" do setup do - CurrentUser.scoped(FactoryBot.create(:admin_user)) do - @artist = FactoryBot.create(:artist) - @artist.ban! - perform_enqueued_jobs - end + @artist = create(:artist, is_banned: true) @post = FactoryBot.create(:post, :tag_string => @artist.name) end