Commit 7c1b33b4 authored by Rémy Coutable's avatar Rémy Coutable

Restore back-compatibility for current members API endpoints

Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 29850364
...@@ -47,7 +47,6 @@ module API ...@@ -47,7 +47,6 @@ module API
end end
end end
# Deprecated
def user_project def user_project
@project ||= find_project(params[:id]) @project ||= find_project(params[:id])
end end
......
...@@ -65,14 +65,29 @@ module API ...@@ -65,14 +65,29 @@ module API
::Members::DestroyService.new(access_requester, access_requester.user).execute ::Members::DestroyService.new(access_requester, access_requester.user).execute
end end
conflict!('Member already exists') if source.members.exists?(user_id: params[:user_id])
source.add_user(params[:user_id], params[:access_level], current_user)
member = source.members.find_by(user_id: params[:user_id]) member = source.members.find_by(user_id: params[:user_id])
# This is to ensure back-compatibility but 409 behavior should be used
# for both project and group members in 9.0!
conflict!('Member already exists') if source_type == 'group' && member
unless member
source.add_user(params[:user_id], params[:access_level], current_user)
member = source.members.find_by(user_id: params[:user_id])
end
if member if member
present member.user, with: Entities::Member, member: member present member.user, with: Entities::Member, member: member
else else
render_api_error!('400 Bad Request', 400) # Since `source.add_user` doesn't return a member object, we have to
# build a new one and populate its errors in order to render them.
member = source.members.build(attributes_for_keys([:user_id, :access_level]))
member.valid? # populate the errors
# This is to ensure back-compatibility but 400 behavior should be used
# for all validation errors in 9.0!
render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level)
render_validation_error!(member)
end end
end end
...@@ -96,6 +111,9 @@ module API ...@@ -96,6 +111,9 @@ module API
if member.update_attributes(access_level: params[:access_level]) if member.update_attributes(access_level: params[:access_level])
present member.user, with: Entities::Member, member: member present member.user, with: Entities::Member, member: member
else else
# This is to ensure back-compatibility but 400 behavior should be used
# for all validation errors in 9.0!
render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level)
render_validation_error!(member) render_validation_error!(member)
end end
end end
...@@ -113,10 +131,23 @@ module API ...@@ -113,10 +131,23 @@ module API
source = find_source(source_type, params[:id]) source = find_source(source_type, params[:id])
required_attributes! [:user_id] required_attributes! [:user_id]
member = 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(member, current_user).execute
::Members::DestroyService.new(member, current_user).execute present member.user, with: Entities::Member, member: member
status :no_content end
end end
end end
end end
......
...@@ -96,9 +96,9 @@ describe API::API, api: true do ...@@ -96,9 +96,9 @@ describe API::API, api: true do
expect(response).to have_http_status(400) expect(response).to have_http_status(400)
end end
it "returns a 400 error when access level is not known" do it "returns a 422 error when access level is not known" do
post api("/groups/#{group_no_members.id}/members", owner), user_id: master.id, access_level: 1234 post api("/groups/#{group_no_members.id}/members", owner), user_id: master.id, access_level: 1234
expect(response).to have_http_status(400) expect(response).to have_http_status(422)
end end
end end
end end
...@@ -156,12 +156,12 @@ describe API::API, api: true do ...@@ -156,12 +156,12 @@ describe API::API, api: true do
expect(response).to have_http_status(400) expect(response).to have_http_status(400)
end end
it 'returns a 400 error when access level is not known' do it 'returns a 422 error when access level is not known' do
put( put(
api("/groups/#{group_with_members.id}/members/#{master.id}", owner), api("/groups/#{group_with_members.id}/members/#{master.id}", owner),
access_level: 1234 access_level: 1234
) )
expect(response).to have_http_status(400) expect(response).to have_http_status(422)
end end
end end
end end
...@@ -182,7 +182,7 @@ describe API::API, api: true do ...@@ -182,7 +182,7 @@ describe API::API, api: true do
delete api("/groups/#{group_with_members.id}/members/#{guest.id}", owner) delete api("/groups/#{group_with_members.id}/members/#{guest.id}", owner)
end.to change { group_with_members.members.count }.by(-1) end.to change { group_with_members.members.count }.by(-1)
expect(response).to have_http_status(204) expect(response).to have_http_status(200)
end end
it "returns a 404 error when user id is not known" do it "returns a 404 error when user id is not known" do
......
...@@ -135,7 +135,7 @@ describe API::Members, api: true do ...@@ -135,7 +135,7 @@ describe API::Members, api: true do
post api("/#{source_type.pluralize}/#{source.id}/members", master), post api("/#{source_type.pluralize}/#{source.id}/members", master),
user_id: master.id, access_level: Member::MASTER user_id: master.id, access_level: Member::MASTER
expect(response).to have_http_status(409) expect(response).to have_http_status(source_type == 'project' ? 201 : 409)
end end
it 'returns 400 when user_id is not given' do it 'returns 400 when user_id is not given' do
...@@ -152,11 +152,11 @@ describe API::Members, api: true do ...@@ -152,11 +152,11 @@ describe API::Members, api: true do
expect(response).to have_http_status(400) expect(response).to have_http_status(400)
end end
it 'returns 400 when access_level is not valid' do it 'returns 422 when access_level is not valid' do
post api("/#{source_type.pluralize}/#{source.id}/members", master), post api("/#{source_type.pluralize}/#{source.id}/members", master),
user_id: stranger.id, access_level: 1234 user_id: stranger.id, access_level: 1234
expect(response).to have_http_status(400) expect(response).to have_http_status(422)
end end
end end
end end
...@@ -204,11 +204,11 @@ describe API::Members, api: true do ...@@ -204,11 +204,11 @@ describe API::Members, api: true do
expect(response).to have_http_status(400) expect(response).to have_http_status(400)
end end
it 'returns 400 when access level is not valid' do it 'returns 422 when access level is not valid' do
put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master), put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master),
access_level: 1234 access_level: 1234
expect(response).to have_http_status(400) expect(response).to have_http_status(422)
end end
end end
end end
...@@ -237,18 +237,18 @@ describe API::Members, api: true do ...@@ -237,18 +237,18 @@ describe API::Members, api: true do
expect do expect do
delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", developer) delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", developer)
expect(response).to have_http_status(204) expect(response).to have_http_status(200)
end.to change { source.members.count }.by(-1) end.to change { source.members.count }.by(-1)
end end
end end
context 'when authenticated as a master/owner' do context 'when authenticated as a master/owner' do
context 'and member is a requester' do context 'and member is a requester' do
it 'returns 404' do it "returns #{source_type == 'project' ? 200 : 404}" do
expect do expect do
delete api("/#{source_type.pluralize}/#{source.id}/members/#{access_requester.id}", master) delete api("/#{source_type.pluralize}/#{source.id}/members/#{access_requester.id}", master)
expect(response).to have_http_status(404) expect(response).to have_http_status(source_type == 'project' ? 200 : 404)
end.not_to change { source.requesters.count } end.not_to change { source.requesters.count }
end end
end end
...@@ -257,15 +257,15 @@ describe API::Members, api: true do ...@@ -257,15 +257,15 @@ describe API::Members, api: true do
expect do expect do
delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master) delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master)
expect(response).to have_http_status(204) expect(response).to have_http_status(200)
end.to change { source.members.count }.by(-1) end.to change { source.members.count }.by(-1)
end end
end end
it 'returns 409 if member does not exist' do it "returns #{source_type == 'project' ? 200 : 404} if member does not exist" do
delete api("/#{source_type.pluralize}/#{source.id}/members/123", master) delete api("/#{source_type.pluralize}/#{source.id}/members/123", master)
expect(response).to have_http_status(404) expect(response).to have_http_status(source_type == 'project' ? 200 : 404)
end end
end end
end end
......
...@@ -62,7 +62,7 @@ describe API::API, api: true do ...@@ -62,7 +62,7 @@ describe API::API, api: true do
expect(json_response['access_level']).to eq(ProjectMember::DEVELOPER) expect(json_response['access_level']).to eq(ProjectMember::DEVELOPER)
end end
it "returns a 409 status if user is already project member" do it "returns a 201 status if user is already project member" do
post api("/projects/#{project.id}/members", user), post api("/projects/#{project.id}/members", user),
user_id: user2.id, user_id: user2.id,
access_level: ProjectMember::DEVELOPER access_level: ProjectMember::DEVELOPER
...@@ -70,7 +70,9 @@ describe API::API, api: true do ...@@ -70,7 +70,9 @@ describe API::API, api: true do
post api("/projects/#{project.id}/members", user), user_id: user2.id, access_level: ProjectMember::DEVELOPER post api("/projects/#{project.id}/members", user), user_id: user2.id, access_level: ProjectMember::DEVELOPER
end.not_to change { ProjectMember.count } end.not_to change { ProjectMember.count }
expect(response).to have_http_status(409) expect(response).to have_http_status(201)
expect(json_response['username']).to eq(user2.username)
expect(json_response['access_level']).to eq(ProjectMember::DEVELOPER)
end end
it "returns a 400 error when user id is not given" do it "returns a 400 error when user id is not given" do
...@@ -83,9 +85,9 @@ describe API::API, api: true do ...@@ -83,9 +85,9 @@ describe API::API, api: true do
expect(response).to have_http_status(400) expect(response).to have_http_status(400)
end end
it "returns a 400 error when access level is not known" do it "returns a 422 error when access level is not known" do
post api("/projects/#{project.id}/members", user), user_id: user2.id, access_level: 1234 post api("/projects/#{project.id}/members", user), user_id: user2.id, access_level: 1234
expect(response).to have_http_status(400) expect(response).to have_http_status(422)
end end
end end
...@@ -109,9 +111,9 @@ describe API::API, api: true do ...@@ -109,9 +111,9 @@ describe API::API, api: true do
expect(response).to have_http_status(400) expect(response).to have_http_status(400)
end end
it "returns a 400 error when access level is not known" do it "returns a 422 error when access level is not known" do
put api("/projects/#{project.id}/members/#{user3.id}", user), access_level: 123 put api("/projects/#{project.id}/members/#{user3.id}", user), access_level: 123
expect(response).to have_http_status(400) expect(response).to have_http_status(422)
end end
end end
...@@ -127,25 +129,27 @@ describe API::API, api: true do ...@@ -127,25 +129,27 @@ describe API::API, api: true do
end.to change { ProjectMember.count }.by(-1) end.to change { ProjectMember.count }.by(-1)
end end
it "returns 404 if team member is not part of a project" do it "returns 200 if team member is not part of a project" do
delete api("/projects/#{project.id}/members/#{user3.id}", user) delete api("/projects/#{project.id}/members/#{user3.id}", user)
expect do expect do
delete api("/projects/#{project.id}/members/#{user3.id}", user) delete api("/projects/#{project.id}/members/#{user3.id}", user)
end.not_to change { ProjectMember.count } end.not_to change { ProjectMember.count }
expect(response).to have_http_status(404) expect(response).to have_http_status(200)
end end
it "returns 404 if team member already removed" do it "returns 200 if team member already removed" do
delete api("/projects/#{project.id}/members/#{user3.id}", user) delete api("/projects/#{project.id}/members/#{user3.id}", user)
delete api("/projects/#{project.id}/members/#{user3.id}", user) delete api("/projects/#{project.id}/members/#{user3.id}", user)
expect(response).to have_http_status(404) expect(response).to have_http_status(200)
end end
it "returns 404 when the user was not member" do it "returns 200 OK when the user was not member" do
expect do expect do
delete api("/projects/#{project.id}/members/1000000", user) delete api("/projects/#{project.id}/members/1000000", user)
end.to change { ProjectMember.count }.by(0) end.to change { ProjectMember.count }.by(0)
expect(response).to have_http_status(404) expect(response).to have_http_status(200)
expect(json_response['id']).to eq(1000000)
expect(json_response['message']).to eq('Access revoked')
end end
context 'when the user is not an admin or owner' do context 'when the user is not an admin or owner' do
...@@ -154,7 +158,8 @@ describe API::API, api: true do ...@@ -154,7 +158,8 @@ describe API::API, api: true do
delete api("/projects/#{project.id}/members/#{user3.id}", user3) delete api("/projects/#{project.id}/members/#{user3.id}", user3)
end.to change { ProjectMember.count }.by(-1) end.to change { ProjectMember.count }.by(-1)
expect(response).to have_http_status(204) expect(response).to have_http_status(200)
expect(json_response['id']).to eq(user3.id)
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment