Merge branch 'fix-system-hook-api' into 'master'

API: Fix Sytem hooks delete behavior

## What does this MR do?

This corrects the delete API for system hooks. Returning 200 is not the right way indicating a hooks is not found.

## What are the relevant issue numbers?

Discussed in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6861/diffs#609af00c90e3d5241064d1404e3e018a3235634a_64_62

See merge request !6883

doc/api/system_hooks.md

@@ -98,11 +98,8 @@ Example response:
 
 ## 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, a JSON object is returned. An error is raised if the
-hook is not found.
+Deletes a system hook. It returns `200 OK` if the hooks is deleted and
+`404 Not Found` if the hook is not found.
 
 ---
 

lib/api/system_hooks.rb

@@ -56,12 +56,10 @@ module API
         requires :id, type: Integer, desc: 'The ID of the system hook'
       end
       delete ":id" do
-        begin
-          hook = SystemHook.find(params[:id])
-          present hook.destroy, with: Entities::Hook
-        rescue
-          # SystemHook raises an Error if no hook with id found
-        end
+        hook = SystemHook.find_by(id: params[:id])
+        not_found!('System hook') unless hook
+
+        present hook.destroy, with: Entities::Hook
       end
     end
   end

spec/requests/api/system_hooks_spec.rb

@@ -73,9 +73,10 @@ describe API::API, api: true  do
       end.to change { SystemHook.count }.by(-1)
     end
 
-    it "returns success if hook id not found" do
-      delete api("/hooks/12345", admin)
-      expect(response).to have_http_status(200)
+    it 'returns 404 if the system hook does not exist' do
+      delete api('/hooks/12345', admin)
+
+      expect(response).to have_http_status(404)
     end
   end
 end