Move current element actions to their own controllers

app/abilities/ability.rb

@@ -4,7 +4,8 @@ class Ability
   include CanCan::Ability
 
   def initialize(user)
-    can [:relation, :way, :node, :query], :browse
+    can :query, :browse
+    can :show, [Node, Way, Relation]
     can [:index, :show], [OldNode, OldWay, OldRelation]
     can [:show, :new], Note
     can :search, :direction

app/controllers/browse_controller.rb

@@ -9,29 +9,5 @@ class BrowseController < ApplicationController
   around_action :web_timeout
   authorize_resource :class => false
 
-  def relation
-    @type = "relation"
-    @feature = Relation.preload(:relation_tags, :containing_relation_members, :changeset => [:changeset_tags, :user], :relation_members => :member).find(params[:id])
-    render "feature"
-  rescue ActiveRecord::RecordNotFound
-    render :action => "not_found", :status => :not_found
-  end
-
-  def way
-    @type = "way"
-    @feature = Way.preload(:way_tags, :containing_relation_members, :changeset => [:changeset_tags, :user], :nodes => [:node_tags, { :ways => :way_tags }]).find(params[:id])
-    render "feature"
-  rescue ActiveRecord::RecordNotFound
-    render :action => "not_found", :status => :not_found
-  end
-
-  def node
-    @type = "node"
-    @feature = Node.preload(:node_tags, :containing_relation_members, :changeset => [:changeset_tags, :user], :ways => :way_tags).find(params[:id])
-    render "feature"
-  rescue ActiveRecord::RecordNotFound
-    render :action => "not_found", :status => :not_found
-  end
-
   def query; end
 end

app/controllers/nodes_controller.rb

@@ -0,0 +1,20 @@
+class NodesController < ApplicationController
+  layout :map_layout
+
+  before_action :authorize_web
+  before_action :set_locale
+  before_action -> { check_database_readable(:need_api => true) }
+  before_action :require_oauth
+
+  authorize_resource
+
+  around_action :web_timeout
+
+  def show
+    @type = "node"
+    @feature = Node.preload(:node_tags, :containing_relation_members, :changeset => [:changeset_tags, :user], :ways => :way_tags).find(params[:id])
+    render "browse/feature"
+  rescue ActiveRecord::RecordNotFound
+    render "browse/not_found", :status => :not_found
+  end
+end

app/controllers/relations_controller.rb

@@ -0,0 +1,20 @@
+class RelationsController < ApplicationController
+  layout :map_layout
+
+  before_action :authorize_web
+  before_action :set_locale
+  before_action -> { check_database_readable(:need_api => true) }
+  before_action :require_oauth
+
+  authorize_resource
+
+  around_action :web_timeout
+
+  def show
+    @type = "relation"
+    @feature = Relation.preload(:relation_tags, :containing_relation_members, :changeset => [:changeset_tags, :user], :relation_members => :member).find(params[:id])
+    render "browse/feature"
+  rescue ActiveRecord::RecordNotFound
+    render "browse/not_found", :status => :not_found
+  end
+end

app/controllers/ways_controller.rb

@@ -0,0 +1,20 @@
+class WaysController < ApplicationController
+  layout :map_layout
+
+  before_action :authorize_web
+  before_action :set_locale
+  before_action -> { check_database_readable(:need_api => true) }
+  before_action :require_oauth
+
+  authorize_resource
+
+  around_action :web_timeout
+
+  def show
+    @type = "way"
+    @feature = Way.preload(:way_tags, :containing_relation_members, :changeset => [:changeset_tags, :user], :nodes => [:node_tags, { :ways => :way_tags }]).find(params[:id])
+    render "browse/feature"
+  rescue ActiveRecord::RecordNotFound
+    render "browse/not_found", :status => :not_found
+  end
+end

app/helpers/geocoder_helper.rb

@@ -3,7 +3,7 @@ module GeocoderHelper
     html_options = { :class => "set_position stretched-link", :data => {} }
 
     url = if result[:type] && result[:id]
-            url_for(:controller => :browse, :action => result[:type], :id => result[:id])
+            url_for(:controller => result[:type].pluralize, :action => :show, :id => result[:id])
           elsif result[:min_lon] && result[:min_lat] && result[:max_lon] && result[:max_lat]
             "/?bbox=#{result[:min_lon]},#{result[:min_lat]},#{result[:max_lon]},#{result[:max_lat]}"
           else

app/views/browse/_relation_member.html.erb

@@ -1,4 +1,4 @@
-<% linked_name = link_to printable_element_name(relation_member.member), { :controller => :browse, :action => relation_member.member_type.downcase, :id => relation_member.member_id.to_s }, { :rel => link_follow(relation_member.member) }
+<% linked_name = link_to printable_element_name(relation_member.member), { :controller => relation_member.member_type.downcase.pluralize, :action => :show, :id => relation_member.member_id.to_s }, { :rel => link_follow(relation_member.member) }
    type_str = t ".type.#{relation_member.member_type.downcase}" %>
 <%= element_list_item relation_member.member_type.downcase, relation_member.member do %>
   <%= if relation_member.member_role.blank?

app/views/browse/_version_actions.erb

@@ -1,5 +1,5 @@
 <div class='secondary-actions'>
-  <%= link_to t("browse.view_details"), :controller => :browse, :action => @type %>
+  <%= link_to t("browse.view_details"), :controller => @type.pluralize, :action => :show %>
   <% if !@feature.redacted? %>
     &middot;
     <%= link_to t("browse.download_xml"), :controller => "api/old_#{@type.pluralize}", :action => :show %>

app/views/browse/feature.html.erb

@@ -2,7 +2,7 @@
 
 <%= render "sidebar_header", :title => t("browse.#{@type}.title_html", :name => printable_element_name(@feature)) %>
 
-<%= render :partial => @type, :object => @feature %>
+<%= render :partial => "browse/#{@type}", :object => @feature %>
 
 <% if @feature.visible? %>
   <div class='secondary-actions'>

app/views/browse/history.html.erb

@@ -7,7 +7,7 @@
 <div class='secondary-actions'>
   <%= link_to t("browse.download_xml"), :controller => "api/old_#{@type.pluralize}", :action => "history" %>
   &middot;
-  <%= link_to t("browse.view_details"), :controller => "browse", :action => @type %>
+  <%= link_to t("browse.view_details"), :controller => @type.pluralize, :action => :show %>
   <% if params[:show_redactions] %>
     &middot;
     <%= link_to t("browse.view_history") %>

config/routes.rb

@@ -109,13 +109,13 @@ OpenStreetMap::Application.routes.draw do
   end
 
   # Data browsing
-  get "/way/:id" => "browse#way", :id => /\d+/, :as => :way
+  get "/way/:id" => "ways#show", :id => /\d+/, :as => :way
   get "/way/:id/history" => "old_ways#index", :id => /\d+/, :as => :way_history
   resources :old_ways, :path => "/way/:id/history", :id => /\d+/, :version => /\d+/, :param => :version, :only => :show
-  get "/node/:id" => "browse#node", :id => /\d+/, :as => :node
+  get "/node/:id" => "nodes#show", :id => /\d+/, :as => :node
   get "/node/:id/history" => "old_nodes#index", :id => /\d+/, :as => :node_history
   resources :old_nodes, :path => "/node/:id/history", :id => /\d+/, :version => /\d+/, :param => :version, :only => :show
-  get "/relation/:id" => "browse#relation", :id => /\d+/, :as => :relation
+  get "/relation/:id" => "relations#show", :id => /\d+/, :as => :relation
   get "/relation/:id/history" => "old_relations#index", :id => /\d+/, :as => :relation_history
   resources :old_relations, :path => "/relation/:id/history", :id => /\d+/, :version => /\d+/, :param => :version, :only => :show
   resources :changesets, :path => "changeset", :id => /\d+/, :only => :show

test/controllers/browse_controller_test.rb

@@ -4,146 +4,15 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
   ##
   # test all routes which lead to this controller
   def test_routes
-    assert_routing(
-      { :path => "/node/1", :method => :get },
-      { :controller => "browse", :action => "node", :id => "1" }
-    )
-    assert_routing(
-      { :path => "/way/1", :method => :get },
-      { :controller => "browse", :action => "way", :id => "1" }
-    )
-    assert_routing(
-      { :path => "/relation/1", :method => :get },
-      { :controller => "browse", :action => "relation", :id => "1" }
-    )
     assert_routing(
       { :path => "/query", :method => :get },
       { :controller => "browse", :action => "query" }
     )
   end
 
-  def test_read_relation
-    relation = create(:relation)
-    sidebar_browse_check :relation_path, relation.id, "browse/feature"
-    assert_select "h4", /^Version/ do
-      assert_select "a[href='#{old_relation_path relation, 1}']", :text => "1", :count => 1
-    end
-    assert_select ".secondary-actions a[href='#{api_relation_path relation}']", :count => 1
-    assert_select ".secondary-actions a[href='#{relation_history_path relation}']", :count => 1
-    assert_select ".secondary-actions a[href='#{old_relation_path relation, 1}']", :count => 0
-  end
-
-  def test_multiple_version_relation_links
-    relation = create(:relation, :with_history, :version => 2)
-    sidebar_browse_check :relation_path, relation.id, "browse/feature"
-    assert_select ".secondary-actions a[href='#{relation_history_path relation}']", :count => 1
-    assert_select ".secondary-actions a[href='#{old_relation_path relation, 1}']", :count => 1
-    assert_select ".secondary-actions a[href='#{old_relation_path relation, 2}']", :count => 1
-  end
-
-  def test_read_way
-    way = create(:way)
-    sidebar_browse_check :way_path, way.id, "browse/feature"
-    assert_select "h4", /^Version/ do
-      assert_select "a[href='#{old_way_path way, 1}']", :text => "1", :count => 1
-    end
-    assert_select ".secondary-actions a[href='#{api_way_path way}']", :count => 1
-    assert_select ".secondary-actions a[href='#{way_history_path way}']", :count => 1
-    assert_select ".secondary-actions a[href='#{old_way_path way, 1}']", :count => 0
-  end
-
-  def test_multiple_version_way_links
-    way = create(:way, :with_history, :version => 2)
-    sidebar_browse_check :way_path, way.id, "browse/feature"
-    assert_select ".secondary-actions a[href='#{way_history_path way}']", :count => 1
-    assert_select ".secondary-actions a[href='#{old_way_path way, 1}']", :count => 1
-    assert_select ".secondary-actions a[href='#{old_way_path way, 2}']", :count => 1
-  end
-
-  def test_read_node
-    node = create(:node)
-    sidebar_browse_check :node_path, node.id, "browse/feature"
-    assert_select "h4", /^Version/ do
-      assert_select "a[href='#{old_node_path node, 1}']", :text => "1", :count => 1
-    end
-    assert_select ".secondary-actions a[href='#{api_node_path node}']", :count => 1
-    assert_select ".secondary-actions a[href='#{node_history_path node}']", :count => 1
-    assert_select ".secondary-actions a[href='#{old_node_path node, 1}']", :count => 0
-  end
-
-  def test_multiple_version_node_links
-    node = create(:node, :with_history, :version => 2)
-    sidebar_browse_check :node_path, node.id, "browse/feature"
-    assert_select ".secondary-actions a[href='#{node_history_path node}']", :count => 1
-    assert_select ".secondary-actions a[href='#{old_node_path node, 1}']", :count => 1
-    assert_select ".secondary-actions a[href='#{old_node_path node, 2}']", :count => 1
-  end
-
-  def test_read_deleted_node
-    node = create(:node, :visible => false)
-    sidebar_browse_check :node_path, node.id, "browse/feature"
-    assert_select "h4", /^Version/ do
-      assert_select "a[href='#{old_node_path node, 1}']", :text => "1", :count => 1
-    end
-    assert_select "a[href='#{api_node_path node}']", :count => 0
-  end
-
-  ##
-  #  Methods to check redaction.
-  #
-  # note that these are presently highly reliant on the structure of the
-  # page for the selection tests, which doesn't work out particularly
-  # well if that structure changes. so... if you change the page layout
-  # then please make it more easily (and robustly) testable!
-  ##
-  def test_redacted_node
-    node = create(:node, :with_history, :deleted, :version => 2)
-    node_v1 = node.old_nodes.find_by(:version => 1)
-    node_v1.redact!(create(:redaction))
-
-    get node_path(:id => node)
-    assert_response :success
-    assert_template "feature"
-
-    # check that we don't show lat/lon for a redacted node.
-    assert_select ".browse-section", 1
-    assert_select ".browse-section.browse-node", 1
-    assert_select ".browse-section.browse-node .latitude", 0
-    assert_select ".browse-section.browse-node .longitude", 0
-  end
-
   def test_query
     get query_path
     assert_response :success
     assert_template "browse/query"
   end
-
-  def test_anonymous_user_feature_page_secondary_actions
-    node = create(:node, :with_history)
-    get node_path(:id => node)
-    assert_response :success
-    assert_select ".secondary-actions a", :text => "View Details", :count => 0
-    assert_select ".secondary-actions a", :text => "View History", :count => 1
-    assert_select ".secondary-actions a", :text => "View Unredacted History", :count => 0
-  end
-
-  def test_regular_user_feature_page_secondary_actions
-    session_for(create(:user))
-    node = create(:node, :with_history)
-    get node_path(:id => node)
-    assert_response :success
-    assert_select ".secondary-actions a", :text => "View Details", :count => 0
-    assert_select ".secondary-actions a", :text => "View History", :count => 1
-    assert_select ".secondary-actions a", :text => "View Unredacted History", :count => 0
-  end
-
-  def test_moderator_user_feature_page_secondary_actions
-    session_for(create(:moderator_user))
-    node = create(:node, :with_history)
-    get node_path(:id => node)
-    assert_response :success
-    assert_select ".secondary-actions a", :text => "View Details", :count => 0
-    assert_select ".secondary-actions a", :text => "View History", :count => 1
-    assert_select ".secondary-actions a", :text => "View Unredacted History", :count => 1
-  end
 end

test/controllers/nodes_controller_test.rb

@@ -0,0 +1,85 @@
+require "test_helper"
+
+class NodesControllerTest < ActionDispatch::IntegrationTest
+  ##
+  # test all routes which lead to this controller
+  def test_routes
+    assert_routing(
+      { :path => "/node/1", :method => :get },
+      { :controller => "nodes", :action => "show", :id => "1" }
+    )
+  end
+
+  def test_show
+    node = create(:node)
+    sidebar_browse_check :node_path, node.id, "browse/feature"
+    assert_select "h4", /^Version/ do
+      assert_select "a[href='#{old_node_path node, 1}']", :text => "1", :count => 1
+    end
+    assert_select ".secondary-actions a[href='#{api_node_path node}']", :count => 1
+    assert_select ".secondary-actions a[href='#{node_history_path node}']", :count => 1
+    assert_select ".secondary-actions a[href='#{old_node_path node, 1}']", :count => 0
+  end
+
+  def test_show_multiple_versions
+    node = create(:node, :with_history, :version => 2)
+    sidebar_browse_check :node_path, node.id, "browse/feature"
+    assert_select ".secondary-actions a[href='#{node_history_path node}']", :count => 1
+    assert_select ".secondary-actions a[href='#{old_node_path node, 1}']", :count => 1
+    assert_select ".secondary-actions a[href='#{old_node_path node, 2}']", :count => 1
+  end
+
+  def test_show_deleted
+    node = create(:node, :visible => false)
+    sidebar_browse_check :node_path, node.id, "browse/feature"
+    assert_select "h4", /^Version/ do
+      assert_select "a[href='#{old_node_path node, 1}']", :text => "1", :count => 1
+    end
+    assert_select "a[href='#{api_node_path node}']", :count => 0
+  end
+
+  def test_show_redacted
+    node = create(:node, :with_history, :deleted, :version => 2)
+    node_v1 = node.old_nodes.find_by(:version => 1)
+    node_v1.redact!(create(:redaction))
+
+    get node_path(node)
+    assert_response :success
+    assert_template "feature"
+
+    # check that we don't show lat/lon for a redacted node.
+    assert_select ".browse-section", 1
+    assert_select ".browse-section.browse-node", 1
+    assert_select ".browse-section.browse-node .latitude", 0
+    assert_select ".browse-section.browse-node .longitude", 0
+  end
+
+  def test_show_secondary_actions_to_anonymous_user
+    node = create(:node, :with_history)
+    get node_path(node)
+    assert_response :success
+    assert_select ".secondary-actions a", :text => "View Details", :count => 0
+    assert_select ".secondary-actions a", :text => "View History", :count => 1
+    assert_select ".secondary-actions a", :text => "View Unredacted History", :count => 0
+  end
+
+  def test_show_secondary_actions_to_regular_user
+    session_for(create(:user))
+    node = create(:node, :with_history)
+    get node_path(node)
+    assert_response :success
+    assert_select ".secondary-actions a", :text => "View Details", :count => 0
+    assert_select ".secondary-actions a", :text => "View History", :count => 1
+    assert_select ".secondary-actions a", :text => "View Unredacted History", :count => 0
+  end
+
+  def test_show_secondary_actions_to_moderator
+    session_for(create(:moderator_user))
+    node = create(:node, :with_history)
+    get node_path(node)
+    assert_response :success
+    assert_select ".secondary-actions a", :text => "View Details", :count => 0
+    assert_select ".secondary-actions a", :text => "View History", :count => 1
+    assert_select ".secondary-actions a", :text => "View Unredacted History", :count => 1
+  end
+end

test/controllers/relations_controller_test.rb

@@ -0,0 +1,31 @@
+require "test_helper"
+
+class RelationsControllerTest < ActionDispatch::IntegrationTest
+  ##
+  # test all routes which lead to this controller
+  def test_routes
+    assert_routing(
+      { :path => "/relation/1", :method => :get },
+      { :controller => "relations", :action => "show", :id => "1" }
+    )
+  end
+
+  def test_show
+    relation = create(:relation)
+    sidebar_browse_check :relation_path, relation.id, "browse/feature"
+    assert_select "h4", /^Version/ do
+      assert_select "a[href='#{old_relation_path relation, 1}']", :text => "1", :count => 1
+    end
+    assert_select ".secondary-actions a[href='#{api_relation_path relation}']", :count => 1
+    assert_select ".secondary-actions a[href='#{relation_history_path relation}']", :count => 1
+    assert_select ".secondary-actions a[href='#{old_relation_path relation, 1}']", :count => 0
+  end
+
+  def test_show_multiple_versions
+    relation = create(:relation, :with_history, :version => 2)
+    sidebar_browse_check :relation_path, relation.id, "browse/feature"
+    assert_select ".secondary-actions a[href='#{relation_history_path relation}']", :count => 1
+    assert_select ".secondary-actions a[href='#{old_relation_path relation, 1}']", :count => 1
+    assert_select ".secondary-actions a[href='#{old_relation_path relation, 2}']", :count => 1
+  end
+end

test/controllers/site_controller_test.rb

@@ -89,13 +89,13 @@ class SiteControllerTest < ActionDispatch::IntegrationTest
   # Test the index page redirects
   def test_index_redirect
     get root_path(:node => 123)
-    assert_redirected_to :controller => :browse, :action => :node, :id => 123
+    assert_redirected_to node_path(123)
 
     get root_path(:way => 123)
-    assert_redirected_to :controller => :browse, :action => :way, :id => 123
+    assert_redirected_to way_path(123)
 
     get root_path(:relation => 123)
-    assert_redirected_to :controller => :browse, :action => :relation, :id => 123
+    assert_redirected_to relation_path(123)
 
     get root_path(:note => 123)
     assert_redirected_to :controller => :notes, :action => :show, :id => 123
@@ -131,16 +131,16 @@ class SiteControllerTest < ActionDispatch::IntegrationTest
     assert_redirected_to :controller => :site, :action => :index, :anchor => "map=3/4.8779296875/3.955078125&layers=T"
 
     get permalink_path(:code => "wBz3--", :node => 1)
-    assert_redirected_to :controller => :browse, :action => :node, :id => 1, :anchor => "map=3/4.8779296875/3.955078125"
+    assert_redirected_to node_path(1, :anchor => "map=3/4.8779296875/3.955078125")
 
     get permalink_path(:code => "wBz3--", :way => 2)
-    assert_redirected_to :controller => :browse, :action => :way, :id => 2, :anchor => "map=3/4.8779296875/3.955078125"
+    assert_redirected_to way_path(2, :anchor => "map=3/4.8779296875/3.955078125")
 
     get permalink_path(:code => "wBz3--", :relation => 3)
-    assert_redirected_to :controller => :browse, :action => :relation, :id => 3, :anchor => "map=3/4.8779296875/3.955078125"
+    assert_redirected_to relation_path(3, :anchor => "map=3/4.8779296875/3.955078125")
 
     get permalink_path(:code => "wBz3--", :changeset => 4)
-    assert_redirected_to changeset_path(:id => 4, :anchor => "map=3/4.8779296875/3.955078125")
+    assert_redirected_to changeset_path(4, :anchor => "map=3/4.8779296875/3.955078125")
   end
 
   # Test the key page

test/controllers/ways_controller_test.rb

@@ -0,0 +1,31 @@
+require "test_helper"
+
+class WaysControllerTest < ActionDispatch::IntegrationTest
+  ##
+  # test all routes which lead to this controller
+  def test_routes
+    assert_routing(
+      { :path => "/way/1", :method => :get },
+      { :controller => "ways", :action => "show", :id => "1" }
+    )
+  end
+
+  def test_show
+    way = create(:way)
+    sidebar_browse_check :way_path, way.id, "browse/feature"
+    assert_select "h4", /^Version/ do
+      assert_select "a[href='#{old_way_path way, 1}']", :text => "1", :count => 1
+    end
+    assert_select ".secondary-actions a[href='#{api_way_path way}']", :count => 1
+    assert_select ".secondary-actions a[href='#{way_history_path way}']", :count => 1
+    assert_select ".secondary-actions a[href='#{old_way_path way, 1}']", :count => 0
+  end
+
+  def test_show_multiple_versions
+    way = create(:way, :with_history, :version => 2)
+    sidebar_browse_check :way_path, way.id, "browse/feature"
+    assert_select ".secondary-actions a[href='#{way_history_path way}']", :count => 1
+    assert_select ".secondary-actions a[href='#{old_way_path way, 1}']", :count => 1
+    assert_select ".secondary-actions a[href='#{old_way_path way, 2}']", :count => 1
+  end
+end