Move the user api methods into a separate controller in the api namespace

app/controllers/api/users_controller.rb

@@ -0,0 +1,66 @@
+module Api
+  class UsersController < ApplicationController
+    layout "site", :except => [:api_details]
+
+    skip_before_action :verify_authenticity_token
+    before_action :disable_terms_redirect, :only => [:api_details]
+    before_action :authorize, :only => [:api_details, :api_gpx_files]
+    before_action :api_deny_access_handler
+
+    authorize_resource
+
+    before_action :check_api_readable
+    around_action :api_call_handle_error
+    before_action :lookup_user_by_id, :only => [:api_read]
+
+    def api_read
+      if @user.visible?
+        render :action => :api_read, :content_type => "text/xml"
+      else
+        head :gone
+      end
+    end
+
+    def api_details
+      @user = current_user
+      render :action => :api_read, :content_type => "text/xml"
+    end
+
+    def api_users
+      raise OSM::APIBadUserInput, "The parameter users is required, and must be of the form users=id[,id[,id...]]" unless params["users"]
+
+      ids = params["users"].split(",").collect(&:to_i)
+
+      raise OSM::APIBadUserInput, "No users were given to search for" if ids.empty?
+
+      @users = User.visible.find(ids)
+
+      render :action => :api_users, :content_type => "text/xml"
+    end
+
+    def api_gpx_files
+      doc = OSM::API.new.get_xml_doc
+      current_user.traces.reload.each do |trace|
+        doc.root << trace.to_xml_node
+      end
+      render :xml => doc.to_s
+    end
+
+    private
+
+    ##
+    # ensure that there is a "user" instance variable
+    def lookup_user_by_id
+      @user = User.find(params[:id])
+    end
+
+    ##
+    #
+    def disable_terms_redirect
+      # this is necessary otherwise going to the user terms page, when
+      # having not agreed already would cause an infinite redirect loop.
+      # it's .now so that this doesn't propagate to other pages.
+      flash.now[:skip_terms] = true
+    end
+  end
+end

app/controllers/users_controller.rb

@@ -1,22 +1,17 @@
 class UsersController < ApplicationController
-  layout "site", :except => [:api_details]
+  layout "site"
 
-  skip_before_action :verify_authenticity_token, :only => [:api_read, :api_users, :api_details, :api_gpx_files, :auth_success]
+  skip_before_action :verify_authenticity_token, :only => [:auth_success]
-  before_action :disable_terms_redirect, :only => [:terms, :save, :logout, :api_details]
+  before_action :disable_terms_redirect, :only => [:terms, :save, :logout]
-  before_action :authorize, :only => [:api_details, :api_gpx_files]
+  before_action :authorize_web
-  before_action :authorize_web, :except => [:api_read, :api_users, :api_details, :api_gpx_files]
+  before_action :set_locale
-  before_action :set_locale, :except => [:api_read, :api_users, :api_details, :api_gpx_files]
-  before_action :api_deny_access_handler, :only => [:api_read, :api_users, :api_details, :api_gpx_files]
 
   authorize_resource
 
   before_action :require_self, :only => [:account]
-  before_action :check_database_readable, :except => [:login, :api_read, :api_users, :api_details, :api_gpx_files]
+  before_action :check_database_readable, :except => [:login]
   before_action :check_database_writable, :only => [:new, :account, :confirm, :confirm_email, :lost_password, :reset_password, :go_public, :make_friend, :remove_friend]
-  before_action :check_api_readable, :only => [:api_read, :api_users, :api_details, :api_gpx_files]
   before_action :require_cookies, :only => [:new, :login, :confirm]
-  around_action :api_call_handle_error, :only => [:api_read, :api_users, :api_details, :api_gpx_files]
-  before_action :lookup_user_by_id, :only => [:api_read]
   before_action :lookup_user_by_name, :only => [:set_status, :delete]
   before_action :allow_thirdparty_images, :only => [:show, :account]
 
@@ -376,39 +371,6 @@ class UsersController < ApplicationController
     end
   end
 
-  def api_read
-    if @user.visible?
-      render :action => :api_read, :content_type => "text/xml"
-    else
-      head :gone
-    end
-  end
-
-  def api_details
-    @user = current_user
-    render :action => :api_read, :content_type => "text/xml"
-  end
-
-  def api_users
-    raise OSM::APIBadUserInput, "The parameter users is required, and must be of the form users=id[,id[,id...]]" unless params["users"]
-
-    ids = params["users"].split(",").collect(&:to_i)
-
-    raise OSM::APIBadUserInput, "No users were given to search for" if ids.empty?
-
-    @users = User.visible.find(ids)
-
-    render :action => :api_users, :content_type => "text/xml"
-  end
-
-  def api_gpx_files
-    doc = OSM::API.new.get_xml_doc
-    current_user.traces.reload.each do |trace|
-      doc.root << trace.to_xml_node
-    end
-    render :xml => doc.to_s
-  end
-
   def show
     @user = User.find_by(:display_name => params[:display_name])
 
@@ -755,12 +717,6 @@ class UsersController < ApplicationController
     head :forbidden if params[:display_name] != current_user.display_name
   end
 
-  ##
-  # ensure that there is a "user" instance variable
-  def lookup_user_by_id
-    @user = User.find(params[:id])
-  end
-
   ##
   # ensure that there is a "user" instance variable
   def lookup_user_by_name

config/routes.rb

@@ -66,10 +66,10 @@ OpenStreetMap::Application.routes.draw do
     get "relations/search" => "api/search#search_relations"
     get "nodes/search" => "api/search#search_nodes"
 
-    get "user/:id" => "users#api_read", :id => /\d+/
+    get "user/:id" => "api/users#api_read", :id => /\d+/
-    get "user/details" => "users#api_details"
+    get "user/details" => "api/users#api_details"
-    get "user/gpx_files" => "users#api_gpx_files"
+    get "user/gpx_files" => "api/users#api_gpx_files"
-    get "users" => "users#api_users", :as => :api_users
+    get "users" => "api/users#api_users", :as => :api_users
 
     get "user/preferences" => "api/user_preferences#read"
     get "user/preferences/:preference_key" => "api/user_preferences#read_one"

test/controllers/api/users_controller.rb

@@ -0,0 +1,199 @@
+require "test_helper"
+
+module Api
+  class UsersControllerTest < ActionController::TestCase
+    def setup
+      stub_hostip_requests
+    end
+
+    ##
+    # test all routes which lead to this controller
+    def test_routes
+      assert_routing(
+        { :path => "/api/0.6/user/1", :method => :get },
+        { :controller => "api/users", :action => "api_read", :id => "1" }
+      )
+      assert_routing(
+        { :path => "/api/0.6/user/details", :method => :get },
+        { :controller => "api/users", :action => "api_details" }
+      )
+      assert_routing(
+        { :path => "/api/0.6/user/gpx_files", :method => :get },
+        { :controller => "api/users", :action => "api_gpx_files" }
+      )
+      assert_routing(
+        { :path => "/api/0.6/users", :method => :get },
+        { :controller => "api/users", :action => "api_users" }
+      )
+    end
+
+    def test_api_read
+      user = create(:user, :description => "test", :terms_agreed => Date.yesterday)
+      # check that a visible user is returned properly
+      get :api_read, :params => { :id => user.id }
+      assert_response :success
+      assert_equal "text/xml", response.content_type
+
+      # check the data that is returned
+      assert_select "description", :count => 1, :text => "test"
+      assert_select "contributor-terms", :count => 1 do
+        assert_select "[agreed='true']"
+      end
+      assert_select "img", :count => 0
+      assert_select "roles", :count => 1 do
+        assert_select "role", :count => 0
+      end
+      assert_select "changesets", :count => 1 do
+        assert_select "[count='0']"
+      end
+      assert_select "traces", :count => 1 do
+        assert_select "[count='0']"
+      end
+      assert_select "blocks", :count => 1 do
+        assert_select "received", :count => 1 do
+          assert_select "[count='0'][active='0']"
+        end
+        assert_select "issued", :count => 0
+      end
+
+      # check that we aren't revealing private information
+      assert_select "contributor-terms[pd]", false
+      assert_select "home", false
+      assert_select "languages", false
+      assert_select "messages", false
+
+      # check that a suspended user is not returned
+      get :api_read, :params => { :id => create(:user, :suspended).id }
+      assert_response :gone
+
+      # check that a deleted user is not returned
+      get :api_read, :params => { :id => create(:user, :deleted).id }
+      assert_response :gone
+
+      # check that a non-existent user is not returned
+      get :api_read, :params => { :id => 0 }
+      assert_response :not_found
+    end
+
+    def test_api_details
+      user = create(:user, :description => "test", :terms_agreed => Date.yesterday, :home_lat => 12.1, :home_lon => 12.1, :languages => ["en"])
+      create(:message, :read, :recipient => user)
+      create(:message, :sender => user)
+
+      # check that nothing is returned when not logged in
+      get :api_details
+      assert_response :unauthorized
+
+      # check that we get a response when logged in
+      basic_authorization user.email, "test"
+      get :api_details
+      assert_response :success
+      assert_equal "text/xml", response.content_type
+
+      # check the data that is returned
+      assert_select "description", :count => 1, :text => "test"
+      assert_select "contributor-terms", :count => 1 do
+        assert_select "[agreed='true'][pd='false']"
+      end
+      assert_select "img", :count => 0
+      assert_select "roles", :count => 1 do
+        assert_select "role", :count => 0
+      end
+      assert_select "changesets", :count => 1 do
+        assert_select "[count='0']", :count => 1
+      end
+      assert_select "traces", :count => 1 do
+        assert_select "[count='0']", :count => 1
+      end
+      assert_select "blocks", :count => 1 do
+        assert_select "received", :count => 1 do
+          assert_select "[count='0'][active='0']"
+        end
+        assert_select "issued", :count => 0
+      end
+      assert_select "home", :count => 1 do
+        assert_select "[lat='12.1'][lon='12.1'][zoom='3']"
+      end
+      assert_select "languages", :count => 1 do
+        assert_select "lang", :count => 1, :text => "en"
+      end
+      assert_select "messages", :count => 1 do
+        assert_select "received", :count => 1 do
+          assert_select "[count='1'][unread='0']"
+        end
+        assert_select "sent", :count => 1 do
+          assert_select "[count='1']"
+        end
+      end
+    end
+
+    def test_api_users
+      user1 = create(:user, :description => "test1", :terms_agreed => Date.yesterday)
+      user2 = create(:user, :description => "test2", :terms_agreed => Date.yesterday)
+      user3 = create(:user, :description => "test3", :terms_agreed => Date.yesterday)
+
+      get :api_users, :params => { :users => user1.id }
+      assert_response :success
+      assert_equal "text/xml", response.content_type
+      assert_select "user", :count => 1 do
+        assert_select "user[id='#{user1.id}']", :count => 1
+        assert_select "user[id='#{user2.id}']", :count => 0
+        assert_select "user[id='#{user3.id}']", :count => 0
+      end
+
+      get :api_users, :params => { :users => user2.id }
+      assert_response :success
+      assert_equal "text/xml", response.content_type
+      assert_select "user", :count => 1 do
+        assert_select "user[id='#{user1.id}']", :count => 0
+        assert_select "user[id='#{user2.id}']", :count => 1
+        assert_select "user[id='#{user3.id}']", :count => 0
+      end
+
+      get :api_users, :params => { :users => "#{user1.id},#{user3.id}" }
+      assert_response :success
+      assert_equal "text/xml", response.content_type
+      assert_select "user", :count => 2 do
+        assert_select "user[id='#{user1.id}']", :count => 1
+        assert_select "user[id='#{user2.id}']", :count => 0
+        assert_select "user[id='#{user3.id}']", :count => 1
+      end
+
+      get :api_users, :params => { :users => create(:user, :suspended).id }
+      assert_response :not_found
+
+      get :api_users, :params => { :users => create(:user, :deleted).id }
+      assert_response :not_found
+
+      get :api_users, :params => { :users => 0 }
+      assert_response :not_found
+    end
+
+    def test_api_gpx_files
+      user = create(:user)
+      trace1 = create(:trace, :user => user) do |trace|
+        create(:tracetag, :trace => trace, :tag => "London")
+      end
+      trace2 = create(:trace, :user => user) do |trace|
+        create(:tracetag, :trace => trace, :tag => "Birmingham")
+      end
+      # check that nothing is returned when not logged in
+      get :api_gpx_files
+      assert_response :unauthorized
+
+      # check that we get a response when logged in
+      basic_authorization user.email, "test"
+      get :api_gpx_files
+      assert_response :success
+      assert_equal "application/xml", response.content_type
+
+      # check the data that is returned
+      assert_select "gpx_file[id='#{trace1.id}']", 1 do
+        assert_select "tag", "London"
+      end
+      assert_select "gpx_file[id='#{trace2.id}']", 1 do
+        assert_select "tag", "Birmingham"
+      end
+    end
+  end
+end

test/controllers/users_controller_test.rb

@@ -8,23 +8,6 @@ class UsersControllerTest < ActionController::TestCase
   ##
   # test all routes which lead to this controller
   def test_routes
-    assert_routing(
-      { :path => "/api/0.6/user/1", :method => :get },
-      { :controller => "users", :action => "api_read", :id => "1" }
-    )
-    assert_routing(
-      { :path => "/api/0.6/user/details", :method => :get },
-      { :controller => "users", :action => "api_details" }
-    )
-    assert_routing(
-      { :path => "/api/0.6/user/gpx_files", :method => :get },
-      { :controller => "users", :action => "api_gpx_files" }
-    )
-    assert_routing(
-      { :path => "/api/0.6/users", :method => :get },
-      { :controller => "users", :action => "api_users" }
-    )
-
     assert_routing(
       { :path => "/login", :method => :get },
       { :controller => "users", :action => "login" }
@@ -1123,175 +1106,6 @@ class UsersControllerTest < ActionController::TestCase
     end
   end
 
-  def test_api_read
-    user = create(:user, :description => "test", :terms_agreed => Date.yesterday)
-    # check that a visible user is returned properly
-    get :api_read, :params => { :id => user.id }
-    assert_response :success
-    assert_equal "text/xml", response.content_type
-
-    # check the data that is returned
-    assert_select "description", :count => 1, :text => "test"
-    assert_select "contributor-terms", :count => 1 do
-      assert_select "[agreed='true']"
-    end
-    assert_select "img", :count => 0
-    assert_select "roles", :count => 1 do
-      assert_select "role", :count => 0
-    end
-    assert_select "changesets", :count => 1 do
-      assert_select "[count='0']"
-    end
-    assert_select "traces", :count => 1 do
-      assert_select "[count='0']"
-    end
-    assert_select "blocks", :count => 1 do
-      assert_select "received", :count => 1 do
-        assert_select "[count='0'][active='0']"
-      end
-      assert_select "issued", :count => 0
-    end
-
-    # check that we aren't revealing private information
-    assert_select "contributor-terms[pd]", false
-    assert_select "home", false
-    assert_select "languages", false
-    assert_select "messages", false
-
-    # check that a suspended user is not returned
-    get :api_read, :params => { :id => create(:user, :suspended).id }
-    assert_response :gone
-
-    # check that a deleted user is not returned
-    get :api_read, :params => { :id => create(:user, :deleted).id }
-    assert_response :gone
-
-    # check that a non-existent user is not returned
-    get :api_read, :params => { :id => 0 }
-    assert_response :not_found
-  end
-
-  def test_api_details
-    user = create(:user, :description => "test", :terms_agreed => Date.yesterday, :home_lat => 12.1, :home_lon => 12.1, :languages => ["en"])
-    create(:message, :read, :recipient => user)
-    create(:message, :sender => user)
-
-    # check that nothing is returned when not logged in
-    get :api_details
-    assert_response :unauthorized
-
-    # check that we get a response when logged in
-    basic_authorization user.email, "test"
-    get :api_details
-    assert_response :success
-    assert_equal "text/xml", response.content_type
-
-    # check the data that is returned
-    assert_select "description", :count => 1, :text => "test"
-    assert_select "contributor-terms", :count => 1 do
-      assert_select "[agreed='true'][pd='false']"
-    end
-    assert_select "img", :count => 0
-    assert_select "roles", :count => 1 do
-      assert_select "role", :count => 0
-    end
-    assert_select "changesets", :count => 1 do
-      assert_select "[count='0']", :count => 1
-    end
-    assert_select "traces", :count => 1 do
-      assert_select "[count='0']", :count => 1
-    end
-    assert_select "blocks", :count => 1 do
-      assert_select "received", :count => 1 do
-        assert_select "[count='0'][active='0']"
-      end
-      assert_select "issued", :count => 0
-    end
-    assert_select "home", :count => 1 do
-      assert_select "[lat='12.1'][lon='12.1'][zoom='3']"
-    end
-    assert_select "languages", :count => 1 do
-      assert_select "lang", :count => 1, :text => "en"
-    end
-    assert_select "messages", :count => 1 do
-      assert_select "received", :count => 1 do
-        assert_select "[count='1'][unread='0']"
-      end
-      assert_select "sent", :count => 1 do
-        assert_select "[count='1']"
-      end
-    end
-  end
-
-  def test_api_users
-    user1 = create(:user, :description => "test1", :terms_agreed => Date.yesterday)
-    user2 = create(:user, :description => "test2", :terms_agreed => Date.yesterday)
-    user3 = create(:user, :description => "test3", :terms_agreed => Date.yesterday)
-
-    get :api_users, :params => { :users => user1.id }
-    assert_response :success
-    assert_equal "text/xml", response.content_type
-    assert_select "user", :count => 1 do
-      assert_select "user[id='#{user1.id}']", :count => 1
-      assert_select "user[id='#{user2.id}']", :count => 0
-      assert_select "user[id='#{user3.id}']", :count => 0
-    end
-
-    get :api_users, :params => { :users => user2.id }
-    assert_response :success
-    assert_equal "text/xml", response.content_type
-    assert_select "user", :count => 1 do
-      assert_select "user[id='#{user1.id}']", :count => 0
-      assert_select "user[id='#{user2.id}']", :count => 1
-      assert_select "user[id='#{user3.id}']", :count => 0
-    end
-
-    get :api_users, :params => { :users => "#{user1.id},#{user3.id}" }
-    assert_response :success
-    assert_equal "text/xml", response.content_type
-    assert_select "user", :count => 2 do
-      assert_select "user[id='#{user1.id}']", :count => 1
-      assert_select "user[id='#{user2.id}']", :count => 0
-      assert_select "user[id='#{user3.id}']", :count => 1
-    end
-
-    get :api_users, :params => { :users => create(:user, :suspended).id }
-    assert_response :not_found
-
-    get :api_users, :params => { :users => create(:user, :deleted).id }
-    assert_response :not_found
-
-    get :api_users, :params => { :users => 0 }
-    assert_response :not_found
-  end
-
-  def test_api_gpx_files
-    user = create(:user)
-    trace1 = create(:trace, :user => user) do |trace|
-      create(:tracetag, :trace => trace, :tag => "London")
-    end
-    trace2 = create(:trace, :user => user) do |trace|
-      create(:tracetag, :trace => trace, :tag => "Birmingham")
-    end
-    # check that nothing is returned when not logged in
-    get :api_gpx_files
-    assert_response :unauthorized
-
-    # check that we get a response when logged in
-    basic_authorization user.email, "test"
-    get :api_gpx_files
-    assert_response :success
-    assert_equal "application/xml", response.content_type
-
-    # check the data that is returned
-    assert_select "gpx_file[id='#{trace1.id}']", 1 do
-      assert_select "tag", "London"
-    end
-    assert_select "gpx_file[id='#{trace2.id}']", 1 do
-      assert_select "tag", "Birmingham"
-    end
-  end
-
   def test_make_friend
     # Get users to work with
     user = create(:user)