pools: refactor #neighbors + fix broken #neighbors tests.

app/models/pool.rb

@@ -315,26 +315,17 @@ class Pool < ApplicationRecord
     self
   end
 
-  def neighbors(post, relation)
-    post_ids =~ /\A#{post.id} (\d+)|(\d+) #{post.id} (\d+)|(\d+) #{post.id}\Z/
+  # XXX finds wrong post when the pool contains multiple copies of the same post (#2042).
+  def previous_post_id(post_id)
+    n = post_id_array.index(post_id) - 1
+    return nil if n < 0
+    post_id_array[n]
+  end
 
-    if $2 && $3
-      if relation == :previous
-        return $2.to_i
-      else
-        return $3.to_i
-      end
-    elsif $1
-      if relation == :next
-        return $1.to_i
-      end
-    elsif $4
-      if relation == :previous
-        return $4.to_i
-      end
-    end
-
-    return nil
+  def next_post_id(post_id)
+    n = post_id_array.index(post_id) + 1
+    return nil if n >= post_id_array.size
+    post_id_array[n]
   end
 
   def cover_post_id
@@ -355,7 +346,6 @@ class Pool < ApplicationRecord
 
   def reload(options = {})
     super
-    @neighbor_posts = nil
     clear_post_id_array
     self
   end

app/views/posts/partials/show/_pool_list_item.html.erb

@@ -5,7 +5,7 @@
     <span class="first">&laquo;</span>
   <% end -%>
 
-  <% pool.neighbors(post, :previous).tap do |previous_post_id| -%>
+  <% pool.previous_post_id(post.id).tap do |previous_post_id| -%>
     <% if previous_post_id %>
       <%= link_to "&lsaquo;&thinsp;prev".html_safe, post_path(previous_post_id, pool_id: pool.id), rel: selected ? "prev" : nil, class: "prev", title: "to page #{pool.page_number(previous_post_id)}" -%>
     <% else -%>
@@ -17,7 +17,7 @@
     <%= link_to("Pool: #{pool.pretty_name}", pool_path(pool), title: "page #{pool.page_number(post.id)}/#{pool.post_count}") -%>
   </span>
 
-  <% pool.neighbors(post, :next).tap do |next_post_id| -%>
+  <% pool.next_post_id(post.id).tap do |next_post_id| -%>
     <% if next_post_id %>
       <%= link_to("next&thinsp;&rsaquo;".html_safe, post_path(next_post_id, pool_id: pool.id), rel: selected ? "next" : nil, class: "next", title: "to page #{pool.page_number(next_post_id)}") -%>
     <% else -%>

test/unit/pool_test.rb

@@ -312,11 +312,6 @@ class PoolTest < ActiveSupport::TestCase
       @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
@@ -343,18 +338,18 @@ class PoolTest < ActiveSupport::TestCase
     end
 
     should "find the neighbors for the first post" do
-      assert_nil(@p1_neighbors.previous)
-      assert_equal(@p2.id, @p1_neighbors.next)
+      assert_nil(@pool.previous_post_id(@p1.id))
+      assert_equal(@p2.id, @pool.next_post_id(@p1.id))
     end
 
     should "find the neighbors for the middle post" do
-      assert_equal(@p1.id, @p2_neighbors.previous)
-      assert_equal(@p3.id, @p2_neighbors.next)
+      assert_equal(@p1.id, @pool.previous_post_id(@p2.id))
+      assert_equal(@p3.id, @pool.next_post_id(@p2.id))
     end
 
     should "find the neighbors for the last post" do
-      assert_equal(@p2.id, @p3_neighbors.previous)
-      assert_nil(@p3_neighbors.next)
+      assert_equal(@p2.id, @pool.previous_post_id(@p3.id))
+      assert_nil(@pool.next_post_id(@p3.id))
     end
   end
 end