[openstreetmap/openstreetmap-website] Replace page numbers with ID based selection for trace indexes (PR #4089)

[Andy Allan]

@gravitystorm requested changes on this pull request.

Overall I'm happy with the idea of moving to cursor-based pagination. I'd like to do the same for all of our existing offset-based pagination, since it all has the same problems as here (degraded performance for increasing pagenumbers, highly unstable urls, etc).

In addition to the line-by-line comments, I wonder if rather than trying to get the logic and edge cases right (and making all the logic etc resuable across controllers) I wonder if we are better off finding a gem that does what we want. There's a few different ones that I've found, such as [rails_cursor_pagination](https://github.com/xing/rails_cursor_pagination), [pagy_cursor](https://github.com/Uysim/pagy-cursor), [etc](https://rubygems.org/search?query=name%3A+cursor++++updated%3A+%3E2021)

>        <li class="page-item">
-        <%= link_to t(".newer"), params.merge(:page => page - 1), :class => "page-link" %>
+        <%= link_to t(".newer"), params.merge(:min_id => traces.first.id + 1, :max_id => nil), :class => "page-link" %>

The logic here isn't working quite right. If I go to the final page of traces on my system (i.e. press "older" a few times) then I get a link to newer traces e.g. "min_id=3". But that link shows me the first page of traces, not the second-to-last.

Since traces are reverse sorted, `traces.where(id >= 3).order_by(id desc).limit(25)` returns traces that are max(id) to max(id)-25 e.g. `1003` to `978`, instead of `27` to `3` which is what was intended.

>        </li>
     <% else %>
       <li class="page-item disabled">
         <span class="page-link"><%= t(".newer") %></span>
       </li>
     <% end %>
 
-    <li class="page-item disabled">
-      <span class="page-link"><%= t(".showing_page", :page => page) %></span>

Unused translation can be removed from `en.yml`

>        </li>
       <!-- my traces -->
       <li class="nav-item">
-        <%= link_to t(".my_traces"), { :action => "mine", :page => nil }, { :class => "nav-link active" } %>
+        <%= link_to t(".my_traces"), { :action => "mine", :max_id => nil }, { :class => "nav-link active" } %>

I don't think any of these `:max_id => nil` are needed, since the link is being built from scratch (other links are built from merging with existing params elsewhere).

Or if I've misunderstood, and they are necessary, then I think a `:min_id => nil` would be needed too.

> @@ -8,51 +8,27 @@ def test_routes
       { :path => "/traces", :method => :get },
       { :controller => "traces", :action => "index" }
     )
-    assert_routing(
-      { :path => "/traces/page/1", :method => :get },
-      { :controller => "traces", :action => "index", :page => "1" }
-    )

We should keep some some redirect tests to make sure the redirects aren't accidentally refactored away in future.

> @@ -54,14 +54,16 @@ def index
 
     @traces = @traces.tagged(params[:tag]) if params[:tag]
 
-    @params = params.permit(:display_name, :tag)
+    @params = params.permit(:display_name, :tag, :min_id, :max_id)

Most implementations I've seen use "before" and "after" as the parameters. I'm not sure why, but maybe it makes it easier when reasoning about id sort order? (see other comment)

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4089#pullrequestreview-1537559541
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4089/review/1537559541 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20230719/73d9be38/attachment-0001.htm>