* meta_search now pulls directly from GitHub

* Updated gems
* [inprogress] New pagination helpers used instead of pagination presenters
* [inprogress] Favorites refactored to use ActiveRecord
* [inprogress] PostSets refactored to use a decorator/dependency injection pattern
* [inprogress] Made pool/post interaction more robust
* Pool#posts now returns an ActiveRelation object
* Fixed unit tests

Gemfile

@@ -18,6 +18,6 @@ gem "haml"
 gem "simple_form"
 gem "mechanize"
 gem "nokogiri"
-gem "meta_search"
+gem "meta_search", :git => "git://github.com/ernie/meta_search.git"
 gem "will_paginate", :git => "git://github.com/mmack/will_paginate.git", :branch => "rails3.1"
 gem "silent-postgres"

Gemfile.lock

@@ -1,3 +1,12 @@
+GIT
+  remote: git://github.com/ernie/meta_search.git
+  revision: 79806e6a9db5dc4eedbd45bccc5929cf6fbf3593
+  specs:
+    meta_search (1.1.0.pre)
+      actionpack (~> 3.1.0.alpha)
+      activerecord (~> 3.1.0.alpha)
+      activesupport (~> 3.1.0.alpha)
+
 GIT
   remote: git://github.com/mmack/will_paginate.git
   revision: 5816b4e8d4382b4b167621a9aea4b8fe72983c37
@@ -7,9 +16,9 @@ GIT
 
 GIT
   remote: http://github.com/EmmanuelOga/ffaker.git
-  revision: baf4891a5351ad01775f3e4bb77c78ceed349360
+  revision: 52feff4ecddbe8834b63beb122b153b65833e309
   specs:
-    ffaker (1.7.0)
+    ffaker (1.8.0)
 
 GEM
   remote: http://gemcutter.org/
@@ -64,11 +73,6 @@ GEM
     mechanize (1.0.0)
       nokogiri (>= 1.2.1)
     memcache-client (1.8.5)
-    meta_search (0.5.4)
-      actionpack (>= 3.0.0.beta4)
-      activerecord (>= 3.0.0.beta4)
-      activesupport (>= 3.0.0.beta4)
-      arel (>= 0.4.0)
     mime-types (1.16)
     mocha (0.9.12)
     multi_json (1.0.3)
@@ -98,14 +102,14 @@ GEM
       rack-ssl (~> 1.3.2)
       rake (>= 0.8.7)
       thor (~> 0.14.6)
-    rake (0.9.0)
+    rake (0.9.2)
     shoulda (2.11.3)
     silent-postgres (0.0.8)
     simple_form (1.4.0)
     simplecov (0.4.2)
       simplecov-html (~> 0.4.4)
     simplecov-html (0.4.5)
-    sprockets (2.0.0.beta.8)
+    sprockets (2.0.0.beta.10)
       hike (~> 1.0)
       rack (~> 1.0)
       tilt (!= 1.3.0, ~> 1.1)
@@ -113,7 +117,7 @@ GEM
       actionmailer
       rake
     thor (0.14.6)
-    tilt (1.3.1)
+    tilt (1.3.2)
     treetop (1.4.9)
       polyglot (>= 0.3.1)
     tzinfo (0.3.27)
@@ -129,7 +133,7 @@ DEPENDENCIES
   imagesize
   mechanize
   memcache-client
-  meta_search
+  meta_search!
   mocha
   nokogiri
   pg

app/models/favorite.rb

@@ -3,7 +3,7 @@ class Favorite < ActiveRecord::Base
   validates_uniqueness_of :post_id, :scope => :user_id
   
   def self.model_for(user_id)
-    mod = user_id % TABLE_COUNT
+    mod = user_id.to_i % TABLE_COUNT
     Object.const_get("Favorite#{mod}")
   end
   

app/models/forum_post.rb

@@ -38,8 +38,10 @@ class ForumPost < ActiveRecord::Base
   end
   
   def update_topic_updated_at
-    topic.update_attribute(:updater_id, CurrentUser.id)
+    if topic
-    topic.touch
+      topic.update_attribute(:updater_id, CurrentUser.id)
+      topic.touch
+    end
   end
   
   def initialize_creator

app/models/pool.rb

@@ -6,9 +6,10 @@ 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 :update_posts
+  after_save :balance_post_ids
   attr_accessible :name, :description, :post_ids, :is_active, :post_id_array
   
   def self.name_to_id(name)
@@ -29,58 +30,90 @@ class Pool < ActiveRecord::Base
     end
   end
   
+  def self.normalize_name(name)
+    name.downcase.gsub(/\s+/, "_")
+  end
+  
+  def self.normalize_post_ids(post_ids)
+    post_ids.gsub(/\s{2,}/, " ").strip
+  end
+  
   def initialize_creator
     self.creator_id = CurrentUser.id
   end
   
   def normalize_name
-    self.name = name.downcase
+    self.name = Pool.normalize_name(name)
   end
   
   def normalize_post_ids
-    self.post_ids = post_ids.gsub(/\s\s+/, " ")
+    self.post_ids = Pool.normalize_post_ids(post_ids)
-    self.post_ids = post_ids.gsub(/^\s+/, "")
-    self.post_ids = post_ids.gsub(/\s+$/, "")
   end
   
   def revert_to!(version)
     self.post_ids = version.post_ids
     save
   end
-  
+
-  def update_posts
+  def contains_post?(post_id)
-    post_id_array.each do |post_id|
+    post_ids =~ /(?:\A| )#{post_id}(?:\Z| )/
-      post = Post.find(post_id)
-      post.add_pool(self)
-    end
   end
   
   def add_post!(post)
-    return if post_ids =~ /(?:\A| )#{post.id}(?:\Z| )/
+    return if contains_post?(post.id)
-    self.post_ids += " #{post.id}"
+    
-    self.post_ids = post_ids.strip
+    increment!(:post_count)
+    update_attribute(:post_ids, "#{post_ids} #{post.id}".strip)
+    post.add_pool!(self)
     clear_post_id_array
-    save
   end
   
   def remove_post!(post)
-    self.post_ids = post_ids.gsub(/(?:\A| )#{post.id}(?:\Z| )/, " ")
+    return unless contains_post?(post.id)
-    self.post_ids = post_ids.strip
+    
+    decrement!(:post_count)
+    update_attribute(:post_ids, Pool.normalize_post_ids(post_ids.gsub(/(?:\A| )#{post.id}(?:\Z| )/, " ")))
+    post.remove_pool!(self)
     clear_post_id_array
-    save
   end
   
   def posts(options = {})
-    offset = options[:offset] || 0
+    if options[:offset]
-    limit = options[:limit] || Danbooru.config.posts_per_page
+      limit = options[:limit] || Danbooru.config.posts_per_page
-    ids = post_id_array[offset, limit]
+      slice = post_id_array.slice(options[:offset], limit)
-    Post.where(["id IN (?)", ids]).order(Favorite.sql_order_clause(ids))
+      if slice && slice.any?
+        Post.where("id in (?)", slice).order(arbitrary_sql_order_clause(slice, "posts"))
+      else
+        Post.where("false")
+      end
+    else
+      Post.where("id IN (?)", post_id_array).order(arbitrary_sql_order_clause(post_id_array, "posts"))
+    end
+  end
+  
+  def balance_post_ids
+    added = post_id_array - post_id_array_was
+    removed = post_id_array_was - post_id_array
+    
+    added.each do |post_id|
+      post = Post.find(post_id)
+      post.add_pool!(self)
+    end
+    
+    removed.each do |post_id|
+      post = Post.find(post_id)
+      post.remove_pool!(self)
+    end
   end
   
   def post_id_array
     @post_id_array ||= post_ids.scan(/\d+/).map(&:to_i)
   end
   
+  def post_id_array_was
+    @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
@@ -88,6 +121,11 @@ class Pool < ActiveRecord::Base
   
   def clear_post_id_array
     @post_id_array = nil
+    @post_id_array_was = nil
+  end
+  
+  def initialize_post_count
+    self.post_count = post_id_array.size
   end
   
   def neighbor_posts(post)
@@ -95,13 +133,13 @@ class Pool < ActiveRecord::Base
       post_ids =~ /\A#{post.id} (\d+)|(\d+) #{post.id} (\d+)|(\d+) #{post.id}\Z/
       
       if $2 && $3
-        {:previous => $2.to_i, :next => $3.to_i}
+        OpenStruct.new(:previous => $2.to_i, :next => $3.to_i)
       elsif $1
-        {:next => $1.to_i}
+        OpenStruct.new(:next => $1.to_i)
       elsif $4
-        {:previous => $4.to_i}
+        OpenStruct.new(:previous => $4.to_i)
       else
-        {}
+        OpenStruct.new
       end
     end
   end

app/models/post.rb

@@ -632,17 +632,20 @@ class Post < ActiveRecord::Base
       end
     end
     
-    def add_pool(pool)
+    def belongs_to_pool?(pool)
-      return if pool_string =~ /(?:\A| )pool:#{pool.id}(?:\Z| )/
+      pool_string =~ /(?:\A| )pool:#{pool.id}(?:\Z| )/
-      self.pool_string += " pool:#{pool.id}"
-      self.pool_string.strip!
-      execute_sql("UPDATE posts SET pool_string = ? WHERE id = ?", pool_string, id)
     end
     
-    def remove_pool(pool)
+    def add_pool!(pool)
-      self.pool_string.gsub!(/(?:\A| )pool:#{pool.id}(?:\Z| )/, " ")
+      return if belongs_to_pool?(pool)
-      self.pool_string.strip!
+      update_attribute(:pool_string, "#{pool_string} pool:#{pool.id}".strip)
-      execute_sql("UPDATE posts SET pool_string = ? WHERE id = ?", pool_string, id)
+      pool.add_post!(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)
     end
   end
   

config/initializers/active_record_extensions.rb

@@ -0,0 +1,39 @@
+module Danbooru
+  module Extensions
+    module ActiveRecord
+      %w(execute select_value select_values select_all).each do |method_name|
+        define_method("#{method_name}_sql") do |sql, *params|
+          connection.__send__(method_name, self.class.sanitize_sql_array([sql, *params]))
+        end
+
+        self.class.__send__(:define_method, "#{method_name}_sql") do |sql, *params|
+          connection.__send__(method_name, sanitize_sql_array([sql, *params]))
+        end
+      end
+      
+      def arbitrary_sql_order_clause(ids, table_name = nil)
+        table_name = self.class.table_name if table_name.nil?
+        
+        if ids.empty?
+          return "#{table_name}.id desc"
+        end
+        
+        conditions = []
+        
+        ids.each_with_index do |x, n|
+          conditions << "when #{x} then #{n}"
+        end
+        
+        "case #{table_name}.id " + conditions.join(" ") + " end"
+      end
+    end
+  end
+end
+
+class ActiveRecord::Base
+  class << self    
+    public :sanitize_sql_array
+  end
+
+  include Danbooru::Extensions::ActiveRecord
+end

config/initializers/core_extensions.rb

@@ -1,17 +1,5 @@
 module Danbooru
   module Extensions
-    module ActiveRecord
-      %w(execute select_value select_values select_all).each do |method_name|
-        define_method("#{method_name}_sql") do |sql, *params|
-          connection.__send__(method_name, self.class.sanitize_sql_array([sql, *params]))
-        end
-
-        self.class.__send__(:define_method, "#{method_name}_sql") do |sql, *params|
-          connection.__send__(method_name, sanitize_sql_array([sql, *params]))
-        end
-      end
-    end
-
     module String
       def to_escaped_for_sql_like
         return self.gsub(/\\/, '\0\0').gsub(/%/, '\\%').gsub(/_/, '\\_').gsub(/\*/, '%')
@@ -24,14 +12,6 @@ module Danbooru
   end
 end
 
-class ActiveRecord::Base
-  class << self    
-    public :sanitize_sql_array
-  end
-
-  include Danbooru::Extensions::ActiveRecord
-end
-
 class String
   include Danbooru::Extensions::String
 end

db/development_structure.sql

@@ -8385,4 +8385,6 @@ INSERT INTO schema_migrations (version) VALUES ('20100826232512');
 
 INSERT INTO schema_migrations (version) VALUES ('20110328215652');
 
-INSERT INTO schema_migrations (version) VALUES ('20110328215701');
+INSERT INTO schema_migrations (version) VALUES ('20110328215701');
+
+INSERT INTO schema_migrations (version) VALUES ('20110607194023');

db/migrate/20100211025616_create_pools.rb

@@ -12,17 +12,6 @@ class CreatePools < ActiveRecord::Migration
     
     add_index :pools, :name
     add_index :pools, :creator_id
-    
-    
-    create_table :pool_versions do |t|
-      t.column :pool_id, :integer
-      t.column :post_ids, :text, :null => false, :default => ""
-      t.column :updater_id, :integer, :null => false
-      t.column :updater_ip_addr, "inet", :null => false
-      t.timestamps
-    end
-    
-    add_index :pool_versions, :pool_id
   end
 
   def self.down

db/migrate/20110607194023_create_pool_versions.rb

@@ -0,0 +1,17 @@
+class CreatePoolVersions < ActiveRecord::Migration
+  def up
+    create_table :pool_versions do |t|
+      t.column :pool_id, :integer
+      t.column :post_ids, :text, :null => false, :default => ""
+      t.column :updater_id, :integer, :null => false
+      t.column :updater_ip_addr, "inet", :null => false
+      t.timestamps
+    end
+    
+    add_index :pool_versions, :pool_id
+  end
+
+  def down
+    drop_table :pool_versions
+  end
+end

test/unit/pool_test.rb

@@ -14,10 +14,6 @@ class PoolTest < ActiveSupport::TestCase
   end
   
   context "A pool" do
-    setup do
-      MEMCACHE.flush_all
-    end
-    
     should "create versions for each distinct user" do
       pool = Factory.create(:pool)
       user = Factory.create(:user)
@@ -39,9 +35,6 @@ class PoolTest < ActiveSupport::TestCase
       p2 = Factory.create(:post)
       p3 = Factory.create(:post)
       p4 = Factory.create(:post)
-      p1.add_pool(pool)
-      p2.add_pool(pool)
-      p3.add_pool(pool)
       pool.add_post!(p1)
       pool.add_post!(p2)
       pool.add_post!(p3)
@@ -52,7 +45,7 @@ class PoolTest < ActiveSupport::TestCase
       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
+      posts = pool.posts.limit(1).offset(1).all
       assert_equal(1, posts.size)
       assert_equal([p2.id], posts.map(&:id))
     end
@@ -62,27 +55,75 @@ class PoolTest < ActiveSupport::TestCase
       p1 = Factory.create(:post)
       p2 = Factory.create(:post)
       p3 = Factory.create(:post)
-      p1.add_pool(pool)
-      p2.add_pool(pool)
-      p3.add_pool(pool)
       pool.add_post!(p1)
       pool.add_post!(p2)
       pool.add_post!(p3)
 
       pool.reload
       neighbors = pool.neighbor_posts(p1)
-      assert_nil(neighbors[:previous])
+      assert_nil(neighbors.previous)
-      assert_equal(p2.id, neighbors[:next])
+      assert_equal(p2.id, neighbors.next)
 
       pool.reload
       neighbors = pool.neighbor_posts(p2)
-      assert_equal(p1.id, neighbors[:previous])
+      assert_equal(p1.id, neighbors.previous)
-      assert_equal(p3.id, neighbors[:next])
+      assert_equal(p3.id, neighbors.next)
 
       pool.reload
       neighbors = pool.neighbor_posts(p3)
-      assert_equal(p2.id, neighbors[:previous])
+      assert_equal(p2.id, neighbors.previous)
-      assert_nil(neighbors[:next])
+      assert_nil(neighbors.next)
+    end
+    
+    should "know what its post_ids were" do
+      p1 = Factory.create(:post)
+      p2 = Factory.create(:post)
+      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
+      p1 = Factory.create(:post)
+      p2 = Factory.create(:post)
+      pool = Factory.create(:pool, :post_ids => "#{p1.id}")
+      pool.post_id_array = [p1.id, p2.id]
+      pool.save
+      p1.reload
+      p2.reload
+      assert_equal("pool:#{pool.id}", p1.pool_string)
+      assert_equal("pool:#{pool.id}", p2.pool_string)
+    end
+    
+    should "set its post count even if post_ids is updated directly" do
+      p1 = Factory.create(:post)
+      p2 = Factory.create(:post)
+      pool = Factory.create(:pool, :post_ids => "#{p1.id}")
+      pool.post_id_array = [p1.id, p2.id]
+      pool.save
+      assert_equal(2, pool.post_count)
+    end
+    
+    should "increment the post count every time a post is added" do
+      p1 = Factory.create(:post)
+      pool = Factory.create(:pool)
+      pool.add_post!(p1)
+      assert_equal(1, pool.post_count)
+    end
+    
+    should "not double increment when the same post is readded" do
+      p1 = Factory.create(:post)
+      pool = Factory.create(:pool)
+      pool.add_post!(p1)
+      pool.add_post!(p1)
+      assert_equal(1, pool.post_count)
+    end
+    
+    should "not double decrement" do
+      p1 = Factory.create(:post)
+      pool = Factory.create(:pool)
+      pool.remove_post!(p1)
+      assert_equal(0, pool.post_count)
     end
   end
   

test/unit/post_sets/pool_test.rb

@@ -16,9 +16,6 @@ module PostSets
         @pool.add_post!(@post_2)
         @pool.add_post!(@post_1)
         @pool.add_post!(@post_3)
-        @post_2.add_pool(@pool)
-        @post_1.add_pool(@pool)
-        @post_3.add_pool(@pool)
         @set = PostSets::Pool.new(@pool, :page => 1)
       end
       

test/unit/post_test.rb

@@ -356,20 +356,31 @@ class PostTest < ActiveSupport::TestCase
   end
   
   context "Pools:" do
+    context "Removing a post from a pool" do
+      should "update the post's pool string" do
+        post = Factory.create(:post)
+        pool = Factory.create(:pool)
+        post.add_pool!(pool)
+        post.remove_pool!(pool)
+        post.reload
+        assert_equal("", post.pool_string)
+        post.remove_pool!(pool)
+        post.reload
+        assert_equal("", post.pool_string)
+      end
+    end
+    
     context "Adding a post to a pool" do
       should "update the post's pool string" do
         post = Factory.create(:post)
         pool = Factory.create(:pool)
-        post.add_pool(pool)
+        post.add_pool!(pool)
         post.reload
         assert_equal("pool:#{pool.id}", post.pool_string)
-        post.add_pool(pool)
+        post.add_pool!(pool)
         post.reload
         assert_equal("pool:#{pool.id}", post.pool_string)
-        post.remove_pool(pool)
+        post.remove_pool!(pool)
-        post.reload
-        assert_equal("", post.pool_string)
-        post.remove_pool(pool)
         post.reload
         assert_equal("", post.pool_string)
       end
@@ -474,7 +485,7 @@ class PostTest < ActiveSupport::TestCase
       post2 = Factory.create(:post)
       post3 = Factory.create(:post)
       pool = Factory.create(:pool)
-      post1.add_pool(pool)
+      post1.add_pool!(pool)
       relation = Post.tag_match("pool:#{pool.name}")
       assert_equal(1, relation.count)
       assert_equal(post1.id, relation.first.id)

test/unit/upload_test.rb

@@ -2,7 +2,7 @@ require_relative '../test_helper'
 
 class UploadTest < ActiveSupport::TestCase
   setup do
-    user = Factory.create(:user)
+    user = Factory.create(:contributor_user)
     CurrentUser.user = user
     CurrentUser.ip_addr = "127.0.0.1"
     MEMCACHE.flush_all

test/unit/user_feedback_test.rb

@@ -1,39 +1,28 @@
 require_relative '../test_helper'
 
 class UserFeedbackTest < ActiveSupport::TestCase
-  setup do
-    user = Factory.create(:user)
-    CurrentUser.user = user
-    CurrentUser.ip_addr = "127.0.0.1"
-    MEMCACHE.flush_all
-  end
-  
-  teardown do
-    CurrentUser.user = nil
-    CurrentUser.ip_addr = nil
-  end
-
   context "A user's feedback" do
+    setup do
+      CurrentUser.ip_addr = "127.0.0.1"
+      MEMCACHE.flush_all
+    end
+
+    teardown do
+      CurrentUser.user = nil
+      CurrentUser.ip_addr = nil
+    end
+    
     should "should not validate if the creator is not privileged" do
       user = Factory.create(:user)
-      admin = Factory.create(:admin_user)
-      moderator = Factory.create(:moderator_user)
-      janitor = Factory.create(:janitor_user)
-      contributor = Factory.create(:contributor_user)
       privileged = Factory.create(:privileged_user)
       member = Factory.create(:user)
       
-      feedback = Factory.create(:user_feedback, :user => user, :creator => admin)
+      CurrentUser.user = privileged
+      feedback = Factory.create(:user_feedback, :user => user)
       assert(feedback.errors.empty?)
-      feedback = Factory.create(:user_feedback, :user => user, :creator => moderator)
+      
-      assert(feedback.errors.empty?)
+      CurrentUser.user = member
-      feedback = Factory.create(:user_feedback, :user => user, :creator => janitor)
+      feedback = Factory.build(:user_feedback, :user => user)
-      assert(feedback.errors.empty?)
-      feedback = Factory.create(:user_feedback, :user => user, :creator => contributor)
-      assert(feedback.errors.empty?)
-      feedback = Factory.create(:user_feedback, :user => user, :creator => privileged)
-      assert(feedback.errors.empty?)
-      feedback = Factory.build(:user_feedback, :user => user, :creator => member)
       feedback.save
       assert(feedback.errors.any?)
     end