From e66c7192949e46480c55f3d74a5b38da95532db3 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 10 Apr 2019 17:06:15 +0200 Subject: [PATCH] Resourceful(ish) naming for api/users_controller --- app/abilities/api_ability.rb | 4 +- app/abilities/api_capability.rb | 4 +- app/controllers/api/users_controller.rb | 20 ++++----- app/views/api/users/_api_user.builder | 44 ------------------- app/views/api/users/_user.builder | 44 +++++++++++++++++++ .../{api_users.builder => index.builder} | 2 +- .../users/{api_read.builder => show.builder} | 2 +- config/routes.rb | 8 ++-- test/controllers/api/users_controller_test.rb | 44 +++++++++---------- 9 files changed, 86 insertions(+), 86 deletions(-) delete mode 100644 app/views/api/users/_api_user.builder create mode 100644 app/views/api/users/_user.builder rename app/views/api/users/{api_users.builder => index.builder} (59%) rename app/views/api/users/{api_read.builder => show.builder} (61%) diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb index 074a7f95d..a71d05d75 100644 --- a/app/abilities/api_ability.rb +++ b/app/abilities/api_ability.rb @@ -15,7 +15,7 @@ class ApiAbility can [:show, :download, :query], Changeset can [:index, :create, :comment, :feed, :show, :search], Note can :index, Tracepoint - can [:api_users, :api_read], User + can [:index, :show], User can [:index, :show], Node can [:index, :show, :full, :ways_for_node], Way can [:index, :show, :full, :relations_for_node, :relations_for_way, :relations_for_relation], Relation @@ -34,7 +34,7 @@ class ApiAbility can [:close, :reopen], Note can [:new, :create], Report can [:create, :show, :update, :destroy, :data], Trace - can [:api_details, :api_gpx_files], User + can [:details, :gpx_files], User can [:read, :read_one, :update, :update_one, :delete_one], UserPreference if user.terms_agreed? diff --git a/app/abilities/api_capability.rb b/app/abilities/api_capability.rb index c3544136b..7d8a13364 100644 --- a/app/abilities/api_capability.rb +++ b/app/abilities/api_capability.rb @@ -8,8 +8,8 @@ class ApiCapability can [:create, :comment, :close, :reopen], Note if capability?(token, :allow_write_notes) can [:show, :data], Trace if capability?(token, :allow_read_gpx) can [:create, :update, :destroy], Trace if capability?(token, :allow_write_gpx) - can [:api_details], User if capability?(token, :allow_read_prefs) - can [:api_gpx_files], User if capability?(token, :allow_read_gpx) + can [:details], User if capability?(token, :allow_read_prefs) + can [:gpx_files], User if capability?(token, :allow_read_gpx) can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs) can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs) diff --git a/app/controllers/api/users_controller.rb b/app/controllers/api/users_controller.rb index f24d50cf1..3180cabcc 100644 --- a/app/controllers/api/users_controller.rb +++ b/app/controllers/api/users_controller.rb @@ -2,29 +2,29 @@ module Api class UsersController < ApiController layout "site", :except => [:api_details] - before_action :disable_terms_redirect, :only => [:api_details] - before_action :authorize, :only => [:api_details, :api_gpx_files] + before_action :disable_terms_redirect, :only => [:details] + before_action :authorize, :only => [:details, :gpx_files] authorize_resource before_action :check_api_readable around_action :api_call_handle_error - before_action :lookup_user_by_id, :only => [:api_read] + before_action :lookup_user_by_id, :only => [:show] - def api_read + def show if @user.visible? - render :action => :api_read, :content_type => "text/xml" + render :action => :show, :content_type => "text/xml" else head :gone end end - def api_details + def details @user = current_user - render :action => :api_read, :content_type => "text/xml" + render :action => :show, :content_type => "text/xml" end - def api_users + def index 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) @@ -33,10 +33,10 @@ module Api @users = User.visible.find(ids) - render :action => :api_users, :content_type => "text/xml" + render :action => :index, :content_type => "text/xml" end - def api_gpx_files + def gpx_files doc = OSM::API.new.get_xml_doc current_user.traces.reload.each do |trace| doc.root << trace.to_xml_node diff --git a/app/views/api/users/_api_user.builder b/app/views/api/users/_api_user.builder deleted file mode 100644 index 91cd562e9..000000000 --- a/app/views/api/users/_api_user.builder +++ /dev/null @@ -1,44 +0,0 @@ -xml.tag! "user", :id => api_user.id, - :display_name => api_user.display_name, - :account_created => api_user.creation_time.xmlschema do - xml.tag! "description", api_user.description if api_user.description - if current_user && current_user == api_user - xml.tag! "contributor-terms", :agreed => api_user.terms_agreed.present?, - :pd => api_user.consider_pd - else - xml.tag! "contributor-terms", :agreed => api_user.terms_agreed.present? - end - xml.tag! "img", :href => user_image_url(api_user) if api_user.image.file? || api_user.image_use_gravatar - xml.tag! "roles" do - api_user.roles.each do |role| - xml.tag! role.role - end - end - xml.tag! "changesets", :count => api_user.changesets.size - xml.tag! "traces", :count => api_user.traces.size - xml.tag! "blocks" do - xml.tag! "received", :count => api_user.blocks.size, - :active => api_user.blocks.active.size - if api_user.moderator? - xml.tag! "issued", :count => api_user.blocks_created.size, - :active => api_user.blocks_created.active.size - end - end - if current_user && current_user == api_user - if api_user.home_lat && api_user.home_lon - xml.tag! "home", :lat => api_user.home_lat, - :lon => api_user.home_lon, - :zoom => api_user.home_zoom - end - if api_user.languages - xml.tag! "languages" do - api_user.languages.split(",") { |lang| xml.tag! "lang", lang } - end - end - xml.tag! "messages" do - xml.tag! "received", :count => api_user.messages.size, - :unread => api_user.new_messages.size - xml.tag! "sent", :count => api_user.sent_messages.size - end - end -end diff --git a/app/views/api/users/_user.builder b/app/views/api/users/_user.builder new file mode 100644 index 000000000..7bf8e18a5 --- /dev/null +++ b/app/views/api/users/_user.builder @@ -0,0 +1,44 @@ +xml.tag! "user", :id => user.id, + :display_name => user.display_name, + :account_created => user.creation_time.xmlschema do + xml.tag! "description", user.description if user.description + if current_user && current_user == user + xml.tag! "contributor-terms", :agreed => user.terms_agreed.present?, + :pd => user.consider_pd + else + xml.tag! "contributor-terms", :agreed => user.terms_agreed.present? + end + xml.tag! "img", :href => user_image_url(user) if user.image.file? || user.image_use_gravatar + xml.tag! "roles" do + user.roles.each do |role| + xml.tag! role.role + end + end + xml.tag! "changesets", :count => user.changesets.size + xml.tag! "traces", :count => user.traces.size + xml.tag! "blocks" do + xml.tag! "received", :count => user.blocks.size, + :active => user.blocks.active.size + if user.moderator? + xml.tag! "issued", :count => user.blocks_created.size, + :active => user.blocks_created.active.size + end + end + if current_user && current_user == user + if user.home_lat && user.home_lon + xml.tag! "home", :lat => user.home_lat, + :lon => user.home_lon, + :zoom => user.home_zoom + end + if user.languages + xml.tag! "languages" do + user.languages.split(",") { |lang| xml.tag! "lang", lang } + end + end + xml.tag! "messages" do + xml.tag! "received", :count => user.messages.size, + :unread => user.new_messages.size + xml.tag! "sent", :count => user.sent_messages.size + end + end +end diff --git a/app/views/api/users/api_users.builder b/app/views/api/users/index.builder similarity index 59% rename from app/views/api/users/api_users.builder rename to app/views/api/users/index.builder index a3c0f4d9a..68d629099 100644 --- a/app/views/api/users/api_users.builder +++ b/app/views/api/users/index.builder @@ -1,4 +1,4 @@ xml.instruct! :xml, :version => "1.0" xml.osm(OSM::API.new.xml_root_attributes) do |osm| - osm << render(:partial => "api_user", :collection => @users) + osm << render(:partial => "user", :collection => @users) end diff --git a/app/views/api/users/api_read.builder b/app/views/api/users/show.builder similarity index 61% rename from app/views/api/users/api_read.builder rename to app/views/api/users/show.builder index 78533279c..dd5e93689 100644 --- a/app/views/api/users/api_read.builder +++ b/app/views/api/users/show.builder @@ -1,4 +1,4 @@ xml.instruct! :xml, :version => "1.0" xml.osm(OSM::API.new.xml_root_attributes) do |osm| - osm << render(:partial => "api_user", :object => @user) + osm << render(:partial => "user", :object => @user) end diff --git a/config/routes.rb b/config/routes.rb index 98869abf7..198932583 100644 --- a/config/routes.rb +++ b/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" => "api/users#api_read", :id => /\d+/ - get "user/details" => "api/users#api_details" - get "user/gpx_files" => "api/users#api_gpx_files" - get "users" => "api/users#api_users", :as => :api_users + get "user/:id" => "api/users#show", :id => /\d+/ + get "user/details" => "api/users#details" + get "user/gpx_files" => "api/users#gpx_files" + get "users" => "api/users#index", :as => :api_users get "user/preferences" => "api/user_preferences#read" get "user/preferences/:preference_key" => "api/user_preferences#read_one" diff --git a/test/controllers/api/users_controller_test.rb b/test/controllers/api/users_controller_test.rb index 5483731db..93a7eeb55 100644 --- a/test/controllers/api/users_controller_test.rb +++ b/test/controllers/api/users_controller_test.rb @@ -11,26 +11,26 @@ module Api def test_routes assert_routing( { :path => "/api/0.6/user/1", :method => :get }, - { :controller => "api/users", :action => "api_read", :id => "1" } + { :controller => "api/users", :action => "show", :id => "1" } ) assert_routing( { :path => "/api/0.6/user/details", :method => :get }, - { :controller => "api/users", :action => "api_details" } + { :controller => "api/users", :action => "details" } ) assert_routing( { :path => "/api/0.6/user/gpx_files", :method => :get }, - { :controller => "api/users", :action => "api_gpx_files" } + { :controller => "api/users", :action => "gpx_files" } ) assert_routing( { :path => "/api/0.6/users", :method => :get }, - { :controller => "api/users", :action => "api_users" } + { :controller => "api/users", :action => "index" } ) end - def test_api_read + def test_show user = create(:user, :description => "test", :terms_agreed => Date.yesterday) # check that a visible user is returned properly - get :api_read, :params => { :id => user.id } + get :show, :params => { :id => user.id } assert_response :success assert_equal "text/xml", response.content_type @@ -63,30 +63,30 @@ module Api assert_select "messages", false # check that a suspended user is not returned - get :api_read, :params => { :id => create(:user, :suspended).id } + get :show, :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 } + get :show, :params => { :id => create(:user, :deleted).id } assert_response :gone # check that a non-existent user is not returned - get :api_read, :params => { :id => 0 } + get :show, :params => { :id => 0 } assert_response :not_found end - def test_api_details + def test_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 + get :details assert_response :unauthorized # check that we get a response when logged in basic_authorization user.email, "test" - get :api_details + get :details assert_response :success assert_equal "text/xml", response.content_type @@ -127,12 +127,12 @@ module Api end end - def test_api_users + def test_index 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 } + get :index, :params => { :users => user1.id } assert_response :success assert_equal "text/xml", response.content_type assert_select "user", :count => 1 do @@ -141,7 +141,7 @@ module Api assert_select "user[id='#{user3.id}']", :count => 0 end - get :api_users, :params => { :users => user2.id } + get :index, :params => { :users => user2.id } assert_response :success assert_equal "text/xml", response.content_type assert_select "user", :count => 1 do @@ -150,7 +150,7 @@ module Api assert_select "user[id='#{user3.id}']", :count => 0 end - get :api_users, :params => { :users => "#{user1.id},#{user3.id}" } + get :index, :params => { :users => "#{user1.id},#{user3.id}" } assert_response :success assert_equal "text/xml", response.content_type assert_select "user", :count => 2 do @@ -159,17 +159,17 @@ module Api assert_select "user[id='#{user3.id}']", :count => 1 end - get :api_users, :params => { :users => create(:user, :suspended).id } + get :index, :params => { :users => create(:user, :suspended).id } assert_response :not_found - get :api_users, :params => { :users => create(:user, :deleted).id } + get :index, :params => { :users => create(:user, :deleted).id } assert_response :not_found - get :api_users, :params => { :users => 0 } + get :index, :params => { :users => 0 } assert_response :not_found end - def test_api_gpx_files + def test_gpx_files user = create(:user) trace1 = create(:trace, :user => user) do |trace| create(:tracetag, :trace => trace, :tag => "London") @@ -178,12 +178,12 @@ module Api create(:tracetag, :trace => trace, :tag => "Birmingham") end # check that nothing is returned when not logged in - get :api_gpx_files + get :gpx_files assert_response :unauthorized # check that we get a response when logged in basic_authorization user.email, "test" - get :api_gpx_files + get :gpx_files assert_response :success assert_equal "application/xml", response.content_type -- 2.43.2