From 3fdd00afc73e05ad413be18d9cdbbe2e3c74b465 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 19 Oct 2016 10:34:31 +0000 Subject: 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 | 7 ++----- lib/api/system_hooks.rb | 10 ++++------ spec/requests/api/system_hooks_spec.rb | 7 ++++--- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/doc/api/system_hooks.md b/doc/api/system_hooks.md index 1802fae14fe..073e99b7147 100644 --- a/doc/api/system_hooks.md +++ b/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. --- diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb index 2e76b91051f..794e34874f4 100644 --- a/lib/api/system_hooks.rb +++ b/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 diff --git a/spec/requests/api/system_hooks_spec.rb b/spec/requests/api/system_hooks_spec.rb index 1ce2658569e..f8a1aed5441 100644 --- a/spec/requests/api/system_hooks_spec.rb +++ b/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 -- cgit v1.2.3