From: Anton Khorev Date: Sun, 23 Feb 2025 04:36:03 +0000 (+0300) Subject: Create api changeset subscription resource X-Git-Tag: live~4^2~3 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/1ffc19192e089c4e75d9db7889d08cd67c1322db?ds=inline Create api changeset subscription resource --- diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb index ef852b69f..b0bd2578f 100644 --- a/app/abilities/api_ability.rb +++ b/app/abilities/api_ability.rb @@ -3,7 +3,7 @@ class ApiAbility include CanCan::Ability - def initialize(user, scopes) + def initialize(user, scopes) # rubocop:disable Metrics/CyclomaticComplexity can :read, [:version, :capability, :permission, :map] if Settings.status != "database_offline" @@ -34,7 +34,8 @@ class ApiAbility can :read, :active_user_blocks_list if scopes.include?("read_prefs") if user.terms_agreed? - can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset if scopes.include?("write_map") + can [:create, :update, :upload, :close], Changeset if scopes.include?("write_map") + can [:create, :destroy], ChangesetSubscription if scopes.include?("write_map") can :create, ChangesetComment if scopes.include?("write_changeset_comments") can [:create, :update, :destroy], [Node, Way, Relation] if scopes.include?("write_map") end diff --git a/app/controllers/api/changeset_subscriptions_controller.rb b/app/controllers/api/changeset_subscriptions_controller.rb new file mode 100644 index 000000000..ccfe3f937 --- /dev/null +++ b/app/controllers/api/changeset_subscriptions_controller.rb @@ -0,0 +1,55 @@ +module Api + class ChangesetSubscriptionsController < ApiController + before_action :check_api_writable + before_action :authorize + + authorize_resource + + before_action :require_public_data + before_action :set_request_formats + + ## + # Adds a subscriber to the changeset + def create + # Check the arguments are sane + raise OSM::APIBadUserInput, "No id was given" unless params[:changeset_id] + + # Extract the arguments + changeset_id = params[:changeset_id].to_i + + # Find the changeset and check it is valid + @changeset = Changeset.find(changeset_id) + raise OSM::APIChangesetAlreadySubscribedError, @changeset if @changeset.subscribers.include?(current_user) + + # Add the subscriber + @changeset.subscribers << current_user + + respond_to do |format| + format.xml + format.json + end + end + + ## + # Removes a subscriber from the changeset + def destroy + # Check the arguments are sane + raise OSM::APIBadUserInput, "No id was given" unless params[:changeset_id] + + # Extract the arguments + changeset_id = params[:changeset_id].to_i + + # Find the changeset and check it is valid + @changeset = Changeset.find(changeset_id) + raise OSM::APIChangesetNotSubscribedError, @changeset unless @changeset.subscribers.include?(current_user) + + # Remove the subscriber + @changeset.subscribers.delete(current_user) + + respond_to do |format| + format.xml + format.json + end + end + end +end diff --git a/app/controllers/api/changesets_controller.rb b/app/controllers/api/changesets_controller.rb index e335f88f0..891e2175b 100644 --- a/app/controllers/api/changesets_controller.rb +++ b/app/controllers/api/changesets_controller.rb @@ -4,13 +4,13 @@ module Api class ChangesetsController < ApiController include QueryMethods - before_action :check_api_writable, :only => [:create, :update, :upload, :subscribe, :unsubscribe] + before_action :check_api_writable, :only => [:create, :update, :upload] before_action :setup_user_auth, :only => [:show] - before_action :authorize, :only => [:create, :update, :upload, :close, :subscribe, :unsubscribe] + before_action :authorize, :only => [:create, :update, :upload, :close] authorize_resource - before_action :require_public_data, :only => [:create, :update, :upload, :close, :subscribe, :unsubscribe] + before_action :require_public_data, :only => [:create, :update, :upload, :close] before_action :set_request_formats, :except => [:create, :close, :upload] skip_around_action :api_call_timeout, :only => [:upload] @@ -152,58 +152,6 @@ module Api end end - ## - # Adds a subscriber to the changeset - def subscribe - # Check the arguments are sane - raise OSM::APIBadUserInput, "No id was given" unless params[:id] - - # Extract the arguments - id = params[:id].to_i - - # Find the changeset and check it is valid - changeset = Changeset.find(id) - raise OSM::APIChangesetAlreadySubscribedError, changeset if changeset.subscribers.include?(current_user) - - # Add the subscriber - changeset.subscribers << current_user - - # Return a copy of the updated changeset - @changeset = changeset - render "show" - - respond_to do |format| - format.xml - format.json - end - end - - ## - # Removes a subscriber from the changeset - def unsubscribe - # Check the arguments are sane - raise OSM::APIBadUserInput, "No id was given" unless params[:id] - - # Extract the arguments - id = params[:id].to_i - - # Find the changeset and check it is valid - changeset = Changeset.find(id) - raise OSM::APIChangesetNotSubscribedError, changeset unless changeset.subscribers.include?(current_user) - - # Remove the subscriber - changeset.subscribers.delete(current_user) - - # Return a copy of the updated changeset - @changeset = changeset - render "show" - - respond_to do |format| - format.xml - format.json - end - end - private #------------------------------------------------------------ diff --git a/app/views/api/changeset_subscriptions/create.json.jbuilder b/app/views/api/changeset_subscriptions/create.json.jbuilder new file mode 100644 index 000000000..7a840c12f --- /dev/null +++ b/app/views/api/changeset_subscriptions/create.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/root_attributes" + +json.changeset do + json.partial! @changeset +end diff --git a/app/views/api/changeset_subscriptions/create.xml.builder b/app/views/api/changeset_subscriptions/create.xml.builder new file mode 100644 index 000000000..b53061d4c --- /dev/null +++ b/app/views/api/changeset_subscriptions/create.xml.builder @@ -0,0 +1,5 @@ +xml.instruct! :xml, :version => "1.0" + +xml.osm(OSM::API.new.xml_root_attributes) do |osm| + osm << render(@changeset) +end diff --git a/app/views/api/changeset_subscriptions/destroy.json.jbuilder b/app/views/api/changeset_subscriptions/destroy.json.jbuilder new file mode 100644 index 000000000..7a840c12f --- /dev/null +++ b/app/views/api/changeset_subscriptions/destroy.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/root_attributes" + +json.changeset do + json.partial! @changeset +end diff --git a/app/views/api/changeset_subscriptions/destroy.xml.builder b/app/views/api/changeset_subscriptions/destroy.xml.builder new file mode 100644 index 000000000..b53061d4c --- /dev/null +++ b/app/views/api/changeset_subscriptions/destroy.xml.builder @@ -0,0 +1,5 @@ +xml.instruct! :xml, :version => "1.0" + +xml.osm(OSM::API.new.xml_root_attributes) do |osm| + osm << render(@changeset) +end diff --git a/app/views/changesets/show.html.erb b/app/views/changesets/show.html.erb index 3f1aeb201..3d9041cab 100644 --- a/app/views/changesets/show.html.erb +++ b/app/views/changesets/show.html.erb @@ -23,14 +23,14 @@ <%= tag.button t(".unsubscribe"), :class => "btn btn-sm btn-primary", :name => "unsubscribe", - :data => { :method => "POST", - :url => api_changeset_unsubscribe_url(@changeset) } %> + :data => { :method => "DELETE", + :url => api_changeset_subscription_path(@changeset) } %> <% else %> <%= tag.button t(".subscribe"), :class => "btn btn-sm btn-primary", :name => "subscribe", :data => { :method => "POST", - :url => api_changeset_subscribe_url(@changeset) } %> + :url => api_changeset_subscription_path(@changeset) } %> <% end %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index ec78f6ffd..e329a4b01 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -18,8 +18,6 @@ OpenStreetMap::Application.routes.draw do get "permissions" => "permissions#show" post "changeset/:id/upload" => "changesets#upload", :as => :changeset_upload, :id => /\d+/ - post "changeset/:id/subscribe" => "changesets#subscribe", :as => :api_changeset_subscribe, :id => /\d+/ - post "changeset/:id/unsubscribe" => "changesets#unsubscribe", :as => :api_changeset_unsubscribe, :id => /\d+/ put "changeset/:id/close" => "changesets#close", :as => :changeset_close, :id => /\d+/ end @@ -27,9 +25,12 @@ OpenStreetMap::Application.routes.draw do resources :changesets, :only => [:index, :create] resources :changesets, :path => "changeset", :id => /\d+/, :only => [:show, :update] do resource :download, :module => :changesets, :only => :show + resource :subscription, :controller => :changeset_subscriptions, :only => [:create, :destroy] resources :changeset_comments, :path => "comment", :only => :create end put "changeset/create" => "changesets#create", :as => nil + post "changeset/:changeset_id/subscribe" => "changeset_subscriptions#create", :changeset_id => /\d+/, :as => nil + post "changeset/:changeset_id/unsubscribe" => "changeset_subscriptions#destroy", :changeset_id => /\d+/, :as => nil resources :changeset_comments, :id => /\d+/, :only => :index do resource :visibility, :module => :changeset_comments, :only => [:create, :destroy] diff --git a/test/controllers/api/changeset_subscriptions_controller_test.rb b/test/controllers/api/changeset_subscriptions_controller_test.rb new file mode 100644 index 000000000..f4c3836e6 --- /dev/null +++ b/test/controllers/api/changeset_subscriptions_controller_test.rb @@ -0,0 +1,115 @@ +require "test_helper" + +module Api + class ChangesetsControllerTest < ActionDispatch::IntegrationTest + ## + # test all routes which lead to this controller + def test_routes + assert_routing( + { :path => "/api/0.6/changeset/1/subscription", :method => :post }, + { :controller => "api/changeset_subscriptions", :action => "create", :changeset_id => "1" } + ) + assert_routing( + { :path => "/api/0.6/changeset/1/subscription.json", :method => :post }, + { :controller => "api/changeset_subscriptions", :action => "create", :changeset_id => "1", :format => "json" } + ) + assert_routing( + { :path => "/api/0.6/changeset/1/subscription", :method => :delete }, + { :controller => "api/changeset_subscriptions", :action => "destroy", :changeset_id => "1" } + ) + assert_routing( + { :path => "/api/0.6/changeset/1/subscription.json", :method => :delete }, + { :controller => "api/changeset_subscriptions", :action => "destroy", :changeset_id => "1", :format => "json" } + ) + end + + def test_create_success + auth_header = bearer_authorization_header + changeset = create(:changeset, :closed) + + assert_difference "changeset.subscribers.count", 1 do + post api_changeset_subscription_path(changeset), :headers => auth_header + end + assert_response :success + + # not closed changeset + changeset = create(:changeset) + assert_difference "changeset.subscribers.count", 1 do + post api_changeset_subscription_path(changeset), :headers => auth_header + end + assert_response :success + end + + def test_create_fail + user = create(:user) + + # unauthorized + changeset = create(:changeset, :closed) + assert_no_difference "changeset.subscribers.count" do + post api_changeset_subscription_path(changeset) + end + assert_response :unauthorized + + auth_header = bearer_authorization_header user + + # bad changeset id + assert_no_difference "changeset.subscribers.count" do + post api_changeset_subscription_path(999111), :headers => auth_header + end + assert_response :not_found + + # trying to subscribe when already subscribed + changeset = create(:changeset, :closed) + changeset.subscribers.push(user) + assert_no_difference "changeset.subscribers.count" do + post api_changeset_subscription_path(changeset), :headers => auth_header + end + assert_response :conflict + end + + def test_destroy_success + user = create(:user) + auth_header = bearer_authorization_header user + changeset = create(:changeset, :closed) + changeset.subscribers.push(user) + + assert_difference "changeset.subscribers.count", -1 do + delete api_changeset_subscription_path(changeset), :headers => auth_header + end + assert_response :success + + # not closed changeset + changeset = create(:changeset) + changeset.subscribers.push(user) + + assert_difference "changeset.subscribers.count", -1 do + delete api_changeset_subscription_path(changeset), :headers => auth_header + end + assert_response :success + end + + def test_destroy_fail + # unauthorized + changeset = create(:changeset, :closed) + assert_no_difference "changeset.subscribers.count" do + delete api_changeset_subscription_path(changeset) + end + assert_response :unauthorized + + auth_header = bearer_authorization_header + + # bad changeset id + assert_no_difference "changeset.subscribers.count" do + delete api_changeset_subscription_path(999111), :headers => auth_header + end + assert_response :not_found + + # trying to unsubscribe when not subscribed + changeset = create(:changeset, :closed) + assert_no_difference "changeset.subscribers.count" do + delete api_changeset_subscription_path(changeset), :headers => auth_header + end + assert_response :not_found + end + end +end diff --git a/test/controllers/api/changesets_controller_test.rb b/test/controllers/api/changesets_controller_test.rb index 87deb3bdf..4b72e432e 100644 --- a/test/controllers/api/changesets_controller_test.rb +++ b/test/controllers/api/changesets_controller_test.rb @@ -33,22 +33,6 @@ module Api { :path => "/api/0.6/changeset/1/upload", :method => :post }, { :controller => "api/changesets", :action => "upload", :id => "1" } ) - assert_routing( - { :path => "/api/0.6/changeset/1/subscribe", :method => :post }, - { :controller => "api/changesets", :action => "subscribe", :id => "1" } - ) - assert_routing( - { :path => "/api/0.6/changeset/1/subscribe.json", :method => :post }, - { :controller => "api/changesets", :action => "subscribe", :id => "1", :format => "json" } - ) - assert_routing( - { :path => "/api/0.6/changeset/1/unsubscribe", :method => :post }, - { :controller => "api/changesets", :action => "unsubscribe", :id => "1" } - ) - assert_routing( - { :path => "/api/0.6/changeset/1/unsubscribe.json", :method => :post }, - { :controller => "api/changesets", :action => "unsubscribe", :id => "1", :format => "json" } - ) assert_routing( { :path => "/api/0.6/changeset/1/close", :method => :put }, { :controller => "api/changesets", :action => "close", :id => "1" } @@ -2503,103 +2487,6 @@ module Api "element limit.") end - ## - # test subscribe success - def test_subscribe_success - auth_header = bearer_authorization_header - changeset = create(:changeset, :closed) - - assert_difference "changeset.subscribers.count", 1 do - post api_changeset_subscribe_path(changeset), :headers => auth_header - end - assert_response :success - - # not closed changeset - changeset = create(:changeset) - assert_difference "changeset.subscribers.count", 1 do - post api_changeset_subscribe_path(changeset), :headers => auth_header - end - assert_response :success - end - - ## - # test subscribe fail - def test_subscribe_fail - user = create(:user) - - # unauthorized - changeset = create(:changeset, :closed) - assert_no_difference "changeset.subscribers.count" do - post api_changeset_subscribe_path(changeset) - end - assert_response :unauthorized - - auth_header = bearer_authorization_header user - - # bad changeset id - assert_no_difference "changeset.subscribers.count" do - post api_changeset_subscribe_path(999111), :headers => auth_header - end - assert_response :not_found - - # trying to subscribe when already subscribed - changeset = create(:changeset, :closed) - changeset.subscribers.push(user) - assert_no_difference "changeset.subscribers.count" do - post api_changeset_subscribe_path(changeset), :headers => auth_header - end - assert_response :conflict - end - - ## - # test unsubscribe success - def test_unsubscribe_success - user = create(:user) - auth_header = bearer_authorization_header user - changeset = create(:changeset, :closed) - changeset.subscribers.push(user) - - assert_difference "changeset.subscribers.count", -1 do - post api_changeset_unsubscribe_path(changeset), :headers => auth_header - end - assert_response :success - - # not closed changeset - changeset = create(:changeset) - changeset.subscribers.push(user) - - assert_difference "changeset.subscribers.count", -1 do - post api_changeset_unsubscribe_path(changeset), :headers => auth_header - end - assert_response :success - end - - ## - # test unsubscribe fail - def test_unsubscribe_fail - # unauthorized - changeset = create(:changeset, :closed) - assert_no_difference "changeset.subscribers.count" do - post api_changeset_unsubscribe_path(changeset) - end - assert_response :unauthorized - - auth_header = bearer_authorization_header - - # bad changeset id - assert_no_difference "changeset.subscribers.count" do - post api_changeset_unsubscribe_path(999111), :headers => auth_header - end - assert_response :not_found - - # trying to unsubscribe when not subscribed - changeset = create(:changeset, :closed) - assert_no_difference "changeset.subscribers.count" do - post api_changeset_unsubscribe_path(changeset), :headers => auth_header - end - assert_response :not_found - end - private ##