From 6485a053a55ac473288eed29d595cafbf7cb1249 Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Wed, 2 Dec 2020 12:44:43 -0600
Subject: [PATCH] aliases: allow aliases to be reversed in one step.

Allow reversing an alias without having to remove the old alias first.
When aliasing A -> B, then if B -> A already exists it will
automatically be removed first.
---
 app/models/tag_alias.rb               | 19 +++++++++++++++----
 test/unit/bulk_update_request_test.rb | 14 +++++++++++---
 test/unit/tag_alias_test.rb           |  2 +-
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb
index 82c79374f..6657b3868 100644
--- a/app/models/tag_alias.rb
+++ b/app/models/tag_alias.rb
@@ -2,6 +2,8 @@ class TagAlias < TagRelationship
   validates_uniqueness_of :antecedent_name, scope: :status, conditions: -> { active }
   validate :absence_of_transitive_relation
 
+  before_create :delete_conflicting_alias
+
   def self.to_aliased(names)
     names = Array(names).map(&:to_s)
     return [] if names.empty?
@@ -13,13 +15,22 @@ class TagAlias < TagRelationship
     TagMover.new(antecedent_name, consequent_name, user: User.system).move!
   end
 
+  # We don't want a -> b && b -> c chains if the b -> c alias was created
+  # first. If the a -> b alias was created first, the new one will be allowed
+  # and the old one will be moved automatically instead.
   def absence_of_transitive_relation
     return if is_rejected?
 
-    # We don't want a -> b && b -> c chains if the b -> c alias was created first.
-    # If the a -> b alias was created first, the new one will be allowed and the old one will be moved automatically instead.
-    if TagAlias.active.exists?(antecedent_name: consequent_name)
-      errors[:base] << "A tag alias for #{consequent_name} already exists"
+    tag_alias = TagAlias.active.find_by(antecedent_name: consequent_name)
+    if tag_alias.present? && tag_alias.consequent_name != antecedent_name
+      errors[:base] << "#{tag_alias.antecedent_name} is already aliased to #{tag_alias.consequent_name}"
     end
   end
+
+  # Allow aliases to be reversed. If A -> B already exists, but we're trying to
+  # create B -> A, then automatically delete A -> B so we can make B -> A.
+  def delete_conflicting_alias
+    tag_alias = TagAlias.active.find_by(antecedent_name: consequent_name, consequent_name: antecedent_name)
+    tag_alias.reject! if tag_alias.present?
+  end
 end
diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb
index 01c4a1502..3bd7f7486 100644
--- a/test/unit/bulk_update_request_test.rb
+++ b/test/unit/bulk_update_request_test.rb
@@ -82,12 +82,20 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           assert_equal(true, TagImplication.where(antecedent_name: "ddd", consequent_name: "ccc", status: "active").exists?)
         end
 
+        should "allow aliases to be reversed in one step" do
+          @alias = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb")
+          @bur = create_bur!("create alias bbb -> aaa", @admin)
+
+          assert_equal(true, @alias.reload.is_deleted?)
+          assert_equal(true, TagAlias.active.exists?(antecedent_name: "bbb", consequent_name: "aaa"))
+        end
+
         should "fail if the alias is invalid" do
-          create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb")
-          @bur = build(:bulk_update_request, script: "create alias bbb -> aaa")
+          @alias = create(:tag_alias, antecedent_name: "bbb", consequent_name: "ccc")
+          @bur = build(:bulk_update_request, script: "create alias aaa -> bbb")
 
           assert_equal(false, @bur.valid?)
-          assert_equal(["Can't create alias bbb -> aaa (A tag alias for aaa already exists)"], @bur.errors.full_messages)
+          assert_equal(["Can't create alias aaa -> bbb (bbb is already aliased to ccc)"], @bur.errors.full_messages)
         end
 
         should "be case-insensitive" do
diff --git a/test/unit/tag_alias_test.rb b/test/unit/tag_alias_test.rb
index 9e1b77b2e..1aeb005b9 100644
--- a/test/unit/tag_alias_test.rb
+++ b/test/unit/tag_alias_test.rb
@@ -138,7 +138,7 @@ class TagAliasTest < ActiveSupport::TestCase
         ta2 = FactoryBot.build(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb")
         ta2.save
         assert(ta2.errors.any?, "Tag alias should be invalid")
-        assert_equal("A tag alias for bbb already exists", ta2.errors.full_messages.join)
+        assert_equal("bbb is already aliased to ccc", ta2.errors.full_messages.join)
       end
     end