aliases/implications: clean up validations.

* Don't return true/false in validations (does nothing).
* Prefer `errors[:base]` over `self.errors[:base]`.
* Add antecedent_wiki / consequent_wiki associations.
* Factor out antecedent_and_consequent_are_different validation.

app/models/tag_alias.rb

@@ -6,9 +6,8 @@ class TagAlias < TagRelationship
   after_save :create_mod_action
   validates_uniqueness_of :antecedent_name
   validate :absence_of_transitive_relation
-  validate :antecedent_and_consequent_are_different
+  validate :consequent_has_wiki_page, on: :create, unless: :skip_secondary_validations
-  validate :consequent_has_wiki_page, :on => :create
+  validate :mininum_antecedent_count, on: :create, unless: :skip_secondary_validations
-  validate :mininum_antecedent_count, :on => :create
 
   module CacheMethods
     extend ActiveSupport::Concern
@@ -101,17 +100,8 @@ class TagAlias < TagRelationship
   def absence_of_transitive_relation
     # 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 self.class.active.exists?(["antecedent_name = ?", consequent_name])
+    if TagAlias.active.exists?(antecedent_name: consequent_name)
-      self.errors[:base] << "A tag alias for #{consequent_name} already exists"
+      errors[:base] << "A tag alias for #{consequent_name} already exists"
-      false
-    end
-  end
-
-  def antecedent_and_consequent_are_different
-    normalize_names
-    if antecedent_name == consequent_name
-      self.errors[:base] << "Cannot alias a tag to itself"
-      false
     end
   end
 
@@ -164,8 +154,6 @@ class TagAlias < TagRelationship
     if antecedent_tag.category != consequent_tag.category && antecedent_tag.category != Tag.categories.general
       consequent_tag.update_attribute(:category, antecedent_tag.category)
     end
-
-    true
   end
 
   def update_posts
@@ -214,19 +202,14 @@ class TagAlias < TagRelationship
   end
 
   def consequent_has_wiki_page
-    return if skip_secondary_validations
+    if consequent_wiki.nil?
-
+      errors[:base] << "The #{consequent_name} tag needs a corresponding wiki page"
-    unless WikiPage.titled(consequent_name).exists?
-      self.errors[:base] << "The #{consequent_name} tag needs a corresponding wiki page"
-      return false
     end
   end
 
   def mininum_antecedent_count
-    return if skip_secondary_validations
+    if antecedent_tag.post_count < 50
-
+      errors[:base] << "The #{antecedent_name} tag must have at least 50 posts for an alias to be created"
-    unless Post.fast_count(antecedent_name) >= 50
-      self.errors[:base] << "The #{antecedent_name} tag must have at least 50 posts for an alias to be created"
     end
   end
 

app/models/tag_implication.rb

@@ -12,8 +12,7 @@ class TagImplication < TagRelationship
   validate :absence_of_transitive_relation
   validate :antecedent_is_not_aliased
   validate :consequent_is_not_aliased
-  validate :antecedent_and_consequent_are_different
+  validate :wiki_pages_present, on: :create, unless: :skip_secondary_validations
-  validate :wiki_pages_present, :on => :create
   scope :old, ->{where("created_at between ? and ?", 2.months.ago, 1.month.ago)}
   scope :pending, ->{where(status: "pending")}
 
@@ -78,9 +77,8 @@ class TagImplication < TagRelationship
   module ValidationMethods
     def absence_of_circular_relation
       # We don't want a -> b && b -> a chains
-      if self.class.active.exists?(["antecedent_name = ? and consequent_name = ?", consequent_name, antecedent_name])
+      if TagImplication.active.exists?(["antecedent_name = ? and consequent_name = ?", consequent_name, antecedent_name])
-        self.errors[:base] << "Tag implication can not create a circular relation with another tag implication"
+        errors[:base] << "Tag implication can not create a circular relation with another tag implication"
-        false
       end
     end
 
@@ -90,45 +88,31 @@ class TagImplication < TagRelationship
       implications = TagImplication.active.where("antecedent_name = ? and consequent_name != ?", antecedent_name, consequent_name)
       implied_tags = implications.flat_map(&:descendant_names)
       if implied_tags.include?(consequent_name)
-        self.errors[:base] << "#{antecedent_name} already implies #{consequent_name} through another implication"
+        errors[:base] << "#{antecedent_name} already implies #{consequent_name} through another implication"
       end
     end
 
     def antecedent_is_not_aliased
       # We don't want to implicate a -> b if a is already aliased to c
       if TagAlias.active.exists?(["antecedent_name = ?", antecedent_name])
-        self.errors[:base] << "Antecedent tag must not be aliased to another tag"
+        errors[:base] << "Antecedent tag must not be aliased to another tag"
-        false
       end
     end
 
     def consequent_is_not_aliased
       # We don't want to implicate a -> b if b is already aliased to c
       if TagAlias.active.exists?(["antecedent_name = ?", consequent_name])
-        self.errors[:base] << "Consequent tag must not be aliased to another tag"
+        errors[:base] << "Consequent tag must not be aliased to another tag"
-        false
-      end
-    end
-
-    def antecedent_and_consequent_are_different
-      normalize_names
-      if antecedent_name == consequent_name
-        self.errors[:base] << "Cannot implicate a tag to itself"
-        false
       end
     end
 
     def wiki_pages_present
-      return if skip_secondary_validations
+      if consequent_wiki.blank?
-
+        errors[:base] << "The #{consequent_name} tag needs a corresponding wiki page"
-      unless WikiPage.titled(consequent_name).exists?
-        self.errors[:base] << "The #{consequent_name} tag needs a corresponding wiki page"
-        return false
       end
 
-      unless WikiPage.titled(antecedent_name).exists?
+      if antecedent_wiki.blank?
-        self.errors[:base] << "The #{antecedent_name} tag needs a corresponding wiki page"
+        errors[:base] << "The #{antecedent_name} tag needs a corresponding wiki page"
-        return false
       end
     end
   end

app/models/tag_relationship.rb

@@ -12,6 +12,8 @@ class TagRelationship < ApplicationRecord
   belongs_to :forum_topic, optional: true
   has_one :antecedent_tag, :class_name => "Tag", :foreign_key => "name", :primary_key => "antecedent_name"
   has_one :consequent_tag, :class_name => "Tag", :foreign_key => "name", :primary_key => "consequent_name"
+  has_one :antecedent_wiki, through: :antecedent_tag, source: :wiki_page
+  has_one :consequent_wiki, through: :consequent_tag, source: :wiki_page
 
   scope :active, ->{where(status: "active")}
   scope :expired, ->{where("created_at < ?", EXPIRY.days.ago)}
@@ -26,6 +28,7 @@ class TagRelationship < ApplicationRecord
   validates :creator, presence: { message: "must exist" }, if: -> { creator_id.present? }
   validates :approver, presence: { message: "must exist" }, if: -> { approver_id.present? }
   validates :forum_topic, presence: { message: "must exist" }, if: -> { forum_topic_id.present? }
+  validate :antecedent_and_consequent_are_different
 
   def initialize_creator
     self.creator_id = CurrentUser.user.id
@@ -163,6 +166,12 @@ class TagRelationship < ApplicationRecord
     end
   end
 
+  def antecedent_and_consequent_are_different
+    if antecedent_name == consequent_name
+      errors[:base] << "Cannot alias or implicate a tag to itself"
+    end
+  end
+
   extend SearchMethods
   include MessageMethods
 end

test/unit/tag_implication_test.rb

@@ -42,6 +42,16 @@ class TagImplicationTest < ActiveSupport::TestCase
       should_not allow_value(-1).for(:creator_id).with_message("must exist", against: :creator)
     end
 
+    context "on secondary validation" do
+      should "warn if either tag is missing a wiki" do
+        ti = FactoryBot.build(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", skip_secondary_validations: false)
+
+        assert(ti.invalid?)
+        assert_includes(ti.errors[:base], "The aaa tag needs a corresponding wiki page")
+        assert_includes(ti.errors[:base], "The bbb tag needs a corresponding wiki page")
+      end
+    end
+
     should "ignore pending implications when building descendant names" do
       ti2 = FactoryBot.build(:tag_implication, :antecedent_name => "b", :consequent_name => "c", :status => "pending")
       ti2.save
@@ -54,6 +64,13 @@ class TagImplicationTest < ActiveSupport::TestCase
       assert_equal(CurrentUser.user.id, ti.creator_id)
     end
 
+    should "not validate when a tag directly implicates itself" do
+      ti = FactoryBot.build(:tag_implication, antecedent_name: "a", consequent_name: "a")
+
+      assert(ti.invalid?)
+      assert_includes(ti.errors[:base], "Cannot alias or implicate a tag to itself")
+    end
+
     should "not validate when a circular relation is created" do
       ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb")
       ti2 = FactoryBot.build(:tag_implication, :antecedent_name => "bbb", :consequent_name => "aaa")
@@ -79,12 +96,14 @@ class TagImplicationTest < ActiveSupport::TestCase
       assert_includes(ti2.errors.full_messages, "Antecedent name has already been taken")
     end
 
-    should "not validate if its consequent is aliased to another tag" do
+    should "not validate if its antecedent or consequent are aliased to another tag" do
-      ta = FactoryBot.create(:tag_alias, :antecedent_name => "bbb", :consequent_name => "ccc")
+      ta1 = FactoryBot.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "a")
+      ta2 = FactoryBot.create(:tag_alias, :antecedent_name => "bbb", :consequent_name => "b")
       ti = FactoryBot.build(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb")
-      ti.save
+
-      assert(ti.errors.any?, "Tag implication should not have validated.")
+      assert(ti.invalid?)
-      assert_equal("Consequent tag must not be aliased to another tag", ti.errors.full_messages.join(""))
+      assert_includes(ti.errors[:base], "Antecedent tag must not be aliased to another tag")
+      assert_includes(ti.errors[:base], "Consequent tag must not be aliased to another tag")
     end
 
     should "calculate all its descendants" do