diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index b3d228009..9d3882959 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -18,6 +18,7 @@ class TagImplication < ApplicationRecord validates :forum_topic, presence: { message: "must exist" }, if: lambda { forum_topic_id.present? } validates_uniqueness_of :antecedent_name, :scope => :consequent_name validate :absence_of_circular_relation + validate :absence_of_transitive_relation validate :antecedent_is_not_aliased validate :consequent_is_not_aliased validate :antecedent_and_consequent_are_different @@ -137,6 +138,16 @@ class TagImplication < ApplicationRecord end end + # If we already have a -> b -> c, don't allow a -> c. + def absence_of_transitive_relation + # Find everything else the antecedent implies, not including the current implication. + implications = TagImplication.active.where("antecedent_name = ? and consequent_name != ?", antecedent_name, consequent_name) + implied_tags = implications.flat_map(&:descendant_names_array) + if implied_tags.include?(consequent_name) + self.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]) diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index a77771850..654c91ab4 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -62,6 +62,35 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase end end + context "for an implication that is redundant with an existing implication" do + should "not validate" do + FactoryGirl.create(:tag_implication, :antecedent_name => "a", :consequent_name => "b") + FactoryGirl.create(:tag_implication, :antecedent_name => "b", :consequent_name => "c") + bur = FactoryGirl.build(:bulk_update_request, :script => "imply a -> c") + bur.save + + assert_equal(["Error: a already implies c through another implication (create implication a -> c)"], bur.errors.full_messages) + end + end + + context "for an implication that is redundant with another implication in the same BUR" do + setup do + FactoryGirl.create(:tag_implication, :antecedent_name => "b", :consequent_name => "c") + @bur = FactoryGirl.build(:bulk_update_request, :script => "imply a -> b\nimply a -> c") + @bur.save + end + + should "not process" do + assert_no_difference("TagImplication.count") do + @bur.approve!(@admin) + end + end + + should_eventually "not validate" do + assert_equal(["Error: a already implies c through another implication (create implication a -> c)"], @bur.errors.full_messages) + end + end + context "with an associated forum topic" do setup do @topic = FactoryGirl.create(:forum_topic, :title => "[bulk] hoge") diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index 5ac1a5ef9..cbcfba0fc 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -66,12 +66,21 @@ class TagImplicationTest < ActiveSupport::TestCase assert_equal("Tag implication can not create a circular relation with another tag implication", ti2.errors.full_messages.join("")) end + should "not validate when a transitive relation is created" do + ti_ab = FactoryGirl.create(:tag_implication, :antecedent_name => "a", :consequent_name => "b") + ti_bc = FactoryGirl.create(:tag_implication, :antecedent_name => "b", :consequent_name => "c") + ti_ac = FactoryGirl.build(:tag_implication, :antecedent_name => "a", :consequent_name => "c") + ti_ac.save + + assert_equal("a already implies c through another implication", ti_ac.errors.full_messages.join("")) + end + should "not allow for duplicates" do ti1 = FactoryGirl.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") ti2 = FactoryGirl.build(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") ti2.save assert(ti2.errors.any?, "Tag implication should not have validated.") - assert_equal("Antecedent name has already been taken", ti2.errors.full_messages.join("")) + 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