Merge pull request #4089 from tomhughes/trace-paging

Replace page numbers with ID based selection for trace indexes

diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb
index 855bbb78af85bc8fc98a1dfe19731e304e8ed5ec..4f263e9b27b3ed0ed8fcd9359d5c583dc0be6db5 100644 (file)
--- 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("id < ?", params[:before]).order(:id => :desc)
+              elsif params[:after]
+                traces.where("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 25e35d2266d76beec4f590437a7285d9d1670450..29e0d37f622e0160a2b882fb90e454411f62a81e 100644 (file)
--- 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 71b25afe08be84d2e3241a7414bd0445647a8f2f..26e52add72998f06b224f6c9b3de939a56290548 100644 (file)
--- 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 8e94b40d7c8177684fc955d7795bc3d5b4ad6af0..c05b92d44e23df1de3861f522f9c64a7b6e98ffd 100644 (file)
--- 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 04a6908df88fc27a35d2b72e2a68f9ade1c360ed..415f8a01bd9b5dc1b677fe8e3261b51ea709c8f4 100644 (file)
--- 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/traces_controller_test.rb b/test/controllers/traces_controller_test.rb
index cd9f5f1270b735a12b2a2d4fe125bf1e15555d42..614b21b1e3d7331ba372023ce813f77dbd5bec23 100644 (file)
--- 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