Add oldest/newest links to shared pagination - with gaps around "newer" and "older" v2

app/controllers/concerns/pagination_methods.rb

@@ -6,8 +6,8 @@ module PaginationMethods
   ##
   # limit selected items to one page, get ids of first item before/after the page
   def get_page_items(items, includes: [], limit: 20)
-    param! :before, Integer, :min => 1
-    param! :after, Integer, :min => 1
+    param! :before, Integer, :min => 0
+    param! :after, Integer, :min => 0
 
     id_column = "#{items.table_name}.id"
     page_items = if params[:before]

app/helpers/pagination_helper.rb

@@ -0,0 +1,18 @@
+module PaginationHelper
+  def pagination_item(params, title, &)
+    link_class = "page-link icon-link text-center"
+    page_link_content = capture(&)
+    if params
+      page_link = link_to page_link_content,
+                          params,
+                          :class => link_class,
+                          :title => title,
+                          :data => { "turbo" => true, "turbo-frame" => "pagination", "turbo-action" => "advance" }
+      tag.li page_link, :class => "page-item d-flex"
+    else
+      page_link = tag.span page_link_content,
+                           :class => link_class
+      tag.li page_link, :class => "page-item d-flex disabled"
+    end
+  end
+end

app/views/shared/_pagination.html.erb

@@ -1,36 +1,30 @@
 <% if older_id || newer_id %>
 
 <% translation_scope ||= "shared.pagination.#{controller.controller_name}" %>
-<nav>
-  <% link_class = "page-link icon-link text-center" %>
+<nav class="d-flex justify-content-between gap-2">
   <ul class="pagination">
-    <% newer_link_content = capture do %>
-      <%= previous_page_svg_tag :class => "flex-shrink-0 d-none d-sm-block" %>
-      <%= t :newer, :scope => translation_scope %>
+    <%= pagination_item(newer_id && @params.merge(:before => nil, :after => nil),
+                        t(:newest, :scope => translation_scope)) do %>
+      <%= previous_page_svg_tag :class => "flex-shrink-0", :count => 2 %>
     <% end %>
-    <% if newer_id -%>
-      <li class="page-item d-flex">
-        <%= link_to newer_link_content, @params.merge(:before => nil, :after => newer_id), :class => link_class, :data => { "turbo" => true, "turbo-frame" => "pagination", "turbo-action" => "advance" } %>
-      </li>
-    <% else -%>
-      <li class="page-item d-flex disabled">
-        <%= tag.span newer_link_content, :class => link_class %>
-      </li>
-    <% end -%>
-
-    <% older_link_content = capture do %>
-      <%= t :older, :scope => translation_scope %>
-      <%= next_page_svg_tag :class => "flex-shrink-0 d-none d-sm-block" %>
+  </ul>
+  <ul class="pagination">
+    <%= pagination_item(newer_id && @params.merge(:before => nil, :after => newer_id),
+                        t(:newer, :scope => translation_scope)) do %>
+      <%= previous_page_svg_tag :class => "flex-shrink-0" %>
+      <span class="d-none d-sm-block"><%= t :newer, :scope => translation_scope %></span>
+    <% end %>
+    <%= pagination_item(older_id && @params.merge(:before => older_id, :after => nil),
+                        t(:older, :scope => translation_scope)) do %>
+      <span class="d-none d-sm-block"><%= t :older, :scope => translation_scope %></span>
+      <%= next_page_svg_tag :class => "flex-shrink-0" %>
+    <% end %>
+  </ul>
+  <ul class="pagination">
+    <%= pagination_item(older_id && @params.merge(:before => nil, :after => 0),
+                        t(:oldest, :scope => translation_scope)) do %>
+      <%= next_page_svg_tag :class => "flex-shrink-0", :count => 2 %>
     <% end %>
-    <% if older_id -%>
-      <li class="page-item d-flex">
-        <%= link_to older_link_content, @params.merge(:before => older_id, :after => nil), :class => link_class, :data => { "turbo" => true, "turbo-frame" => "pagination", "turbo-action" => "advance" } %>
-      </li>
-    <% else -%>
-      <li class="page-item d-flex disabled">
-        <%= tag.span older_link_content, :class => link_class %>
-      </li>
-    <% end -%>
   </ul>
 </nav>
 

config/locales/en.yml

@@ -1942,24 +1942,38 @@ en:
       changeset_comments:
         older: Older Comments
         newer: Newer Comments
+        oldest: Oldest Comments
+        newest: Newest Comments
       diary_comments:
         older: Older Comments
         newer: Newer Comments
+        oldest: Oldest Comments
+        newest: Newest Comments
       diary_entries:
         older: Older Entries
         newer: Newer Entries
+        oldest: Oldest Entries
+        newest: Newest Entries
       issues:
         older: Older Issues
         newer: Newer Issues
+        oldest: Oldest Issues
+        newest: Newest Issues
       traces:
         older: Older Traces
         newer: Newer Traces
+        oldest: Oldest Traces
+        newest: Newest Traces
       user_blocks:
         older: Older Blocks
         newer: Newer Blocks
+        oldest: Oldest Blocks
+        newest: Newest Blocks
       users:
         older: Older Users
         newer: Newer Users
+        oldest: Oldest Users
+        newest: Newest Users
   site:
     about:
       heading_html: "%{copyright}OpenStreetMap %{br} contributors"

test/controllers/diary_entries_controller_test.rb

@@ -443,46 +443,47 @@ def test_index_language
   def test_index_paged
     # Create several pages worth of diary entries
     create_list(:diary_entry, 50)
+    next_path = diary_entries_path
 
     # Try and get the index
-    get diary_entries_path
+    get next_path
     assert_response :success
     assert_select "article.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
+    check_no_page_link "Newer Entries"
+    next_path = check_page_link "Older Entries"
 
     # Try and get the second page
-    get css_select("li.page-item .page-link").last["href"]
+    get next_path
     assert_response :success
     assert_select "article.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
+    check_page_link "Newer Entries"
+    next_path = check_page_link "Older Entries"
 
     # Try and get the third page
-    get css_select("li.page-item .page-link").last["href"]
+    get next_path
     assert_response :success
     assert_select "article.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
+    next_path = check_page_link "Newer Entries"
+    check_no_page_link "Older Entries"
 
     # Go back to the second page
-    get css_select("li.page-item .page-link").first["href"]
+    get next_path
     assert_response :success
     assert_select "article.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
+    next_path = check_page_link "Newer Entries"
+    check_page_link "Older Entries"
 
     # Go back to the first page
-    get css_select("li.page-item .page-link").first["href"]
+    get next_path
     assert_response :success
     assert_select "article.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
+    check_no_page_link "Newer Entries"
+    check_page_link "Older Entries"
   end
 
   def test_index_invalid_paged
     # Try some invalid paged accesses
-    %w[-1 0 fred].each do |id|
+    %w[-1 fred].each do |id|
       get diary_entries_path(:before => id)
       assert_redirected_to :controller => :errors, :action => :bad_request
 
@@ -967,4 +968,14 @@ def check_diary_index(*entries)
       assert_select "a[href=?]", "/user/#{ERB::Util.u(entry.user.display_name)}/diary/#{entry.id}"
     end
   end
+
+  def check_no_page_link(name)
+    assert_select "a.page-link", { :text => /#{Regexp.quote(name)}/, :count => 0 }, "unexpected #{name} page link"
+  end
+
+  def check_page_link(name)
+    assert_select "a.page-link", { :text => /#{Regexp.quote(name)}/ }, "missing #{name} page link" do |buttons|
+      return buttons.first.attributes["href"].value
+    end
+  end
 end

test/controllers/traces_controller_test.rb

@@ -205,51 +205,52 @@ def test_index_user
   def test_index_paged
     # Create several pages worth of traces
     create_list(:trace, 50)
+    next_path = traces_path
 
     # Try and get the index
-    get traces_path
+    get next_path
     assert_response :success
     assert_select "table#trace_list tbody", :count => 1 do
       assert_select "tr", :count => 20
     end
-    assert_select "li.page-item.disabled span.page-link", :text => "Newer Traces", :count => 2
-    assert_select "li.page-item a.page-link", :text => "Older Traces", :count => 2
+    check_no_page_link "Newer Traces"
+    next_path = check_page_link "Older Traces"
 
     # Try and get the second page
-    get css_select("li.page-item a.page-link").last["href"]
+    get next_path
     assert_response :success
     assert_select "table#trace_list tbody", :count => 1 do
       assert_select "tr", :count => 20
     end
-    assert_select "li.page-item a.page-link", :text => "Newer Traces", :count => 2
-    assert_select "li.page-item a.page-link", :text => "Older Traces", :count => 2
+    check_page_link "Newer Traces"
+    next_path = check_page_link "Older Traces"
 
     # Try and get the third page
-    get css_select("li.page-item a.page-link").last["href"]
+    get next_path
     assert_response :success
     assert_select "table#trace_list tbody", :count => 1 do
       assert_select "tr", :count => 10
     end
-    assert_select "li.page-item a.page-link", :text => "Newer Traces", :count => 2
-    assert_select "li.page-item.disabled span.page-link", :text => "Older Traces", :count => 2
+    next_path = check_page_link "Newer Traces"
+    check_no_page_link "Older Traces"
 
     # Go back to the second page
-    get css_select("li.page-item a.page-link").first["href"]
+    get next_path
     assert_response :success
     assert_select "table#trace_list tbody", :count => 1 do
       assert_select "tr", :count => 20
     end
-    assert_select "li.page-item a.page-link", :text => "Newer Traces", :count => 2
-    assert_select "li.page-item a.page-link", :text => "Older Traces", :count => 2
+    next_path = check_page_link "Newer Traces"
+    check_page_link "Older Traces"
 
     # Go back to the first page
-    get css_select("li.page-item a.page-link").first["href"]
+    get next_path
     assert_response :success
     assert_select "table#trace_list tbody", :count => 1 do
       assert_select "tr", :count => 20
     end
-    assert_select "li.page-item.disabled span.page-link", :text => "Newer Traces", :count => 2
-    assert_select "li.page-item a.page-link", :text => "Older Traces", :count => 2
+    check_no_page_link "Newer Traces"
+    check_page_link "Older Traces"
   end
 
   # Check a multi-page index of tagged traces
@@ -258,56 +259,57 @@ def test_index_tagged_paged
     create_list(:trace, 100) do |trace, index|
       create(:tracetag, :trace => trace, :tag => "London") if index.even?
     end
+    next_path = traces_path :tag => "London"
 
     # Try and get the index
-    get traces_path(:tag => "London")
+    get next_path
     assert_response :success
     assert_select "table#trace_list tbody", :count => 1 do
       assert_select "tr", :count => 20
     end
-    assert_select "li.page-item.disabled span.page-link", :text => "Newer Traces", :count => 2
-    assert_select "li.page-item a.page-link", :text => "Older Traces", :count => 2
+    check_no_page_link "Newer Traces"
+    next_path = check_page_link "Older Traces"
 
     # Try and get the second page
-    get css_select("li.page-item a.page-link").last["href"]
+    get next_path
     assert_response :success
     assert_select "table#trace_list tbody", :count => 1 do
       assert_select "tr", :count => 20
     end
-    assert_select "li.page-item a.page-link", :text => "Newer Traces", :count => 2
-    assert_select "li.page-item a.page-link", :text => "Older Traces", :count => 2
+    check_page_link "Newer Traces"
+    next_path = check_page_link "Older Traces"
 
     # Try and get the third page
-    get css_select("li.page-item a.page-link").last["href"]
+    get next_path
     assert_response :success
     assert_select "table#trace_list tbody", :count => 1 do
       assert_select "tr", :count => 10
     end
-    assert_select "li.page-item a.page-link", :text => "Newer Traces", :count => 2
-    assert_select "li.page-item.disabled span.page-link", :text => "Older Traces", :count => 2
+    next_path = check_page_link "Newer Traces"
+    check_no_page_link "Older Traces"
 
     # Go back to the second page
-    get css_select("li.page-item a.page-link").first["href"]
+    get next_path
     assert_response :success
     assert_select "table#trace_list tbody", :count => 1 do
       assert_select "tr", :count => 20
     end
-    assert_select "li.page-item a.page-link", :text => "Newer Traces", :count => 2
-    assert_select "li.page-item a.page-link", :text => "Older Traces", :count => 2
+    next_path = check_page_link "Newer Traces"
+    check_page_link "Older Traces"
 
     # Go back to the first page
-    get css_select("li.page-item a.page-link").first["href"]
+    get next_path
     assert_response :success
     assert_select "table#trace_list tbody", :count => 1 do
       assert_select "tr", :count => 20
     end
-    assert_select "li.page-item.disabled span.page-link", :text => "Newer Traces", :count => 2
-    assert_select "li.page-item a.page-link", :text => "Older Traces", :count => 2
+    check_no_page_link "Newer Traces"
+    check_page_link "Older Traces"
   end
 
   def test_index_invalid_paged
     # Try some invalid paged accesses
-    %w[-1 0 fred].each do |id|
+    %w[-1 fred].each do |id|
       get traces_path(:before => id)
       assert_redirected_to :controller => :errors, :action => :bad_request
 
@@ -670,6 +672,16 @@ def check_trace_index(traces)
     end
   end
 
+  def check_no_page_link(name)
+    assert_select "a.page-link", { :text => /#{Regexp.quote(name)}/, :count => 0 }, "unexpected #{name} page link"
+  end
+
+  def check_page_link(name)
+    assert_select "a.page-link", { :text => /#{Regexp.quote(name)}/ }, "missing #{name} page link" do |buttons|
+      return buttons.first.attributes["href"].value
+    end
+  end
+
   def check_trace_show(trace)
     assert_response :success
     assert_template "show"

test/controllers/user_blocks_controller_test.rb

@@ -92,7 +92,7 @@ def test_index_paged
   ##
   # test the index action with invalid pages
   def test_index_invalid_paged
-    %w[-1 0 fred].each do |id|
+    %w[-1 fred].each do |id|
       get user_blocks_path(:before => id)
       assert_redirected_to :controller => :errors, :action => :bad_request
 

test/controllers/users/diary_comments_controller_test.rb

@@ -58,7 +58,7 @@ def test_index
     def test_index_invalid_paged
       user = create(:user)
 
-      %w[-1 0 fred].each do |id|
+      %w[-1 fred].each do |id|
         get user_diary_comments_path(user, :before => id)
         assert_redirected_to :controller => "/errors", :action => :bad_request
 

test/controllers/users/issued_blocks_controller_test.rb

@@ -83,7 +83,7 @@ def test_show_paged
     def test_show_invalid_paged
       user = create(:moderator_user)
 
-      %w[-1 0 fred].each do |id|
+      %w[-1 fred].each do |id|
         get user_issued_blocks_path(user, :before => id)
         assert_redirected_to :controller => "/errors", :action => :bad_request