jobs: migrate mass updates to ActiveJob.

Also fixes a bug where mod actions weren't logged on mass updates.
Creating the mod action silently failed because it was called when
CurrentUser wasn' set.

app/controllers/moderator/tags_controller.rb

@@ -1,13 +1,12 @@
 module Moderator
   class TagsController < ApplicationController
     before_action :moderator_only
-    rescue_from TagBatchChange::Error, :with => :error
 
     def edit
     end
 
     def update
-      Delayed::Job.enqueue(TagBatchChange.new(params[:tag][:antecedent], params[:tag][:consequent], CurrentUser.user.id, CurrentUser.ip_addr), :queue => "default")
+      TagBatchChangeJob.perform_later(params[:tag][:antecedent], params[:tag][:consequent], CurrentUser.user, CurrentUser.ip_addr)
       redirect_to edit_moderator_tag_path, :notice => "Post changes queued"
     end
 

app/jobs/tag_batch_change_job.rb

@@ -0,0 +1,79 @@
+class TagBatchChangeJob < ApplicationJob
+  class Error < Exception ; end
+
+  queue_as :default
+
+  def perform(antecedent, consequent, updater, updater_ip_addr)
+    raise Error.new("antecedent is missing") if antecedent.blank?
+
+    normalized_antecedent = TagAlias.to_aliased(::Tag.scan_tags(antecedent.mb_chars.downcase))
+    normalized_consequent = TagAlias.to_aliased(::Tag.scan_tags(consequent.mb_chars.downcase))
+
+    CurrentUser.without_safe_mode do
+      CurrentUser.scoped(updater, updater_ip_addr) do
+        migrate_posts(normalized_antecedent, normalized_consequent)
+        migrate_saved_searches(normalized_antecedent, normalized_consequent)
+        migrate_blacklists(normalized_antecedent, normalized_consequent)
+
+        ModAction.log("processed mass update: #{antecedent} -> #{consequent}", :mass_update)
+      end
+    end
+  end
+
+  def self.estimate_update_count(antecedent, consequent)
+    CurrentUser.without_safe_mode do
+      PostReadOnly.tag_match(antecedent).count
+    end
+  end
+
+  def migrate_posts(normalized_antecedent, normalized_consequent)
+    ::Post.tag_match(normalized_antecedent.join(" ")).find_each do |post|
+      post.reload
+      tags = (post.tag_array - normalized_antecedent + normalized_consequent).join(" ")
+      post.update(tag_string: tags)
+    end
+  end
+
+  def migrate_saved_searches(normalized_antecedent, normalized_consequent)
+    tags = Tag.scan_tags(normalized_antecedent.join(" "), strip_metatags: true)
+
+    # https://www.postgresql.org/docs/current/static/functions-array.html
+    saved_searches = SavedSearch.where("string_to_array(query, ' ') @> ARRAY[?]", tags)
+    saved_searches.find_each do |ss|
+      ss.query = (ss.query.split - tags + normalized_consequent).uniq.join(" ")
+      ss.save
+    end
+  end
+
+  # this can't handle negated tags or other special cases
+  def migrate_blacklists(normalized_antecedent, normalized_consequent)
+    query = normalized_antecedent
+    adds = normalized_consequent
+    arel = query.inject(User.none) do |scope, x|
+      scope.or(User.where("blacklisted_tags like ?", "%" + x.to_escaped_for_sql_like + "%"))
+    end
+
+    arel.find_each do |user|
+      changed = false
+
+      begin
+        repl = user.blacklisted_tags.split(/\r\n|\r|\n/).map do |line|
+          list = Tag.scan_tags(line)
+
+          if (list & query).size != query.size
+            next line
+          end
+
+          changed = true
+          (list - query + adds).join(" ")
+        end
+
+        if changed
+          user.update(blacklisted_tags: repl.join("\n"))
+        end
+      rescue Exception => e
+        DanbooruLogger.log(e)
+      end
+    end
+  end
+end

app/logical/alias_and_implication_importer.rb

@@ -89,7 +89,7 @@ class AliasAndImplicationImporter
         sum + TagImplication.new(antecedent_name: token[1], consequent_name: token[2]).estimate_update_count
 
       when :mass_update
-        sum + Moderator::TagBatchChange.new(token[1], token[2]).estimate_update_count
+        sum + TagBatchChangeJob.estimate_update_count(token[1], token[2])
 
       when :change_category
         sum + Tag.find_by_name(token[1]).try(:post_count) || 0
@@ -156,7 +156,7 @@ private
           tag_implication.reject!(update_topic: false)
 
         when :mass_update
-          Delayed::Job.enqueue(Moderator::TagBatchChange.new(token[1], token[2], CurrentUser.id, CurrentUser.ip_addr), :queue => "default")
+          TagBatchChangeJob.perform_later(token[1], token[2], CurrentUser.user, CurrentUser.ip_addr)
 
         when :change_category
           tag = Tag.find_by_name(token[1])

app/logical/moderator/tag_batch_change.rb

@@ -1,80 +0,0 @@
-module Moderator
-  class TagBatchChange < Struct.new(:antecedent, :consequent, :updater_id, :updater_ip_addr)
-    class Error < Exception ; end
-
-    def perform
-      raise Error.new("antecedent is missing") if antecedent.blank?
-
-      normalized_antecedent = TagAlias.to_aliased(::Tag.scan_tags(antecedent.mb_chars.downcase))
-      normalized_consequent = TagAlias.to_aliased(::Tag.scan_tags(consequent.mb_chars.downcase))
-      updater = User.find(updater_id)
-
-      CurrentUser.without_safe_mode do
-        CurrentUser.scoped(updater, updater_ip_addr) do
-          migrate_posts(normalized_antecedent, normalized_consequent)
-          migrate_saved_searches(normalized_antecedent, normalized_consequent)
-          migrate_blacklists(normalized_antecedent, normalized_consequent)
-        end
-      end
-
-      ModAction.log("processed mass update: #{antecedent} -> #{consequent}",:mass_update)
-    end
-
-    def estimate_update_count
-      PostReadOnly.tag_match(antecedent).count
-    end
-
-    def migrate_posts(normalized_antecedent, normalized_consequent)
-      ::Post.tag_match(normalized_antecedent.join(" ")).find_each do |post|
-        post.reload
-        tags = (post.tag_array - normalized_antecedent + normalized_consequent).join(" ")
-        post.update(tag_string: tags)
-      end
-    end
-
-    def migrate_saved_searches(normalized_antecedent, normalized_consequent)
-      if SavedSearch.enabled?
-        tags = Tag.scan_tags(normalized_antecedent.join(" "), strip_metatags: true)
-
-        # https://www.postgresql.org/docs/current/static/functions-array.html
-        saved_searches = SavedSearch.where("string_to_array(query, ' ') @> ARRAY[?]", tags)
-        saved_searches.find_each do |ss|
-          ss.query = (ss.query.split - tags + normalized_consequent).uniq.join(" ")
-          ss.save
-        end
-      end
-    end
-
-    # this can't handle negated tags or other special cases
-    def migrate_blacklists(normalized_antecedent, normalized_consequent)
-      query = normalized_antecedent
-      adds = normalized_consequent
-      arel = query.inject(User.none) do |scope, x|
-        scope.or(User.where("blacklisted_tags like ?", "%" + x.to_escaped_for_sql_like + "%"))
-      end
-
-      arel.find_each do |user|
-        changed = false
-
-        begin
-          repl = user.blacklisted_tags.split(/\r\n|\r|\n/).map do |line|
-            list = Tag.scan_tags(line)
-
-            if (list & query).size != query.size
-              next line
-            end
-
-            changed = true
-            (list - query + adds).join(" ")
-          end
-
-          if changed
-            user.update(blacklisted_tags: repl.join("\n"))
-          end
-        rescue Exception => e
-          DanbooruLogger.log(e)
-        end
-      end
-    end
-  end
-end

test/jobs/tag_batch_change_job_test.rb

@@ -0,0 +1,55 @@
+require "test_helper"
+
+class TagBatchChangeJobTest < ActiveJob::TestCase
+  context "a tag batch change" do
+    setup do
+      @user = create(:moderator_user)
+      @post = create(:post, :tag_string => "aaa")
+    end
+
+    context "#estimate_update_count" do
+      should "find the correct count" do
+        assert_equal(1, TagBatchChangeJob.estimate_update_count("aaa", "bbb"))
+      end
+    end
+
+    should "execute" do
+      TagBatchChangeJob.perform_now("aaa", "bbb", @user, "127.0.0.1")
+      assert_equal("bbb", @post.reload.tag_string)
+    end
+
+    should "move saved searches" do
+      ss = create(:saved_search, user: @user, query: "123 ... 456")
+      TagBatchChangeJob.perform_now("...", "bbb", @user, "127.0.0.1")
+      assert_equal("123 456 bbb", ss.reload.normalized_query)
+    end
+
+    should "move blacklists" do
+      @user.update(blacklisted_tags: "123 456\n789\n")
+      TagBatchChangeJob.perform_now("456", "xxx", @user, "127.0.0.1")
+
+      assert_equal("123 xxx\n789", @user.reload.blacklisted_tags)
+    end
+
+    should "move only saved searches that match the mass update exactly" do
+      ss = create(:saved_search, user: @user, query: "123 ... 456")
+
+      TagBatchChangeJob.perform_now("1", "bbb", @user, "127.0.0.1")
+      assert_equal("... 123 456", ss.reload.normalized_query, "expected '123' to remain unchanged")
+
+      TagBatchChangeJob.perform_now("123 456", "789", @user, "127.0.0.1")
+      assert_equal("... 789", ss.reload.normalized_query, "expected '123 456' to be changed to '789'")
+    end
+
+    should "log a modaction" do
+      TagBatchChangeJob.perform_now("1", "2", @user, "127.0.0.1")
+      assert_equal("mass_update", ModAction.last.category)
+    end
+
+    should "raise an error if there is no predicate" do
+      assert_raises(TagBatchChangeJob::Error) do
+        TagBatchChangeJob.perform_now("", "bbb", @user, "127.0.0.1")
+      end
+    end
+  end
+end

test/unit/bulk_update_request_test.rb

@@ -64,15 +64,17 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
 
     context "on approval" do
       setup do
+        @post = create(:post, tag_string: "foo aaa")
         @script = %q(
           create alias foo -> bar
           create implication bar -> baz
+          mass update aaa -> bbb
         )
 
         @bur = FactoryBot.create(:bulk_update_request, :script => @script)
         @bur.approve!(@admin)
 
-        assert_enqueued_jobs(2)
+        assert_enqueued_jobs(3)
         workoff_active_jobs
 
         @ta = TagAlias.where(:antecedent_name => "foo", :consequent_name => "bar").first
@@ -92,6 +94,10 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
         assert_equal("active", @ti.status)
       end
 
+      should "process mass updates" do
+        assert_equal("bar baz bbb", @post.reload.tag_string)
+      end
+
       should "set the alias/implication approvers" do
         assert_equal(@admin.id, @ta.approver.id)
         assert_equal(@admin.id, @ti.approver.id)

test/unit/moderator/tag_batch_change_test.rb

@@ -1,77 +0,0 @@
-require "test_helper"
-
-module Moderator
-  class TagBatchChangeTest < ActiveSupport::TestCase
-    def setup
-      super
-      mock_saved_search_service!
-    end
-
-    context "a tag batch change" do
-      setup do
-        @user = FactoryBot.create(:moderator_user)
-        CurrentUser.user = @user
-        CurrentUser.ip_addr = "127.0.0.1"
-        @post = FactoryBot.create(:post, :tag_string => "aaa")
-      end
-
-      teardown do
-        CurrentUser.user = nil
-        CurrentUser.ip_addr = nil
-      end
-
-      context "#estimate_update_count" do
-        setup do
-          @change = TagBatchChange.new("aaa", "bbb", @user.id, "127.0.0.1")
-        end
-
-        should "find the correct count" do
-          assert_equal(1, @change.estimate_update_count)
-        end
-      end
-
-      should "execute" do
-        tag_batch_change = TagBatchChange.new("aaa", "bbb", @user.id, "127.0.0.1")
-        tag_batch_change.perform
-        @post.reload
-        assert_equal("bbb", @post.tag_string)
-      end
-
-      should "move saved searches" do
-        ss = FactoryBot.create(:saved_search, :user => @user, :query => "123 ... 456")
-        tag_batch_change = TagBatchChange.new("...", "bbb", @user.id, "127.0.0.1")
-        tag_batch_change.perform
-
-        assert_equal("123 456 bbb", ss.reload.normalized_query)
-      end
-
-      should "move blacklists" do
-        @user.update(blacklisted_tags: "123 456\n789\n")
-        tag_batch_change = TagBatchChange.new("456", "xxx", @user.id, "127.0.0.1")
-        tag_batch_change.perform
-        @user.reload
-        assert_equal("123 xxx\n789", @user.blacklisted_tags)
-      end
-
-      should "move only saved searches that match the mass update exactly" do
-        ss = FactoryBot.create(:saved_search, :user => @user, :query => "123 ... 456")
-        tag_batch_change = TagBatchChange.new("1", "bbb", @user.id, "127.0.0.1")
-        tag_batch_change.perform
-
-        assert_equal("... 123 456", ss.reload.normalized_query, "expected '123' to remain unchanged")
-
-        tag_batch_change = TagBatchChange.new("123 456", "789", @user.id, "127.0.0.1")
-        tag_batch_change.perform
-
-        assert_equal("... 789", ss.reload.normalized_query, "expected '123 456' to be changed to '789'")
-      end
-
-      should "raise an error if there is no predicate" do
-        tag_batch_change = TagBatchChange.new("", "bbb", @user.id, "127.0.0.1")
-        assert_raises(TagBatchChange::Error) do
-          tag_batch_change.perform
-        end
-      end
-    end
-  end
-end