BURs: allow reapproving failed BURs containing alias or implication removals.

Make it possible to reapprove failed BURs that removed aliases or
implications.

Before if a BUR failed midway through, and we tried to reapprove it,
then it would fail when it got to a `remove alias` line because the
alias had already been removed. Now we keep going if we try to remove an
alias or implication that has already been removed.

app/logical/bulk_update_request_processor.rb

@@ -79,7 +79,10 @@ class BulkUpdateRequestProcessor
 
         when :remove_alias
           tag_alias = TagAlias.active.find_by(antecedent_name: args[0], consequent_name: args[1])
-          if tag_alias.nil?
+
+          if validation_context == :approval
+            # ignore non-existing aliases when approving a BUR
+          elsif tag_alias.nil?
             errors.add(:base, "Can't remove alias #{args[0]} -> #{args[1]} (alias doesn't exist)")
           else
             tag_alias.update(status: "deleted")
@@ -87,7 +90,10 @@ class BulkUpdateRequestProcessor
 
         when :remove_implication
           tag_implication = TagImplication.active.find_by(antecedent_name: args[0], consequent_name: args[1])
-          if tag_implication.nil?
+
+          if validation_context == :approval
+            # ignore non-existing implication when approving a BUR
+          elsif tag_implication.nil?
             errors.add(:base, "Can't remove implication #{args[0]} -> #{args[1]} (implication doesn't exist)")
           else
             tag_implication.update(status: "deleted")
@@ -147,12 +153,12 @@ class BulkUpdateRequestProcessor
           TagImplication.approve!(antecedent_name: args[0], consequent_name: args[1], approver: approver, forum_topic: forum_topic)
 
         when :remove_alias
-          tag_alias = TagAlias.active.find_by!(antecedent_name: args[0], consequent_name: args[1])
-          tag_alias.reject!(User.system)
+          tag_alias = TagAlias.active.find_by(antecedent_name: args[0], consequent_name: args[1])
+          tag_alias&.reject!(User.system)
 
         when :remove_implication
-          tag_implication = TagImplication.active.find_by!(antecedent_name: args[0], consequent_name: args[1])
-          tag_implication.reject!(User.system)
+          tag_implication = TagImplication.active.find_by(antecedent_name: args[0], consequent_name: args[1])
+          tag_implication&.reject!(User.system)
 
         when :mass_update
           BulkUpdateRequestProcessor.mass_update(args[0], args[1])

test/unit/bulk_update_request_test.rb

@@ -220,7 +220,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           assert_equal("approved", @bur.reload.status)
         end
 
-        should "fail if the alias isn't active" do
+        should "fail to validate if the alias isn't active" do
           create(:tag_alias, antecedent_name: "foo", consequent_name: "bar", status: "deleted")
           @bur = build(:bulk_update_request, script: "remove alias foo -> bar")
 
@@ -228,13 +228,25 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           assert_equal(["Can't remove alias foo -> bar (alias doesn't exist)"], @bur.errors[:base])
         end
 
-        should "fail if the alias doesn't already exist" do
+        should "fail to validate if the alias doesn't already exist" do
           @bur = build(:bulk_update_request, script: "remove alias foo -> bar")
 
           assert_equal(false, @bur.valid?)
           assert_equal(["Can't remove alias foo -> bar (alias doesn't exist)"], @bur.errors[:base])
         end
 
+        should "allow reapproving a failed BUR when the alias has already been removed" do
+          @alias = create(:tag_alias, antecedent_name: "foo", consequent_name: "bar")
+          @bur = create(:bulk_update_request, script: "unalias foo -> bar", status: "failed")
+          @alias.reject!
+
+          @bur.approve!(@admin)
+          perform_enqueued_jobs
+
+          assert_equal(true, @alias.reload.is_deleted?)
+          assert_equal("approved", @bur.reload.status)
+        end
+
         should "be processed sequentially after the create alias command" do
           @bur = create_bur!("create alias foo -> bar\nremove alias foo -> bar", @admin)
 
@@ -256,7 +268,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           assert_equal("approved", @bur.reload.status)
         end
 
-        should "fail if the implication isn't active" do
+        should "fail to validate if the implication isn't active" do
           create(:tag_implication, antecedent_name: "foo", consequent_name: "bar", status: "deleted")
           @bur = build(:bulk_update_request, script: "remove implication foo -> bar")
 
@@ -264,13 +276,25 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           assert_equal(["Can't remove implication foo -> bar (implication doesn't exist)"], @bur.errors[:base])
         end
 
-        should "fail if the implication doesn't already exist" do
+        should "fail to validate if the implication doesn't already exist" do
           @bur = build(:bulk_update_request, script: "remove implication foo -> bar")
 
           assert_equal(false, @bur.valid?)
           assert_equal(["Can't remove implication foo -> bar (implication doesn't exist)"], @bur.errors[:base])
         end
 
+        should "allow reapproving a failed BUR when the implication has already been removed" do
+          @implication = create(:tag_implication, antecedent_name: "foo", consequent_name: "bar")
+          @bur = create(:bulk_update_request, script: "unimply foo -> bar", status: "failed")
+          @implication.reject!
+
+          @bur.approve!(@admin)
+          perform_enqueued_jobs
+
+          assert_equal(true, @implication.reload.is_deleted?)
+          assert_equal("approved", @bur.reload.status)
+        end
+
         should "be processed sequentially after the create implication command" do
           @bur = create_bur!("imply foo -> bar\nunimply foo -> bar", @admin)