From 86c58687b22f788ad7c821af55abece2f9d89d50 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Mon, 20 Feb 2017 19:18:12 +0100 Subject: Return 204 for delete endpoints --- Gemfile | 2 +- Gemfile.lock | 14 +++++++------- lib/api/award_emoji.rb | 1 - lib/api/boards.rb | 4 +--- lib/api/branches.rb | 6 +----- lib/api/broadcast_messages.rb | 2 +- lib/api/environments.rb | 2 +- lib/api/files.rb | 5 +---- lib/api/labels.rb | 4 ++-- lib/api/members.rb | 20 +++----------------- lib/api/notes.rb | 2 -- lib/api/project_hooks.rb | 9 +++------ lib/api/projects.rb | 1 - lib/api/runners.rb | 5 +---- lib/api/services.rb | 4 +--- lib/api/snippets.rb | 3 ++- lib/api/system_hooks.rb | 2 +- lib/api/tags.rb | 6 +----- lib/api/triggers.rb | 2 -- lib/api/users.rb | 4 ++-- lib/api/variables.rb | 3 +-- spec/requests/api/access_requests_spec.rb | 4 ++-- spec/requests/api/award_emoji_spec.rb | 16 ++++++++-------- spec/requests/api/boards_spec.rb | 3 +-- spec/requests/api/branches_spec.rb | 7 +++---- spec/requests/api/broadcast_messages_spec.rb | 7 +++++-- spec/requests/api/deploy_keys_spec.rb | 2 ++ spec/requests/api/environments_spec.rb | 2 +- spec/requests/api/files_spec.rb | 11 ++--------- spec/requests/api/groups_spec.rb | 4 ++-- spec/requests/api/issues_spec.rb | 4 ++-- spec/requests/api/labels_spec.rb | 5 +++-- spec/requests/api/members_spec.rb | 12 ++++++------ spec/requests/api/merge_requests_spec.rb | 2 +- spec/requests/api/notes_spec.rb | 6 +++--- spec/requests/api/project_hooks_spec.rb | 8 ++------ spec/requests/api/project_snippets_spec.rb | 4 ++-- spec/requests/api/projects_spec.rb | 7 +++++-- spec/requests/api/runners_spec.rb | 15 ++++++++++----- spec/requests/api/services_spec.rb | 2 +- spec/requests/api/snippets_spec.rb | 2 +- spec/requests/api/system_hooks_spec.rb | 2 ++ spec/requests/api/tags_spec.rb | 4 ++-- spec/requests/api/triggers_spec.rb | 3 ++- spec/requests/api/users_spec.rb | 20 ++++++++++++++------ spec/requests/api/variables_spec.rb | 3 ++- 46 files changed, 114 insertions(+), 142 deletions(-) diff --git a/Gemfile b/Gemfile index a3d036792e8..429454b9ed6 100644 --- a/Gemfile +++ b/Gemfile @@ -68,7 +68,7 @@ gem 'gollum-rugged_adapter', '~> 0.4.2', require: false gem 'github-linguist', '~> 4.7.0', require: 'linguist' # API -gem 'grape', '~> 0.18.0' +gem 'grape', '~> 0.19.0' gem 'grape-entity', '~> 0.6.0' gem 'rack-cors', '~> 0.4.0', require: 'rack/cors' diff --git a/Gemfile.lock b/Gemfile.lock index 5ff18442c4f..472cee510cb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -304,7 +304,7 @@ GEM multi_json (~> 1.11) os (~> 0.9) signet (~> 0.7) - grape (0.18.0) + grape (0.19.1) activesupport builder hashie (>= 2.1.0) @@ -353,8 +353,8 @@ GEM json (~> 1.8) multi_xml (>= 0.5.2) httpclient (2.8.2) - i18n (0.8.0) - ice_nine (0.11.1) + i18n (0.8.1) + ice_nine (0.11.2) influxdb (0.2.3) cause json @@ -417,7 +417,7 @@ GEM minitest (5.7.0) mousetrap-rails (1.4.6) multi_json (1.12.1) - multi_xml (0.5.5) + multi_xml (0.6.0) multipart-post (2.0.0) mustermann (0.4.0) tool (~> 0.2) @@ -758,7 +758,7 @@ GEM eventmachine (~> 1.0, >= 1.0.4) rack (>= 1, < 3) thor (0.19.4) - thread_safe (0.3.5) + thread_safe (0.3.6) tilt (2.0.6) timecop (0.8.1) timfel-krb5-auth (0.8.3) @@ -886,7 +886,7 @@ DEPENDENCIES gollum-rugged_adapter (~> 0.4.2) gon (~> 6.1.0) google-api-client (~> 0.8.6) - grape (~> 0.18.0) + grape (~> 0.19.0) grape-entity (~> 0.6.0) haml_lint (~> 0.21.0) hamlit (~> 2.6.1) @@ -1011,4 +1011,4 @@ DEPENDENCIES wikicloth (= 0.8.1) BUNDLED WITH - 1.14.3 + 1.14.4 diff --git a/lib/api/award_emoji.rb b/lib/api/award_emoji.rb index 301271118d4..07a1bcdbe18 100644 --- a/lib/api/award_emoji.rb +++ b/lib/api/award_emoji.rb @@ -83,7 +83,6 @@ module API unauthorized! unless award.user == current_user || current_user.admin? award.destroy - present award, with: Entities::AwardEmoji end end end diff --git a/lib/api/boards.rb b/lib/api/boards.rb index f4226e5a89d..b6843c1b6af 100644 --- a/lib/api/boards.rb +++ b/lib/api/boards.rb @@ -127,9 +127,7 @@ module API service = ::Boards::Lists::DestroyService.new(user_project, current_user) - if service.execute(list) - present list, with: Entities::List - else + unless service.execute(list) render_api_error!({ error: 'List could not be deleted!' }, 400) end end diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 34f136948c2..73a7e939627 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -124,11 +124,7 @@ module API result = DeleteBranchService.new(user_project, current_user). execute(params[:branch]) - if result[:status] == :success - { - branch: params[:branch] - } - else + if result[:status] != :success render_api_error!(result[:message], result[:return_code]) end end diff --git a/lib/api/broadcast_messages.rb b/lib/api/broadcast_messages.rb index 1217002bf8e..395c401203c 100644 --- a/lib/api/broadcast_messages.rb +++ b/lib/api/broadcast_messages.rb @@ -91,7 +91,7 @@ module API delete ':id' do message = find_message - present message.destroy, with: Entities::BroadcastMessage + message.destroy end end end diff --git a/lib/api/environments.rb b/lib/api/environments.rb index 1a7e68f0528..dbdf29a9640 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -79,7 +79,7 @@ module API environment = user_project.environments.find(params[:environment_id]) - present environment.destroy, with: Entities::Environment + environment.destroy end end end diff --git a/lib/api/files.rb b/lib/api/files.rb index 500f9d3c787..9c4e43d77cc 100644 --- a/lib/api/files.rb +++ b/lib/api/files.rb @@ -118,10 +118,7 @@ module API file_params = declared_params(include_missing: false) result = ::Files::DestroyService.new(user_project, current_user, commit_params(file_params)).execute - if result[:status] == :success - status(200) - commit_response(file_params) - else + if result[:status] != :success render_api_error!(result[:message], 400) end end diff --git a/lib/api/labels.rb b/lib/api/labels.rb index d2955af3f95..59f0e7cb647 100644 --- a/lib/api/labels.rb +++ b/lib/api/labels.rb @@ -1,7 +1,7 @@ module API class Labels < Grape::API include PaginationParams - + before { authenticate! } params do @@ -56,7 +56,7 @@ module API label = user_project.labels.find_by(title: params[:name]) not_found!('Label') unless label - present label.destroy, with: Entities::Label, current_user: current_user, project: user_project + label.destroy end desc 'Update an existing label. At least one optional parameter is required.' do diff --git a/lib/api/members.rb b/lib/api/members.rb index 5f6913d1a27..baf85e6075a 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -93,24 +93,10 @@ module API end delete ":id/members/:user_id" do source = find_source(source_type, params[:id]) + # Ensure that memeber exists + source.members.find_by!(user_id: params[:user_id]) - # This is to ensure back-compatibility but find_by! should be used - # in that casse in 9.0! - member = source.members.find_by(user_id: params[:user_id]) - - # This is to ensure back-compatibility but this should be removed in - # favor of find_by! in 9.0! - not_found!("Member: user_id:#{params[:user_id]}") if source_type == 'group' && member.nil? - - # This is to ensure back-compatibility but 204 behavior should be used - # for all DELETE endpoints in 9.0! - if member.nil? - { message: "Access revoked", id: params[:user_id].to_i } - else - ::Members::DestroyService.new(source, current_user, declared_params).execute - - present member.user, with: Entities::Member, member: member - end + ::Members::DestroyService.new(source, current_user, declared_params).execute end end end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index f559a7f74a0..3b3e45cbd06 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -132,8 +132,6 @@ module API authorize! :admin_note, note ::Notes::DestroyService.new(user_project, current_user).execute(note) - - present note, with: Entities::Note end end end diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb index f7a28d7ad10..57a5f97dc7f 100644 --- a/lib/api/project_hooks.rb +++ b/lib/api/project_hooks.rb @@ -90,12 +90,9 @@ module API requires :hook_id, type: Integer, desc: 'The ID of the hook to delete' end delete ":id/hooks/:hook_id" do - begin - present user_project.hooks.destroy(params[:hook_id]), with: Entities::ProjectHook - rescue - # ProjectHook can raise Error if hook_id not found - not_found!("Error deleting hook #{params[:hook_id]}") - end + hook = user_project.hooks.find(params.delete(:hook_id)) + + hook.destroy end end end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index b89bddc7e29..310a896e958 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -353,7 +353,6 @@ module API not_found!('Group Link') unless link link.destroy - no_content! end desc 'Upload a file' diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 252e59bfa58..2e41f16f8c6 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -78,9 +78,8 @@ module API delete ':id' do runner = get_runner(params[:id]) authenticate_delete_runner!(runner) - runner.destroy! - present runner, with: Entities::Runner + runner.destroy! end end @@ -136,8 +135,6 @@ module API forbidden!("Only one project associated with the runner. Please remove the runner instead") if runner.projects.count == 1 runner_project.destroy - - present runner, with: Entities::Runner end end diff --git a/lib/api/services.rb b/lib/api/services.rb index ad856115485..79a5f27dc4d 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -654,9 +654,7 @@ module API hash.merge!(key => nil) end - if service.update_attributes(attrs.merge(active: false)) - true - else + unless service.update_attributes(attrs.merge(active: false)) render_api_error!('400 Bad Request', 400) end end diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb index ac03fbd2a3d..0f86fdb3075 100644 --- a/lib/api/snippets.rb +++ b/lib/api/snippets.rb @@ -118,9 +118,10 @@ module API delete ':id' do snippet = snippets_for_current_user.find_by(id: params.delete(:id)) return not_found!('Snippet') unless snippet + authorize! :destroy_personal_snippet, snippet + snippet.destroy - no_content! end desc 'Get a raw snippet' do diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb index d038a3fa828..ed7b23b474a 100644 --- a/lib/api/system_hooks.rb +++ b/lib/api/system_hooks.rb @@ -66,7 +66,7 @@ module API hook = SystemHook.find_by(id: params[:id]) not_found!('System hook') unless hook - present hook.destroy, with: Entities::Hook + hook.destroy end end end diff --git a/lib/api/tags.rb b/lib/api/tags.rb index 86759ab882f..d31ef9de26b 100644 --- a/lib/api/tags.rb +++ b/lib/api/tags.rb @@ -66,11 +66,7 @@ module API result = ::Tags::DestroyService.new(user_project, current_user). execute(params[:tag_name]) - if result[:status] == :success - { - tag_name: params[:tag_name] - } - else + if result[:status] != :success render_api_error!(result[:message], result[:return_code]) end end diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index ea0ad852633..b7c9c5f2b7f 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -93,8 +93,6 @@ module API return not_found!('Trigger') unless trigger trigger.destroy - - present trigger, with: Entities::Trigger end end end diff --git a/lib/api/users.rb b/lib/api/users.rb index 94b2b6653d2..7bb4b76f830 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -236,7 +236,7 @@ module API key = user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key - present key.destroy, with: Entities::SSHKey + key.destroy end desc 'Add an email address to a specified user. Available only for admins.' do @@ -422,7 +422,7 @@ module API key = current_user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key - present key.destroy, with: Entities::SSHKey + key.destroy end desc "Get the currently authenticated user's email addresses" do diff --git a/lib/api/variables.rb b/lib/api/variables.rb index f623b1dfe9f..259bc051d52 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -81,10 +81,9 @@ module API end delete ':id/variables/:key' do variable = user_project.variables.find_by(key: params[:key]) - return not_found!('Variable') unless variable - present variable.destroy, with: Entities::Variable + variable.destroy end end end diff --git a/spec/requests/api/access_requests_spec.rb b/spec/requests/api/access_requests_spec.rb index 919c98d6437..46edbd49b28 100644 --- a/spec/requests/api/access_requests_spec.rb +++ b/spec/requests/api/access_requests_spec.rb @@ -200,7 +200,7 @@ describe API::AccessRequests, api: true do expect do delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", access_requester) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) end.to change { source.requesters.count }.by(-1) end end @@ -210,7 +210,7 @@ describe API::AccessRequests, api: true do expect do delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", master) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) end.to change { source.requesters.count }.by(-1) end diff --git a/spec/requests/api/award_emoji_spec.rb b/spec/requests/api/award_emoji_spec.rb index 6cc1ef315db..9756991162e 100644 --- a/spec/requests/api/award_emoji_spec.rb +++ b/spec/requests/api/award_emoji_spec.rb @@ -242,9 +242,9 @@ describe API::AwardEmoji, api: true do it 'deletes the award' do expect do delete api("/projects/#{project.id}/issues/#{issue.id}/award_emoji/#{award_emoji.id}", user) - end.to change { issue.award_emoji.count }.from(1).to(0) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) + end.to change { issue.award_emoji.count }.from(1).to(0) end it 'returns a 404 error when the award emoji can not be found' do @@ -258,9 +258,9 @@ describe API::AwardEmoji, api: true do it 'deletes the award' do expect do delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/award_emoji/#{downvote.id}", user) - end.to change { merge_request.award_emoji.count }.from(1).to(0) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) + end.to change { merge_request.award_emoji.count }.from(1).to(0) end it 'returns a 404 error when note id not found' do @@ -277,9 +277,9 @@ describe API::AwardEmoji, api: true do it 'deletes the award' do expect do delete api("/projects/#{project.id}/snippets/#{snippet.id}/award_emoji/#{award.id}", user) - end.to change { snippet.award_emoji.count }.from(1).to(0) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) + end.to change { snippet.award_emoji.count }.from(1).to(0) end end end @@ -290,9 +290,9 @@ describe API::AwardEmoji, api: true do it 'deletes the award' do expect do delete api("/projects/#{project.id}/issues/#{issue.id}/notes/#{note.id}/award_emoji/#{rocket.id}", user) - end.to change { note.award_emoji.count }.from(1).to(0) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) + end.to change { note.award_emoji.count }.from(1).to(0) end end end diff --git a/spec/requests/api/boards_spec.rb b/spec/requests/api/boards_spec.rb index 71df534ebe1..87c36639cd4 100644 --- a/spec/requests/api/boards_spec.rb +++ b/spec/requests/api/boards_spec.rb @@ -195,8 +195,7 @@ describe API::Boards, api: true do it "deletes the list if an admin requests it" do delete api("#{base_url}/#{dev_list.id}", owner) - expect(response).to have_http_status(200) - expect(json_response['position']).to eq(1) + expect(response).to have_http_status(204) end end end diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb index cacdb21c692..ab5a7e4d3de 100644 --- a/spec/requests/api/branches_spec.rb +++ b/spec/requests/api/branches_spec.rb @@ -325,15 +325,14 @@ describe API::Branches, api: true do it "removes branch" do delete api("/projects/#{project.id}/repository/branches/#{branch_name}", user) - expect(response).to have_http_status(200) - expect(json_response['branch']).to eq(branch_name) + + expect(response).to have_http_status(204) end it "removes a branch with dots in the branch name" do delete api("/projects/#{project.id}/repository/branches/with.1.2.3", user) - expect(response).to have_http_status(200) - expect(json_response['branch']).to eq("with.1.2.3") + expect(response).to have_http_status(204) end it 'returns 404 if branch not exists' do diff --git a/spec/requests/api/broadcast_messages_spec.rb b/spec/requests/api/broadcast_messages_spec.rb index 921d8714173..024fa66848c 100644 --- a/spec/requests/api/broadcast_messages_spec.rb +++ b/spec/requests/api/broadcast_messages_spec.rb @@ -174,8 +174,11 @@ describe API::BroadcastMessages, api: true do end it 'deletes the broadcast message for admins' do - expect { delete api("/broadcast_messages/#{message.id}", admin) } - .to change { BroadcastMessage.count }.by(-1) + expect do + delete api("/broadcast_messages/#{message.id}", admin) + + expect(response).to have_http_status(204) + end.to change { BroadcastMessage.count }.by(-1) end end end diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb index 7e682e91bd1..4f4b18cf0e0 100644 --- a/spec/requests/api/deploy_keys_spec.rb +++ b/spec/requests/api/deploy_keys_spec.rb @@ -116,6 +116,8 @@ describe API::DeployKeys, api: true do it 'should delete existing key' do expect do delete api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin) + + expect(response).to have_http_status(204) end.to change{ project.deploy_keys.count }.by(-1) end diff --git a/spec/requests/api/environments_spec.rb b/spec/requests/api/environments_spec.rb index d0958d39d44..d66eb63fd0a 100644 --- a/spec/requests/api/environments_spec.rb +++ b/spec/requests/api/environments_spec.rb @@ -122,7 +122,7 @@ describe API::Environments, api: true do it 'returns a 200 for an existing environment' do delete api("/projects/#{project.id}/environments/#{environment.id}", user) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) end it 'returns a 404 for non existing id' do diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index 29d67b5259e..31b1aca6d73 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -201,11 +201,7 @@ describe API::Files, api: true do it "deletes existing file in project repo" do delete api("/projects/#{project.id}/repository/files", user), valid_params - expect(response).to have_http_status(200) - expect(json_response['file_path']).to eq(file_path) - last_commit = project.repository.commit.raw - expect(last_commit.author_email).to eq(user.email) - expect(last_commit.author_name).to eq(user.name) + expect(response).to have_http_status(204) end it "returns a 400 bad request if no params given" do @@ -228,10 +224,7 @@ describe API::Files, api: true do delete api("/projects/#{project.id}/repository/files", user), valid_params - expect(response).to have_http_status(200) - last_commit = project.repository.commit.raw - expect(last_commit.author_email).to eq(author_email) - expect(last_commit.author_name).to eq(author_name) + expect(response).to have_http_status(204) end end end diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index fb3dc1b074e..b0ba3ea912d 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -467,7 +467,7 @@ describe API::Groups, api: true do it "removes group" do delete api("/groups/#{group1.id}", user1) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) end it "does not remove a group if not an owner" do @@ -496,7 +496,7 @@ describe API::Groups, api: true do it "removes any existing group" do delete api("/groups/#{group2.id}", admin) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) end it "does not remove a non existing group" do diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 7cb75310204..ddc2e51821e 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -1175,8 +1175,8 @@ describe API::Issues, api: true do it "deletes the issue if an admin requests it" do delete api("/projects/#{project.id}/issues/#{issue.id}", owner) - expect(response).to have_http_status(200) - expect(json_response['state']).to eq 'opened' + + expect(response).to have_http_status(204) end end diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb index af271dbd4f5..a1adaba7b98 100644 --- a/spec/requests/api/labels_spec.rb +++ b/spec/requests/api/labels_spec.rb @@ -175,9 +175,10 @@ describe API::Labels, api: true do end describe 'DELETE /projects/:id/labels' do - it 'returns 200 for existing label' do + it 'returns 204 for existing label' do delete api("/projects/#{project.id}/labels", user), name: 'label1' - expect(response).to have_http_status(200) + + expect(response).to have_http_status(204) end it 'returns 404 for non existing label' do diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 127498ed109..2d37d026a39 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -263,18 +263,18 @@ describe API::Members, api: true do expect do delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", developer) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) end.to change { source.members.count }.by(-1) end end context 'when authenticated as a master/owner' do context 'and member is a requester' do - it "returns #{source_type == 'project' ? 200 : 404}" do + it 'returns 404' do expect do delete api("/#{source_type.pluralize}/#{source.id}/members/#{access_requester.id}", master) - expect(response).to have_http_status(source_type == 'project' ? 200 : 404) + expect(response).to have_http_status(404) end.not_to change { source.requesters.count } end end @@ -283,15 +283,15 @@ describe API::Members, api: true do expect do delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) end.to change { source.members.count }.by(-1) end end - it "returns #{source_type == 'project' ? 200 : 404} if member does not exist" do + it 'returns 404 if member does not exist' do delete api("/#{source_type.pluralize}/#{source.id}/members/123", master) - expect(response).to have_http_status(source_type == 'project' ? 200 : 404) + expect(response).to have_http_status(404) end end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index b87d0cd7de9..5522154899c 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -411,7 +411,7 @@ describe API::MergeRequests, api: true do it "destroys the merge request owners can destroy" do delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) end end end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 3cca4468be7..9d3c821b692 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -373,7 +373,7 @@ describe API::Notes, api: true do delete api("/projects/#{project.id}/issues/#{issue.id}/"\ "notes/#{issue_note.id}", user) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) # Check if note is really deleted delete api("/projects/#{project.id}/issues/#{issue.id}/"\ "notes/#{issue_note.id}", user) @@ -392,7 +392,7 @@ describe API::Notes, api: true do delete api("/projects/#{project.id}/snippets/#{snippet.id}/"\ "notes/#{snippet_note.id}", user) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) # Check if note is really deleted delete api("/projects/#{project.id}/snippets/#{snippet.id}/"\ "notes/#{snippet_note.id}", user) @@ -412,7 +412,7 @@ describe API::Notes, api: true do delete api("/projects/#{project.id}/merge_requests/"\ "#{merge_request.id}/notes/#{merge_request_note.id}", user) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) # Check if note is really deleted delete api("/projects/#{project.id}/merge_requests/"\ "#{merge_request.id}/notes/#{merge_request_note.id}", user) diff --git a/spec/requests/api/project_hooks_spec.rb b/spec/requests/api/project_hooks_spec.rb index 20c76bd2c05..f286568547d 100644 --- a/spec/requests/api/project_hooks_spec.rb +++ b/spec/requests/api/project_hooks_spec.rb @@ -183,13 +183,9 @@ describe API::ProjectHooks, 'ProjectHooks', api: true do it "deletes hook from project" do expect do delete api("/projects/#{project.id}/hooks/#{hook.id}", user) - end.to change {project.hooks.count}.by(-1) - expect(response).to have_http_status(200) - end - it "returns success when deleting hook" do - delete api("/projects/#{project.id}/hooks/#{hook.id}", user) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) + end.to change {project.hooks.count}.by(-1) end it "returns a 404 error when deleting non existent hook" do diff --git a/spec/requests/api/project_snippets_spec.rb b/spec/requests/api/project_snippets_spec.rb index da9df56401b..e382d89c6ef 100644 --- a/spec/requests/api/project_snippets_spec.rb +++ b/spec/requests/api/project_snippets_spec.rb @@ -189,7 +189,7 @@ describe API::ProjectSnippets, api: true do delete api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) end it 'returns 404 for invalid snippet id' do @@ -212,7 +212,7 @@ describe API::ProjectSnippets, api: true do end it 'returns 404 for invalid snippet id' do - delete api("/projects/#{snippet.project.id}/snippets/1234", admin) + get api("/projects/#{snippet.project.id}/snippets/1234", admin) expect(response).to have_http_status(404) expect(json_response['message']).to eq('404 Snippet Not Found') diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 5de4426f3bd..d2acce90a24 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -820,8 +820,9 @@ describe API::Projects, api: true do it 'deletes existing project snippet' do expect do delete api("/projects/#{project.id}/snippets/#{snippet.id}", user) + + expect(response).to have_http_status(204) end.to change { Snippet.count }.by(-1) - expect(response).to have_http_status(200) end it 'returns 404 when deleting unknown snippet id' do @@ -905,8 +906,10 @@ describe API::Projects, api: true do project_fork_target.reload expect(project_fork_target.forked_from_project).not_to be_nil expect(project_fork_target.forked?).to be_truthy + delete api("/projects/#{project_fork_target.id}/fork", admin) - expect(response).to have_http_status(200) + + expect(response).to have_http_status(204) project_fork_target.reload expect(project_fork_target.forked_from_project).to be_nil expect(project_fork_target.forked?).not_to be_truthy diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 103d6755888..8a82543a830 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -277,8 +277,9 @@ describe API::Runners, api: true do it 'deletes runner' do expect do delete api("/runners/#{shared_runner.id}", admin) + + expect(response).to have_http_status(204) end.to change{ Ci::Runner.shared.count }.by(-1) - expect(response).to have_http_status(200) end end @@ -286,15 +287,17 @@ describe API::Runners, api: true do it 'deletes unused runner' do expect do delete api("/runners/#{unused_specific_runner.id}", admin) + + expect(response).to have_http_status(204) end.to change{ Ci::Runner.specific.count }.by(-1) - expect(response).to have_http_status(200) end it 'deletes used runner' do expect do delete api("/runners/#{specific_runner.id}", admin) + + expect(response).to have_http_status(204) end.to change{ Ci::Runner.specific.count }.by(-1) - expect(response).to have_http_status(200) end end @@ -327,8 +330,9 @@ describe API::Runners, api: true do it 'deletes runner for one owned project' do expect do delete api("/runners/#{specific_runner.id}", user) + + expect(response).to have_http_status(204) end.to change{ Ci::Runner.specific.count }.by(-1) - expect(response).to have_http_status(200) end end end @@ -457,8 +461,9 @@ describe API::Runners, api: true do it "disables project's runner" do expect do delete api("/projects/#{project.id}/runners/#{two_projects_runner.id}", user) + + expect(response).to have_http_status(204) end.to change{ project.runners.count }.by(-1) - expect(response).to have_http_status(200) end end diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index 776dc655650..fd334934ca5 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -55,7 +55,7 @@ describe API::Services, api: true do it "deletes #{service}" do delete api("/projects/#{project.id}/services/#{dashed_service}", user) - expect(response).to have_http_status(200) + expect(response).to have_http_status(204) project.send(service_method).reload expect(project.send(service_method).activated?).to be_falsey end diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb index 41def7cd1d4..5219f6eed42 100644 --- a/spec/requests/api/snippets_spec.rb +++ b/spec/requests/api/snippets_spec.rb @@ -74,7 +74,7 @@ describe API::Snippets, api: true do end it 'returns 404 for invalid snippet id' do - delete api("/snippets/1234", user) + get api("/snippets/1234/raw", user) expect(response).to have_http_status(404) expect(json_response['message']).to eq('404 Snippet Not Found') diff --git a/spec/requests/api/system_hooks_spec.rb b/spec/requests/api/system_hooks_spec.rb index b59da632c00..d1e10f12657 100644 --- a/spec/requests/api/system_hooks_spec.rb +++ b/spec/requests/api/system_hooks_spec.rb @@ -91,6 +91,8 @@ describe API::SystemHooks, api: true do it "deletes a hook" do expect do delete api("/hooks/#{hook.id}", admin) + + expect(response).to have_http_status(204) end.to change { SystemHook.count }.by(-1) end diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index 8a4f078182f..b132d033a61 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -137,8 +137,8 @@ describe API::Tags, api: true do context 'delete tag' do it 'deletes an existing tag' do delete api("/projects/#{project.id}/repository/tags/#{tag_name}", user) - expect(response).to have_http_status(200) - expect(json_response['tag_name']).to eq(tag_name) + + expect(response).to have_http_status(204) end it 'raises 404 if the tag does not exist' do diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 92dfc2aa277..153e2791cbe 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -190,8 +190,9 @@ describe API::Triggers do it 'deletes trigger' do expect do delete api("/projects/#{project.id}/triggers/#{trigger.token}", user) + + expect(response).to have_http_status(204) end.to change{project.triggers.count}.by(-1) - expect(response).to have_http_status(200) end it 'responds with 404 Not Found if requesting non-existing trigger' do diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 603da9f49fc..e5e4c84755f 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -540,10 +540,12 @@ describe API::Users, api: true do it 'deletes existing key' do user.keys << key user.save + expect do delete api("/users/#{user.id}/keys/#{key.id}", admin) + + expect(response).to have_http_status(204) end.to change { user.keys.count }.by(-1) - expect(response).to have_http_status(200) end it 'returns 404 error if user not found' do @@ -637,10 +639,12 @@ describe API::Users, api: true do it 'deletes existing email' do user.emails << email user.save + expect do delete api("/users/#{user.id}/emails/#{email.id}", admin) + + expect(response).to have_http_status(204) end.to change { user.emails.count }.by(-1) - expect(response).to have_http_status(200) end it 'returns 404 error if user not found' do @@ -671,10 +675,10 @@ describe API::Users, api: true do it "deletes user" do delete api("/users/#{user.id}", admin) - expect(response).to have_http_status(200) + + expect(response).to have_http_status(204) expect { User.find(user.id) }.to raise_error ActiveRecord::RecordNotFound expect { Namespace.find(namespace.id) }.to raise_error ActiveRecord::RecordNotFound - expect(json_response['email']).to eq(user.email) end it "does not delete for unauthenticated user" do @@ -869,10 +873,12 @@ describe API::Users, api: true do it "deletes existed key" do user.keys << key user.save + expect do delete api("/user/keys/#{key.id}", user) + + expect(response).to have_http_status(204) end.to change{user.keys.count}.by(-1) - expect(response).to have_http_status(200) end it "returns 404 if key ID not found" do @@ -976,10 +982,12 @@ describe API::Users, api: true do it "deletes existed email" do user.emails << email user.save + expect do delete api("/user/emails/#{email.id}", user) + + expect(response).to have_http_status(204) end.to change{user.emails.count}.by(-1) - expect(response).to have_http_status(200) end it "returns 404 if email ID not found" do diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index 769f04c5057..0c1413119e0 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -152,8 +152,9 @@ describe API::Variables, api: true do it 'deletes variable' do expect do delete api("/projects/#{project.id}/variables/#{variable.key}", user) + + expect(response).to have_http_status(204) end.to change{project.variables.count}.by(-1) - expect(response).to have_http_status(200) end it 'responds with 404 Not Found if requesting non-existing variable' do -- cgit v1.2.3