From: Tom Hughes <tom@compton.nu>
Date: Thu, 27 Jul 2023 19:42:03 +0000 (+0100)
Subject: Merge remote-tracking branch 'upstream/pull/4108'
X-Git-Tag: live~1958
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/e932972a4a3683bbebc99659f9fb8739beb4eba3?hp=0a66990911baf62807d6bd50b8d0885a227c6e09

Merge remote-tracking branch 'upstream/pull/4108'
---

diff --git a/app/controllers/api/changesets_controller.rb b/app/controllers/api/changesets_controller.rb
index 56070951a..a08edff53 100644
--- a/app/controllers/api/changesets_controller.rb
+++ b/app/controllers/api/changesets_controller.rb
@@ -19,6 +19,9 @@ module Api
     # Helper methods for checking consistency
     include ConsistencyValidations
 
+    DEFAULT_QUERY_LIMIT = 100
+    MAX_QUERY_LIMIT = 100
+
     ##
     # Return XML giving the basic info about the changeset. Does not
     # return anything about the nodes, ways and relations in the changeset.
@@ -171,7 +174,7 @@ module Api
       changesets = conditions_ids(changesets, params["changesets"])
 
       # sort and limit the changesets
-      changesets = changesets.order("created_at DESC").limit(100)
+      changesets = changesets.order("created_at DESC").limit(result_limit)
 
       # preload users, tags and comments, and render result
       @changesets = changesets.preload(:user, :changeset_tags, :comments)
@@ -383,5 +386,19 @@ module Api
         changesets.where(:id => ids)
       end
     end
+
+    ##
+    # Get the maximum number of results to return
+    def result_limit
+      if params[:limit]
+        if params[:limit].to_i.positive? && params[:limit].to_i <= MAX_QUERY_LIMIT
+          params[:limit].to_i
+        else
+          raise OSM::APIBadUserInput, "Changeset limit must be between 1 and #{MAX_QUERY_LIMIT}"
+        end
+      else
+        DEFAULT_QUERY_LIMIT
+      end
+    end
   end
 end
diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb
index 855bbb78a..0b7dbc94a 100644
--- a/app/controllers/traces_controller.rb
+++ b/app/controllers/traces_controller.rb
@@ -40,30 +40,38 @@ class TracesController < ApplicationController
     # 2 - all traces, not logged in = all public traces
     # 3 - user's traces, logged in as same user = all user's traces
     # 4 - user's traces, not logged in as that user = all user's public traces
-    @traces = if target_user.nil? # all traces
-                if current_user
-                  Trace.visible_to(current_user) # 1
-                else
-                  Trace.visible_to_all # 2
-                end
-              elsif current_user && current_user == target_user
-                current_user.traces # 3 (check vs user id, so no join + can't pick up non-public traces by changing name)
-              else
-                target_user.traces.visible_to_all # 4
-              end
+    traces = if target_user.nil? # all traces
+               if current_user
+                 Trace.visible_to(current_user) # 1
+               else
+                 Trace.visible_to_all # 2
+               end
+             elsif current_user && current_user == target_user
+               current_user.traces # 3 (check vs user id, so no join + can't pick up non-public traces by changing name)
+             else
+               target_user.traces.visible_to_all # 4
+             end
 
-    @traces = @traces.tagged(params[:tag]) if params[:tag]
+    traces = traces.tagged(params[:tag]) if params[:tag]
 
-    @params = params.permit(:display_name, :tag)
+    traces = traces.visible
 
-    @page = (params[:page] || 1).to_i
-    @page_size = 20
+    @params = params.permit(:display_name, :tag, :before, :after)
 
-    @traces = @traces.visible
-    @traces = @traces.order(:id => :desc)
-    @traces = @traces.offset((@page - 1) * @page_size)
-    @traces = @traces.limit(@page_size)
+    @traces = if params[:before]
+                traces.where("gpx_files.id < ?", params[:before]).order(:id => :desc)
+              elsif params[:after]
+                traces.where("gpx_files.id > ?", params[:after]).order(:id => :asc)
+              else
+                traces.order(:id => :desc)
+              end
+
+    @traces = @traces.limit(20)
     @traces = @traces.includes(:user, :tags)
+    @traces = @traces.sort.reverse
+
+    @newer_traces = @traces.count.positive? && traces.exists?(["gpx_files.id > ?", @traces.first.id])
+    @older_traces = @traces.count.positive? && traces.exists?(["gpx_files.id < ?", @traces.last.id])
 
     # final helper vars for view
     @target_user = target_user
diff --git a/app/views/traces/_trace_paging_nav.html.erb b/app/views/traces/_trace_paging_nav.html.erb
index 25e35d226..29e0d37f6 100644
--- a/app/views/traces/_trace_paging_nav.html.erb
+++ b/app/views/traces/_trace_paging_nav.html.erb
@@ -1,8 +1,8 @@
 <nav>
   <ul class="pagination">
-    <% if page > 1 %>
+    <% if newer_traces %>
       <li class="page-item">
-        <%= link_to t(".newer"), params.merge(:page => page - 1), :class => "page-link" %>
+        <%= link_to t(".newer"), params.merge(:before => nil, :after => traces.first.id), :class => "page-link" %>
       </li>
     <% else %>
       <li class="page-item disabled">
@@ -10,17 +10,13 @@
       </li>
     <% end %>
 
-    <li class="page-item disabled">
-      <span class="page-link"><%= t(".showing_page", :page => page) %></span>
-    </li>
-
-    <% if traces.size < page_size %>
-      <li class="page-item disabled">
-        <span class="page-link"><%= t(".older") %></span>
+    <% if older_traces %>
+      <li class="page-item">
+        <%= link_to t(".older"), params.merge(:before => traces.last.id, :after => nil), :class => "page-link" %>
       </li>
     <% else %>
-      <li class="page-item">
-        <%= link_to t(".older"), params.merge(:page => page + 1), :class => "page-link" %>
+      <li class="page-item disabled">
+        <span class="page-link"><%= t(".older") %></span>
       </li>
     <% end %>
   </ul>
diff --git a/app/views/traces/index.html.erb b/app/views/traces/index.html.erb
index 71b25afe0..26e52add7 100644
--- a/app/views/traces/index.html.erb
+++ b/app/views/traces/index.html.erb
@@ -8,7 +8,7 @@
       </li>
       <% if params[:tag] %>
         <li>
-          <%= link_to t(".remove_tag_filter", :tag => params[:tag]), { :controller => "traces", :action => "index", :display_name => nil, :tag => nil, :page => nil } %>
+          <%= link_to t(".remove_tag_filter", :tag => params[:tag]), { :controller => "traces", :action => "index", :tag => nil } %>
         </li>
       <% end %>
     </ul>
@@ -17,33 +17,33 @@
     <% if @target_user.blank? %>
       <!-- public traces -->
       <li class="nav-item">
-        <%= link_to t(".all_traces"), { :controller => "traces", :action => "index", :display_name => nil, :page => nil }, { :class => "nav-link active" } %>
+        <%= link_to t(".all_traces"), { :controller => "traces", :action => "index", :display_name => nil }, { :class => "nav-link active" } %>
       </li>
       <% if current_user %>
         <li class="nav-item">
-          <%= link_to t(".my_traces"), { :action => "mine", :page => nil }, { :class => "nav-link" } %>
+          <%= link_to t(".my_traces"), { :action => "mine" }, { :class => "nav-link" } %>
         </li>
       <% end %>
     <% elsif current_user && current_user == @target_user %>
       <li class="nav-item">
-        <%= link_to t(".all_traces"), { :controller => "traces", :action => "index", :display_name => nil, :page => nil }, { :class => "nav-link" } %>
+        <%= link_to t(".all_traces"), { :controller => "traces", :action => "index", :display_name => nil }, { :class => "nav-link" } %>
       </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" }, { :class => "nav-link active" } %>
       </li>
     <% else %>
       <!-- public_traces_from @target_user -->
       <li class="nav-item">
-        <%= link_to t(".all_traces"), { :controller => "traces", :action => "index", :display_name => nil, :page => nil }, { :class => "nav-link" } %>
+        <%= link_to t(".all_traces"), { :controller => "traces", :action => "index", :display_name => nil }, { :class => "nav-link" } %>
       </li>
       <% if current_user && current_user != @target_user %>
         <li class="nav-item">
-          <%= link_to t(".my_traces"), { :action => "mine", :page => nil }, { :class => "nav-link" } %>
+          <%= link_to t(".my_traces"), { :action => "mine" }, { :class => "nav-link" } %>
         </li>
       <% end %>
       <li class="nav-item">
-        <%= link_to t(".public_traces_from", :user => @target_user&.display_name), { :action => "mine", :page => nil }, { :class => "nav-link active" } %>
+        <%= link_to t(".public_traces_from", :user => @target_user&.display_name), { :action => "mine" }, { :class => "nav-link active" } %>
       </li>
     <% end %>
 
@@ -66,7 +66,7 @@
 <% end %>
 
 <% if @traces.size > 0 %>
-  <%= render "trace_paging_nav", :page => @page, :page_size => @page_size, :traces => @traces, :params => @params %>
+  <%= render "trace_paging_nav", :older_traces => @older_traces, :newer_traces => @newer_traces, :traces => @traces, :params => @params %>
 
   <table id="trace_list" class="table table-borderless table-striped">
     <tbody>
@@ -74,7 +74,7 @@
     </tbody>
   </table>
 
-  <%= render "trace_paging_nav", :page => @page, :page_size => @page_size, :traces => @traces, :params => @params %>
+  <%= render "trace_paging_nav", :older_traces => @older_traces, :newer_traces => @newer_traces, :traces => @traces, :params => @params %>
 <% else %>
   <h2><%= t ".empty_title" %></h2>
   <p><%= t ".empty_upload_html", :upload_link => link_to(t(".upload_new"), new_trace_path),
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 8e94b40d7..c05b92d44 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -2434,7 +2434,6 @@ en:
       visibility: "Visibility:"
       confirm_delete: "Delete this trace?"
     trace_paging_nav:
-      showing_page: "Page %{page}"
       older: "Older Traces"
       newer: "Newer Traces"
     trace:
diff --git a/config/routes.rb b/config/routes.rb
index 04a6908df..415f8a01b 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -189,23 +189,23 @@ OpenStreetMap::Application.routes.draw do
 
   # traces
   resources :traces, :except => [:show]
-  get "/user/:display_name/traces/tag/:tag/page/:page" => "traces#index", :page => /[1-9][0-9]*/
+  get "/user/:display_name/traces/tag/:tag/page/:page", :page => /[1-9][0-9]*/, :to => redirect(:path => "/user/%{display_name}/traces/tag/%{tag}")
   get "/user/:display_name/traces/tag/:tag" => "traces#index"
-  get "/user/:display_name/traces/page/:page" => "traces#index", :page => /[1-9][0-9]*/
+  get "/user/:display_name/traces/page/:page", :page => /[1-9][0-9]*/, :to => redirect(:path => "/user/%{display_name}/traces")
   get "/user/:display_name/traces" => "traces#index"
   get "/user/:display_name/traces/tag/:tag/rss" => "traces#georss", :defaults => { :format => :rss }
   get "/user/:display_name/traces/rss" => "traces#georss", :defaults => { :format => :rss }
   get "/user/:display_name/traces/:id" => "traces#show", :as => "show_trace"
   get "/user/:display_name/traces/:id/picture" => "traces#picture", :as => "trace_picture"
   get "/user/:display_name/traces/:id/icon" => "traces#icon", :as => "trace_icon"
-  get "/traces/tag/:tag/page/:page" => "traces#index", :page => /[1-9][0-9]*/
+  get "/traces/tag/:tag/page/:page", :page => /[1-9][0-9]*/, :to => redirect(:path => "/traces/tag/%{tag}")
   get "/traces/tag/:tag" => "traces#index"
-  get "/traces/page/:page" => "traces#index", :page => /[1-9][0-9]*/
+  get "/traces/page/:page", :page => /[1-9][0-9]*/, :to => redirect(:path => "/traces")
   get "/traces/tag/:tag/rss" => "traces#georss", :defaults => { :format => :rss }
   get "/traces/rss" => "traces#georss", :defaults => { :format => :rss }
-  get "/traces/mine/tag/:tag/page/:page" => "traces#mine", :page => /[1-9][0-9]*/
+  get "/traces/mine/tag/:tag/page/:page", :page => /[1-9][0-9]*/, :to => redirect(:path => "/traces/mine/tag/%{tag}")
   get "/traces/mine/tag/:tag" => "traces#mine"
-  get "/traces/mine/page/:page" => "traces#mine"
+  get "/traces/mine/page/:page", :page => /[1-9][0-9]*/, :to => redirect(:path => "/traces/mine")
   get "/traces/mine" => "traces#mine"
   get "/trace/create", :to => redirect(:path => "/traces/new")
   get "/trace/:id/data" => "traces#data", :id => /\d+/, :as => "trace_data"
diff --git a/test/controllers/api/changesets_controller_test.rb b/test/controllers/api/changesets_controller_test.rb
index cb82b5664..40b8bc9f2 100644
--- a/test/controllers/api/changesets_controller_test.rb
+++ b/test/controllers/api/changesets_controller_test.rb
@@ -1938,6 +1938,31 @@ module Api
       assert_response :bad_request, "should be a bad request since changesets is empty"
     end
 
+    ##
+    # test the query functionality of changesets with the limit parameter
+    def test_query_limit
+      user = create(:user)
+      changeset1 = create(:changeset, :closed, :user => user, :created_at => Time.utc(2008, 1, 1, 0, 0, 0), :closed_at => Time.utc(2008, 1, 2, 0, 0, 0))
+      changeset2 = create(:changeset, :closed, :user => user, :created_at => Time.utc(2008, 2, 1, 0, 0, 0), :closed_at => Time.utc(2008, 2, 2, 0, 0, 0))
+      changeset3 = create(:changeset, :closed, :user => user, :created_at => Time.utc(2008, 3, 1, 0, 0, 0), :closed_at => Time.utc(2008, 3, 2, 0, 0, 0))
+      changeset4 = create(:changeset, :closed, :user => user, :created_at => Time.utc(2008, 4, 1, 0, 0, 0), :closed_at => Time.utc(2008, 4, 2, 0, 0, 0))
+      changeset5 = create(:changeset, :closed, :user => user, :created_at => Time.utc(2008, 5, 1, 0, 0, 0), :closed_at => Time.utc(2008, 5, 2, 0, 0, 0))
+
+      get changesets_path
+      assert_response :success
+      assert_changesets [changeset5, changeset4, changeset3, changeset2, changeset1]
+
+      get changesets_path(:limit => "3")
+      assert_response :success
+      assert_changesets [changeset5, changeset4, changeset3]
+
+      get changesets_path(:limit => "0")
+      assert_response :bad_request
+
+      get changesets_path(:limit => "101")
+      assert_response :bad_request
+    end
+
     ##
     # check that errors are returned if garbage is inserted
     # into query strings
diff --git a/test/controllers/traces_controller_test.rb b/test/controllers/traces_controller_test.rb
index cd9f5f127..fe3ecdea6 100644
--- a/test/controllers/traces_controller_test.rb
+++ b/test/controllers/traces_controller_test.rb
@@ -8,51 +8,27 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
       { :path => "/traces", :method => :get },
       { :controller => "traces", :action => "index" }
     )
-    assert_routing(
-      { :path => "/traces/page/1", :method => :get },
-      { :controller => "traces", :action => "index", :page => "1" }
-    )
     assert_routing(
       { :path => "/traces/tag/tagname", :method => :get },
       { :controller => "traces", :action => "index", :tag => "tagname" }
     )
-    assert_routing(
-      { :path => "/traces/tag/tagname/page/1", :method => :get },
-      { :controller => "traces", :action => "index", :tag => "tagname", :page => "1" }
-    )
     assert_routing(
       { :path => "/user/username/traces", :method => :get },
       { :controller => "traces", :action => "index", :display_name => "username" }
     )
-    assert_routing(
-      { :path => "/user/username/traces/page/1", :method => :get },
-      { :controller => "traces", :action => "index", :display_name => "username", :page => "1" }
-    )
     assert_routing(
       { :path => "/user/username/traces/tag/tagname", :method => :get },
       { :controller => "traces", :action => "index", :display_name => "username", :tag => "tagname" }
     )
-    assert_routing(
-      { :path => "/user/username/traces/tag/tagname/page/1", :method => :get },
-      { :controller => "traces", :action => "index", :display_name => "username", :tag => "tagname", :page => "1" }
-    )
 
     assert_routing(
       { :path => "/traces/mine", :method => :get },
       { :controller => "traces", :action => "mine" }
     )
-    assert_routing(
-      { :path => "/traces/mine/page/1", :method => :get },
-      { :controller => "traces", :action => "mine", :page => "1" }
-    )
     assert_routing(
       { :path => "/traces/mine/tag/tagname", :method => :get },
       { :controller => "traces", :action => "mine", :tag => "tagname" }
     )
-    assert_routing(
-      { :path => "/traces/mine/tag/tagname/page/1", :method => :get },
-      { :controller => "traces", :action => "mine", :tag => "tagname", :page => "1" }
-    )
 
     assert_routing(
       { :path => "/traces/rss", :method => :get },
@@ -112,6 +88,24 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
       { :path => "/traces/1", :method => :delete },
       { :controller => "traces", :action => "destroy", :id => "1" }
     )
+
+    get "/traces/page/1"
+    assert_redirected_to "/traces"
+
+    get "/traces/tag/tagname/page/1"
+    assert_redirected_to "/traces/tag/tagname"
+
+    get "/user/username/traces/page/1"
+    assert_redirected_to "/user/username/traces"
+
+    get "/user/username/traces/tag/tagname/page/1"
+    assert_redirected_to "/user/username/traces/tag/tagname"
+
+    get "/traces/mine/page/1"
+    assert_redirected_to "/traces/mine"
+
+    get "/traces/mine/tag/tagname/page/1"
+    assert_redirected_to "/traces/mine/tag/tagname"
   end
 
   # Check that the index of traces is displayed
@@ -227,13 +221,97 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
     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
 
     # Try and get the second page
-    get traces_path(:page => 2)
+    get css_select("li.page-item a.page-link").last["href"]
+    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
+
+    # Try and get the third page
+    get css_select("li.page-item a.page-link").last["href"]
+    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
+
+    # Go back to the second page
+    get css_select("li.page-item a.page-link").first["href"]
+    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
+
+    # Go back to the first page
+    get css_select("li.page-item a.page-link").first["href"]
+    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
+  end
+
+  # Check a multi-page index of tagged traces
+  def test_index_tagged_paged
+    # Create several pages worth of traces
+    create_list(:trace, 100) do |trace, index|
+      create(:tracetag, :trace => trace, :tag => "London") if index.even?
+    end
+
+    # Try and get the index
+    get traces_path(:tag => "London")
+    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
+
+    # Try and get the second page
+    get css_select("li.page-item a.page-link").last["href"]
+    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
+
+    # Try and get the third page
+    get css_select("li.page-item a.page-link").last["href"]
+    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
+
+    # Go back to the second page
+    get css_select("li.page-item a.page-link").first["href"]
+    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
+
+    # Go back to the first page
+    get css_select("li.page-item a.page-link").first["href"]
     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
   end
 
   # Check the RSS feed