revamped pool tests

app/logical/post_sets/favorite.rb

@@ -27,7 +27,7 @@ module PostSets
     end
     
     def relation
-      ::Favorite.model_for(user.id).where("user_id = ?", user.id).order("id desc")
+      ::Favorite.model_for(user.id).where("user_id = ?", user.id).includes(:post).order("id desc")
     end
   end  
 end

app/logical/post_sets/pool.rb

@@ -19,7 +19,7 @@ module PostSets
     end
 
     def posts
-      @posts ||= pool.posts(pagination_options).limit(limit).all
+      @posts ||= pool.posts(pagination_options.merge(:limit => limit)).all
     end
     
     def reload

app/logical/post_sets/sequential.rb

@@ -10,7 +10,7 @@ module PostSets
     
     def slice(relation)
       if before_id
-        relation.where("id < ?", before_id).limit(limit).all
+        relation.where("id < ?", before_id).order("id desc").limit(limit).all
       elsif after_id
         relation.where("id > ?", after_id).order("id asc").limit(limit).all.reverse
       else

app/models/pool.rb

@@ -6,26 +6,28 @@ class Pool < ActiveRecord::Base
   has_many :versions, :class_name => "PoolVersion", :dependent => :destroy, :order => "pool_versions.id ASC"
   before_validation :normalize_name
   before_validation :normalize_post_ids
-  before_validation :initialize_post_count
   before_validation :initialize_creator, :on => :create
   after_save :create_version
-  after_save :balance_post_ids
+  attr_accessible :name, :description, :post_ids, :is_active, :post_count
-  attr_accessible :name, :description, :post_ids, :is_active, :post_id_array
   
   def self.name_to_id(name)
     if name =~ /^\d+$/
       name.to_i
     else
-      select_value_sql("SELECT id FROM pools WHERE name = ?", name.downcase)
+      select_value_sql("SELECT id FROM pools WHERE name = ?", name.downcase).to_i
     end
   end
   
+  def self.id_to_name(id)
+    select_value_sql("SELECT name FROM pools WHERE id = ?", id)
+  end
+  
   def self.create_anonymous(creator, creator_ip_addr)
     Pool.new do |pool|
       pool.name = "TEMP:#{Time.now.to_f}.#{rand(1_000_000)}"
       pool.creator = creator
       pool.save
-      pool.name = "anonymous:#{pool.id}"
+      pool.name = "anon:#{pool.id}"
       pool.save
     end
   end
@@ -43,56 +45,58 @@ class Pool < ActiveRecord::Base
   end
   
   def normalize_name
-    self.name = Pool.normalize_name(name)
+    self.name = self.class.normalize_name(name)
   end
   
   def normalize_post_ids
-    self.post_ids = Pool.normalize_post_ids(post_ids)
+    self.post_ids = self.class.normalize_post_ids(post_ids)
   end
   
   def revert_to!(version)
     self.post_ids = version.post_ids
-    save
+    synchronize_posts!
   end
 
-  def contains_post?(post_id)
+  def contains?(post_id)
     post_ids =~ /(?:\A| )#{post_id}(?:\Z| )/
   end
   
-  def add_post!(post)
+  def add!(post)
-    return if contains_post?(post.id)
+    return if contains?(post.id)
     
-    increment!(:post_count)
+    update_attributes(:post_ids => add_number_to_string(post.id, post_ids), :post_count => post_count + 1)
-    update_attribute(:post_ids, "#{post_ids} #{post.id}".strip)
     post.add_pool!(self)
     clear_post_id_array
   end
   
-  def remove_post!(post)
+  def remove!(post)
-    return unless contains_post?(post.id)
+    return unless contains?(post.id)
     
-    decrement!(:post_count)
+    update_attributes(:post_ids => remove_number_from_string(post.id, post_ids), :post_count => post_count - 1)
-    update_attribute(:post_ids, Pool.normalize_post_ids(post_ids.gsub(/(?:\A| )#{post.id}(?:\Z| )/, " ")))
     post.remove_pool!(self)
     clear_post_id_array
   end
   
-  def posts(options = {})
+  def add_number_to_string(number, string)
-    limit = options[:limit] || Danbooru.config.posts_per_page
+    "#{string} #{number}"
+  end
   
-    if options[:offset]
+  def remove_number_from_string(number, string)
-      slice = post_id_array.slice(options[:offset], limit)
+    string.gsub(/(?:\A| )#{number}(?:\Z| )/, " ")
-      if slice && slice.any?
+  end
-        Post.where("id in (?)", slice).order(arbitrary_sql_order_clause(slice, "posts"))
+  
-      else
+  def posts(options = {})
-        Post.where("false")
+    offset = options[:offset] || 0
-      end
+    limit = options[:limit] || Danbooru.config.posts_per_page
+    slice = post_id_array.slice(offset, limit)
+    if slice && slice.any?
+      Post.where("id in (?)", slice).order(arbitrary_sql_order_clause(slice, "posts"))
     else
-      Post.where("id IN (?)", post_id_array).order(arbitrary_sql_order_clause(post_id_array, "posts"))
+      Post.where("false")
     end
   end
   
-  def balance_post_ids
+  def synchronize_posts!
     added = post_id_array - post_id_array_was
     removed = post_id_array_was - post_id_array
     
@@ -105,6 +109,9 @@ class Pool < ActiveRecord::Base
       post = Post.find(post_id)
       post.remove_pool!(self)
     end
+    
+    self.post_count = post_id_array.size
+    save
   end
   
   def post_id_array
@@ -115,21 +122,12 @@ class Pool < ActiveRecord::Base
     @post_id_array_was ||= post_ids_was.scan(/\d+/).map(&:to_i)
   end
   
-  def post_id_array=(array)
-    self.post_ids = array.join(" ")
-    clear_post_id_array
-  end
-  
   def clear_post_id_array
     @post_id_array = nil
     @post_id_array_was = nil
   end
   
-  def initialize_post_count
+  def neighbors(post)
-    self.post_count = post_id_array.size
-  end
-  
-  def neighbor_posts(post)
     @neighbor_posts ||= begin
       post_ids =~ /\A#{post.id} (\d+)|(\d+) #{post.id} (\d+)|(\d+) #{post.id}\Z/
       

app/models/post.rb

@@ -627,13 +627,13 @@ class Post < ActiveRecord::Base
     def add_pool!(pool)
       return if belongs_to_pool?(pool)
       update_attribute(:pool_string, "#{pool_string} pool:#{pool.id}".strip)
-      pool.add_post!(self)
+      pool.add!(self)
     end
     
     def remove_pool!(pool)
       return unless belongs_to_pool?(pool)
       update_attribute(:pool_string, pool_string.gsub(/(?:\A| )pool:#{pool.id}(?:\Z| )/, " ").strip)
-      pool.remove_post!(self)
+      pool.remove!(self)
     end
   end
   

config/routes.rb

@@ -55,6 +55,7 @@ Danbooru::Application.routes.draw do
     member do
       put :revert
     end
+    resource :order, :only => [:edit, :update]
   end
   resources :pool_versions, :only => [:index]
   resources :posts do

test/unit/pool_test.rb

@@ -13,125 +13,245 @@ class PoolTest < ActiveSupport::TestCase
     CurrentUser.ip_addr = nil
   end
   
-  context "A pool" do
+  context "A name" do
-    should "create versions for each distinct user" do
+    setup do
-      pool = Factory.create(:pool)
+      @pool = Factory.create(:pool)
-      user = Factory.create(:user)
+    end
-      assert_equal(1, pool.versions(true).size)
+    
-      pool.post_ids = "1"
+    should "be mapped to a pool id" do
+      assert_equal(@pool.id, Pool.name_to_id(@pool.name))
+    end
+  end
+  
+  context "An id number" do
+    setup do
+      @pool = Factory.create(:pool)
+    end
+    
+    should "be mapped to a pool id" do
+      assert_equal(@pool.id, Pool.name_to_id(@pool.id.to_s))
+    end
+    
+    should "be mapped to its name" do
+      assert_equal(@pool.name, Pool.id_to_name(@pool.id))
+    end
+  end
+  
+  context "Reverting a pool" do
+    setup do
+      @pool = Factory.create(:pool)
+      @p1 = Factory.create(:post)
+      @p2 = Factory.create(:post)
+      @p3 = Factory.create(:post)
       CurrentUser.ip_addr = "1.2.3.4"
-      pool.save
+      @pool.add!(@p1)
-      assert_equal(2, pool.versions(true).size)
+      CurrentUser.ip_addr = "1.2.3.5"
-      pool.post_ids = "1 2"
+      @pool.add!(@p2)
-      pool.save
+      CurrentUser.ip_addr = "1.2.3.6"
-      assert_equal(2, pool.versions(true).size)
+      @pool.add!(@p3)
-      pool.revert_to!(PoolVersion.first)
+      CurrentUser.ip_addr = "1.2.3.7"
-      assert_equal("", pool.post_ids)
+      @pool.remove!(@p1)
+      CurrentUser.ip_addr = "1.2.3.8"
+      @pool.revert_to!(@pool.versions.all[1])
     end
     
-    should "have posts" do
+    should "have the correct versions" do
-      pool = Factory.create(:pool)
+      assert_equal(6, @pool.versions.size)
-      p1 = Factory.create(:post)
+      assert_equal("", @pool.versions.all[0].post_ids)
-      p2 = Factory.create(:post)
+      assert_equal("#{@p1.id}", @pool.versions.all[1].post_ids)
-      p3 = Factory.create(:post)
+      assert_equal("#{@p1.id} #{@p2.id}", @pool.versions.all[2].post_ids)
-      p4 = Factory.create(:post)
+      assert_equal("#{@p1.id} #{@p2.id} #{@p3.id}", @pool.versions.all[3].post_ids)
-      pool.add_post!(p1)
+      assert_equal("#{@p2.id} #{@p3.id}", @pool.versions.all[4].post_ids)
-      pool.add_post!(p2)
-      pool.add_post!(p3)
-      pool.reload
-      
-      assert_equal("#{p1.id} #{p2.id} #{p3.id}", pool.post_ids)
-      assert_equal([p1.id, p2.id, p3.id], pool.post_id_array)
-      posts = pool.posts.all
-      assert_equal(3, posts.size)
-      assert_equal([p1.id, p2.id, p3.id], posts.map(&:id))
-      posts = pool.posts.limit(1).offset(1).all
-      assert_equal(1, posts.size)
-      assert_equal([p2.id], posts.map(&:id))
     end
     
-    should "return the neighboring posts for any member element" do
+    should "update its post_ids" do
-      pool = Factory.create(:pool)
+      assert_equal("#{@p1.id}", @pool.post_ids)
-      p1 = Factory.create(:post)
-      p2 = Factory.create(:post)
-      p3 = Factory.create(:post)
-      pool.add_post!(p1)
-      pool.add_post!(p2)
-      pool.add_post!(p3)
-
-      pool.reload
-      neighbors = pool.neighbor_posts(p1)
-      assert_nil(neighbors.previous)
-      assert_equal(p2.id, neighbors.next)
-
-      pool.reload
-      neighbors = pool.neighbor_posts(p2)
-      assert_equal(p1.id, neighbors.previous)
-      assert_equal(p3.id, neighbors.next)
-
-      pool.reload
-      neighbors = pool.neighbor_posts(p3)
-      assert_equal(p2.id, neighbors.previous)
-      assert_nil(neighbors.next)
     end
     
-    should "know what its post_ids were" do
+    should "update any old posts that were removed" do
-      p1 = Factory.create(:post)
+      @p2.reload
-      p2 = Factory.create(:post)
+      assert_equal("", @p2.pool_string)
-      pool = Factory.create(:pool, :post_ids => "#{p1.id}")
-      pool.post_id_array = [p1.id, p2.id]
-      assert_equal([p1.id], pool.post_id_array_was)
     end
     
-    should "update its posts if the post_ids is updated directly" do
+    should "update any new posts that were added" do
-      p1 = Factory.create(:post)
+      @p1.reload
-      p2 = Factory.create(:post)
+      assert_equal("pool:#{@pool.id}", @p1.pool_string)
-      pool = Factory.create(:pool, :post_ids => "#{p1.id}")
+    end
-      pool.post_id_array = [p1.id, p2.id]
+  end
-      pool.save
+  
-      p1.reload
+  context "Updating a pool" do
-      p2.reload
+    setup do
-      assert_equal("pool:#{pool.id}", p1.pool_string)
+      @pool = Factory.create(:pool)
-      assert_equal("pool:#{pool.id}", p2.pool_string)
+      @p1 = Factory.create(:post)
+      @p2 = Factory.create(:post)
     end
     
-    should "set its post count even if post_ids is updated directly" do
+    context "by adding a new post" do
-      p1 = Factory.create(:post)
+      setup do
-      p2 = Factory.create(:post)
+        @pool.add!(@p1)
-      pool = Factory.create(:pool, :post_ids => "#{p1.id}")
+      end
-      pool.post_id_array = [p1.id, p2.id]
+      
-      pool.save
+      should "add the post to the pool" do
-      assert_equal(2, pool.post_count)
+        assert_equal("#{@p1.id}", @pool.post_ids)
+      end
+      
+      should "add the pool to the post" do
+        assert_equal("pool:#{@pool.id}", @p1.pool_string)
+      end
+      
+      should "increment the post count" do
+        assert_equal(1, @pool.post_count)
+      end
+      
+      context "to a pool that already has the post" do
+        setup do
+          @pool.add!(@p1)
+        end
+        
+        should "not double add the post to the pool" do
+          assert_equal("#{@p1.id}", @pool.post_ids)
+        end
+
+        should "not double add the pool to the post" do
+          assert_equal("pool:#{@pool.id}", @p1.pool_string)
+        end
+
+        should "not double increment the post count" do
+          assert_equal(1, @pool.post_count)
+        end
+      end
     end
     
-    should "increment the post count every time a post is added" do
+    context "by removing a post" do
-      p1 = Factory.create(:post)
+      setup do
-      pool = Factory.create(:pool)
+        @pool.add!(@p1)
-      pool.add_post!(p1)
+      end
-      assert_equal(1, pool.post_count)
+      
+      context "that is in the pool" do
+        setup do
+          @pool.remove!(@p1)
+        end
+
+        should "remove the post from the pool" do
+          assert_equal("", @pool.post_ids)
+        end
+
+        should "remove the pool from the post" do
+          assert_equal("", @p1.pool_string)
+        end
+
+        should "update the post count" do
+          assert_equal(0, @pool.post_count)
+        end
+      end
+      
+      context "that is not in the pool" do
+        setup do
+          @pool.remove!(@p2)
+        end
+        
+        should "not affect the pool" do
+          assert_equal("#{@p1.id}", @pool.post_ids)
+        end
+        
+        should "not affect the post" do
+          assert_equal("pool:#{@pool.id}", @p1.pool_string)
+        end
+        
+        should "not affect the post count" do
+          assert_equal(1, @pool.post_count)
+        end
+      end
     end
     
-    should "not double increment when the same post is readded" do
+    should "create new versions for each distinct user" do
-      p1 = Factory.create(:post)
+      assert_equal(1, @pool.versions(true).size)
-      pool = Factory.create(:pool)
+      @pool.post_ids = "#{@p1.id}"
-      pool.add_post!(p1)
+      CurrentUser.ip_addr = "1.2.3.4"
-      pool.add_post!(p1)
+      @pool.save
-      assert_equal(1, pool.post_count)
+      assert_equal(2, @pool.versions(true).size)
+      @pool.post_ids = "#{@p1.id} #{@p2.id}"
+      @pool.save
+      assert_equal(2, @pool.versions(true).size)
     end
     
-    should "not double decrement" do
+    should "know what its post ids were previously" do
-      p1 = Factory.create(:post)
+      @pool.post_ids = "#{@p1.id}"
-      pool = Factory.create(:pool)
+      assert_equal("", @pool.post_ids_was)
-      pool.remove_post!(p1)
+      assert_equal([], @pool.post_id_array_was)
-      assert_equal(0, pool.post_count)
+    end
+    
+    should "normalize its name" do
+      @pool.update_attributes(:name => "A B")
+      assert_equal("a_b", @pool.name)
+    end
+    
+    should "normalize its post ids" do
+      @pool.update_attributes(:post_ids => " 1  2 ")
+      assert_equal("1 2", @pool.post_ids)
+    end
+  end
+  
+  context "An existing pool" do
+    setup do
+      @pool = Factory.create(:pool)
+      @p1 = Factory.create(:post)
+      @p2 = Factory.create(:post)
+      @p3 = Factory.create(:post)
+      @pool.add!(@p1)
+      @pool.add!(@p2)
+      @pool.add!(@p3)
+      @p1_neighbors = @pool.neighbors(@p1)
+      @pool.reload # clear cached neighbors
+      @p2_neighbors = @pool.neighbors(@p2)
+      @pool.reload # clear cached neighbors
+      @p3_neighbors = @pool.neighbors(@p3)
+    end
+    
+    context "that is synchronized" do
+      setup do
+        @pool.reload
+        @pool.post_ids = "#{@p2.id}"
+        @pool.synchronize_posts!
+      end
+      
+      should "update the pool" do
+        @pool.reload
+        assert_equal(1, @pool.post_count)
+        assert_equal("#{@p2.id}", @pool.post_ids)
+      end
+      
+      should "update the posts" do
+        @p1.reload
+        @p2.reload
+        @p3.reload
+        assert_equal("", @p1.pool_string)
+        assert_equal("pool:#{@pool.id}", @p2.pool_string)
+        assert_equal("", @p3.pool_string)
+      end
+    end
+    
+    should "find the neighbors for the first post" do
+      assert_nil(@p1_neighbors.previous)
+      assert_equal(@p2.id, @p1_neighbors.next)
+    end
+    
+    should "find the neighbors for the middle post" do
+      assert_equal(@p1.id, @p2_neighbors.previous)
+      assert_equal(@p3.id, @p2_neighbors.next)
+    end
+    
+    should "find the neighbors for the last post" do
+      assert_equal(@p2.id, @p3_neighbors.previous)
+      assert_nil(@p3_neighbors.next)
     end
   end
 
   context "An anonymous pool" do
-    should "have a name starting with anonymous" do
+    should "have a name starting with anon" do
       user = Factory.create(:user)
       pool = Pool.create_anonymous(user, "127.0.0.1")
-      assert_match(/^anonymous:\d+$/, pool.name)
+      assert_match(/^anon:\d+$/, pool.name)
     end
   end
 end