Merge pull request #4106 from tomhughes/diary-paging

Replace page numbers with ID based selection for diary indexes

app/controllers/diary_entries_controller.rb

@@ -17,7 +17,7 @@ class DiaryEntriesController < ApplicationController
 
       if @user
         @title = t ".user_title", :user => @user.display_name
-        @entries = @user.diary_entries
+        entries = @user.diary_entries
       else
         render_unknown_user params[:display_name]
         return
@@ -25,7 +25,7 @@ class DiaryEntriesController < ApplicationController
     elsif params[:friends]
       if current_user
         @title = t ".title_friends"
-        @entries = DiaryEntry.where(:user_id => current_user.friends)
+        entries = DiaryEntry.where(:user_id => current_user.friends)
       else
         require_user
         return
@@ -33,32 +33,40 @@ class DiaryEntriesController < ApplicationController
     elsif params[:nearby]
       if current_user
         @title = t ".title_nearby"
-        @entries = DiaryEntry.where(:user_id => current_user.nearby)
+        entries = DiaryEntry.where(:user_id => current_user.nearby)
       else
         require_user
         return
       end
     else
-      @entries = DiaryEntry.joins(:user).where(:users => { :status => %w[active confirmed] })
+      entries = DiaryEntry.joins(:user).where(:users => { :status => %w[active confirmed] })
 
       if params[:language]
         @title = t ".in_language_title", :language => Language.find(params[:language]).english_name
-        @entries = @entries.where(:language_code => params[:language])
+        entries = entries.where(:language_code => params[:language])
       else
         @title = t ".title"
       end
     end
 
+    entries = entries.visible unless can? :unhide, DiaryEntry
+
     @params = params.permit(:display_name, :friends, :nearby, :language)
 
-    @page = (params[:page] || 1).to_i
+    @entries = if params[:before]
-    @page_size = 20
+                 entries.where("diary_entries.id < ?", params[:before]).order(:id => :desc)
+               elsif params[:after]
+                 entries.where("diary_entries.id > ?", params[:after]).order(:id => :asc)
+               else
+                 entries.order(:id => :desc)
+               end
 
-    @entries = @entries.visible unless can? :unhide, DiaryEntry
+    @entries = @entries.limit(20)
-    @entries = @entries.order("created_at DESC")
-    @entries = @entries.offset((@page - 1) * @page_size)
-    @entries = @entries.limit(@page_size)
     @entries = @entries.includes(:user, :language)
+    @entries = @entries.sort.reverse
+
+    @newer_entries = @entries.count.positive? && entries.exists?(["diary_entries.id > ?", @entries.first.id])
+    @older_entries = @entries.count.positive? && entries.exists?(["diary_entries.id < ?", @entries.last.id])
   end
 
   def show

app/views/diary_entries/index.html.erb

@@ -38,9 +38,9 @@
 
   <nav>
     <ul class="pagination">
-      <% if @entries.size >= @page_size -%>
+      <% if @older_entries -%>
         <li class="page-item">
-          <%= link_to t(".older_entries"), @params.merge(:page => @page + 1), :class => "page-link" %>
+          <%= link_to t(".older_entries"), @params.merge(:before => @entries.last.id), :class => "page-link" %>
         </li>
       <% else -%>
         <li class="page-item disabled">
@@ -48,9 +48,9 @@
         </li>
       <% end -%>
 
-      <% if @page > 1 -%>
+      <% if @newer_entries -%>
         <li class="page-item">
-          <%= link_to t(".newer_entries"), @params.merge(:page => @page - 1), :class => "page-link" %>
+          <%= link_to t(".newer_entries"), @params.merge(:after => @entries.first.id), :class => "page-link" %>
         </li>
       <% else -%>
         <li class="page-item disabled">

test/controllers/diary_entries_controller_test.rb

@@ -564,11 +564,36 @@ class DiaryEntriesControllerTest < ActionDispatch::IntegrationTest
     get diary_entries_path
     assert_response :success
     assert_select "div.diary_post", :count => 20
+    assert_select "li.page-item a.page-link", :text => "Older Entries", :count => 1
+    assert_select "li.page-item.disabled span.page-link", :text => "Newer Entries", :count => 1
 
     # Try and get the second page
-    get diary_entries_path(:page => 2)
+    get css_select("li.page-item a.page-link").first["href"]
     assert_response :success
     assert_select "div.diary_post", :count => 20
+    assert_select "li.page-item a.page-link", :text => "Older Entries", :count => 1
+    assert_select "li.page-item a.page-link", :text => "Newer Entries", :count => 1
+
+    # Try and get the third page
+    get css_select("li.page-item a.page-link").first["href"]
+    assert_response :success
+    assert_select "div.diary_post", :count => 10
+    assert_select "li.page-item.disabled span.page-link", :text => "Older Entries", :count => 1
+    assert_select "li.page-item a.page-link", :text => "Newer Entries", :count => 1
+
+    # Go back to the second page
+    get css_select("li.page-item a.page-link").last["href"]
+    assert_response :success
+    assert_select "div.diary_post", :count => 20
+    assert_select "li.page-item a.page-link", :text => "Older Entries", :count => 1
+    assert_select "li.page-item a.page-link", :text => "Newer Entries", :count => 1
+
+    # Go back to the first page
+    get css_select("li.page-item a.page-link").last["href"]
+    assert_response :success
+    assert_select "div.diary_post", :count => 20
+    assert_select "li.page-item a.page-link", :text => "Older Entries", :count => 1
+    assert_select "li.page-item.disabled span.page-link", :text => "Newer Entries", :count => 1
   end
 
   def test_rss