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

app/abilities/ability.rb

@@ -22,7 +22,7 @@ class Ability
       can [:create, :update], :password
       can :read, Redaction
       can [:create, :destroy], :session
-      can [:read, :data, :georss], Trace
+      can [:read, :data], Trace
       can [:read, :create, :suspended, :auth_success, :auth_failure], User
       can :read, UserBlock
     end

app/controllers/traces/feeds_controller.rb

@@ -0,0 +1,20 @@
+module Traces
+  class FeedsController < ApplicationController
+    before_action :authorize_web
+    before_action :set_locale
+    before_action :check_database_readable
+
+    authorize_resource :class => Trace
+
+    def show
+      @traces = Trace.visible_to_all.visible
+
+      @traces = @traces.joins(:user).where(:users => { :display_name => params[:display_name] }) if params[:display_name]
+
+      @traces = @traces.tagged(params[:tag]) if params[:tag]
+      @traces = @traces.order("timestamp DESC")
+      @traces = @traces.limit(20)
+      @traces = @traces.includes(:user)
+    end
+  end
+end

app/controllers/traces_controller.rb

@@ -2,7 +2,7 @@ class TracesController < ApplicationController
   include UserMethods
   include PaginationMethods
 
-  layout "site", :except => :georss
+  layout "site"
 
   before_action :authorize_web
   before_action :set_locale
@@ -192,17 +192,6 @@ class TracesController < ApplicationController
     head :not_found
   end
 
-  def georss
-    @traces = Trace.visible_to_all.visible
-
-    @traces = @traces.joins(:user).where(:users => { :display_name => params[:display_name] }) if params[:display_name]
-
-    @traces = @traces.tagged(params[:tag]) if params[:tag]
-    @traces = @traces.order("timestamp DESC")
-    @traces = @traces.limit(20)
-    @traces = @traces.includes(:user)
-  end
-
   private
 
   def do_create(file, tags, description, visibility)

app/views/traces/feeds/show.rss.builder

@@ -7,14 +7,14 @@ xml.rss("version" => "2.0",
   xml.channel do
     xml.title t(".title")
     xml.description t(".title")
-    xml.link url_for(:controller => :traces, :action => :index, :only_path => false)
+    xml.link url_for(:controller => "/traces", :action => :index, :only_path => false)
 
     xml.image do
       xml.url image_url("mag_map-rss2.0.png")
       xml.title t(".title")
       xml.width 100
       xml.height 100
-      xml.link url_for(:controller => :traces, :action => :index, :only_path => false)
+      xml.link url_for(:controller => "/traces", :action => :index, :only_path => false)
     end
 
     @traces.each do |trace|

app/views/traces/index.html.erb

@@ -16,8 +16,8 @@
 
   <div class="d-flex flex-column flex-md-row-reverse align-items-md-end">
     <div class="pb-1 ps-1 d-flex flex-wrap flex-shrink-0 gap-1 justify-content-end">
-      <%= link_to({ :action => :georss, :display_name => @target_user&.display_name, :tag => params[:tag] },
+      <%= link_to traces_feed_path(:display_name => @target_user&.display_name, :tag => params[:tag]),
-                  { :class => "btn btn-secondary btn-sm" }) do %>
+                  :class => "btn btn-secondary btn-sm" do %>
         <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" class="align-text-bottom">
           <circle cx="2" cy="14" r="2" fill="white" />
           <path d="M 8 14 a 6 6 0 0 0 -6 -6 M 14 14 a 12 12 0 0 0 -12 -12" fill="none" stroke="white" stroke-width="3" stroke-linecap="round" />
@@ -66,7 +66,7 @@
 <% end %>
 
 <% content_for :auto_discovery_link_tag do %>
-  <%= auto_discovery_link_tag :rss, :action => "georss", :display_name => @target_user&.display_name, :tag => params[:tag] %>
+  <%= auto_discovery_link_tag :rss, traces_feed_path(:display_name => @target_user&.display_name, :tag => params[:tag]) %>
 <% end %>
 
 <% if @traces.size > 0 %>

config/locales/en.yml

@@ -2616,13 +2616,14 @@ en:
     offline:
       heading: "GPX Storage Offline"
       message: "The GPX file storage and upload system is currently unavailable."
-    georss:
+    feeds:
-      title: "OpenStreetMap GPS Traces"
+      show:
-    description:
+        title: "OpenStreetMap GPS Traces"
-      description_with_count:
+      description:
-        one: "GPX file with %{count} point from %{user}"
+        description_with_count:
-        other: "GPX file with %{count} points from %{user}"
+          one: "GPX file with %{count} point from %{user}"
-      description_without_count: "GPX file from %{user}"
+          other: "GPX file with %{count} points from %{user}"
+        description_without_count: "GPX file from %{user}"
   application:
     permission_denied: You do not have permission to access that action
     require_cookies:

config/routes.rb

@@ -211,30 +211,30 @@ OpenStreetMap::Application.routes.draw do
   post "/preview/:type" => "site#preview", :as => :preview
 
   # traces
-  resources :traces, :except => [:show]
+  resources :traces, :id => /\d+/, :except => [:show]
   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", :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/:id" => "traces#show", :id => /\d+/, :as => "show_trace"
-  get "/user/:display_name/traces/rss" => "traces#georss", :defaults => { :format => :rss }
+  scope "/user/:display_name/traces/:trace_id", :module => :traces, :trace_id => /\d+/ do
-  get "/user/:display_name/traces/:id" => "traces#show", :as => "show_trace"
-  scope "/user/:display_name/traces/:trace_id", :module => :traces do
     get "picture" => "pictures#show", :as => "trace_picture"
     get "icon" => "icons#show", :as => "trace_icon"
   end
   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", :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", :page => /[1-9][0-9]*/, :to => redirect(:path => "/traces/mine/tag/%{tag}")
   get "/traces/mine/tag/:tag" => "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"
-  get "/trace/:id/edit", :to => redirect(:path => "/traces/%{id}/edit")
+  get "/trace/:id/edit", :id => /\d+/, :to => redirect(:path => "/traces/%{id}/edit")
+
+  namespace :traces, :path => "" do
+    resource :feed, :path => "(/user/:display_name)/traces(/tag/:tag)/rss", :only => :show, :defaults => { :format => :rss }
+  end
 
   # diary pages
   resources :diary_entries, :path => "diary", :only => [:new, :create, :index] do

test/controllers/traces/feeds_controller_test.rb

@@ -0,0 +1,109 @@
+require "test_helper"
+
+module Traces
+  class FeedsControllerTest < ActionDispatch::IntegrationTest
+    ##
+    # test all routes which lead to this controller
+    def test_routes
+      assert_routing(
+        { :path => "/traces/rss", :method => :get },
+        { :controller => "traces/feeds", :action => "show", :format => :rss }
+      )
+      assert_routing(
+        { :path => "/traces/tag/tagname/rss", :method => :get },
+        { :controller => "traces/feeds", :action => "show", :tag => "tagname", :format => :rss }
+      )
+      assert_routing(
+        { :path => "/user/username/traces/rss", :method => :get },
+        { :controller => "traces/feeds", :action => "show", :display_name => "username", :format => :rss }
+      )
+      assert_routing(
+        { :path => "/user/username/traces/tag/tagname/rss", :method => :get },
+        { :controller => "traces/feeds", :action => "show", :display_name => "username", :tag => "tagname", :format => :rss }
+      )
+    end
+
+    def test_show
+      user = create(:user)
+      # The fourth test below is surprisingly sensitive to timestamp ordering when the timestamps are equal.
+      trace_a = create(:trace, :visibility => "public", :timestamp => 4.seconds.ago) do |trace|
+        create(:tracetag, :trace => trace, :tag => "London")
+      end
+      trace_b = create(:trace, :visibility => "public", :timestamp => 3.seconds.ago) do |trace|
+        create(:tracetag, :trace => trace, :tag => "Birmingham")
+      end
+      create(:trace, :visibility => "private", :user => user, :timestamp => 2.seconds.ago) do |trace|
+        create(:tracetag, :trace => trace, :tag => "London")
+      end
+      create(:trace, :visibility => "private", :user => user, :timestamp => 1.second.ago) do |trace|
+        create(:tracetag, :trace => trace, :tag => "Birmingham")
+      end
+
+      # First with the public feed
+      get traces_feed_path
+      check_trace_feed [trace_b, trace_a]
+
+      # Restrict traces to those with a given tag
+      get traces_feed_path(:tag => "London")
+      check_trace_feed [trace_a]
+    end
+
+    def test_show_user
+      user = create(:user)
+      second_user = create(:user)
+      create(:user)
+      create(:trace)
+      trace_b = create(:trace, :visibility => "public", :timestamp => 4.seconds.ago, :user => user)
+      trace_c = create(:trace, :visibility => "public", :timestamp => 3.seconds.ago, :user => user) do |trace|
+        create(:tracetag, :trace => trace, :tag => "London")
+      end
+      create(:trace, :visibility => "private")
+
+      # Test a user with no traces
+      get traces_feed_path(:display_name => second_user)
+      check_trace_feed []
+
+      # Test the user with the traces - should see only public ones
+      get traces_feed_path(:display_name => user)
+      check_trace_feed [trace_c, trace_b]
+
+      # Should only see traces with the correct tag when a tag is specified
+      get traces_feed_path(:display_name => user, :tag => "London")
+      check_trace_feed [trace_c]
+
+      # Should no traces if the user does not exist
+      get traces_feed_path(:display_name => "UnknownUser")
+      check_trace_feed []
+    end
+
+    private
+
+    def check_trace_feed(traces)
+      assert_response :success
+      assert_template "traces/feeds/show"
+      assert_equal "application/rss+xml", @response.media_type
+      assert_select "rss", :count => 1 do
+        assert_select "channel", :count => 1 do
+          assert_select "title"
+          assert_select "description"
+          assert_select "link"
+          assert_select "image"
+          assert_select "item", :count => traces.length do |items|
+            traces.zip(items).each do |trace, item|
+              assert_select item, "title", trace.name
+              assert_select item, "link", "http://www.example.com/user/#{ERB::Util.u(trace.user.display_name)}/traces/#{trace.id}"
+              assert_select item, "guid", "http://www.example.com/user/#{ERB::Util.u(trace.user.display_name)}/traces/#{trace.id}"
+              assert_select item, "description" do
+                assert_dom_encoded do
+                  assert_select "img[src='#{trace_icon_url trace.user, trace}']"
+                end
+              end
+              # assert_select item, "dc:creator", trace.user.display_name
+              assert_select item, "pubDate", trace.timestamp.rfc822
+            end
+          end
+        end
+      end
+    end
+  end
+end

test/controllers/traces_controller_test.rb

@@ -30,23 +30,6 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
       { :controller => "traces", :action => "mine", :tag => "tagname" }
     )
 
-    assert_routing(
-      { :path => "/traces/rss", :method => :get },
-      { :controller => "traces", :action => "georss", :format => :rss }
-    )
-    assert_routing(
-      { :path => "/traces/tag/tagname/rss", :method => :get },
-      { :controller => "traces", :action => "georss", :tag => "tagname", :format => :rss }
-    )
-    assert_routing(
-      { :path => "/user/username/traces/rss", :method => :get },
-      { :controller => "traces", :action => "georss", :display_name => "username", :format => :rss }
-    )
-    assert_routing(
-      { :path => "/user/username/traces/tag/tagname/rss", :method => :get },
-      { :controller => "traces", :action => "georss", :display_name => "username", :tag => "tagname", :format => :rss }
-    )
-
     assert_routing(
       { :path => "/user/username/traces/1", :method => :get },
       { :controller => "traces", :action => "show", :display_name => "username", :id => "1" }
@@ -333,61 +316,6 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
     end
   end
 
-  # Check the RSS feed
-  def test_rss
-    user = create(:user)
-    # The fourth test below is surprisingly sensitive to timestamp ordering when the timestamps are equal.
-    trace_a = create(:trace, :visibility => "public", :timestamp => 4.seconds.ago) do |trace|
-      create(:tracetag, :trace => trace, :tag => "London")
-    end
-    trace_b = create(:trace, :visibility => "public", :timestamp => 3.seconds.ago) do |trace|
-      create(:tracetag, :trace => trace, :tag => "Birmingham")
-    end
-    create(:trace, :visibility => "private", :user => user, :timestamp => 2.seconds.ago) do |trace|
-      create(:tracetag, :trace => trace, :tag => "London")
-    end
-    create(:trace, :visibility => "private", :user => user, :timestamp => 1.second.ago) do |trace|
-      create(:tracetag, :trace => trace, :tag => "Birmingham")
-    end
-
-    # First with the public feed
-    get traces_rss_path
-    check_trace_feed [trace_b, trace_a]
-
-    # Restrict traces to those with a given tag
-    get traces_rss_path(:tag => "London")
-    check_trace_feed [trace_a]
-  end
-
-  # Check the RSS feed for a specific user
-  def test_rss_user
-    user = create(:user)
-    second_user = create(:user)
-    create(:user)
-    create(:trace)
-    trace_b = create(:trace, :visibility => "public", :timestamp => 4.seconds.ago, :user => user)
-    trace_c = create(:trace, :visibility => "public", :timestamp => 3.seconds.ago, :user => user) do |trace|
-      create(:tracetag, :trace => trace, :tag => "London")
-    end
-    create(:trace, :visibility => "private")
-
-    # Test a user with no traces
-    get traces_rss_path(:display_name => second_user.display_name)
-    check_trace_feed []
-
-    # Test the user with the traces - should see only public ones
-    get traces_rss_path(:display_name => user.display_name)
-    check_trace_feed [trace_c, trace_b]
-
-    # Should only see traces with the correct tag when a tag is specified
-    get traces_rss_path(:display_name => user.display_name, :tag => "London")
-    check_trace_feed [trace_c]
-
-    # Should no traces if the user does not exist
-    get traces_rss_path(:display_name => "UnknownUser")
-    check_trace_feed []
-  end
-
   # Test showing a trace
   def test_show
     public_trace_file = create(:trace, :visibility => "public")
@@ -721,34 +649,6 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
 
   private
 
-  def check_trace_feed(traces)
-    assert_response :success
-    assert_template "georss"
-    assert_equal "application/rss+xml", @response.media_type
-    assert_select "rss", :count => 1 do
-      assert_select "channel", :count => 1 do
-        assert_select "title"
-        assert_select "description"
-        assert_select "link"
-        assert_select "image"
-        assert_select "item", :count => traces.length do |items|
-          traces.zip(items).each do |trace, item|
-            assert_select item, "title", trace.name
-            assert_select item, "link", "http://www.example.com/user/#{ERB::Util.u(trace.user.display_name)}/traces/#{trace.id}"
-            assert_select item, "guid", "http://www.example.com/user/#{ERB::Util.u(trace.user.display_name)}/traces/#{trace.id}"
-            assert_select item, "description" do
-              assert_dom_encoded do
-                assert_select "img[src='#{trace_icon_url trace.user, trace}']"
-              end
-            end
-            # assert_select item, "dc:creator", trace.user.display_name
-            assert_select item, "pubDate", trace.timestamp.rfc822
-          end
-        end
-      end
-    end
-  end
-
   def check_trace_index(traces)
     assert_response :success
     assert_template "index"