Create api changeset subscription resource

diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb
index ef852b69fe72f48cde9434c74235f54fd0fff0a4..b0bd2578f11fe9f03242c9174f108837552ac812 100644 (file)
--- a/app/abilities/api_ability.rb
+++ b/app/abilities/api_ability.rb
@@ -3,7 +3,7 @@
 class ApiAbility
   include CanCan::Ability
 
 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"
     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 :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
           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 (file)
index 0000000..ccfe3f9
--- /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 e335f88f0653bc31042c7f6d37ee157bf2083623..891e2175b5e1def95f65ab078c613207b4cef485 100644 (file)
--- 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
 
   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 :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
 
 
     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]
     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
 
       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
 
     #------------------------------------------------------------
     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 (file)
index 0000000..7a840c1
--- /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 (file)
index 0000000..b53061d
--- /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 (file)
index 0000000..7a840c1
--- /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 (file)
index 0000000..b53061d
--- /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 3f1aeb2017b6e00d1bd479abbb104d4236ca144c..3d9041cab6c9c6b23077c5d02559f07735930218 100644 (file)
--- 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",
           <%= 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",
         <% 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 %>
       </div>
     <% end %>
         <% end %>
       </div>
     <% end %>

diff --git a/config/routes.rb b/config/routes.rb
index ec78f6ffd30b32bded2ee28d9e7a76cd444569c7..e329a4b0167b25a294015451aa361e81f938d339 100644 (file)
--- 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+/
     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
 
     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
     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
       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]
 
     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 (file)
index 0000000..f4c3836
--- /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 87deb3bdfcb2c1b064eb8b7be3ed8c4fd5b9836b..4b72e432e50721a5e947a10860fdd7ca440d5876 100644 (file)
--- 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" }
       )
         { :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" }
       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
 
                  "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
 
     ##
     private
 
     ##