API: changed status codes for project hooks functions

Different status codes in the API lib are returned on hook creation, update or deletion. If a required parameter is not given (e.g. `url` in `/projects/:id/hooks/:hook_id`) status code 400 (Bad request) is returned. On hook deletion a 200 status code is returned, regardless if the hook is present or not. This makes the DELETE function an idempotent operation. Appropriate tests are added to check these status codes.

API: changed status codes for project hooks functions
Different status codes in the API lib are returned on hook creation, update or deletion. If a required parameter is not given (e.g. `url` in `/projects/:id/hooks/:hook_id`) status code 400 (Bad request) is returned. On hook deletion a 200 status code is returned, regardless if the hook is present or not. This makes the DELETE function an idempotent operation. Appropriate tests are added to check these status codes.
Sebastian Ziebell
1 parent 44938026
Showing 2 changed files with 34 additions and 6 deletions Show diff stats
lib/api/projects.rb
spec/requests/api/projects_spec.rb
@@ -195,11 +195,14 @@ module Gitlab
       #   POST /projects/:id/hooks
       post ":id/hooks" do
         authorize! :admin_project, user_project
+
+        error!("Url not given", 400) unless params.has_key? :url
+
         @hook = user_project.hooks.new({"url" => params[:url]})
         if @hook.save
           present @hook, with: Entities::Hook
         else
-          error!({'message' => '404 Not found'}, 404)
+          not_found!
         end
       end
@@ -215,7 +218,7 @@ module Gitlab
         @hook = user_project.hooks.find(params[:hook_id])
         authorize! :admin_project, user_project
-        error!("Url not given", 400) if !params.has_key? :url
+        error!("Url not given", 400) unless params.has_key? :url
         attrs = attributes_for_keys [:url]
         if @hook.update_attributes attrs
@@ -234,8 +237,13 @@ module Gitlab
       #   DELETE /projects/:id/hooks
       delete ":id/hooks" do
         authorize! :admin_project, user_project
-        @hook = user_project.hooks.find(params[:hook_id])
-        @hook.destroy
+        error!("Hook id not given", 400) unless params.has_key? :hook_id
+
+        begin
+          @hook = ProjectHook.find(params[:hook_id])
+          @hook.destroy
+        rescue
+        end
       end
       # Get a project repository branches
@@ -6,8 +6,8 @@ describe Gitlab::API do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
   let(:user3) { create(:user) }
-  let!(:hook) { create(:project_hook, project: project, url: "http://example.com") }
   let!(:project) { create(:project, namespace: user.namespace ) }
+  let!(:hook) { create(:project_hook, project: project, url: "http://example.com") }
   let!(:snippet) { create(:snippet, author: user, project: project, title: 'example') }
   let!(:users_project) { create(:users_project, user: user, project: project, project_access: UsersProject::MASTER) }
   let!(:users_project2) { create(:users_project, user: user3, project: project, project_access: UsersProject::DEVELOPER) }
@@ -290,6 +290,11 @@ describe Gitlab::API do
       }.to change {project.hooks.count}.by(1)
       response.status.should == 201
     end
+
+    it "should return a 400 error if url not given" do
+      post api("/projects/#{project.id}/hooks", user)
+      response.status.should == 400
+    end
   end
   describe "PUT /projects/:id/hooks/:hook_id" do
@@ -300,7 +305,7 @@ describe Gitlab::API do
       json_response['url'].should == 'http://example.org'
     end
-    it "should return 404 error if hook id is not found" do
+    it "should return 404 error if hook id not found" do
       put api("/projects/#{project.id}/hooks/1234", user), url: 'http://example.org'
       response.status.should == 404
     end
@@ -319,6 +324,21 @@ describe Gitlab::API do
       }.to change {project.hooks.count}.by(-1)
       response.status.should == 200
     end
+
+    it "should return success when deleting hook" do
+      delete api("/projects/#{project.id}/hooks", user), hook_id: hook.id
+      response.status.should == 200
+    end
+
+    it "should return success when deleting non existent hook" do
+      delete api("/projects/#{project.id}/hooks", user), hook_id: 42
+      response.status.should == 200
+    end
+
+    it "should return a 400 error if hook id not given" do
+      delete api("/projects/#{project.id}/hooks", user)
+      response.status.should == 400
+    end
   end
   describe "GET /projects/:id/repository/tags" do