Commit 32f1eaaf0f966ccc45635693679bcc8658e71815
1 parent
ecf53bb9
Exists in
master
and in
4 other branches
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
Showing
3 changed files
with
36 additions
and
12 deletions
Show diff stats
doc/api/system_hooks.md
| ... | ... | @@ -8,7 +8,10 @@ Get list of system hooks |
| 8 | 8 | GET /hooks |
| 9 | 9 | ``` |
| 10 | 10 | |
| 11 | -Will return hooks with status `200 OK` on success, or `404 Not found` on fail. | |
| 11 | +Parameters: | |
| 12 | + | |
| 13 | ++ **none** | |
| 14 | + | |
| 12 | 15 | |
| 13 | 16 | ## Add new system hook hook |
| 14 | 17 | |
| ... | ... | @@ -20,7 +23,6 @@ Parameters: |
| 20 | 23 | |
| 21 | 24 | + `url` (required) - The hook URL |
| 22 | 25 | |
| 23 | -Will return status `201 Created` on success, or `404 Not found` on fail. | |
| 24 | 26 | |
| 25 | 27 | ## Test system hook |
| 26 | 28 | |
| ... | ... | @@ -32,10 +34,12 @@ Parameters: |
| 32 | 34 | |
| 33 | 35 | + `id` (required) - The ID of hook |
| 34 | 36 | |
| 35 | -Will return hook with status `200 OK` on success, or `404 Not found` on fail. | |
| 36 | 37 | |
| 37 | 38 | ## Delete system hook |
| 38 | 39 | |
| 40 | +Deletes a system hook. This is an idempotent API function and returns `200 Ok` even if the hook | |
| 41 | +is not available. If the hook is deleted it is also returned as JSON. | |
| 42 | + | |
| 39 | 43 | ``` |
| 40 | 44 | DELETE /hooks/:id |
| 41 | 45 | ``` |
| ... | ... | @@ -43,5 +47,3 @@ DELETE /hooks/:id |
| 43 | 47 | Parameters: |
| 44 | 48 | |
| 45 | 49 | + `id` (required) - The ID of hook |
| 46 | - | |
| 47 | -Will return status `200 OK` on success, or `404 Not found` on fail. | |
| 48 | 50 | \ No newline at end of file | ... | ... |
lib/api/system_hooks.rb
| 1 | 1 | module Gitlab |
| 2 | 2 | # Hooks API |
| 3 | 3 | class SystemHooks < Grape::API |
| 4 | - before { authenticated_as_admin! } | |
| 4 | + before { | |
| 5 | + authenticate! | |
| 6 | + authenticated_as_admin! | |
| 7 | + } | |
| 5 | 8 | |
| 6 | 9 | resource :hooks do |
| 7 | 10 | # Get the list of system hooks |
| ... | ... | @@ -21,6 +24,7 @@ module Gitlab |
| 21 | 24 | # POST /hooks |
| 22 | 25 | post do |
| 23 | 26 | attrs = attributes_for_keys [:url] |
| 27 | + required_attributes! [:url] | |
| 24 | 28 | @hook = SystemHook.new attrs |
| 25 | 29 | if @hook.save |
| 26 | 30 | present @hook, with: Entities::Hook |
| ... | ... | @@ -47,13 +51,19 @@ module Gitlab |
| 47 | 51 | data |
| 48 | 52 | end |
| 49 | 53 | |
| 50 | - # Delete a hook | |
| 51 | - # | |
| 54 | + # Delete a hook. This is an idempotent function. | |
| 55 | + # | |
| 56 | + # Parameters: | |
| 57 | + # id (required) - ID of the hook | |
| 52 | 58 | # Example Request: |
| 53 | 59 | # DELETE /hooks/:id |
| 54 | 60 | delete ":id" do |
| 55 | - @hook = SystemHook.find(params[:id]) | |
| 56 | - @hook.destroy | |
| 61 | + begin | |
| 62 | + @hook = SystemHook.find(params[:id]) | |
| 63 | + @hook.destroy | |
| 64 | + rescue | |
| 65 | + # SystemHook raises an Error if no hook with id found | |
| 66 | + end | |
| 57 | 67 | end |
| 58 | 68 | end |
| 59 | 69 | end | ... | ... |
spec/requests/api/system_hooks_spec.rb
| ... | ... | @@ -10,6 +10,13 @@ describe Gitlab::API do |
| 10 | 10 | before { stub_request(:post, hook.url) } |
| 11 | 11 | |
| 12 | 12 | describe "GET /hooks" do |
| 13 | + context "when no user" do | |
| 14 | + it "should return authentication error" do | |
| 15 | + get api("/hooks") | |
| 16 | + response.status.should == 401 | |
| 17 | + end | |
| 18 | + end | |
| 19 | + | |
| 13 | 20 | context "when not an admin" do |
| 14 | 21 | it "should return forbidden error" do |
| 15 | 22 | get api("/hooks", user) |
| ... | ... | @@ -34,9 +41,9 @@ describe Gitlab::API do |
| 34 | 41 | }.to change { SystemHook.count }.by(1) |
| 35 | 42 | end |
| 36 | 43 | |
| 37 | - it "should respond with 404 on failure" do | |
| 44 | + it "should respond with 400 if url not given" do | |
| 38 | 45 | post api("/hooks", admin) |
| 39 | - response.status.should == 404 | |
| 46 | + response.status.should == 400 | |
| 40 | 47 | end |
| 41 | 48 | |
| 42 | 49 | it "should not create new hook without url" do |
| ... | ... | @@ -65,5 +72,10 @@ describe Gitlab::API do |
| 65 | 72 | delete api("/hooks/#{hook.id}", admin) |
| 66 | 73 | }.to change { SystemHook.count }.by(-1) |
| 67 | 74 | end |
| 75 | + | |
| 76 | + it "should return success if hook id not found" do | |
| 77 | + delete api("/hooks/12345", admin) | |
| 78 | + response.status.should == 200 | |
| 79 | + end | |
| 68 | 80 | end |
| 69 | 81 | end |
| 70 | 82 | \ No newline at end of file | ... | ... |