diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index 633062f7c..3d68044ec 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -1,23 +1,26 @@ class TagImplication < ActiveRecord::Base attr_accessor :updater_id, :updater_ip_addr + before_save :clear_cache before_save :update_descendant_names after_save :update_descendant_names_for_parent - after_save :clear_cache after_destroy :clear_cache after_save :update_posts belongs_to :creator, :class_name => "User" belongs_to :updater, :class_name => "User" validates_presence_of :updater_id, :updater_ip_addr, :creator_id + validates_uniqueness_of :antecedent_name, :scope => :consequent_name + validate :absence_of_circular_relation def self.with_descendants(names) - names.map do |name| - ti = find_by_antecedent_name(name) - if ti - [name, ti.descendant_names_array] - else - name - end - end.flatten + ([names] + where(["antecedent_name IN (?)", Array(names)]).all.map {|x| x.descendant_names_array}).flatten + end + + def absence_of_circular_relation + # We don't want a -> b && b -> a chains + if self.class.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 + end end def parent @@ -44,15 +47,21 @@ class TagImplication < ActiveRecord::Base def update_descendant_names self.descendant_names = descendants.join(" ") + self.class.logger.debug "#{antecedent_name}> updating descendants to #{descendant_names}" end - def update_descendant_names! + def update_descendant_names!(updater_id, updater_ip_addr) update_descendant_names - save + self.updater_id = updater_id + self.updater_ip_addr = updater_ip_addr + save! end def update_descendant_names_for_parent - parent.update_descendant_names! if parent + if parent + self.class.logger.debug "#{antecedent_name}> updating parent #{parent.antecedent_name}" + parent.update_descendant_names!(updater_id, updater_ip_addr) + end end def clear_cache @@ -62,7 +71,7 @@ class TagImplication < ActiveRecord::Base def update_posts Post.find_by_tags(antecedent_name).find_each do |post| escaped_antecedent_name = Regexp.escape(antecedent_name) - fixed_tags = post.tag_string.sub(/\A#{escaped_antecedent_name} | #{escaped_antecedent_name} | #{escaped_antecedent_name}\Z/, " #{descendant_names} ").strip + fixed_tags = post.tag_string.sub(/\A#{escaped_antecedent_name} | #{escaped_antecedent_name} | #{escaped_antecedent_name}\Z/, " #{antecedent_name} #{descendant_names} ").strip post.update_attributes( :tag_string => fixed_tags, :updater_id => updater_id, diff --git a/test/unit/tag_alias_test.rb b/test/unit/tag_alias_test.rb index f8ecab769..b4c090269 100644 --- a/test/unit/tag_alias_test.rb +++ b/test/unit/tag_alias_test.rb @@ -39,7 +39,8 @@ class TagAliasTest < ActiveSupport::TestCase should "not validate for transitive relations" do ta1 = Factory.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb") assert_difference("TagAlias.count", 0) do - ta3 = TagAlias.create(:antecedent_name => "bbb", :consequent_name => "ddd", :updater_id => ta1.creator_id, :updater_ip_addr => "127.0.0.1") + ta3 = Factory.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) end diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index c606cbb77..4acce8b63 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -6,7 +6,23 @@ class TagImplicationTest < ActiveSupport::TestCase MEMCACHE.flush_all @user = Factory.create(:user) end - + + should "not validate when a circular relation is created" do + ti1 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") + ti2 = Factory.build(:tag_implication, :antecedent_name => "bbb", :consequent_name => "aaa") + ti2.save + assert(ti2.errors.any?, "Tag implication should not have validated.") + assert_equal("Tag implication can not create a circular relation with another tag implication", ti2.errors.full_messages.join("")) + end + + should "not allow for duplicates" do + ti1 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") + ti2 = Factory.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("")) + end + should "clear the cache upon saving" do ti1 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") assert_equal(["bbb"], ti1.descendant_names_array) @@ -19,27 +35,65 @@ class TagImplicationTest < ActiveSupport::TestCase assert_nil(MEMCACHE.get("ti:aaa")) end - # should "clear the cache upon destruction" do - # ti1 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") - # assert_equal("bbb", ti1.descendant_names) - # assert_equal(["bbb"], ti1.descendant_names_array) - # assert_equal(["bbb"], MEMCACHE.get("ti:aaa")) - # ti1.destroy - # assert_nil(MEMCACHE.get("ti:aaa")) - # end - # - # should "calculate all its descendants" do - # ti1 = Factory.create(:tag_implication, :antecedent_name => "bbb", :consequent_name => "ccc") - # assert_equal(["ccc"], ti1.descendant_names_array) - # ti2 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") - # assert_equal(["bbb", "ccc"], ti2.descendant_names_array) - # ti1.reload - # assert_equal(["ccc"], ti1.descendant_names_array) - # end + should "clear the cache upon destruction" do + ti1 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") + ti1.destroy + assert_nil(MEMCACHE.get("ti:aaa")) + end - should "cache its descendants" - should "update its descendants on save" - should "update the decendants for its parent on save" - should "update any affected post upon save" + should "calculate all its descendants" do + ti1 = Factory.create(:tag_implication, :antecedent_name => "bbb", :consequent_name => "ccc") + assert_equal("ccc", ti1.descendant_names) + assert_equal(["ccc"], ti1.descendant_names_array) + ti2 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") + assert_equal("bbb ccc", ti2.descendant_names) + assert_equal(["bbb", "ccc"], ti2.descendant_names_array) + ti1.reload + assert_equal("ccc", ti1.descendant_names) + assert_equal(["ccc"], ti1.descendant_names_array) + end + + should "cache its descendants" do + ti1 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") + assert_equal(["bbb"], ti1.descendant_names_array) + assert_equal(["bbb"], MEMCACHE.get("ti:aaa")) + end + + should "update its descendants on save" do + ti1 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") + ti2 = Factory.create(:tag_implication, :antecedent_name => "ccc", :consequent_name => "ddd") + ti2.update_attributes( + :antecedent_name => "bbb", + :updater_id => @user.id, + :updater_ip_addr => "127.0.0.1" + ) + ti1.reload + ti2.reload + assert_equal("bbb ddd", ti1.descendant_names) + assert_equal("ddd", ti2.descendant_names) + end + + should "update the decendants for its parent on save" do + ti1 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") + ti2 = Factory.create(:tag_implication, :antecedent_name => "bbb", :consequent_name => "ccc") + ti3 = Factory.create(:tag_implication, :antecedent_name => "ccc", :consequent_name => "ddd") + ti4 = Factory.create(:tag_implication, :antecedent_name => "ccc", :consequent_name => "eee") + ti1.reload + ti2.reload + ti3.reload + ti4.reload + assert_equal("bbb ccc eee ddd", ti1.descendant_names) + assert_equal("ccc eee ddd", ti2.descendant_names) + assert_equal("ddd", ti3.descendant_names) + assert_equal("eee", ti4.descendant_names) + end + + should "update any affected post upon save" do + p1 = Factory.create(:post, :tag_string => "aaa bbb ccc") + ti1 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "xxx") + ti2 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "yyy") + p1.reload + assert_equal("aaa yyy xxx bbb ccc", p1.tag_string) + end end end