From 32f1eaaf0f966ccc45635693679bcc8658e71815 Mon Sep 17 00:00:00 2001
From: Sebastian Ziebell <sebastian.ziebell@asquera.de>
Date: Thu, 7 Mar 2013 17:56:11 +0100
Subject: [PATCH] API: system hooks API functions and documentation updated

* updated system hooks documentation and code comments
* fixed access to system hooks if no user given resulting in a `500 Server Error`
* added tests
---
 doc/api/system_hooks.md                | 12 +++++++-----
 lib/api/system_hooks.rb                | 20 +++++++++++++++-----
 spec/requests/api/system_hooks_spec.rb | 16 ++++++++++++++--
 3 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/doc/api/system_hooks.md b/doc/api/system_hooks.md
index f6e11ed238e..dca22c43f83 100644
--- a/doc/api/system_hooks.md
+++ b/doc/api/system_hooks.md
@@ -8,7 +8,10 @@ Get list of system hooks
 GET /hooks
 ```
 
-Will return hooks with status `200 OK` on success, or `404 Not found` on fail.
+Parameters:
+
++ **none**
+
 
 ## Add new system hook hook
 
@@ -20,7 +23,6 @@ Parameters:
 
 + `url` (required) - The hook URL
 
-Will return status `201 Created` on success, or `404 Not found` on fail.
 
 ## Test system hook
 
@@ -32,10 +34,12 @@ Parameters:
 
 + `id` (required) - The ID of hook
 
-Will return hook with status `200 OK` on success, or `404 Not found` on fail.
 
 ## Delete system hook
 
+Deletes a system hook. This is an idempotent API function and returns `200 Ok` even if the hook
+is not available. If the hook is deleted it is also returned as JSON.
+
 ```
 DELETE /hooks/:id
 ```
@@ -43,5 +47,3 @@ DELETE /hooks/:id
 Parameters:
 
 + `id` (required) - The ID of hook
-
-Will return status `200 OK` on success, or `404 Not found` on fail.
\ No newline at end of file
diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb
index 665a1cdd0d2..da0b005d69d 100644
--- a/lib/api/system_hooks.rb
+++ b/lib/api/system_hooks.rb
@@ -1,7 +1,10 @@
 module Gitlab
   # Hooks API
   class SystemHooks < Grape::API
-    before { authenticated_as_admin! }
+    before {
+      authenticate!
+      authenticated_as_admin!
+    }
 
     resource :hooks do
       # Get the list of system hooks
@@ -21,6 +24,7 @@ module Gitlab
       #   POST /hooks
       post do
         attrs = attributes_for_keys [:url]
+        required_attributes! [:url]
         @hook = SystemHook.new attrs
         if @hook.save
           present @hook, with: Entities::Hook
@@ -47,13 +51,19 @@ module Gitlab
         data
       end
 
-      # Delete a hook
-      #
+      # Delete a hook. This is an idempotent function.
+      # 
+      # Parameters:
+      #   id (required) - ID of the hook
       # Example Request:
       #   DELETE /hooks/:id
       delete ":id" do
-        @hook = SystemHook.find(params[:id])
-        @hook.destroy
+        begin
+          @hook = SystemHook.find(params[:id])
+          @hook.destroy
+        rescue
+          # SystemHook raises an Error if no hook with id found
+        end
       end
     end
   end
diff --git a/spec/requests/api/system_hooks_spec.rb b/spec/requests/api/system_hooks_spec.rb
index 9842ae91ec3..fe1b324c921 100644
--- a/spec/requests/api/system_hooks_spec.rb
+++ b/spec/requests/api/system_hooks_spec.rb
@@ -10,6 +10,13 @@ describe Gitlab::API do
   before { stub_request(:post, hook.url) }
 
   describe "GET /hooks" do
+    context "when no user" do
+      it "should return authentication error" do
+        get api("/hooks")
+        response.status.should == 401
+      end
+    end
+
     context "when not an admin" do
       it "should return forbidden error" do
         get api("/hooks", user)
@@ -34,9 +41,9 @@ describe Gitlab::API do
       }.to change { SystemHook.count }.by(1)
     end
 
-    it "should respond with 404 on failure" do
+    it "should respond with 400 if url not given" do
       post api("/hooks", admin)
-      response.status.should == 404
+      response.status.should == 400
     end
 
     it "should not create new hook without url" do
@@ -65,5 +72,10 @@ describe Gitlab::API do
         delete api("/hooks/#{hook.id}", admin)
       }.to change { SystemHook.count }.by(-1)
     end
+
+    it "should return success if hook id not found" do
+      delete api("/hooks/12345", admin)
+      response.status.should == 200
+    end
   end
 end
\ No newline at end of file
-- 
GitLab