Commit 86c58687 authored by Robert Schilling's avatar Robert Schilling

Return 204 for delete endpoints

parent 7733f285
...@@ -68,7 +68,7 @@ gem 'gollum-rugged_adapter', '~> 0.4.2', require: false ...@@ -68,7 +68,7 @@ gem 'gollum-rugged_adapter', '~> 0.4.2', require: false
gem 'github-linguist', '~> 4.7.0', require: 'linguist' gem 'github-linguist', '~> 4.7.0', require: 'linguist'
# API # API
gem 'grape', '~> 0.18.0' gem 'grape', '~> 0.19.0'
gem 'grape-entity', '~> 0.6.0' gem 'grape-entity', '~> 0.6.0'
gem 'rack-cors', '~> 0.4.0', require: 'rack/cors' gem 'rack-cors', '~> 0.4.0', require: 'rack/cors'
......
...@@ -304,7 +304,7 @@ GEM ...@@ -304,7 +304,7 @@ GEM
multi_json (~> 1.11) multi_json (~> 1.11)
os (~> 0.9) os (~> 0.9)
signet (~> 0.7) signet (~> 0.7)
grape (0.18.0) grape (0.19.1)
activesupport activesupport
builder builder
hashie (>= 2.1.0) hashie (>= 2.1.0)
...@@ -353,8 +353,8 @@ GEM ...@@ -353,8 +353,8 @@ GEM
json (~> 1.8) json (~> 1.8)
multi_xml (>= 0.5.2) multi_xml (>= 0.5.2)
httpclient (2.8.2) httpclient (2.8.2)
i18n (0.8.0) i18n (0.8.1)
ice_nine (0.11.1) ice_nine (0.11.2)
influxdb (0.2.3) influxdb (0.2.3)
cause cause
json json
...@@ -417,7 +417,7 @@ GEM ...@@ -417,7 +417,7 @@ GEM
minitest (5.7.0) minitest (5.7.0)
mousetrap-rails (1.4.6) mousetrap-rails (1.4.6)
multi_json (1.12.1) multi_json (1.12.1)
multi_xml (0.5.5) multi_xml (0.6.0)
multipart-post (2.0.0) multipart-post (2.0.0)
mustermann (0.4.0) mustermann (0.4.0)
tool (~> 0.2) tool (~> 0.2)
...@@ -758,7 +758,7 @@ GEM ...@@ -758,7 +758,7 @@ GEM
eventmachine (~> 1.0, >= 1.0.4) eventmachine (~> 1.0, >= 1.0.4)
rack (>= 1, < 3) rack (>= 1, < 3)
thor (0.19.4) thor (0.19.4)
thread_safe (0.3.5) thread_safe (0.3.6)
tilt (2.0.6) tilt (2.0.6)
timecop (0.8.1) timecop (0.8.1)
timfel-krb5-auth (0.8.3) timfel-krb5-auth (0.8.3)
...@@ -886,7 +886,7 @@ DEPENDENCIES ...@@ -886,7 +886,7 @@ DEPENDENCIES
gollum-rugged_adapter (~> 0.4.2) gollum-rugged_adapter (~> 0.4.2)
gon (~> 6.1.0) gon (~> 6.1.0)
google-api-client (~> 0.8.6) google-api-client (~> 0.8.6)
grape (~> 0.18.0) grape (~> 0.19.0)
grape-entity (~> 0.6.0) grape-entity (~> 0.6.0)
haml_lint (~> 0.21.0) haml_lint (~> 0.21.0)
hamlit (~> 2.6.1) hamlit (~> 2.6.1)
...@@ -1011,4 +1011,4 @@ DEPENDENCIES ...@@ -1011,4 +1011,4 @@ DEPENDENCIES
wikicloth (= 0.8.1) wikicloth (= 0.8.1)
BUNDLED WITH BUNDLED WITH
1.14.3 1.14.4
...@@ -83,7 +83,6 @@ module API ...@@ -83,7 +83,6 @@ module API
unauthorized! unless award.user == current_user || current_user.admin? unauthorized! unless award.user == current_user || current_user.admin?
award.destroy award.destroy
present award, with: Entities::AwardEmoji
end end
end end
end end
......
...@@ -127,9 +127,7 @@ module API ...@@ -127,9 +127,7 @@ module API
service = ::Boards::Lists::DestroyService.new(user_project, current_user) service = ::Boards::Lists::DestroyService.new(user_project, current_user)
if service.execute(list) unless service.execute(list)
present list, with: Entities::List
else
render_api_error!({ error: 'List could not be deleted!' }, 400) render_api_error!({ error: 'List could not be deleted!' }, 400)
end end
end end
......
...@@ -124,11 +124,7 @@ module API ...@@ -124,11 +124,7 @@ module API
result = DeleteBranchService.new(user_project, current_user). result = DeleteBranchService.new(user_project, current_user).
execute(params[:branch]) execute(params[:branch])
if result[:status] == :success if result[:status] != :success
{
branch: params[:branch]
}
else
render_api_error!(result[:message], result[:return_code]) render_api_error!(result[:message], result[:return_code])
end end
end end
......
...@@ -91,7 +91,7 @@ module API ...@@ -91,7 +91,7 @@ module API
delete ':id' do delete ':id' do
message = find_message message = find_message
present message.destroy, with: Entities::BroadcastMessage message.destroy
end end
end end
end end
......
...@@ -79,7 +79,7 @@ module API ...@@ -79,7 +79,7 @@ module API
environment = user_project.environments.find(params[:environment_id]) environment = user_project.environments.find(params[:environment_id])
present environment.destroy, with: Entities::Environment environment.destroy
end end
end end
end end
......
...@@ -118,10 +118,7 @@ module API ...@@ -118,10 +118,7 @@ module API
file_params = declared_params(include_missing: false) file_params = declared_params(include_missing: false)
result = ::Files::DestroyService.new(user_project, current_user, commit_params(file_params)).execute result = ::Files::DestroyService.new(user_project, current_user, commit_params(file_params)).execute
if result[:status] == :success if result[:status] != :success
status(200)
commit_response(file_params)
else
render_api_error!(result[:message], 400) render_api_error!(result[:message], 400)
end end
end end
......
module API module API
class Labels < Grape::API class Labels < Grape::API
include PaginationParams include PaginationParams
before { authenticate! } before { authenticate! }
params do params do
...@@ -56,7 +56,7 @@ module API ...@@ -56,7 +56,7 @@ module API
label = user_project.labels.find_by(title: params[:name]) label = user_project.labels.find_by(title: params[:name])
not_found!('Label') unless label not_found!('Label') unless label
present label.destroy, with: Entities::Label, current_user: current_user, project: user_project label.destroy
end end
desc 'Update an existing label. At least one optional parameter is required.' do desc 'Update an existing label. At least one optional parameter is required.' do
......
...@@ -93,24 +93,10 @@ module API ...@@ -93,24 +93,10 @@ module API
end end
delete ":id/members/:user_id" do delete ":id/members/:user_id" do
source = find_source(source_type, params[:id]) 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 ::Members::DestroyService.new(source, current_user, declared_params).execute
# 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
end end
end end
end end
......
...@@ -132,8 +132,6 @@ module API ...@@ -132,8 +132,6 @@ module API
authorize! :admin_note, note authorize! :admin_note, note
::Notes::DestroyService.new(user_project, current_user).execute(note) ::Notes::DestroyService.new(user_project, current_user).execute(note)
present note, with: Entities::Note
end end
end end
end end
......
...@@ -90,12 +90,9 @@ module API ...@@ -90,12 +90,9 @@ module API
requires :hook_id, type: Integer, desc: 'The ID of the hook to delete' requires :hook_id, type: Integer, desc: 'The ID of the hook to delete'
end end
delete ":id/hooks/:hook_id" do delete ":id/hooks/:hook_id" do
begin hook = user_project.hooks.find(params.delete(:hook_id))
present user_project.hooks.destroy(params[:hook_id]), with: Entities::ProjectHook
rescue hook.destroy
# ProjectHook can raise Error if hook_id not found
not_found!("Error deleting hook #{params[:hook_id]}")
end
end end
end end
end end
......
...@@ -353,7 +353,6 @@ module API ...@@ -353,7 +353,6 @@ module API
not_found!('Group Link') unless link not_found!('Group Link') unless link
link.destroy link.destroy
no_content!
end end
desc 'Upload a file' desc 'Upload a file'
......
...@@ -78,9 +78,8 @@ module API ...@@ -78,9 +78,8 @@ module API
delete ':id' do delete ':id' do
runner = get_runner(params[:id]) runner = get_runner(params[:id])
authenticate_delete_runner!(runner) authenticate_delete_runner!(runner)
runner.destroy!
present runner, with: Entities::Runner runner.destroy!
end end
end end
...@@ -136,8 +135,6 @@ module API ...@@ -136,8 +135,6 @@ module API
forbidden!("Only one project associated with the runner. Please remove the runner instead") if runner.projects.count == 1 forbidden!("Only one project associated with the runner. Please remove the runner instead") if runner.projects.count == 1
runner_project.destroy runner_project.destroy
present runner, with: Entities::Runner
end end
end end
......
...@@ -654,9 +654,7 @@ module API ...@@ -654,9 +654,7 @@ module API
hash.merge!(key => nil) hash.merge!(key => nil)
end end
if service.update_attributes(attrs.merge(active: false)) unless service.update_attributes(attrs.merge(active: false))
true
else
render_api_error!('400 Bad Request', 400) render_api_error!('400 Bad Request', 400)
end end
end end
......
...@@ -118,9 +118,10 @@ module API ...@@ -118,9 +118,10 @@ module API
delete ':id' do delete ':id' do
snippet = snippets_for_current_user.find_by(id: params.delete(:id)) snippet = snippets_for_current_user.find_by(id: params.delete(:id))
return not_found!('Snippet') unless snippet return not_found!('Snippet') unless snippet
authorize! :destroy_personal_snippet, snippet authorize! :destroy_personal_snippet, snippet
snippet.destroy snippet.destroy
no_content!
end end
desc 'Get a raw snippet' do desc 'Get a raw snippet' do
......
...@@ -66,7 +66,7 @@ module API ...@@ -66,7 +66,7 @@ module API
hook = SystemHook.find_by(id: params[:id]) hook = SystemHook.find_by(id: params[:id])
not_found!('System hook') unless hook not_found!('System hook') unless hook
present hook.destroy, with: Entities::Hook hook.destroy
end end
end end
end end
......
...@@ -66,11 +66,7 @@ module API ...@@ -66,11 +66,7 @@ module API
result = ::Tags::DestroyService.new(user_project, current_user). result = ::Tags::DestroyService.new(user_project, current_user).
execute(params[:tag_name]) execute(params[:tag_name])
if result[:status] == :success if result[:status] != :success
{
tag_name: params[:tag_name]
}
else
render_api_error!(result[:message], result[:return_code]) render_api_error!(result[:message], result[:return_code])
end end
end end
......
...@@ -93,8 +93,6 @@ module API ...@@ -93,8 +93,6 @@ module API
return not_found!('Trigger') unless trigger return not_found!('Trigger') unless trigger
trigger.destroy trigger.destroy
present trigger, with: Entities::Trigger
end end
end end
end end
......
...@@ -236,7 +236,7 @@ module API ...@@ -236,7 +236,7 @@ module API
key = user.keys.find_by(id: params[:key_id]) key = user.keys.find_by(id: params[:key_id])
not_found!('Key') unless key not_found!('Key') unless key
present key.destroy, with: Entities::SSHKey key.destroy
end end
desc 'Add an email address to a specified user. Available only for admins.' do desc 'Add an email address to a specified user. Available only for admins.' do
...@@ -422,7 +422,7 @@ module API ...@@ -422,7 +422,7 @@ module API
key = current_user.keys.find_by(id: params[:key_id]) key = current_user.keys.find_by(id: params[:key_id])
not_found!('Key') unless key not_found!('Key') unless key
present key.destroy, with: Entities::SSHKey key.destroy
end end
desc "Get the currently authenticated user's email addresses" do desc "Get the currently authenticated user's email addresses" do
......
...@@ -81,10 +81,9 @@ module API ...@@ -81,10 +81,9 @@ module API
end end
delete ':id/variables/:key' do delete ':id/variables/:key' do
variable = user_project.variables.find_by(key: params[:key]) variable = user_project.variables.find_by(key: params[:key])
return not_found!('Variable') unless variable return not_found!('Variable') unless variable
present variable.destroy, with: Entities::Variable variable.destroy
end end
end end
end end
......
...@@ -200,7 +200,7 @@ describe API::AccessRequests, api: true do ...@@ -200,7 +200,7 @@ describe API::AccessRequests, api: true do
expect do expect do
delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", access_requester) 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.to change { source.requesters.count }.by(-1)
end end
end end
...@@ -210,7 +210,7 @@ describe API::AccessRequests, api: true do ...@@ -210,7 +210,7 @@ describe API::AccessRequests, api: true do
expect do expect do
delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", master) 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.to change { source.requesters.count }.by(-1)
end end
......
...@@ -242,9 +242,9 @@ describe API::AwardEmoji, api: true do ...@@ -242,9 +242,9 @@ describe API::AwardEmoji, api: true do
it 'deletes the award' do it 'deletes the award' do
expect do expect do
delete api("/projects/#{project.id}/issues/#{issue.id}/award_emoji/#{award_emoji.id}", user) 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 end
it 'returns a 404 error when the award emoji can not be found' do it 'returns a 404 error when the award emoji can not be found' do
...@@ -258,9 +258,9 @@ describe API::AwardEmoji, api: true do ...@@ -258,9 +258,9 @@ describe API::AwardEmoji, api: true do
it 'deletes the award' do it 'deletes the award' do
expect do expect do
delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/award_emoji/#{downvote.id}", user) 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 end
it 'returns a 404 error when note id not found' do it 'returns a 404 error when note id not found' do
...@@ -277,9 +277,9 @@ describe API::AwardEmoji, api: true do ...@@ -277,9 +277,9 @@ describe API::AwardEmoji, api: true do
it 'deletes the award' do it 'deletes the award' do
expect do expect do
delete api("/projects/#{project.id}/snippets/#{snippet.id}/award_emoji/#{award.id}", user) 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 end
end end
...@@ -290,9 +290,9 @@ describe API::AwardEmoji, api: true do ...@@ -290,9 +290,9 @@ describe API::AwardEmoji, api: true do
it 'deletes the award' do it 'deletes the award' do
expect do expect do
delete api("/projects/#{project.id}/issues/#{issue.id}/notes/#{note.id}/award_emoji/#{rocket.id}", user) 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 end
end end
...@@ -195,8 +195,7 @@ describe API::Boards, api: true do ...@@ -195,8 +195,7 @@ describe API::Boards, api: true do
it "deletes the list if an admin requests it" do it "deletes the list if an admin requests it" do
delete api("#{base_url}/#{dev_list.id}", owner) delete api("#{base_url}/#{dev_list.id}", owner)
expect(response).to have_http_status(200) expect(response).to have_http_status(204)
expect(json_response['position']).to eq(1)
end end
end end
end end
......
...@@ -325,15 +325,14 @@ describe API::Branches, api: true do ...@@ -325,15 +325,14 @@ describe API::Branches, api: true do
it "removes branch" do it "removes branch" do
delete api("/projects/#{project.id}/repository/branches/#{branch_name}", user) 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 end
it "removes a branch with dots in the branch name" do it "removes a branch with dots in the branch name" do
delete api("/projects/#{project.id}/repository/branches/with.1.2.3", user) delete api("/projects/#{project.id}/repository/branches/with.1.2.3", user)
expect(response).to have_http_status(200) expect(response).to have_http_status(204)
expect(json_response['branch']).to eq("with.1.2.3")
end end
it 'returns 404 if branch not exists' do it 'returns 404 if branch not exists' do
......
...@@ -174,8 +174,11 @@ describe API::BroadcastMessages, api: true do ...@@ -174,8 +174,11 @@ describe API::BroadcastMessages, api: true do
end end
it 'deletes the broadcast message for admins' do it 'deletes the broadcast message for admins' do
expect { delete api("/broadcast_messages/#{message.id}", admin) } expect do
.to change { BroadcastMessage.count }.by(-1) delete api("/broadcast_messages/#{message.id}", admin)
expect(response).to have_http_status(204)
end.to change { BroadcastMessage.count }.by(-1)
end end
end end
end end
...@@ -116,6 +116,8 @@ describe API::DeployKeys, api: true do ...@@ -116,6 +116,8 @@ describe API::DeployKeys, api: true do
it 'should delete existing key' do it 'should delete existing key' do
expect do expect do
delete api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin) 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.to change{ project.deploy_keys.count }.by(-1)
end end
......
...@@ -122,7 +122,7 @@ describe API::Environments, api: true do ...@@ -122,7 +122,7 @@ describe API::Environments, api: true do
it 'returns a 200 for an existing environment' do it 'returns a 200 for an existing environment' do
delete api("/projects/#{project.id}/environments/#{environment.id}", user) delete api("/projects/#{project.id}/environments/#{environment.id}", user)
expect(response).to have_http_status(200) expect(response).to have_http_status(204)
end end
it 'returns a 404 for non existing id' do it 'returns a 404 for non existing id' do
......
...@@ -201,11 +201,7 @@ describe API::Files, api: true do ...@@ -201,11 +201,7 @@ describe API::Files, api: true do
it "deletes existing file in project repo" do it "deletes existing file in project repo" do
delete api("/projects/#{project.id}/repository/files", user), valid_params delete api("/projects/#{project.id}/repository/files", user), valid_params
expect(response).to have_http_status(200) expect(response).to have_http_status(204)
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)
end end
it "returns a 400 bad request if no params given" do it "returns a 400 bad request if no params given" do
...@@ -228,10 +224,7 @@ describe API::Files, api: true do ...@@ -228,10 +224,7 @@ describe API::Files, api: true do
delete api("/projects/#{project.id}/repository/files", user), valid_params delete api("/projects/#{project.id}/repository/files", user), valid_params
expect(response).to have_http_status(200) expect(response).to have_http_status(204)
last_commit = project.repository.commit.raw
expect(last_commit.author_email).to eq(author_email)
expect(last_commit.author_name).to eq(author_name)
end end
end end
end end
......
...@@ -467,7 +467,7 @@ describe API::Groups, api: true do ...@@ -467,7 +467,7 @@ describe API::Groups, api: true do
it "removes group" do it "removes group" do
delete api("/groups/#{group1.id}", user1) delete api("/groups/#{group1.id}", user1)
expect(response).to have_http_status(200) expect(response).to have_http_status(204)
end end
it "does not remove a group if not an owner" do it "does not remove a group if not an owner" do
...@@ -496,7 +496,7 @@ describe API::Groups, api: true do ...@@ -496,7 +496,7 @@ describe API::Groups, api: true do
it "removes any existing group" do it "removes any existing group" do
delete api("/groups/#{group2.id}", admin) delete api("/groups/#{group2.id}", admin)
expect(response).to have_http_status(200) expect(response).to have_http_status(204)
end end
it "does not remove a non existing group" do it "does not remove a non existing group" do
......
...@@ -1175,8 +1175,8 @@ describe API::Issues, api: true do ...@@ -1175,8 +1175,8 @@ describe API::Issues, api: true do
it "deletes the issue if an admin requests it" do it "deletes the issue if an admin requests it" do
delete api("/projects/#{project.id}/issues/#{issue.id}", owner) 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
end end