BURs: add processing and failed states.

When a BUR is approved, put it in a `processing` state. After it
successfully finishes processing, put it in the `approved` state. If it
fails processing, put it in the `failed` state.

If approving the BUR fails with a validation error, for example because
the alias already exists or an implication lacks a wiki, then leave the
BUR in the `pending` state. The `failed` state is only for unexpected
errors during processing.

app/logical/bulk_update_request_processor.rb

@@ -171,6 +171,11 @@ class BulkUpdateRequestProcessor
         raise Error, "Unknown command: #{command}"
       end
     end
+
+    bulk_update_request.update!(status: "approved")
+  rescue StandardError
+    bulk_update_request.update!(status: "failed")
+    raise
   end
 
   # The list of tags in the script. Used for search BURs by tag.

app/logical/d_text.rb

@@ -146,12 +146,19 @@ class DText
         embedded_script = "[expand]#{obj.processor.to_dtext}[/expand]"
       end
 
-      if obj.is_approved?
+      case obj.status
+      when "approved"
         "The \"bulk update request ##{obj.id}\":#{Routes.bulk_update_request_path(obj)} has been approved by <@#{obj.approver.name}>.\n\n#{embedded_script}"
-      elsif obj.is_pending?
+      when "pending"
         "The \"bulk update request ##{obj.id}\":#{Routes.bulk_update_request_path(obj)} is pending approval.\n\n#{embedded_script}"
-      elsif obj.is_rejected?
+      when "rejected"
         "The \"bulk update request ##{obj.id}\":#{Routes.bulk_update_request_path(obj)} has been rejected.\n\n#{embedded_script}"
+      when "processing"
+        "The \"bulk update request ##{obj.id}\":#{Routes.bulk_update_request_path(obj)} is being processed.\n\n#{embedded_script}"
+      when "failed"
+        "The \"bulk update request ##{obj.id}\":#{Routes.bulk_update_request_path(obj)} has failed.\n\n#{embedded_script}"
+      else
+        raise ArgumentError, "unknown bulk update request status"
       end
     end
   end

app/models/bulk_update_request.rb

@@ -1,4 +1,6 @@
 class BulkUpdateRequest < ApplicationRecord
+  STATUSES = %w[pending approved rejected processing failed]
+
   attr_accessor :title, :reason
 
   belongs_to :user
@@ -10,16 +12,18 @@ class BulkUpdateRequest < ApplicationRecord
   validates :script, presence: true
   validates :title, presence: true, if: ->(rec) { rec.forum_topic_id.blank? }
   validates :forum_topic, presence: true, if: ->(rec) { rec.forum_topic_id.present? }
-  validates :status, inclusion: { in: %w[pending approved rejected] }
+  validates :status, inclusion: { in: STATUSES }
   validate :validate_script, if: :script_changed?
 
   before_save :update_tags, if: :script_changed?
   after_create :create_forum_topic
 
-  scope :pending_first, -> { order(Arel.sql("(case status when 'pending' then 0 when 'approved' then 1 else 2 end)")) }
+  scope :pending_first, -> { order(Arel.sql("(case status when 'failed' then 0 when 'processing' then 1 when 'pending' then 2 when 'approved' then 3 when 'rejected' then 4 else 5 end)")) }
   scope :pending, -> {where(status: "pending")}
   scope :approved, -> { where(status: "approved") }
   scope :rejected, -> { where(status: "rejected") }
+  scope :processing, -> { where(status: "processing") }
+  scope :failed, -> { where(status: "failed") }
   scope :has_topic, -> { where.not(forum_topic: nil) }
 
   module SearchMethods
@@ -62,7 +66,7 @@ class BulkUpdateRequest < ApplicationRecord
       transaction do
         CurrentUser.scoped(approver) do
           processor.validate!(:approval)
-          update!(status: "approved", approver: approver)
+          update!(status: "processing", approver: approver)
           processor.process_later!
           forum_updater.update("The #{bulk_update_request_link} (forum ##{forum_post.id}) has been approved by @#{approver.name}.")
         end
@@ -116,7 +120,7 @@ class BulkUpdateRequest < ApplicationRecord
   end
 
   def is_approved?
-    status == "approved"
+    status.in?(%w[approved processing])
   end
 
   def is_rejected?

app/models/tag_relationship.rb

@@ -1,4 +1,6 @@
 class TagRelationship < ApplicationRecord
+  STATUSES = %w[active deleted retired]
+
   self.abstract_class = true
 
   belongs_to :creator, class_name: "User"
@@ -15,7 +17,7 @@ class TagRelationship < ApplicationRecord
   scope :retired, -> {where(status: "retired")}
 
   before_validation :normalize_names
-  validates :status, inclusion: { in: %w[active deleted retired] }
+  validates :status, inclusion: { in: STATUSES }
   validates :antecedent_name, presence: true
   validates :consequent_name, presence: true
   validates :approver, presence: { message: "must exist" }, if: -> { approver_id.present? }

app/views/bulk_update_requests/_search.html.erb

@@ -2,7 +2,7 @@
   <%= f.input :user_name, label: "Creator", input_html: { value: params[:search][:user_name], data: { autocomplete: "user" } } %>
   <%= f.input :approver_name, label: "Approver", input_html: { value: params[:search][:approver_name], data: { autocomplete: "user" } } %>
   <%= f.input :tags_include_any, label: "Tags", input_html: { value: params[:search][:tags_include_any], data: { autocomplete: "tag-query" } } %>
-  <%= f.input :status, label: "Status", collection: %w[pending approved rejected], include_blank: true, selected: params[:search][:status] %>
+  <%= f.input :status, label: "Status", collection: %w[pending approved rejected processing failed], include_blank: true, selected: params[:search][:status] %>
   <%= f.input :order, collection: [%w[Status status_desc], %w[Last\ updated updated_at_desc], %w[Newest id_desc], %w[Oldest id_asc]], include_blank: false, selected: params[:search][:order] %>
   <%= f.submit "Search" %>
 <% end %>

test/functional/bulk_update_requests_controller_test.rb

@@ -181,7 +181,10 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest
       context "for an admin" do
         should "succeed" do
           post_auth approve_bulk_update_request_path(@bulk_update_request), @admin
+
           assert_response :redirect
+          assert_equal("processing", @bulk_update_request.reload.status)
+          perform_enqueued_jobs
           assert_equal("approved", @bulk_update_request.reload.status)
         end
       end

test/unit/bulk_update_request_test.rb

@@ -3,7 +3,8 @@ require 'test_helper'
 class BulkUpdateRequestTest < ActiveSupport::TestCase
   def create_bur!(script, approver)
     bur = create(:bulk_update_request, script: script)
-    perform_enqueued_jobs { bur.approve!(approver) }
+    bur.approve!(approver)
+    perform_enqueued_jobs
     bur
   end
 
@@ -32,6 +33,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
 
           assert_equal(true, @bur.valid?)
           assert_equal(true, @tag.reload.artist?)
+          assert_equal("approved", @bur.reload.status)
         end
 
         should "fail if the tag doesn't already exist" do
@@ -53,6 +55,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           @alias = TagAlias.find_by(antecedent_name: "foo", consequent_name: "bar")
           assert_equal(true, @alias.present?)
           assert_equal(true, @alias.is_active?)
+          assert_equal("approved", @bur.reload.status)
         end
 
         should "rename the aliased tag's artist entry and wiki page" do
@@ -126,6 +129,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           @implication = TagImplication.find_by(antecedent_name: "foo", consequent_name: "bar")
           assert_equal(true, @implication.present?)
           assert_equal(true, @implication.is_active?)
+          assert_equal("approved", @bur.reload.status)
         end
 
         should "fail for an implication that is redundant with an existing implication" do
@@ -189,6 +193,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           @alias = TagAlias.find_by(antecedent_name: "foo", consequent_name: "bar")
           assert_equal(true, @alias.present?)
           assert_equal(true, @alias.is_deleted?)
+          assert_equal("approved", @bur.reload.status)
         end
 
         should "fail if the alias isn't active" do
@@ -212,6 +217,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           @alias = TagAlias.find_by(antecedent_name: "foo", consequent_name: "bar")
           assert_equal(true, @alias.present?)
           assert_equal(true, @alias.is_deleted?)
+          assert_equal("approved", @bur.reload.status)
         end
       end
 
@@ -223,6 +229,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           @implication = TagImplication.find_by(antecedent_name: "foo", consequent_name: "bar")
           assert_equal(true, @implication.present?)
           assert_equal(true, @implication.is_deleted?)
+          assert_equal("approved", @bur.reload.status)
         end
 
         should "fail if the implication isn't active" do
@@ -246,6 +253,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           @ti = TagImplication.find_by(antecedent_name: "foo", consequent_name: "bar")
           assert_equal(true, @ti.present?)
           assert_equal(true, @ti.is_deleted?)
+          assert_equal("approved", @bur.reload.status)
         end
       end
 
@@ -257,6 +265,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
 
         should "update the tags" do
           assert_equal("bar", @post.reload.tag_string)
+          assert_equal("approved", @bur.reload.status)
         end
 
         should "be case-sensitive" do
@@ -264,6 +273,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           @bur = create_bur!("mass update source:imageboard -> source:Imageboard", @admin)
 
           assert_equal("Imageboard", @post.reload.source)
+          assert_equal("approved", @bur.reload.status)
         end
       end
 
@@ -276,8 +286,8 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
         end
 
         should "rename the tags" do
-          assert_equal("approved", @bur.status)
           assert_equal("bar blah", @post.reload.tag_string)
+          assert_equal("approved", @bur.reload.status)
         end
 
         should "move the tag's artist entry and wiki page" do
@@ -337,18 +347,20 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
       context "the nuke command" do
         should "remove tags" do
           @post = create(:post, tag_string: "foo bar")
-          create_bur!("nuke bar", @admin)
+          @bur = create_bur!("nuke bar", @admin)
 
           assert_equal("foo", @post.reload.tag_string)
+          assert_equal("approved", @bur.reload.status)
         end
 
         should "remove implications" do
           @ti1 = create(:tag_implication, antecedent_name: "fly", consequent_name: "insect")
           @ti2 = create(:tag_implication, antecedent_name: "insect", consequent_name: "bug")
-          create_bur!("nuke insect", @admin)
+          @bur = create_bur!("nuke insect", @admin)
 
           assert_equal("deleted", @ti1.reload.status)
           assert_equal("deleted", @ti2.reload.status)
+          assert_equal("approved", @bur.reload.status)
         end
 
         should "remove pools" do
@@ -357,6 +369,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           @bur = create_bur!("nuke pool:#{@pool.id}", @admin)
 
           assert_equal([], @pool.post_ids)
+          assert_equal("approved", @bur.reload.status)
         end
       end
 
@@ -365,20 +378,22 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           @p1 = create(:post, tag_string: "maid_dress")
           @p2 = create(:post, tag_string: "maid")
 
-          create_bur!("mass update maid_dress -> maid dress\nalias maid_dress -> maid", @admin)
+          @bur = create_bur!("mass update maid_dress -> maid dress\nalias maid_dress -> maid", @admin)
 
           assert_equal("dress maid", @p1.reload.tag_string)
           assert_equal("maid", @p2.reload.tag_string)
+          assert_equal("approved", @bur.reload.status)
         end
       end
 
       context "that reverses an alias by removing and recreating it" do
         should "not fail with an alias conflict" do
           @ta = create(:tag_alias, antecedent_name: "rabbit", consequent_name: "bunny")
-          create_bur!("unalias rabbit -> bunny\nalias bunny -> rabbit", @admin)
+          @bur = create_bur!("unalias rabbit -> bunny\nalias bunny -> rabbit", @admin)
 
           assert_equal("deleted", @ta.reload.status)
           assert_equal("active", TagAlias.find_by(antecedent_name: "bunny", consequent_name: "rabbit").status)
+          assert_equal("approved", @bur.reload.status)
         end
       end
     end
@@ -494,6 +509,26 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
         assert_equal(@admin.id, @ta.approver.id)
         assert_equal(@admin.id, @ti.approver.id)
       end
+
+      should "set the BUR as approved" do
+        assert_equal("approved", @bur.reload.status)
+      end
+
+      should "set the BUR as failed if there is an unexpected error during processing" do
+        @bur = create(:bulk_update_request, script: "alias one -> two")
+        TagAlias.any_instance.stubs(:process!).raises(RuntimeError.new("oh no"))
+
+        assert_equal("pending", @bur.status)
+        @bur.approve!(@admin)
+        assert_equal("processing", @bur.status)
+
+        assert_raises(RuntimeError) { perform_enqueued_jobs }
+        assert_equal("failed", @bur.reload.status)
+
+        assert_equal("active", TagAlias.find_by!(antecedent_name: "one", consequent_name: "two").status)
+        assert_equal("alias one -> two", @bur.script)
+        assert_equal(@admin, @bur.approver)
+      end
     end
 
     should "create a forum topic" do
@@ -511,7 +546,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
         @req = FactoryBot.create(:bulk_update_request, :script => "create alias AAA -> BBB", :forum_topic_id => @topic.id, :forum_post_id => @post.id, :title => "[bulk] hoge")
       end
 
-      should "gracefully handle validation errors during approval" do
+      should "leave the BUR pending if there is a validation error during approval" do
         @req.stubs(:update!).raises(BulkUpdateRequestProcessor::Error.new("blah"))
         assert_equal("pending", @req.reload.status)
       end

test/unit/d_text_test.rb

@@ -76,13 +76,22 @@ class DTextTest < ActiveSupport::TestCase
       end
 
       should "parse [ta:<id>], [ti:<id>], [bur:<id>] pseudo tags" do
-        @bur = create(:bulk_update_request)
+        @bur = create(:bulk_update_request, approver: create(:admin_user))
         @ti = create(:tag_implication)
         @ta = create(:tag_alias)
 
-        assert_match(/bulk update request/, DText.format_text("[bur:#{@bur.id}]"))
+        BulkUpdateRequest::STATUSES.each do |status|
-        assert_match(/implication ##{@ti.id}/, DText.format_text("[ti:#{@ti.id}]"))
+          @bur.update!(status: status)
-        assert_match(/alias ##{@ta.id}/, DText.format_text("[ta:#{@ta.id}]"))
+          assert_match(/bulk update request/, DText.format_text("[bur:#{@bur.id}]"))
+        end
+
+        TagRelationship::STATUSES.each do |status|
+          @ta.update!(status: status)
+          @ti.update!(status: status)
+
+          assert_match(/implication ##{@ti.id}/, DText.format_text("[ti:#{@ti.id}]"))
+          assert_match(/alias ##{@ta.id}/, DText.format_text("[ta:#{@ta.id}]"))
+        end
       end
 
       should "link artist tags to the artist page instead of the wiki page" do