From c5b5ca98256ecbe8f95ce416373cadd287ae6a03 Mon Sep 17 00:00:00 2001 From: Toks Date: Sun, 6 Oct 2013 16:56:39 -0400 Subject: [PATCH] fixes #1382 --- app/models/tag_alias.rb | 20 ++++++++++++++-- app/models/tag_implication.rb | 9 +++++++ ..._update_aliased_implication_consequents.rb | 13 ++++++++++ test/factories/tag_alias.rb | 5 +++- test/factories/tag_implication.rb | 5 +++- test/unit/tag_alias_test.rb | 24 +++++++++++++++---- test/unit/tag_implication_test.rb | 10 +++++++- 7 files changed, 76 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20131006193238_update_aliased_implication_consequents.rb diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb index 8c03620c4..3be94b726 100644 --- a/app/models/tag_alias.rb +++ b/app/models/tag_alias.rb @@ -72,6 +72,7 @@ class TagAlias < ActiveRecord::Base def process! update_column(:status, "processing") + move_aliases_and_implications clear_all_cache ensure_category_consistency update_posts @@ -107,13 +108,28 @@ class TagAlias < ActiveRecord::Base end def absence_of_transitive_relation - # We don't want a -> b && b -> c chains - if self.class.exists?(["antecedent_name = ?", consequent_name]) || self.class.exists?(["consequent_name = ?", antecedent_name]) + # 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.exists?(["antecedent_name = ?", consequent_name]) self.errors[:base] << "Tag alias can not create a transitive relation with another tag alias" false end end + def move_aliases_and_implications + aliases = TagAlias.where(["consequent_name = ?", antecedent_name]) + aliases.each do |ta| + ta.consequent_name = self.consequent_name + ta.save + end + + implications = TagImplication.where(["consequent_name = ?", antecedent_name]) + implications.each do |ti| + ti.consequent_name = self.consequent_name + ti.save + end + end + def ensure_tags_exist Tag.find_or_create_by_name(antecedent_name) Tag.find_or_create_by_name(consequent_name) diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index caa7659ce..c97ea3543 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -8,6 +8,7 @@ class TagImplication < ActiveRecord::Base validates_presence_of :creator_id, :antecedent_name, :consequent_name validates_uniqueness_of :antecedent_name, :scope => :consequent_name validate :absence_of_circular_relation + validate :consequent_is_not_aliased module DescendantMethods extend ActiveSupport::Concern @@ -125,6 +126,14 @@ class TagImplication < ActiveRecord::Base end end + def consequent_is_not_aliased + # We don't want to implicate a -> b if b is already aliased to c + if TagAlias.exists?(["antecedent_name = ?", consequent_name]) + self.errors[:base] << "Consequent tag must not be aliased to another tag" + false + end + end + def update_posts Post.without_timeout do Post.raw_tag_match(antecedent_name).find_each do |post| diff --git a/db/migrate/20131006193238_update_aliased_implication_consequents.rb b/db/migrate/20131006193238_update_aliased_implication_consequents.rb new file mode 100644 index 000000000..40bd169f9 --- /dev/null +++ b/db/migrate/20131006193238_update_aliased_implication_consequents.rb @@ -0,0 +1,13 @@ +class UpdateAliasedImplicationConsequents < ActiveRecord::Migration + def change + execute "set statement_timeout = 0" + TagImplication.find_each do |ti| + ta = TagAlias.where("antecedent_name = ? AND status != ?", ti.consequent_name, "pending").first + if ta + ti.consequent_name = ta.consequent_name + ti.save + ta.update_posts + end + end + end +end \ No newline at end of file diff --git a/test/factories/tag_alias.rb b/test/factories/tag_alias.rb index e0b022750..80798877b 100644 --- a/test/factories/tag_alias.rb +++ b/test/factories/tag_alias.rb @@ -2,9 +2,12 @@ FactoryGirl.define do factory :tag_alias do antecedent_name "aaa" consequent_name "bbb" + status "active" after(:create) do |tag_alias| - tag_alias.process! + unless tag_alias.status == "pending" + tag_alias.process! + end end end end diff --git a/test/factories/tag_implication.rb b/test/factories/tag_implication.rb index e328fad8a..6d901ebf6 100644 --- a/test/factories/tag_implication.rb +++ b/test/factories/tag_implication.rb @@ -2,9 +2,12 @@ FactoryGirl.define do factory :tag_implication do antecedent_name "aaa" consequent_name "bbb" + status "active" after(:create) do |tag_implication| - tag_implication.process! + unless tag_implication.status == "pending" + tag_implication.process! + end end end end diff --git a/test/unit/tag_alias_test.rb b/test/unit/tag_alias_test.rb index d32c04185..ec90c2845 100644 --- a/test/unit/tag_alias_test.rb +++ b/test/unit/tag_alias_test.rb @@ -53,15 +53,29 @@ class TagAliasTest < ActiveSupport::TestCase end should "not validate for transitive relations" do - ta1 = FactoryGirl.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb") + ta1 = FactoryGirl.create(:tag_alias, :antecedent_name => "bbb", :consequent_name => "ccc") assert_difference("TagAlias.count", 0) do - ta3 = FactoryGirl.build(:tag_alias, :antecedent_name => "bbb", :consequent_name => "ddd") - ta3.save - assert(ta3.errors.any?, "Tag alias should be invalid") - assert_equal("Tag alias can not create a transitive relation with another tag alias", ta3.errors.full_messages.join) + ta2 = FactoryGirl.build(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb") + ta2.save + assert(ta2.errors.any?, "Tag alias should be invalid") + assert_equal("Tag alias can not create a transitive relation with another tag alias", ta2.errors.full_messages.join) end end + should "move existing aliases" do + ta1 = FactoryGirl.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb", :status => "active") + ta2 = FactoryGirl.create(:tag_alias, :antecedent_name => "bbb", :consequent_name => "ccc", :status => "active") + ta1.reload + assert_equal("ccc", ta1.consequent_name) + end + + should "move existing implications" do + ti = FactoryGirl.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb", :status => "active") + ta = FactoryGirl.create(:tag_alias, :antecedent_name => "bbb", :consequent_name => "ccc", :status => "active") + ti.reload + assert_equal("ccc", ti.consequent_name) + end + should "not push the antecedent's category to the consequent if the antecedent is general" do tag1 = FactoryGirl.create(:tag, :name => "aaa") tag2 = FactoryGirl.create(:tag, :name => "bbb", :category => 1) diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index 4ae1f708e..6f2645166 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -17,7 +17,7 @@ class TagImplicationTest < ActiveSupport::TestCase end should "ignore pending implications when building descendant names" do - ti2 = FactoryGirl.build(:tag_implication, :antecedent_name => "b", :consequent_name => "c") + ti2 = FactoryGirl.build(:tag_implication, :antecedent_name => "b", :consequent_name => "c", :status => "pending") ti2.save ti1 = FactoryGirl.create(:tag_implication, :antecedent_name => "a", :consequent_name => "b") assert_equal("b", ti1.descendant_names) @@ -44,6 +44,14 @@ class TagImplicationTest < ActiveSupport::TestCase assert_equal("Antecedent name has already been taken", ti2.errors.full_messages.join("")) end + should "not validate if its consequent is aliased to another tag" do + ta = FactoryGirl.create(:tag_alias, :antecedent_name => "bbb", :consequent_name => "ccc") + ti = FactoryGirl.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("")) + end + should "calculate all its descendants" do ti1 = FactoryGirl.create(:tag_implication, :antecedent_name => "bbb", :consequent_name => "ccc") assert_equal("ccc", ti1.descendant_names)