API: Make subscription API more RESTfuL

changelogs/unreleased-ee/api-subscription-restful.yml

+---
+title: 'API: Use `post ":id/#{type}/:subscribable_id/subscribe"` to subscribe and
+  `post ":id/#{type}/:subscribable_id/unsubscribe"` to unsubscribe from a resource'
+merge_request: 1274
+author: Robert Schilling

doc/api/v3_to_v4.md

@@ -29,4 +29,4 @@ changes are in V4:
 - Removed `DELETE projects/:id/deploy_keys/:key_id/disable`. Use `DELETE projects/:id/deploy_keys/:key_id` instead
 - Moved `PUT /users/:id/(block|unblock)` to `POST /users/:id/(block|unblock)`
 - Labels filter on `projects/:id/issues` and `/issues` now matches only issues containing all labels (i.e.: Logical AND, not OR)
-
+- Make subscription API more RESTful. Use `post ":id/#{type}/:subscribable_id/subscribe"` to subscribe and `post ":id/#{type}/:subscribable_id/unsubscribe"` to unsubscribe from a resource.

lib/api/api.rb

@@ -17,6 +17,7 @@ module API
       mount ::API::V3::Projects
       mount ::API::V3::ProjectSnippets
       mount ::API::V3::Repositories
+      mount ::API::V3::Subscriptions
       mount ::API::V3::SystemHooks
       mount ::API::V3::Tags
       mount ::API::V3::Templates

lib/api/subscriptions.rb

@@ -21,7 +21,7 @@ module API
         desc 'Subscribe to a resource' do
           success entity_class
         end
-        post ":id/#{type}/:subscribable_id/subscription" do
+        post ":id/#{type}/:subscribable_id/subscribe" do
           resource = instance_exec(params[:subscribable_id], &finder)
 
           if resource.subscribed?(current_user, user_project)
@@ -35,7 +35,7 @@ module API
         desc 'Unsubscribe from a resource' do
           success entity_class
         end
-        delete ":id/#{type}/:subscribable_id/subscription" do
+        post ":id/#{type}/:subscribable_id/unsubscribe" do
           resource = instance_exec(params[:subscribable_id], &finder)
 
           if !resource.subscribed?(current_user, user_project)

lib/api/v3/subscriptions.rb

+module API
+  module V3
+    class Subscriptions < Grape::API
+      before { authenticate! }
+
+      subscribable_types = {
+        'merge_request' => proc { |id| find_merge_request_with_access(id, :update_merge_request) },
+        'merge_requests' => proc { |id| find_merge_request_with_access(id, :update_merge_request) },
+        'issues' => proc { |id| find_project_issue(id) },
+        'labels' => proc { |id| find_project_label(id) },
+      }
+
+      params do
+        requires :id, type: String, desc: 'The ID of a project'
+        requires :subscribable_id, type: String, desc: 'The ID of a resource'
+      end
+      resource :projects do
+        subscribable_types.each do |type, finder|
+          type_singularized = type.singularize
+          entity_class = ::API::Entities.const_get(type_singularized.camelcase)
+
+          desc 'Subscribe to a resource' do
+            success entity_class
+          end
+          post ":id/#{type}/:subscribable_id/subscription" do
+            resource = instance_exec(params[:subscribable_id], &finder)
+
+            if resource.subscribed?(current_user, user_project)
+              not_modified!
+            else
+              resource.subscribe(current_user, user_project)
+              present resource, with: entity_class, current_user: current_user, project: user_project
+            end
+          end
+
+          desc 'Unsubscribe from a resource' do
+            success entity_class
+          end
+          delete ":id/#{type}/:subscribable_id/subscription" do
+            resource = instance_exec(params[:subscribable_id], &finder)
+
+            if !resource.subscribed?(current_user, user_project)
+              not_modified!
+            else
+              resource.unsubscribe(current_user, user_project)
+              present resource, with: entity_class, current_user: current_user, project: user_project
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/requests/api/issues_spec.rb

@@ -1266,55 +1266,55 @@ describe API::Issues, api: true  do
     end
   end
 
-  describe 'POST :id/issues/:issue_id/subscription' do
+  describe 'POST :id/issues/:issue_id/subscribe' do
     it 'subscribes to an issue' do
-      post api("/projects/#{project.id}/issues/#{issue.id}/subscription", user2)
+      post api("/projects/#{project.id}/issues/#{issue.id}/subscribe", user2)
 
       expect(response).to have_http_status(201)
       expect(json_response['subscribed']).to eq(true)
     end
 
     it 'returns 304 if already subscribed' do
-      post api("/projects/#{project.id}/issues/#{issue.id}/subscription", user)
+      post api("/projects/#{project.id}/issues/#{issue.id}/subscribe", user)
 
       expect(response).to have_http_status(304)
     end
 
     it 'returns 404 if the issue is not found' do
-      post api("/projects/#{project.id}/issues/123/subscription", user)
+      post api("/projects/#{project.id}/issues/123/subscribe", user)
 
       expect(response).to have_http_status(404)
     end
 
     it 'returns 404 if the issue is confidential' do
-      post api("/projects/#{project.id}/issues/#{confidential_issue.id}/subscription", non_member)
+      post api("/projects/#{project.id}/issues/#{confidential_issue.id}/subscribe", non_member)
 
       expect(response).to have_http_status(404)
     end
   end
 
-  describe 'DELETE :id/issues/:issue_id/subscription' do
+  describe 'POST :id/issues/:issue_id/unsubscribe' do
     it 'unsubscribes from an issue' do
-      delete api("/projects/#{project.id}/issues/#{issue.id}/subscription", user)
+      post api("/projects/#{project.id}/issues/#{issue.id}/unsubscribe", user)
 
-      expect(response).to have_http_status(200)
+      expect(response).to have_http_status(201)
       expect(json_response['subscribed']).to eq(false)
     end
 
     it 'returns 304 if not subscribed' do
-      delete api("/projects/#{project.id}/issues/#{issue.id}/subscription", user2)
+      post api("/projects/#{project.id}/issues/#{issue.id}/unsubscribe", user2)
 
       expect(response).to have_http_status(304)
     end
 
     it 'returns 404 if the issue is not found' do
-      delete api("/projects/#{project.id}/issues/123/subscription", user)
+      post api("/projects/#{project.id}/issues/123/unsubscribe", user)
 
       expect(response).to have_http_status(404)
     end
 
     it 'returns 404 if the issue is confidential' do
-      delete api("/projects/#{project.id}/issues/#{confidential_issue.id}/subscription", non_member)
+      post api("/projects/#{project.id}/issues/#{confidential_issue.id}/unsubscribe", non_member)
 
       expect(response).to have_http_status(404)
     end

spec/requests/api/labels_spec.rb

@@ -318,10 +318,10 @@ describe API::Labels, api: true  do
     end
   end
 
-  describe "POST /projects/:id/labels/:label_id/subscription" do
+  describe "POST /projects/:id/labels/:label_id/subscribe" do
     context "when label_id is a label title" do
       it "subscribes to the label" do
-        post api("/projects/#{project.id}/labels/#{label1.title}/subscription", user)
+        post api("/projects/#{project.id}/labels/#{label1.title}/subscribe", user)
 
         expect(response).to have_http_status(201)
         expect(json_response["name"]).to eq(label1.title)
@@ -331,7 +331,7 @@ describe API::Labels, api: true  do
 
     context "when label_id is a label ID" do
       it "subscribes to the label" do
-        post api("/projects/#{project.id}/labels/#{label1.id}/subscription", user)
+        post api("/projects/#{project.id}/labels/#{label1.id}/subscribe", user)
 
         expect(response).to have_http_status(201)
         expect(json_response["name"]).to eq(label1.title)
@@ -343,7 +343,7 @@ describe API::Labels, api: true  do
       before { label1.subscribe(user, project) }
 
       it "returns 304" do
-        post api("/projects/#{project.id}/labels/#{label1.id}/subscription", user)
+        post api("/projects/#{project.id}/labels/#{label1.id}/subscribe", user)
 
         expect(response).to have_http_status(304)
       end
@@ -351,21 +351,21 @@ describe API::Labels, api: true  do
 
     context "when label ID is not found" do
       it "returns 404 error" do
-        post api("/projects/#{project.id}/labels/1234/subscription", user)
+        post api("/projects/#{project.id}/labels/1234/subscribe", user)
 
         expect(response).to have_http_status(404)
       end
     end
   end
 
-  describe "DELETE /projects/:id/labels/:label_id/subscription" do
+  describe "POST /projects/:id/labels/:label_id/unsubscribe" do
     before { label1.subscribe(user, project) }
 
     context "when label_id is a label title" do
       it "unsubscribes from the label" do
-        delete api("/projects/#{project.id}/labels/#{label1.title}/subscription", user)
+        post api("/projects/#{project.id}/labels/#{label1.title}/unsubscribe", user)
 
-        expect(response).to have_http_status(200)
+        expect(response).to have_http_status(201)
         expect(json_response["name"]).to eq(label1.title)
         expect(json_response["subscribed"]).to be_falsey
       end
@@ -373,9 +373,9 @@ describe API::Labels, api: true  do
 
     context "when label_id is a label ID" do
       it "unsubscribes from the label" do
-        delete api("/projects/#{project.id}/labels/#{label1.id}/subscription", user)
+        post api("/projects/#{project.id}/labels/#{label1.id}/unsubscribe", user)
 
-        expect(response).to have_http_status(200)
+        expect(response).to have_http_status(201)
         expect(json_response["name"]).to eq(label1.title)
         expect(json_response["subscribed"]).to be_falsey
       end
@@ -385,7 +385,7 @@ describe API::Labels, api: true  do
       before { label1.unsubscribe(user, project) }
 
       it "returns 304" do
-        delete api("/projects/#{project.id}/labels/#{label1.id}/subscription", user)
+        post api("/projects/#{project.id}/labels/#{label1.id}/unsubscribe", user)
 
         expect(response).to have_http_status(304)
       end
@@ -393,7 +393,7 @@ describe API::Labels, api: true  do
 
     context "when label ID is not found" do
       it "returns 404 error" do
-        delete api("/projects/#{project.id}/labels/1234/subscription", user)
+        post api("/projects/#{project.id}/labels/1234/unsubscribe", user)
 
         expect(response).to have_http_status(404)
       end

spec/requests/api/merge_requests_spec.rb

@@ -740,22 +740,22 @@ describe API::MergeRequests, api: true  do
     end
   end
 
-  describe 'POST :id/merge_requests/:merge_request_id/subscription' do
+  describe 'POST :id/merge_requests/:merge_request_id/subscribe' do
     it 'subscribes to a merge request' do
-      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", admin)
+      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscribe", admin)
 
       expect(response).to have_http_status(201)
       expect(json_response['subscribed']).to eq(true)
     end
 
     it 'returns 304 if already subscribed' do
-      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", user)
+      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscribe", user)
 
       expect(response).to have_http_status(304)
     end
 
     it 'returns 404 if the merge request is not found' do
-      post api("/projects/#{project.id}/merge_requests/123/subscription", user)
+      post api("/projects/#{project.id}/merge_requests/123/subscribe", user)
 
       expect(response).to have_http_status(404)
     end
@@ -764,28 +764,28 @@ describe API::MergeRequests, api: true  do
       guest = create(:user)
       project.team << [guest, :guest]
 
-      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest)
+      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscribe", guest)
 
       expect(response).to have_http_status(403)
     end
   end
 
-  describe 'DELETE :id/merge_requests/:merge_request_id/subscription' do
+  describe 'POST :id/merge_requests/:merge_request_id/unsubscribe' do
     it 'unsubscribes from a merge request' do
-      delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", user)
+      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/unsubscribe", user)
 
-      expect(response).to have_http_status(200)
+      expect(response).to have_http_status(201)
       expect(json_response['subscribed']).to eq(false)
     end
 
     it 'returns 304 if not subscribed' do
-      delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", admin)
+      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/unsubscribe", admin)
 
       expect(response).to have_http_status(304)
     end
 
     it 'returns 404 if the merge request is not found' do
-      post api("/projects/#{project.id}/merge_requests/123/subscription", user)
+      post api("/projects/#{project.id}/merge_requests/123/unsubscribe", user)
 
       expect(response).to have_http_status(404)
     end
@@ -794,7 +794,7 @@ describe API::MergeRequests, api: true  do
       guest = create(:user)
       project.team << [guest, :guest]
 
-      delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest)
+      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/unsubscribe", guest)
 
       expect(response).to have_http_status(403)
     end

spec/requests/api/v3/labels_spec.rb

@@ -67,4 +67,86 @@ describe API::V3::Labels, api: true  do
       expect(priority_label_response['subscribed']).to be_falsey
     end
   end
+
+  describe "POST /projects/:id/labels/:label_id/subscription" do
+    context "when label_id is a label title" do
+      it "subscribes to the label" do
+        post v3_api("/projects/#{project.id}/labels/#{label1.title}/subscription", user)
+
+        expect(response).to have_http_status(201)
+        expect(json_response["name"]).to eq(label1.title)
+        expect(json_response["subscribed"]).to be_truthy
+      end
+    end
+
+    context "when label_id is a label ID" do
+      it "subscribes to the label" do
+        post v3_api("/projects/#{project.id}/labels/#{label1.id}/subscription", user)
+
+        expect(response).to have_http_status(201)
+        expect(json_response["name"]).to eq(label1.title)
+        expect(json_response["subscribed"]).to be_truthy
+      end
+    end
+
+    context "when user is already subscribed to label" do
+      before { label1.subscribe(user, project) }
+
+      it "returns 304" do
+        post v3_api("/projects/#{project.id}/labels/#{label1.id}/subscription", user)
+
+        expect(response).to have_http_status(304)
+      end
+    end
+
+    context "when label ID is not found" do
+      it "returns 404 error" do
+        post v3_api("/projects/#{project.id}/labels/1234/subscription", user)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
+
+  describe "DELETE /projects/:id/labels/:label_id/subscription" do
+    before { label1.subscribe(user, project) }
+
+    context "when label_id is a label title" do
+      it "unsubscribes from the label" do
+        delete v3_api("/projects/#{project.id}/labels/#{label1.title}/subscription", user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response["name"]).to eq(label1.title)
+        expect(json_response["subscribed"]).to be_falsey
+      end
+    end
+
+    context "when label_id is a label ID" do
+      it "unsubscribes from the label" do
+        delete v3_api("/projects/#{project.id}/labels/#{label1.id}/subscription", user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response["name"]).to eq(label1.title)
+        expect(json_response["subscribed"]).to be_falsey
+      end
+    end
+
+    context "when user is already unsubscribed from label" do
+      before { label1.unsubscribe(user, project) }
+
+      it "returns 304" do
+        delete v3_api("/projects/#{project.id}/labels/#{label1.id}/subscription", user)
+
+        expect(response).to have_http_status(304)
+      end
+    end
+
+    context "when label ID is not found" do
+      it "returns 404 error" do
+        delete v3_api("/projects/#{project.id}/labels/1234/subscription", user)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
 end