From 1d8a3bf09f917e083230d13f5b418390479fe408 Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Mon, 20 Sep 2021 16:29:38 -0500
Subject: [PATCH] BURs: allow failed BURs to be reapproved.

Fix it so that you can reapprove a failed BUR to run it again. Before
this would fail because it would end up trying to create the aliases
or implications again, which would fail because they already existed.
Now it ignores when an alias or implication already exists. It will
however finish tagging the posts if they haven't been fully moved.
---
 app/models/tag_alias.rb               |  4 ++-
 app/models/tag_relationship.rb        | 15 ++++++++++-
 test/unit/bulk_update_request_test.rb | 36 +++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb
index 1afdb43ac..4070e61e0 100644
--- a/app/models/tag_alias.rb
+++ b/app/models/tag_alias.rb
@@ -1,5 +1,7 @@
 class TagAlias < TagRelationship
-  validates_uniqueness_of :antecedent_name, scope: :status, conditions: -> { active }
+  # Validate that the alias doesn't exist yet when it's created or when a BUR
+  # is requested, but not when a BUR is approved (to allow failed BURs to be reapproved)
+  validates_uniqueness_of :antecedent_name, scope: :status, conditions: -> { active }, on: %i[create update request]
   validate :absence_of_transitive_relation
 
   before_create :delete_conflicting_relationships
diff --git a/app/models/tag_relationship.rb b/app/models/tag_relationship.rb
index 2c7fecfe8..58dfd17e4 100644
--- a/app/models/tag_relationship.rb
+++ b/app/models/tag_relationship.rb
@@ -115,8 +115,21 @@ class TagRelationship < ApplicationRecord
     end
   end
 
+  # Create a new alias or implication, set it to active, and update the posts.
+  # This method is idempotent; if the alias or implication already exists, keep
+  # it and update the posts again.
+  #
+  # @param antecedent_name [String] the left-hand side of the alias or implication
+  # @param consequent_name [String] the right-hand side of the alias or implication
+  # @param approver [User] the user approving the alias or implication
+  # @param forum_topic [ForumTopic] the forum topic where the alias or implication was requested.
   def self.approve!(antecedent_name:, consequent_name:, approver:, forum_topic: nil)
-    tag_relationship = create!(creator: approver, approver: approver, antecedent_name: antecedent_name, consequent_name: consequent_name, forum_topic: forum_topic)
+    tag_relationship = find_or_create_by!(antecedent_name: antecedent_name, consequent_name: consequent_name, status: "active") do |relationship|
+      relationship.creator = approver
+      relationship.approver = approver
+      relationship.forum_topic = forum_topic
+    end
+
     tag_relationship.process!
   end
 
diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb
index 0930e0876..c2f3ba57e 100644
--- a/test/unit/bulk_update_request_test.rb
+++ b/test/unit/bulk_update_request_test.rb
@@ -539,6 +539,42 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
       end
     end
 
+    context "when a bulk update request fails" do
+      should "allow it to be approved again" do
+        @post = create(:post, tag_string: "foo aaa")
+        @bur = create(:bulk_update_request, script: "alias foo -> bar")
+
+        TagAlias.any_instance.stubs(:process!).raises(RuntimeError.new("oh no"))
+        @bur.approve!(@admin)
+        assert_raises(RuntimeError) { perform_enqueued_jobs }
+
+        assert_equal("aaa foo", @post.reload.tag_string)
+
+        assert_equal("failed", @bur.reload.status)
+        assert_not_nil(@bur.forum_topic)
+        assert_equal(@admin, @bur.approver)
+
+        @ta = TagAlias.find_by!(antecedent_name: "foo", consequent_name: "bar")
+        assert_equal("active", @ta.status)
+        assert_equal(@admin, @ta.approver)
+        assert_equal(@bur.forum_topic, @ta.forum_topic)
+
+        TagAlias.any_instance.unstub(:process!)
+        @bur.approve!(@admin)
+        perform_enqueued_jobs
+
+        assert_equal("aaa bar", @post.reload.tag_string)
+
+        assert_equal("approved", @bur.reload.status)
+        assert_not_nil(@bur.forum_topic)
+        assert_equal(@admin, @bur.approver)
+
+        assert_equal("active", @ta.reload.status)
+        assert_equal(@admin, @ta.approver)
+        assert_equal(@bur.forum_topic, @ta.forum_topic)
+      end
+    end
+
     should "create a forum topic" do
       bur = create(:bulk_update_request, reason: "zzz", script: "create alias aaa -> bbb")