From c27ba02b883d002ba1c1356946f6e005fe69a6d4 Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 26 Dec 2018 17:30:07 -0600 Subject: [PATCH] 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 | 33 +++++++--------------------- app/models/tag_implication.rb | 36 +++++++++---------------------- app/models/tag_relationship.rb | 9 ++++++++ test/unit/tag_implication_test.rb | 29 ++++++++++++++++++++----- 4 files changed, 51 insertions(+), 56 deletions(-) diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb index be721f75d..58c36721e 100644 --- a/app/models/tag_alias.rb +++ b/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 - validate :mininum_antecedent_count, :on => :create + validate :consequent_has_wiki_page, on: :create, unless: :skip_secondary_validations + validate :mininum_antecedent_count, on: :create, unless: :skip_secondary_validations 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]) - self.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 + if TagAlias.active.exists?(antecedent_name: consequent_name) + errors[:base] << "A tag alias for #{consequent_name} already exists" 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 - - unless WikiPage.titled(consequent_name).exists? - self.errors[:base] << "The #{consequent_name} tag needs a corresponding wiki page" - return false + if consequent_wiki.nil? + errors[:base] << "The #{consequent_name} tag needs a corresponding wiki page" end end def mininum_antecedent_count - return if skip_secondary_validations - - 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" + if antecedent_tag.post_count < 50 + errors[:base] << "The #{antecedent_name} tag must have at least 50 posts for an alias to be created" end end diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index 4bca6bff0..9d9a851ad 100644 --- a/app/models/tag_implication.rb +++ b/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 + validate :wiki_pages_present, on: :create, unless: :skip_secondary_validations 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]) - self.errors[:base] << "Tag implication can not create a circular relation with another tag implication" - false + if TagImplication.active.exists?(["antecedent_name = ? and consequent_name = ?", consequent_name, antecedent_name]) + errors[:base] << "Tag implication can not create a circular relation with another tag implication" 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" - false + errors[:base] << "Antecedent tag must not be aliased to another tag" 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" - 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 + errors[:base] << "Consequent tag must not be aliased to another tag" end end def wiki_pages_present - return if skip_secondary_validations - - unless WikiPage.titled(consequent_name).exists? - self.errors[:base] << "The #{consequent_name} tag needs a corresponding wiki page" - return false + if consequent_wiki.blank? + errors[:base] << "The #{consequent_name} tag needs a corresponding wiki page" end - unless WikiPage.titled(antecedent_name).exists? - self.errors[:base] << "The #{antecedent_name} tag needs a corresponding wiki page" - return false + if antecedent_wiki.blank? + errors[:base] << "The #{antecedent_name} tag needs a corresponding wiki page" end end end diff --git a/app/models/tag_relationship.rb b/app/models/tag_relationship.rb index 649f2bbad..deb39def8 100644 --- a/app/models/tag_relationship.rb +++ b/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 diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index cdc243de9..aac6b0904 100644 --- a/test/unit/tag_implication_test.rb +++ b/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 - ta = FactoryBot.create(:tag_alias, :antecedent_name => "bbb", :consequent_name => "ccc") + should "not validate if its antecedent or consequent are aliased to another tag" do + 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_equal("Consequent tag must not be aliased to another tag", ti.errors.full_messages.join("")) + + assert(ti.invalid?) + 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