From 54dec279495bdd91b841f709f817234aa943fd21 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Fri, 15 Mar 2024 03:24:45 +0300 Subject: [PATCH] Move current element actions to their own controllers --- app/abilities/ability.rb | 3 +- app/controllers/browse_controller.rb | 24 ---- app/controllers/nodes_controller.rb | 20 +++ app/controllers/relations_controller.rb | 20 +++ app/controllers/ways_controller.rb | 20 +++ app/helpers/geocoder_helper.rb | 2 +- app/views/browse/_relation_member.html.erb | 2 +- app/views/browse/_version_actions.erb | 2 +- app/views/browse/feature.html.erb | 2 +- app/views/browse/history.html.erb | 2 +- config/routes.rb | 6 +- test/controllers/browse_controller_test.rb | 131 ------------------ test/controllers/nodes_controller_test.rb | 85 ++++++++++++ test/controllers/relations_controller_test.rb | 31 +++++ test/controllers/site_controller_test.rb | 14 +- test/controllers/ways_controller_test.rb | 31 +++++ 16 files changed, 224 insertions(+), 171 deletions(-) create mode 100644 app/controllers/nodes_controller.rb create mode 100644 app/controllers/relations_controller.rb create mode 100644 app/controllers/ways_controller.rb create mode 100644 test/controllers/nodes_controller_test.rb create mode 100644 test/controllers/relations_controller_test.rb create mode 100644 test/controllers/ways_controller_test.rb diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index 3802fb826..b43cc6b29 100644 --- a/app/abilities/ability.rb +++ b/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 diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb index f69acc158..a3e65a1b7 100644 --- a/app/controllers/browse_controller.rb +++ b/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 diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb new file mode 100644 index 000000000..a05fa8cd2 --- /dev/null +++ b/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 diff --git a/app/controllers/relations_controller.rb b/app/controllers/relations_controller.rb new file mode 100644 index 000000000..8aeefd6cc --- /dev/null +++ b/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 diff --git a/app/controllers/ways_controller.rb b/app/controllers/ways_controller.rb new file mode 100644 index 000000000..d4abe2db7 --- /dev/null +++ b/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 diff --git a/app/helpers/geocoder_helper.rb b/app/helpers/geocoder_helper.rb index f819c1ac5..882f2c835 100644 --- a/app/helpers/geocoder_helper.rb +++ b/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 diff --git a/app/views/browse/_relation_member.html.erb b/app/views/browse/_relation_member.html.erb index 16842f569..7ea75a106 100644 --- a/app/views/browse/_relation_member.html.erb +++ b/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? diff --git a/app/views/browse/_version_actions.erb b/app/views/browse/_version_actions.erb index 63cadeb3f..94e18e457 100644 --- a/app/views/browse/_version_actions.erb +++ b/app/views/browse/_version_actions.erb @@ -1,5 +1,5 @@
- <%= link_to t("browse.view_details"), :controller => :browse, :action => @type %> + <%= link_to t("browse.view_details"), :controller => @type.pluralize, :action => :show %> <% if !@feature.redacted? %> · <%= link_to t("browse.download_xml"), :controller => "api/old_#{@type.pluralize}", :action => :show %> diff --git a/app/views/browse/feature.html.erb b/app/views/browse/feature.html.erb index 7f96b74b9..29f911f79 100644 --- a/app/views/browse/feature.html.erb +++ b/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? %>
diff --git a/app/views/browse/history.html.erb b/app/views/browse/history.html.erb index d257d25a2..1595df021 100644 --- a/app/views/browse/history.html.erb +++ b/app/views/browse/history.html.erb @@ -7,7 +7,7 @@
<%= link_to t("browse.download_xml"), :controller => "api/old_#{@type.pluralize}", :action => "history" %> · - <%= 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] %> · <%= link_to t("browse.view_history") %> diff --git a/config/routes.rb b/config/routes.rb index 819baf65a..63e58a075 100644 --- a/config/routes.rb +++ b/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 diff --git a/test/controllers/browse_controller_test.rb b/test/controllers/browse_controller_test.rb index 4bdad7a4d..1844dcc6c 100644 --- a/test/controllers/browse_controller_test.rb +++ b/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 diff --git a/test/controllers/nodes_controller_test.rb b/test/controllers/nodes_controller_test.rb new file mode 100644 index 000000000..cd1b61f52 --- /dev/null +++ b/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 diff --git a/test/controllers/relations_controller_test.rb b/test/controllers/relations_controller_test.rb new file mode 100644 index 000000000..cbaa51c05 --- /dev/null +++ b/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 diff --git a/test/controllers/site_controller_test.rb b/test/controllers/site_controller_test.rb index f5ff74064..5bd8df18d 100644 --- a/test/controllers/site_controller_test.rb +++ b/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 diff --git a/test/controllers/ways_controller_test.rb b/test/controllers/ways_controller_test.rb new file mode 100644 index 000000000..6c3c5a8a3 --- /dev/null +++ b/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 -- 2.43.2