Move changeset show action to changesets controller

app/abilities/ability.rb

@@ -4,8 +4,7 @@ class Ability
   include CanCan::Ability
 
   def initialize(user)
-    can [:relation, :relation_history, :way, :way_history, :node, :node_history,
-         :changeset, :query], :browse
+    can [:relation, :relation_history, :way, :way_history, :node, :node_history, :query], :browse
     can [:show], OldNode
     can [:show], OldWay
     can [:show], OldRelation
@@ -17,7 +16,7 @@ class Ability
     can [:token, :request_token, :access_token, :test_request], :oauth
 
     if Settings.status != "database_offline"
-      can [:index, :feed], Changeset
+      can [:index, :feed, :show], Changeset
       can :index, ChangesetComment
       can [:confirm, :confirm_resend, :confirm_email], :confirmation
       can [:index, :rss, :show, :comments], DiaryEntry

app/controllers/browse_controller.rb

@@ -57,24 +57,5 @@ class BrowseController < ApplicationController
     render :action => "not_found", :status => :not_found
   end
 
-  def changeset
-    @type = "changeset"
-    @changeset = Changeset.find(params[:id])
-    @comments = if current_user&.moderator?
-                  @changeset.comments.unscope(:where => :visible).includes(:author)
-                else
-                  @changeset.comments.includes(:author)
-                end
-    @node_pages, @nodes = paginate(:old_nodes, :conditions => { :changeset_id => @changeset.id }, :per_page => 20, :parameter => "node_page")
-    @way_pages, @ways = paginate(:old_ways, :conditions => { :changeset_id => @changeset.id }, :per_page => 20, :parameter => "way_page")
-    @relation_pages, @relations = paginate(:old_relations, :conditions => { :changeset_id => @changeset.id }, :per_page => 20, :parameter => "relation_page")
-    if @changeset.user.active? && @changeset.user.data_public?
-      @next_by_user = @changeset.user.changesets.where("id > ?", @changeset.id).reorder(:id => :asc).first
-      @prev_by_user = @changeset.user.changesets.where("id < ?", @changeset.id).reorder(:id => :desc).first
-    end
-  rescue ActiveRecord::RecordNotFound
-    render :action => "not_found", :status => :not_found
-  end
-
   def query; end
 end

app/controllers/changesets_controller.rb

@@ -8,7 +8,8 @@ class ChangesetsController < ApplicationController
 
   before_action :authorize_web
   before_action :set_locale
-  before_action -> { check_database_readable(:need_api => true) }, :only => [:index, :feed]
+  before_action -> { check_database_readable(:need_api => true) }, :only => [:index, :feed, :show]
+  before_action :require_oauth, :only => :show
   before_action :check_database_writable, :only => [:subscribe, :unsubscribe]
 
   authorize_resource
@@ -75,6 +76,26 @@ class ChangesetsController < ApplicationController
     index
   end
 
+  def show
+    @type = "changeset"
+    @changeset = Changeset.find(params[:id])
+    @comments = if current_user&.moderator?
+                  @changeset.comments.unscope(:where => :visible).includes(:author)
+                else
+                  @changeset.comments.includes(:author)
+                end
+    @node_pages, @nodes = paginate(:old_nodes, :conditions => { :changeset_id => @changeset.id }, :per_page => 20, :parameter => "node_page")
+    @way_pages, @ways = paginate(:old_ways, :conditions => { :changeset_id => @changeset.id }, :per_page => 20, :parameter => "way_page")
+    @relation_pages, @relations = paginate(:old_relations, :conditions => { :changeset_id => @changeset.id }, :per_page => 20, :parameter => "relation_page")
+    if @changeset.user.active? && @changeset.user.data_public?
+      @next_by_user = @changeset.user.changesets.where("id > ?", @changeset.id).reorder(:id => :asc).first
+      @prev_by_user = @changeset.user.changesets.where("id < ?", @changeset.id).reorder(:id => :desc).first
+    end
+    render :layout => map_layout
+  rescue ActiveRecord::RecordNotFound
+    render :template => "browse/not_found", :status => :not_found, :layout => map_layout
+  end
+
   ##
   # subscribe to a changeset
   def subscribe

app/views/changeset_comments/_comments.rss.builder

@@ -2,8 +2,8 @@ comments.each do |comment|
   xml.item do
     xml.title t(".comment", :author => comment.author.display_name, :changeset_id => comment.changeset.id.to_s)
 
-    xml.link url_for(:controller => "browse", :action => "changeset", :id => comment.changeset.id, :anchor => "c#{comment.id}", :only_path => false)
-    xml.guid url_for(:controller => "browse", :action => "changeset", :id => comment.changeset.id, :anchor => "c#{comment.id}", :only_path => false)
+    xml.link changeset_url(comment.changeset, :anchor => "c#{comment.id}")
+    xml.guid changeset_url(comment.changeset, :anchor => "c#{comment.id}")
 
     xml.description do
       xml.cdata! render(:partial => "comment", :object => comment, :formats => [:html])

app/views/changesets/show.html.erb

@@ -8,7 +8,7 @@
   </p>
   <p class="details"><%= changeset_details(@changeset) %></p>
 
-  <%= render :partial => "tag_details", :object => @changeset.tags.except("comment") %>
+  <%= render :partial => "browse/tag_details", :object => @changeset.tags.except("comment") %>
 
   <div class="row">
     <div class="col">

app/views/dashboards/_contact.html.erb

@@ -25,9 +25,7 @@
       <% if changeset %>
         <%= t(".latest_edit_html", :ago => friendly_date_ago(changeset.created_at)) %>
         <% comment = changeset.tags["comment"].to_s == "" ? t("browse.no_comment") : changeset.tags["comment"] %>
-        <q><%= link_to(comment,
-                       { :controller => "browse", :action => "changeset", :id => changeset.id },
-                       { :title => t("changesets.changeset.view_changeset_details") }) %></q>
+        <q><%= link_to comment, changeset_path(changeset), :title => t("changesets.changeset.view_changeset_details") %></q>
       <% else %>
        <%= t "changesets.changeset.no_edits" %>
       <% end %>

config/locales/en.yml

@@ -334,7 +334,6 @@ en:
     common_details:
       coordinates_html: "%{latitude}, %{longitude}"
     changeset:
-      title: "Changeset: %{id}"
       belongs_to: "Author"
       node: "Nodes (%{count})"
       node_paginated: "Nodes (%{x}-%{y} of %{count})"
@@ -342,13 +341,6 @@ en:
       way_paginated: "Ways (%{x}-%{y} of %{count})"
       relation: "Relations (%{count})"
       relation_paginated: "Relations (%{x}-%{y} of %{count})"
-      hidden_comment_by_html: "Hidden comment from %{user} %{time_ago}"
-      comment_by_html: "Comment from %{user} %{time_ago}"
-      changesetxml: "Changeset XML"
-      osmchangexml: "osmChange XML"
-      join_discussion: "Log in to join the discussion"
-      discussion: Discussion
-      still_open: "Changeset still open - discussion will open once the changeset is closed."
     node:
       title_html: "Node: %{name}"
       history_title_html: "Node History: %{name}"
@@ -477,6 +469,15 @@ en:
       title: "No such changeset"
       heading: "No entry with the id: %{id}"
       body: "Sorry, there is no changeset with the id %{id}. Please check your spelling, or maybe the link you clicked is wrong."
+    show:
+      title: "Changeset: %{id}"
+      discussion: Discussion
+      join_discussion: "Log in to join the discussion"
+      still_open: "Changeset still open - discussion will open once the changeset is closed."
+      comment_by_html: "Comment from %{user} %{time_ago}"
+      hidden_comment_by_html: "Hidden comment from %{user} %{time_ago}"
+      changesetxml: "Changeset XML"
+      osmchangexml: "osmChange XML"
     timeout:
       sorry: "Sorry, the list of changesets you requested took too long to retrieve."
   changeset_comments:

config/routes.rb

@@ -118,7 +118,7 @@ OpenStreetMap::Application.routes.draw do
   get "/relation/:id" => "browse#relation", :id => /\d+/, :as => :relation
   get "/relation/:id/history" => "browse#relation_history", :id => /\d+/, :as => :relation_history
   resources :old_relations, :path => "/relation/:id/history", :id => /\d+/, :version => /\d+/, :param => :version, :only => :show
-  get "/changeset/:id" => "browse#changeset", :as => :changeset, :id => /\d+/
+  resources :changesets, :path => "changeset", :id => /\d+/, :only => :show
   get "/changeset/:id/comments/feed" => "changeset_comments#index", :as => :changeset_comments_feed, :id => /\d*/, :defaults => { :format => "rss" }
   resources :notes, :path => "note", :only => [:show, :new]
 

test/controllers/browse_controller_test.rb

@@ -28,10 +28,6 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
       { :path => "/relation/1/history", :method => :get },
       { :controller => "browse", :action => "relation_history", :id => "1" }
     )
-    assert_routing(
-      { :path => "/changeset/1", :method => :get },
-      { :controller => "browse", :action => "changeset", :id => "1" }
-    )
     assert_routing(
       { :path => "/query", :method => :get },
       { :controller => "browse", :action => "query" }
@@ -40,7 +36,7 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
 
   def test_read_relation
     relation = create(:relation)
-    browse_check :relation_path, relation.id, "browse/feature"
+    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
@@ -51,7 +47,7 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
 
   def test_multiple_version_relation_links
     relation = create(:relation, :with_history, :version => 2)
-    browse_check :relation_path, relation.id, "browse/feature"
+    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
@@ -59,7 +55,7 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
 
   def test_read_relation_history
     relation = create(:relation, :with_history)
-    browse_check :relation_history_path, relation.id, "browse/history"
+    sidebar_browse_check :relation_history_path, relation.id, "browse/history"
     assert_select "h4", /^Version/ do
       assert_select "a[href='#{old_relation_path relation, 1}']", :text => "1", :count => 1
     end
@@ -67,7 +63,7 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
 
   def test_read_way
     way = create(:way)
-    browse_check :way_path, way.id, "browse/feature"
+    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
@@ -78,7 +74,7 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
 
   def test_multiple_version_way_links
     way = create(:way, :with_history, :version => 2)
-    browse_check :way_path, way.id, "browse/feature"
+    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
@@ -86,7 +82,7 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
 
   def test_read_way_history
     way = create(:way, :with_history)
-    browse_check :way_history_path, way.id, "browse/history"
+    sidebar_browse_check :way_history_path, way.id, "browse/history"
     assert_select "h4", /^Version/ do
       assert_select "a[href='#{old_way_path way, 1}']", :text => "1", :count => 1
     end
@@ -94,7 +90,7 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
 
   def test_read_node
     node = create(:node)
-    browse_check :node_path, node.id, "browse/feature"
+    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
@@ -105,7 +101,7 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
 
   def test_multiple_version_node_links
     node = create(:node, :with_history, :version => 2)
-    browse_check :node_path, node.id, "browse/feature"
+    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
@@ -113,7 +109,7 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
 
   def test_read_deleted_node
     node = create(:node, :visible => false)
-    browse_check :node_path, node.id, "browse/feature"
+    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
@@ -122,35 +118,12 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
 
   def test_read_node_history
     node = create(:node, :with_history)
-    browse_check :node_history_path, node.id, "browse/history"
+    sidebar_browse_check :node_history_path, node.id, "browse/history"
     assert_select "h4", /^Version/ do
       assert_select "a[href='#{old_node_path node, 1}']", :text => "1", :count => 1
     end
   end
 
-  def test_read_changeset
-    user = create(:user)
-    changeset = create(:changeset, :user => user)
-    create(:changeset, :user => user)
-    browse_check :changeset_path, changeset.id, "browse/changeset"
-  end
-
-  def test_read_private_changeset
-    user = create(:user)
-    changeset = create(:changeset, :user => create(:user, :data_public => false))
-    create(:changeset, :user => user)
-    browse_check :changeset_path, changeset.id, "browse/changeset"
-  end
-
-  def test_read_changeset_element_links
-    changeset = create(:changeset)
-    node = create(:node, :with_history, :changeset => changeset)
-
-    browse_check :changeset_path, changeset.id, "browse/changeset"
-    assert_dom "a[href='#{node_path node}']", :count => 1
-    assert_dom "a[href='#{old_node_path node, 1}']", :count => 1
-  end
-
   ##
   #  Methods to check redaction.
   #
@@ -234,42 +207,4 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert_template "browse/query"
   end
-
-  private
-
-  # This is a convenience method for most of the above checks
-  # First we check that when we don't have an id, it will correctly return a 404
-  # then we check that we get the correct 404 when a non-existant id is passed
-  # then we check that it will get a successful response, when we do pass an id
-  def browse_check(path, id, template)
-    path_method = method(path)
-
-    assert_raise ActionController::UrlGenerationError do
-      get path_method.call
-    end
-
-    assert_raise ActionController::UrlGenerationError do
-      get path_method.call(:id => -10) # we won't have an id that's negative
-    end
-
-    get path_method.call(:id => 0)
-    assert_response :not_found
-    assert_template "browse/not_found"
-    assert_template :layout => "map"
-
-    get path_method.call(:id => 0), :xhr => true
-    assert_response :not_found
-    assert_template "browse/not_found"
-    assert_template :layout => "xhr"
-
-    get path_method.call(:id => id)
-    assert_response :success
-    assert_template template
-    assert_template :layout => "map"
-
-    get path_method.call(:id => id), :xhr => true
-    assert_response :success
-    assert_template template
-    assert_template :layout => "xhr"
-  end
 end

test/controllers/changesets_controller_test.rb

@@ -4,6 +4,10 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest
   ##
   # test all routes which lead to this controller
   def test_routes
+    assert_routing(
+      { :path => "/changeset/1", :method => :get },
+      { :controller => "changesets", :action => "show", :id => "1" }
+    )
     assert_routing(
       { :path => "/user/name/history", :method => :get },
       { :controller => "changesets", :action => "index", :display_name => "name" }
@@ -252,6 +256,37 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
   end
 
+  def test_show
+    changeset = create(:changeset)
+    create(:changeset_tag, :changeset => changeset, :k => "comment", :v => "tested-changeset-comment")
+    commenting_user = create(:user)
+    changeset_comment = create(:changeset_comment, :changeset => changeset, :author => commenting_user, :body => "Unwanted comment")
+
+    sidebar_browse_check :changeset_path, changeset.id, "changesets/show"
+    assert_dom "h2", :text => "Changeset: #{changeset.id}"
+    assert_dom "p", :text => "tested-changeset-comment"
+    assert_dom "li#c#{changeset_comment.id}" do
+      assert_dom "> small", :text => /^Comment from #{commenting_user.display_name}/
+    end
+  end
+
+  def test_show_private_changeset
+    user = create(:user)
+    changeset = create(:changeset, :user => create(:user, :data_public => false))
+    create(:changeset, :user => user)
+
+    sidebar_browse_check :changeset_path, changeset.id, "changesets/show"
+  end
+
+  def test_show_element_links
+    changeset = create(:changeset)
+    node = create(:node, :with_history, :changeset => changeset)
+
+    sidebar_browse_check :changeset_path, changeset.id, "changesets/show"
+    assert_dom "a[href='#{node_path node}']", :count => 1
+    assert_dom "a[href='#{old_node_path node, 1}']", :count => 1
+  end
+
   ##
   # This should display the last 20 non-empty changesets
   def test_feed

test/controllers/site_controller_test.rb

@@ -147,7 +147,7 @@ class SiteControllerTest < ActionDispatch::IntegrationTest
 
     get permalink_path(:code => "wBz3--", :changeset => 4)
     assert_response :redirect
-    assert_redirected_to :controller => :browse, :action => :changeset, :id => 4, :anchor => "map=3/4.8779296875/3.955078125"
+    assert_redirected_to changeset_path(:id => 4, :anchor => "map=3/4.8779296875/3.955078125")
   end
 
   # Test the key page

test/test_helper.rb

@@ -383,5 +383,41 @@ module ActiveSupport
     ensure
       unfreeze_time
     end
+
+    # This is a convenience method for checks of resources rendered in a map view sidebar
+    # First we check that when we don't have an id, it will correctly return a 404
+    # then we check that we get the correct 404 when a non-existant id is passed
+    # then we check that it will get a successful response, when we do pass an id
+    def sidebar_browse_check(path, id, template)
+      path_method = method(path)
+
+      assert_raise ActionController::UrlGenerationError do
+        get path_method.call
+      end
+
+      assert_raise ActionController::UrlGenerationError do
+        get path_method.call(:id => -10) # we won't have an id that's negative
+      end
+
+      get path_method.call(:id => 0)
+      assert_response :not_found
+      assert_template "browse/not_found"
+      assert_template :layout => "map"
+
+      get path_method.call(:id => 0), :xhr => true
+      assert_response :not_found
+      assert_template "browse/not_found"
+      assert_template :layout => "xhr"
+
+      get path_method.call(:id => id)
+      assert_response :success
+      assert_template template
+      assert_template :layout => "map"
+
+      get path_method.call(:id => id), :xhr => true
+      assert_response :success
+      assert_template template
+      assert_template :layout => "xhr"
+    end
   end
 end