Fixed some major bugs with implications, cleaning up the cache

expiration process and adding some additional tests.

Cleaned up the unit tests.  They should all run cleanly with rake
test:units now.

Gemfile

@@ -15,5 +15,7 @@ gem "delayed_job"
 gem "super_exception_notifier"
 gem "haml"
 gem "simple_form"
+gem "mechanize"
+gem "nokogiri"
 
 gem "will_paginate", :git => "http://github.com/mislav/will_paginate.git", :branch => "rails3"

Gemfile.lock

@@ -58,10 +58,13 @@ GEM
       i18n (~> 0.4.1)
       mime-types (~> 1.16)
       treetop (~> 1.4.8)
+    mechanize (1.0.0)
+      nokogiri (>= 1.2.1)
     memcache-client (1.8.5)
     mime-types (1.16)
     mocha (0.9.9)
       rake
+    nokogiri (1.4.3.1)
     pg (0.9.0)
     polyglot (0.3.1)
     rack (1.2.1)
@@ -102,8 +105,10 @@ DEPENDENCIES
   ffaker!
   haml
   imagesize
+  mechanize
   memcache-client
   mocha
+  nokogiri
   pg
   rails (= 3.0.0)
   shoulda

app/logical/pixiv_proxy.rb

@@ -23,7 +23,7 @@ class PixivProxy
 		mech = create_mechanize
 		hash = {}
 		mech.get(url) do |page|
-			hash[:artist] = page.search("div#profile/div/a/img").attr("alt")
+			hash[:artist] = page.search("a.avatar_m").attr("title").value
 			hash[:listing_url] = "/member_illust.php?id=" + url[/id=(\d+)/, 1]
 		end
 		hash
@@ -34,13 +34,13 @@ class PixivProxy
 		mech = create_mechanize
 		hash = {}
 		mech.get(url) do |page|
-		  if page.search("div#profile/div/a/img")
+		  if page.search("a.avatar_m")
-  			hash[:artist] = page.search("div#profile/div/a/img").attr("alt")
+  			hash[:artist] = page.search("a.avatar_m").attr("title").value
-  			hash[:image_url] = page.search("div#profile/div/a/img").attr("src").sub("_m.", ".")
+  			hash[:image_url] = page.search("div.works_display/a/img").attr("src").value.sub("_m.", ".")
-  			hash[:profile_url] = page.search("div#profile/div/a").attr("href")
+  			hash[:profile_url] = page.search("a.avatar_m").attr("href").value
-  			hash[:jp_tags] = page.search("div#tag_area/span#tags/a").map do |node|
+  			hash[:jp_tags] = page.search("span#tags/a").map do |node|
   				[node.inner_text, node.attribute("href").to_s]
-  			end
+  			end.reject {|x| x[0].empty?}
 			else
 			  hash[:artist] = "?"
   			hash[:image_url] = "?"
@@ -80,10 +80,10 @@ class PixivProxy
 	end
 
 	def self.create_mechanize
-		mech = WWW::Mechanize.new
+		mech = Mechanize.new
 		
 		mech.get("http://www.pixiv.net") do |page|
-			page.form_with(:action => "login.php") do |form|
+			page.form_with(:action => "/login.php") do |form|
 				form.pixiv_id = "uroobnad"
 				form.pass = "uroobnad556"
 			end.click_button

app/models/tag_alias.rb

@@ -1,9 +1,13 @@
 class TagAlias < ActiveRecord::Base
   attr_accessor :creator_ip_addr
   after_save :update_posts
-  after_commit :clear_cache
+  after_save :clear_cache
-  after_commit :clear_remote_cache
+  after_save :clear_remote_cache
-  validates_presence_of :creator_id, :creator_ip_addr
+  after_save :update_cache
+  after_destroy :clear_cache
+  after_destroy :clear_remote_cache
+  before_validation :initialize_creator, :on => :create
+  validates_presence_of :creator_id
   validates_uniqueness_of :antecedent_name
   validate :absence_of_transitive_relation
   belongs_to :creator, :class_name => "User"
@@ -21,6 +25,10 @@ class TagAlias < ActiveRecord::Base
     alias_hash.values.flatten.uniq
   end
   
+  def initialize_creator
+    self.creator_id = CurrentUser.user.id
+  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])
@@ -48,11 +56,9 @@ class TagAlias < ActiveRecord::Base
       escaped_antecedent_name = Regexp.escape(antecedent_name)
       fixed_tags = post.tag_string.sub(/(?:\A| )#{escaped_antecedent_name}(?:\Z| )/, " #{consequent_name} ").strip
       
-      CurrentUser.scoped(creator, creator_ip_addr) do
+      post.update_attributes(
-        post.update_attributes(
+        :tag_string => fixed_tags
-          :tag_string => fixed_tags
+      )
-        )
-      end
     end
   end
 end

app/models/tag_implication.rb

@@ -1,19 +1,104 @@
 class TagImplication < ActiveRecord::Base
-  attr_accessor :creator_ip_addr
   before_save :clear_cache
   before_save :update_descendant_names
   after_save :update_descendant_names_for_parent
-  after_destroy :clear_cache
+  after_save :update_cache
   after_save :update_posts
+  after_destroy :clear_cache
+  after_destroy :clear_remote_cache
   belongs_to :creator, :class_name => "User"
-  validates_presence_of :creator_id, :creator_ip_addr
+  before_validation :initialize_creator, :on => :create
+  validates_presence_of :creator_id
   validates_uniqueness_of :antecedent_name, :scope => :consequent_name
   validate :absence_of_circular_relation
   
-  def self.with_descendants(names)
+  module CacheMethods
-    Cache.get_multi(names.flatten, "ti") do |name|
+    def clear_cache
-      ([name] + where(["antecedent_name = ?", name]).all.map {|x| x.descendant_names_array}).flatten
+      Cache.delete("ti:#{Cache.sanitize(antecedent_name)}")
-    end.values.flatten.uniq
+      @descendants = nil
+    end
+
+    def clear_remote_cache
+      Danbooru.config.other_server_hosts.each do |server|
+        Net::HTTP.delete(URI.parse("http://#{server}/tag_implications/#{id}/cache"))
+      end
+    end
+
+    def update_cache
+      descendant_names_array
+      true
+    end
+  end
+
+  module DescendantMethods
+    extend ActiveSupport::Concern
+    
+    module ClassMethods
+      def with_descendants(names)
+        names + Cache.get_multi(names.flatten, "ti") do |name|
+          ([name] + where(["antecedent_name = ?", name]).all.map {|x| x.descendant_names_array}).flatten
+        end.values.flatten.uniq
+      end
+    end
+    
+    def descendants
+      @descendants ||= begin
+        [].tap do |all|
+          children = [consequent_name]
+
+          until children.empty?
+            all.concat(children)
+            children = self.class.where(["antecedent_name IN (?)", children]).all.map(&:consequent_name)
+          end
+        end
+      end
+    end
+
+    def descendant_names_array
+      Cache.get("ti:#{Cache.sanitize(antecedent_name)}") do
+        descendant_names.split(/ /)
+      end
+    end
+
+    def update_descendant_names
+      self.descendant_names = descendants.join(" ")
+    end
+
+    def update_descendant_names!
+      update_descendant_names
+      save!
+    end
+
+    def update_descendant_names_for_parent
+      p = parent
+      
+      while p
+        p.update_descendant_names!
+        p = p.parent
+      end
+    end
+
+    def clear_descendants_cache
+      @descendants = nil
+    end
+  end
+  
+  module ParentMethods
+    def parent
+      @parent ||= self.class.where(["consequent_name = ?", antecedent_name]).first
+    end
+    
+    def clear_parent_cache
+      @parent = nil
+    end
+  end
+  
+  include CacheMethods
+  include DescendantMethods
+  include ParentMethods
+  
+  def initialize_creator
+    self.creator_id = CurrentUser.user.id
   end
   
   def absence_of_circular_relation
@@ -24,57 +109,13 @@ class TagImplication < ActiveRecord::Base
     end
   end
   
-  def parent
-    @parent ||= self.class.where(["consequent_name = ?", antecedent_name]).first
-  end
-  
-  def descendants
-    @descendants ||= begin
-      [].tap do |all|
-        children = [consequent_name]
-    
-        until children.empty?
-          all += children
-          children = self.class.where(["antecedent_name IN (?)", children]).all.map(&:consequent_name)
-        end
-      end
-    end
-  end
-  
-  def descendant_names_array
-    Cache.get("ti:#{Cache.sanitize(antecedent_name)}") do
-      descendant_names.split(/ /)
-    end
-  end
-  
-  def update_descendant_names
-    self.descendant_names = descendants.join(" ")
-  end
-  
-  def update_descendant_names!
-    update_descendant_names
-    save!
-  end
-  
-  def update_descendant_names_for_parent
-    if parent
-      parent.update_descendant_names!
-    end
-  end
-
-  def clear_cache
-    Cache.delete("ti:#{Cache.sanitize(antecedent_name)}")
-  end
-  
   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}(?:\Z| )/, " #{antecedent_name} #{descendant_names} ").strip
-      CurrentUser.scoped(creator, creator_ip_addr) do
+      post.update_attributes(
-        post.update_attributes(
+        :tag_string => fixed_tags
-          :tag_string => fixed_tags
+      )
-        )
-      end
     end
   end
   
@@ -83,12 +124,4 @@ class TagImplication < ActiveRecord::Base
     clear_parent_cache
     clear_descendants_cache
   end
-  
-  def clear_descendants_cache
-    @descendants = nil
-  end
-  
-  def clear_parent_cache
-    @parent = nil
-  end
 end

app/models/upload.rb

@@ -10,6 +10,7 @@ class Upload < ActiveRecord::Base
   before_validation :initialize_uploader, :on => :create
   before_validation :initialize_status, :on => :create
   before_create :convert_cgi_file
+  after_destroy :delete_temp_file
   validate :uploader_is_not_limited
   
   module ValidationMethods
@@ -70,6 +71,8 @@ class Upload < ActiveRecord::Base
       end
     rescue Exception => x
       update_attribute(:status, "error: #{x} - #{x.message}")
+    ensure
+      delete_temp_file
     end
 
     def convert_to_post
@@ -100,6 +103,10 @@ class Upload < ActiveRecord::Base
   end
   
   module FileMethods
+    def delete_temp_file
+      FileUtils.rm_f(temp_file_path)
+    end
+    
     def move_file
       FileUtils.mv(file_path, md5_file_path)
     end
@@ -223,7 +230,7 @@ class Upload < ActiveRecord::Base
     end
     
     def temp_file_path
-      File.join(Rails.root, "tmp", "#{Time.now.to_f}.#{$PROCESS_ID}")
+      @temp_file_path ||= File.join(Rails.root, "tmp", "#{Time.now.to_f}.#{$PROCESS_ID}")
     end
   end
   

app/models/wiki_page.rb

@@ -29,14 +29,14 @@ class WikiPage < ActiveRecord::Base
     titled(title).select("title, id").first
   end
   
-  def revert_to(version, reverter_id, reverter_ip_addr)
+  def revert_to(version)
     self.title = version.title
     self.body = version.body
     self.is_locked = version.is_locked
   end
   
-  def revert_to!(version, reverter_id, reverter_ip_addr)
+  def revert_to!(version)
-    revert_to(version, reverter_id, reverter_ip_addr)
+    revert_to(version)
     save!
   end
 

test/factories/tag_alias.rb

@@ -1,4 +1,2 @@
 Factory.define(:tag_alias) do |f|
-  f.creator {|x| x.association(:user)}
-  f.creator_ip_addr "127.0.0.1"
 end

test/factories/tag_implication.rb

@@ -1,4 +1,2 @@
 Factory.define(:tag_implication) do |f|
-  f.creator {|x| x.association(:user)}
-  f.creator_ip_addr "127.0.0.1"
 end

test/factories/wiki_page.rb

@@ -2,6 +2,4 @@ Factory.define(:wiki_page) do |f|
   f.creator {|x| x.association(:user)}
   f.title {|x| Faker::Lorem.words}
   f.body {Faker::Lorem.sentences}
-  f.updater_id {|x| x.creator_id}
-  f.updater_ip_addr "127.0.0.1"
 end

test/unit/artist_test.rb

@@ -48,7 +48,7 @@ class ArtistTest < ActiveSupport::TestCase
     should "parse urls" do
       artist = Factory.create(:artist, :name => "rembrandt", :url_string => "http://rembrandt.com/test.jpg http://aaa.com")
       artist.reload
-      assert_equal(["http://aaa.com", "http://rembrandt.com/test.jpg"], artist.artist_urls.map(&:to_s).sort)
+      assert_equal(["http://aaa.com", "http://rembrandt.com/test.jpg"], artist.urls.map(&:to_s).sort)
     end
     
     should "make sure old urls are deleted" do
@@ -56,7 +56,7 @@ class ArtistTest < ActiveSupport::TestCase
       artist.url_string = "http://not.rembrandt.com/test.jpg"
       artist.save
       artist.reload
-      assert_equal(["http://not.rembrandt.com/test.jpg"], artist.artist_urls.map(&:to_s).sort)
+      assert_equal(["http://not.rembrandt.com/test.jpg"], artist.urls.map(&:to_s).sort)
     end
 
     should "find matches by url" do

test/unit/post_test.rb

@@ -521,11 +521,15 @@ class PostTest < ActiveSupport::TestCase
     end
   
     should "return posts for the <uploader> metatag" do
-      user = Factory.create(:user)
+      second_user = Factory.create(:user)
-      post1 = Factory.create(:post, :uploader => user)
+      post1 = Factory.create(:post)
-      post2 = Factory.create(:post)
+      
-      post3 = Factory.create(:post)
+      CurrentUser.scoped(second_user, "127.0.0.2") do
-      relation = Post.find_by_tags("uploader:#{user.name}")
+        post2 = Factory.create(:post)
+        post3 = Factory.create(:post)
+      end
+      
+      relation = Post.find_by_tags("uploader:#{CurrentUser.user.name}")
       assert_equal(1, relation.count)
       assert_equal(post1.id, relation.first.id)
     end

test/unit/tag_alias_test.rb

@@ -14,6 +14,11 @@ class TagAliasTest < ActiveSupport::TestCase
       CurrentUser.ip_addr = nil
     end
         
+    should "populate the creator information" do
+      ta = Factory.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb")
+      assert_equal(CurrentUser.user.id, ta.creator_id)
+    end
+        
     should "convert a tag to its normalized version" do
       tag1 = Factory.create(:tag, :name => "aaa")
       tag2 = Factory.create(:tag, :name => "bbb")
@@ -53,5 +58,17 @@ class TagAliasTest < ActiveSupport::TestCase
         assert_equal("Tag alias can not create a transitive relation with another tag alias", ta3.errors.full_messages.join)
       end
     end
+    
+    should "record the alias's creator in the tag history" do
+      user = Factory.create(:user)
+      p1 = nil
+      CurrentUser.scoped(user, "127.0.0.1") do
+        p1 = Factory.create(:post, :tag_string => "aaa bbb ccc")
+      end
+      ta1 = Factory.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "xxx")
+      p1.reload
+      assert_not_equal("uploader:#{ta1.creator_id}", p1.uploader_string)
+      assert_equal(ta1.creator_id, p1.versions.last.updater_id)
+    end
   end
 end

test/unit/tag_implication_test.rb

@@ -15,6 +15,11 @@ class TagImplicationTest < ActiveSupport::TestCase
       CurrentUser.ip_addr = nil
     end
     
+    should "populate the creator information" do
+      ti = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb")
+      assert_equal(CurrentUser.user.id, ti.creator_id)
+    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")
@@ -38,7 +43,7 @@ class TagImplicationTest < ActiveSupport::TestCase
       ti1.update_attributes(
         :consequent_name => "ccc"
       )
-      assert_nil(MEMCACHE.get("ti:aaa"))
+      assert_equal(["ccc"], MEMCACHE.get("ti:aaa"))
     end
     
     should "clear the cache upon destruction" do
@@ -99,5 +104,17 @@ class TagImplicationTest < ActiveSupport::TestCase
       p1.reload
       assert_equal("aaa yyy xxx bbb ccc", p1.tag_string)
     end
+    
+    should "record the implication's creator in the tag history" do
+      user = Factory.create(:user)
+      p1 = nil
+      CurrentUser.scoped(user, "127.0.0.1") do
+        p1 = Factory.create(:post, :tag_string => "aaa bbb ccc")
+      end
+      ti1 = Factory.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "xxx")
+      p1.reload
+      assert_not_equal("uploader:#{ti1.creator_id}", p1.uploader_string)
+      assert_equal(ti1.creator_id, p1.versions.last.updater_id)
+    end
   end
 end

test/unit/upload_test.rb

@@ -11,6 +11,8 @@ class UploadTest < ActiveSupport::TestCase
   teardown do
     CurrentUser.user = nil
     CurrentUser.ip_addr = nil
+    
+    @upload.delete_temp_file if @upload
   end
   
   context "An upload" do    
@@ -165,4 +167,15 @@ class UploadTest < ActiveSupport::TestCase
     assert_equal(post.id, @upload.post_id)
     assert_equal("completed", @upload.status)    
   end
+  
+  should "delete the temporary file upon completion" do
+    @upload = Factory.create(:source_upload,
+      :rating => "s",
+      :uploader_ip_addr => "127.0.0.1",
+      :tag_string => "hoge foo"
+    )
+    
+    @upload.process!
+    assert(!File.exists?(@upload.temp_file_path))
+  end
 end

test/unit/wiki_page_test.rb

@@ -28,8 +28,6 @@ class WikiPageTest < ActiveSupport::TestCase
     
     should "create versions" do
       wp = nil
-      user = Factory.create(:user)
-      reverter = Factory.create(:user)
       
       assert_difference("WikiPageVersion.count") do
         wp = Factory.create(:wiki_page, :title => "xxx")
@@ -37,15 +35,29 @@ class WikiPageTest < ActiveSupport::TestCase
 
       assert_difference("WikiPageVersion.count") do
         wp.title = "yyy"
-        wp.updater_id = user.id
-        wp.updater_ip_addr = "127.0.0.1"
         wp.save
       end
+    end
     
+    should "revert to a prior version" do
+      wp = Factory.create(:wiki_page, :title => "xxx")
+      wp.title = "yyy"
+      wp.save
       version = WikiPageVersion.first
-      wp.revert_to!(version, reverter.id, "127.0.0.1")
+      wp.revert_to!(version)
-      
+      wp.reload
       assert_equal("xxx", wp.title)
     end
+    
+    should "differentiate between updater and creator" do
+      user = Factory.create(:user)
+      wp = Factory.create(:wiki_page, :title => "xxx")
+      CurrentUser.scoped(user, "127.0.0.1") do
+        wp.title = "yyy"
+        wp.save
+      end
+      version = WikiPageVersion.first
+      assert_not_equal(wp.creator_id, version.updater_id)      
+    end
   end
 end