diff --git a/app/logical/bulk_update_request_processor.rb b/app/logical/bulk_update_request_processor.rb index 65767f00d..d91f48c1e 100644 --- a/app/logical/bulk_update_request_processor.rb +++ b/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. diff --git a/app/logical/d_text.rb b/app/logical/d_text.rb index 39568af11..9da6bf7ae 100644 --- a/app/logical/d_text.rb +++ b/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 diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb index dc0ae4bb0..0d9390541 100644 --- a/app/models/bulk_update_request.rb +++ b/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? diff --git a/app/models/tag_relationship.rb b/app/models/tag_relationship.rb index 6cbd532c7..2c7fecfe8 100644 --- a/app/models/tag_relationship.rb +++ b/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? } diff --git a/app/views/bulk_update_requests/_search.html.erb b/app/views/bulk_update_requests/_search.html.erb index 11c3d26f8..63b89c04e 100644 --- a/app/views/bulk_update_requests/_search.html.erb +++ b/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 %> diff --git a/test/functional/bulk_update_requests_controller_test.rb b/test/functional/bulk_update_requests_controller_test.rb index 0864f8a54..709b4b9d2 100644 --- a/test/functional/bulk_update_requests_controller_test.rb +++ b/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 diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index 2ee22b05c..d5f9c0e3c 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/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 diff --git a/test/unit/d_text_test.rb b/test/unit/d_text_test.rb index 710abd0ef..023a33f91 100644 --- a/test/unit/d_text_test.rb +++ b/test/unit/d_text_test.rb @@ -76,13 +76,22 @@ class DTextTest < ActiveSupport::TestCase end should "parse [ta:], [ti:], [bur:] 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}]")) - assert_match(/implication ##{@ti.id}/, DText.format_text("[ti:#{@ti.id}]")) - assert_match(/alias ##{@ta.id}/, DText.format_text("[ta:#{@ta.id}]")) + BulkUpdateRequest::STATUSES.each do |status| + @bur.update!(status: status) + 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