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

@@ -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
 

app/models/artist.rb

@@ -193,33 +193,28 @@ class Artist < ApplicationRecord
   end
 
   module BanMethods
-    def unban!(current_user = CurrentUser.user)
+    def unban!(current_user)
-      Post.transaction do
+      with_lock do
-        ti = TagImplication.find_by(antecedent_name: name, consequent_name: "banned_artist")
+        ti = TagImplication.active.find_by(antecedent_name: name, consequent_name: "banned_artist")
-        ti&.destroy
+        ti&.update!(status: "deleted")
 
-        Post.raw_tag_match(name).find_each do |post|
+        BulkUpdateRequestProcessor.mass_update(name, "-status:banned -banned_artist", user: current_user)
-          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
 
-        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)
+    def ban!(banner)
-      Post.transaction do
+      with_lock do
-        Post.raw_tag_match(name).each { |post| post.ban!(banner) }
+        BulkUpdateRequestProcessor.mass_update(name, "status:banned", user: banner)
 
-        # potential race condition but unlikely
+        unless TagImplication.active.exists?(antecedent_name: name, consequent_name: "banned_artist")
-        unless TagImplication.where(:antecedent_name => name, :consequent_name => "banned_artist").exists?
           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

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
 

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!(@admin)
-          @artist.unban!
-        end
 
         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

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 = create(:artist, is_banned: true)
-            @artist = FactoryBot.create(:artist)
-            @artist.ban!
-            perform_enqueued_jobs
-          end
           @post = FactoryBot.create(:post, :tag_string => @artist.name)
         end