New AccessRequests API endpoints for Group & Project

Also, mutualize AccessRequests and Members endpoints for Group &
Project.
New API documentation for the AccessRequests endpoints.

Signed-off-by: Rémy Coutable <remy@rymai.me>

7 files changed, 590 insertions, 25 deletions

diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index 44cd3c08718..2277f4e13bf 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -10,7 +10,7 @@ describe Member, models: true do
 
     it { is_expected.to validate_presence_of(:user) }
     it { is_expected.to validate_presence_of(:source) }
-    it { is_expected.to validate_inclusion_of(:access_level).in_array(Gitlab::Access.values) }
+    it { is_expected.to validate_inclusion_of(:access_level).in_array(Gitlab::Access.all_values) }
 
     it_behaves_like 'an object with email-formated attributes', :invite_email do
       subject { build(:project_member) }
diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb
index 28673de3189..913d74645a7 100644
--- a/spec/models/members/project_member_spec.rb
+++ b/spec/models/members/project_member_spec.rb
@@ -27,6 +27,7 @@ describe ProjectMember, models: true do
   describe 'validations' do
     it { is_expected.to allow_value('Project').for(:source_type) }
     it { is_expected.not_to allow_value('project').for(:source_type) }
+    it { is_expected.to validate_inclusion_of(:access_level).in_array(Gitlab::Access.values) }
   end
 
   describe 'modules' do
@@ -40,7 +41,7 @@ describe ProjectMember, models: true do
   end
 
   describe "#destroy" do
-    let(:owner)   { create(:project_member, access_level: ProjectMember::OWNER) }
+    let(:owner)   { create(:project_member, access_level: ProjectMember::MASTER) }
     let(:project) { owner.project }
     let(:master)  { create(:project_member, project: project) }
 
diff --git a/spec/requests/api/access_requests_spec.rb b/spec/requests/api/access_requests_spec.rb
new file mode 100644
index 00000000000..d78494b76fa
--- /dev/null
+++ b/spec/requests/api/access_requests_spec.rb
@@ -0,0 +1,246 @@
+require 'spec_helper'
+
+describe API::AccessRequests, api: true  do
+  include ApiHelpers
+
+  let(:master) { create(:user) }
+  let(:developer) { create(:user) }
+  let(:access_requester) { create(:user) }
+  let(:stranger) { create(:user) }
+
+  let(:project) do
+    project = create(:project, :public, creator_id: master.id, namespace: master.namespace)
+    project.team << [developer, :developer]
+    project.team << [master, :master]
+    project.request_access(access_requester)
+    project
+  end
+
+  let(:group) do
+    group = create(:group, :public)
+    group.add_developer(developer)
+    group.add_owner(master)
+    group.request_access(access_requester)
+    group
+  end
+
+  shared_examples 'GET /:sources/:id/access_requests' do |source_type|
+    context "with :sources == #{source_type.pluralize}" do
+      it_behaves_like 'a 404 response when source is private' do
+        let(:route) { get api("/#{source_type.pluralize}/#{source.id}/access_requests", stranger) }
+      end
+
+      context 'when authenticated as a non-master/owner' do
+        %i[developer access_requester stranger].each do |type|
+          context "as a #{type}" do
+            it 'returns 403' do
+              user = public_send(type)
+              get api("/#{source_type.pluralize}/#{source.id}/access_requests", user)
+
+              expect(response).to have_http_status(403)
+            end
+          end
+        end
+      end
+
+      context 'when authenticated as a master/owner' do
+        it 'returns access requesters' do
+          get api("/#{source_type.pluralize}/#{source.id}/access_requests", master)
+
+          expect(response).to have_http_status(200)
+          expect(json_response).to be_an Array
+          expect(json_response.size).to eq(1)
+        end
+      end
+    end
+  end
+
+  shared_examples 'POST /:sources/:id/access_requests' do |source_type|
+    context "with :sources == #{source_type.pluralize}" do
+      it_behaves_like 'a 404 response when source is private' do
+        let(:route) { post api("/#{source_type.pluralize}/#{source.id}/access_requests", stranger) }
+      end
+
+      context 'when authenticated as a member' do
+        %i[developer master].each do |type|
+          context "as a #{type}" do
+            it 'returns 400' do
+              expect do
+                user = public_send(type)
+                post api("/#{source_type.pluralize}/#{source.id}/access_requests", user)
+
+                expect(response).to have_http_status(400)
+              end.not_to change { source.requesters.count }
+            end
+          end
+        end
+      end
+
+      context 'when authenticated as an access requester' do
+        it 'returns 400' do
+          expect do
+            post api("/#{source_type.pluralize}/#{source.id}/access_requests", access_requester)
+
+            expect(response).to have_http_status(400)
+          end.not_to change { source.requesters.count }
+        end
+      end
+
+      context 'when authenticated as a stranger' do
+        it 'returns 201' do
+          expect do
+            post api("/#{source_type.pluralize}/#{source.id}/access_requests", stranger)
+
+            expect(response).to have_http_status(201)
+          end.to change { source.requesters.count }.by(1)
+
+          # User attributes
+          expect(json_response['id']).to eq(stranger.id)
+          expect(json_response['name']).to eq(stranger.name)
+          expect(json_response['username']).to eq(stranger.username)
+          expect(json_response['state']).to eq(stranger.state)
+          expect(json_response['avatar_url']).to eq(stranger.avatar_url)
+          expect(json_response['web_url']).to eq(Gitlab::Routing.url_helpers.user_url(stranger))
+
+          # Member attributes
+          expect(json_response['requested_at']).to be_present
+        end
+      end
+    end
+  end
+
+  shared_examples 'PUT /:sources/:id/access_requests/:user_id/approve' do |source_type|
+    context "with :sources == #{source_type.pluralize}" do
+      it_behaves_like 'a 404 response when source is private' do
+        let(:route) { put api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}/approve", stranger) }
+      end
+
+      context 'when authenticated as a non-master/owner' do
+        %i[developer access_requester stranger].each do |type|
+          context "as a #{type}" do
+            it 'returns 403' do
+              user = public_send(type)
+              put api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}/approve", user)
+
+              expect(response).to have_http_status(403)
+            end
+          end
+        end
+      end
+
+      context 'when authenticated as a master/owner' do
+        it 'returns 201' do
+          expect do
+            put api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}/approve", master),
+                access_level: Member::MASTER
+
+            expect(response).to have_http_status(201)
+          end.to change { source.members.count }.by(1)
+          # User attributes
+          expect(json_response['id']).to eq(access_requester.id)
+          expect(json_response['name']).to eq(access_requester.name)
+          expect(json_response['username']).to eq(access_requester.username)
+          expect(json_response['state']).to eq(access_requester.state)
+          expect(json_response['avatar_url']).to eq(access_requester.avatar_url)
+          expect(json_response['web_url']).to eq(Gitlab::Routing.url_helpers.user_url(access_requester))
+
+          # Member attributes
+          expect(json_response['access_level']).to eq(Member::MASTER)
+        end
+
+        context 'user_id does not match an existing access requester' do
+          it 'returns 404' do
+            expect do
+              put api("/#{source_type.pluralize}/#{source.id}/access_requests/#{stranger.id}/approve", master)
+
+              expect(response).to have_http_status(404)
+            end.not_to change { source.members.count }
+          end
+        end
+      end
+    end
+  end
+
+  shared_examples 'DELETE /:sources/:id/access_requests/:user_id' do |source_type|
+    context "with :sources == #{source_type.pluralize}" do
+      it_behaves_like 'a 404 response when source is private' do
+        let(:route) { delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", stranger) }
+      end
+
+      context 'when authenticated as a non-master/owner' do
+        %i[developer stranger].each do |type|
+          context "as a #{type}" do
+            it 'returns 403' do
+              user = public_send(type)
+              delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", user)
+
+              expect(response).to have_http_status(403)
+            end
+          end
+        end
+      end
+
+      context 'when authenticated as the access requester' do
+        it 'returns 200' do
+          expect do
+            delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", access_requester)
+
+            expect(response).to have_http_status(200)
+          end.to change { source.requesters.count }.by(-1)
+        end
+      end
+
+      context 'when authenticated as a master/owner' do
+        it 'returns 200' do
+          expect do
+            delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", master)
+
+            expect(response).to have_http_status(200)
+          end.to change { source.requesters.count }.by(-1)
+        end
+
+        context 'user_id does not match an existing access requester' do
+          it 'returns 404' do
+            expect do
+              delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{stranger.id}", master)
+
+              expect(response).to have_http_status(404)
+            end.not_to change { source.requesters.count }
+          end
+        end
+      end
+    end
+  end
+
+  it_behaves_like 'GET /:sources/:id/access_requests', 'project' do
+    let(:source) { project }
+  end
+
+  it_behaves_like 'GET /:sources/:id/access_requests', 'group' do
+    let(:source) { group }
+  end
+
+  it_behaves_like 'POST /:sources/:id/access_requests', 'project' do
+    let(:source) { project }
+  end
+
+  it_behaves_like 'POST /:sources/:id/access_requests', 'group' do
+    let(:source) { group }
+  end
+
+  it_behaves_like 'PUT /:sources/:id/access_requests/:user_id/approve', 'project' do
+    let(:source) { project }
+  end
+
+  it_behaves_like 'PUT /:sources/:id/access_requests/:user_id/approve', 'group' do
+    let(:source) { group }
+  end
+
+  it_behaves_like 'DELETE /:sources/:id/access_requests/:user_id', 'project' do
+    let(:source) { project }
+  end
+
+  it_behaves_like 'DELETE /:sources/:id/access_requests/:user_id', 'group' do
+    let(:source) { group }
+  end
+end
diff --git a/spec/requests/api/group_members_spec.rb b/spec/requests/api/group_members_spec.rb
index 8bd6a8062ae..cdb3d7ddf4a 100644
--- a/spec/requests/api/group_members_spec.rb
+++ b/spec/requests/api/group_members_spec.rb
@@ -96,9 +96,9 @@ describe API::API, api: true  do
         expect(response).to have_http_status(400)
       end
 
-      it "returns a 422 error when access level is not known" do
+      it "returns a 400 error when access level is not known" do
         post api("/groups/#{group_no_members.id}/members", owner), user_id: master.id, access_level: 1234
-        expect(response).to have_http_status(422)
+        expect(response).to have_http_status(400)
       end
     end
   end
@@ -156,12 +156,12 @@ describe API::API, api: true  do
         expect(response).to have_http_status(400)
       end
 
-      it 'returns a 422 error when access level is not known' do
+      it 'returns a 400 error when access level is not known' do
         put(
           api("/groups/#{group_with_members.id}/members/#{master.id}", owner),
           access_level: 1234
         )
-        expect(response).to have_http_status(422)
+        expect(response).to have_http_status(400)
       end
     end
   end
@@ -182,7 +182,7 @@ describe API::API, api: true  do
           delete api("/groups/#{group_with_members.id}/members/#{guest.id}", owner)
         end.to change { group_with_members.members.count }.by(-1)
 
-        expect(response).to have_http_status(200)
+        expect(response).to have_http_status(204)
       end
 
       it "returns a 404 error when user id is not known" do
diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb
new file mode 100644
index 00000000000..b11a16487ac
--- /dev/null
+++ b/spec/requests/api/members_spec.rb
@@ -0,0 +1,312 @@
+require 'spec_helper'
+
+describe API::Members, api: true  do
+  include ApiHelpers
+
+  let(:master) { create(:user) }
+  let(:developer) { create(:user) }
+  let(:access_requester) { create(:user) }
+  let(:stranger) { create(:user) }
+
+  let(:project) do
+    project = create(:project, :public, creator_id: master.id, namespace: master.namespace)
+    project.team << [developer, :developer]
+    project.team << [master, :master]
+    project.request_access(access_requester)
+    project
+  end
+
+  let!(:group) do
+    group = create(:group, :public)
+    group.add_developer(developer)
+    group.add_owner(master)
+    group.request_access(access_requester)
+    group
+  end
+
+  shared_examples 'GET /:sources/:id/members' do |source_type|
+    context "with :sources == #{source_type.pluralize}" do
+      it_behaves_like 'a 404 response when source is private' do
+        let(:route) { get api("/#{source_type.pluralize}/#{source.id}/members", stranger) }
+      end
+
+      context 'when authenticated as a non-member' do
+        %i[access_requester stranger].each do |type|
+          context "as a #{type}" do
+            it 'returns 200' do
+              user = public_send(type)
+              get api("/#{source_type.pluralize}/#{source.id}/members", user)
+
+              expect(response).to have_http_status(200)
+              expect(json_response.size).to eq(2)
+            end
+          end
+        end
+      end
+
+      it 'finds members with query string' do
+        get api("/#{source_type.pluralize}/#{source.id}/members", developer), query: master.username
+
+        expect(response).to have_http_status(200)
+        expect(json_response.count).to eq(1)
+        expect(json_response.first['username']).to eq(master.username)
+      end
+    end
+  end
+
+  shared_examples 'GET /:sources/:id/members/:user_id' do |source_type|
+    context "with :sources == #{source_type.pluralize}" do
+      it_behaves_like 'a 404 response when source is private' do
+        let(:route) { get api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", stranger) }
+      end
+
+      context 'when authenticated as a non-member' do
+        %i[access_requester stranger].each do |type|
+          context "as a #{type}" do
+            it 'returns 200' do
+              user = public_send(type)
+              get api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", user)
+
+              expect(response).to have_http_status(200)
+              # User attributes
+              expect(json_response['id']).to eq(developer.id)
+              expect(json_response['name']).to eq(developer.name)
+              expect(json_response['username']).to eq(developer.username)
+              expect(json_response['state']).to eq(developer.state)
+              expect(json_response['avatar_url']).to eq(developer.avatar_url)
+              expect(json_response['web_url']).to eq(Gitlab::Routing.url_helpers.user_url(developer))
+
+              # Member attributes
+              expect(json_response['access_level']).to eq(Member::DEVELOPER)
+            end
+          end
+        end
+      end
+    end
+  end
+
+  shared_examples 'POST /:sources/:id/members' do |source_type|
+    context "with :sources == #{source_type.pluralize}" do
+      it_behaves_like 'a 404 response when source is private' do
+        let(:route) { post api("/#{source_type.pluralize}/#{source.id}/members", stranger) }
+      end
+
+      context 'when authenticated as a non-member or member with insufficient rights' do
+        %i[access_requester stranger developer].each do |type|
+          context "as a #{type}" do
+            it 'returns 403' do
+              user = public_send(type)
+              post api("/#{source_type.pluralize}/#{source.id}/members", user)
+
+              expect(response).to have_http_status(403)
+            end
+          end
+        end
+      end
+
+      context 'when authenticated as a master/owner' do
+        context 'and new member is already a requester' do
+          it 'transforms the requester into a proper member' do
+            expect do
+              post api("/#{source_type.pluralize}/#{source.id}/members", master),
+                   user_id: access_requester.id, access_level: Member::MASTER
+
+              expect(response).to have_http_status(201)
+            end.to change { source.members.count }.by(1)
+            expect(source.requesters.count).to eq(0)
+            expect(json_response['id']).to eq(access_requester.id)
+            expect(json_response['access_level']).to eq(Member::MASTER)
+          end
+        end
+
+        it 'creates a new member' do
+          expect do
+            post api("/#{source_type.pluralize}/#{source.id}/members", master),
+                 user_id: stranger.id, access_level: Member::DEVELOPER
+
+            expect(response).to have_http_status(201)
+          end.to change { source.members.count }.by(1)
+          expect(json_response['id']).to eq(stranger.id)
+          expect(json_response['access_level']).to eq(Member::DEVELOPER)
+        end
+      end
+
+      it "returns #{source_type == 'project' ? 201 : 409} if member already exists" do
+        post api("/#{source_type.pluralize}/#{source.id}/members", master),
+             user_id: master.id, access_level: Member::MASTER
+
+        expect(response).to have_http_status(409)
+      end
+
+      it 'returns 400 when user_id is not given' do
+        post api("/#{source_type.pluralize}/#{source.id}/members", master),
+             access_level: Member::MASTER
+
+        expect(response).to have_http_status(400)
+      end
+
+      it 'returns 400 when access_level is not given' do
+        post api("/#{source_type.pluralize}/#{source.id}/members", master),
+             user_id: stranger.id
+
+        expect(response).to have_http_status(400)
+      end
+
+      it 'returns 400 when access_level is not valid' do
+        post api("/#{source_type.pluralize}/#{source.id}/members", master),
+             user_id: stranger.id, access_level: 1234
+
+        expect(response).to have_http_status(400)
+      end
+    end
+  end
+
+  shared_examples 'PUT /:sources/:id/members/:user_id' do |source_type|
+    context "with :sources == #{source_type.pluralize}" do
+      it_behaves_like 'a 404 response when source is private' do
+        let(:route) { put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", stranger) }
+      end
+
+      context 'when authenticated as a non-member or member with insufficient rights' do
+        %i[access_requester stranger developer].each do |type|
+          context "as a #{type}" do
+            it 'returns 403' do
+              user = public_send(type)
+              put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", user)
+
+              expect(response).to have_http_status(403)
+            end
+          end
+        end
+      end
+
+      context 'when authenticated as a master/owner' do
+        it 'updates the member' do
+          put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master),
+              access_level: Member::MASTER
+
+          expect(response).to have_http_status(200)
+          expect(json_response['id']).to eq(developer.id)
+          expect(json_response['access_level']).to eq(Member::MASTER)
+        end
+      end
+
+      it 'returns 409 if member does not exist' do
+        put api("/#{source_type.pluralize}/#{source.id}/members/123", master),
+            access_level: Member::MASTER
+
+        expect(response).to have_http_status(404)
+      end
+
+      it 'returns 400 when access_level is not given' do
+        put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master)
+
+        expect(response).to have_http_status(400)
+      end
+
+      it 'returns 400 when access level is not valid' do
+        put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master),
+            access_level: 1234
+
+        expect(response).to have_http_status(400)
+      end
+    end
+  end
+
+  shared_examples 'DELETE /:sources/:id/members/:user_id' do |source_type|
+    context "with :sources == #{source_type.pluralize}" do
+      it_behaves_like 'a 404 response when source is private' do
+        let(:route) { delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", stranger) }
+      end
+
+      context 'when authenticated as a non-member or member with insufficient rights' do
+        %i[access_requester stranger].each do |type|
+          context "as a #{type}" do
+            it 'returns 403' do
+              user = public_send(type)
+              delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", user)
+
+              expect(response).to have_http_status(403)
+            end
+          end
+        end
+      end
+
+      context 'when authenticated as a member and deleting themself' do
+        it 'deletes the member' do
+          expect do
+            delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", developer)
+
+            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 404' do
+            expect do
+              delete api("/#{source_type.pluralize}/#{source.id}/members/#{access_requester.id}", master)
+
+              expect(response).to have_http_status(404)
+            end.not_to change { source.requesters.count }
+          end
+        end
+
+        it 'deletes the member' do
+          expect do
+            delete api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master)
+
+            expect(response).to have_http_status(204)
+          end.to change { source.members.count }.by(-1)
+        end
+      end
+
+      it 'returns 409 if member does not exist' do
+        delete api("/#{source_type.pluralize}/#{source.id}/members/123", master)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
+
+  it_behaves_like 'GET /:sources/:id/members', 'project' do
+    let(:source) { project }
+  end
+
+  it_behaves_like 'GET /:sources/:id/members', 'group' do
+    let(:source) { group }
+  end
+
+  it_behaves_like 'GET /:sources/:id/members/:user_id', 'project' do
+    let(:source) { project }
+  end
+
+  it_behaves_like 'GET /:sources/:id/members/:user_id', 'group' do
+    let(:source) { group }
+  end
+
+  it_behaves_like 'POST /:sources/:id/members', 'project' do
+    let(:source) { project }
+  end
+
+  it_behaves_like 'POST /:sources/:id/members', 'group' do
+    let(:source) { group }
+  end
+
+  it_behaves_like 'PUT /:sources/:id/members/:user_id', 'project' do
+    let(:source) { project }
+  end
+
+  it_behaves_like 'PUT /:sources/:id/members/:user_id', 'group' do
+    let(:source) { group }
+  end
+
+  it_behaves_like 'DELETE /:sources/:id/members/:user_id', 'project' do
+    let(:source) { project }
+  end
+
+  it_behaves_like 'DELETE /:sources/:id/members/:user_id', 'group' do
+    let(:source) { group }
+  end
+end
diff --git a/spec/requests/api/project_members_spec.rb b/spec/requests/api/project_members_spec.rb
index 13cc0d81ac8..195d5bb3f8f 100644
--- a/spec/requests/api/project_members_spec.rb
+++ b/spec/requests/api/project_members_spec.rb
@@ -62,7 +62,7 @@ describe API::API, api: true  do
       expect(json_response['access_level']).to eq(ProjectMember::DEVELOPER)
     end
 
-    it "returns a 201 status if user is already project member" do
+    it "returns a 409 status if user is already project member" do
       post api("/projects/#{project.id}/members", user),
            user_id: user2.id,
            access_level: ProjectMember::DEVELOPER
@@ -70,9 +70,7 @@ describe API::API, api: true  do
         post api("/projects/#{project.id}/members", user), user_id: user2.id, access_level: ProjectMember::DEVELOPER
       end.not_to change { ProjectMember.count }
 
-      expect(response).to have_http_status(201)
-      expect(json_response['username']).to eq(user2.username)
-      expect(json_response['access_level']).to eq(ProjectMember::DEVELOPER)
+      expect(response).to have_http_status(409)
     end
 
     it "returns a 400 error when user id is not given" do
@@ -85,9 +83,9 @@ describe API::API, api: true  do
       expect(response).to have_http_status(400)
     end
 
-    it "returns a 422 error when access level is not known" do
+    it "returns a 400 error when access level is not known" do
       post api("/projects/#{project.id}/members", user), user_id: user2.id, access_level: 1234
-      expect(response).to have_http_status(422)
+      expect(response).to have_http_status(400)
     end
   end
 
@@ -111,9 +109,9 @@ describe API::API, api: true  do
       expect(response).to have_http_status(400)
     end
 
-    it "returns a 422 error when access level is not known" do
+    it "returns a 400 error when access level is not known" do
       put api("/projects/#{project.id}/members/#{user3.id}", user), access_level: 123
-      expect(response).to have_http_status(422)
+      expect(response).to have_http_status(400)
     end
   end
 
@@ -129,27 +127,25 @@ describe API::API, api: true  do
       end.to change { ProjectMember.count }.by(-1)
     end
 
-    it "returns 200 if team member is not part of a project" do
+    it "returns 404 if team member is not part of a project" do
       delete api("/projects/#{project.id}/members/#{user3.id}", user)
       expect do
         delete api("/projects/#{project.id}/members/#{user3.id}", user)
       end.not_to change { ProjectMember.count }
-      expect(response).to have_http_status(200)
+      expect(response).to have_http_status(404)
     end
 
-    it "returns 200 if team member already removed" do
+    it "returns 404 if team member already removed" do
       delete api("/projects/#{project.id}/members/#{user3.id}", user)
       delete api("/projects/#{project.id}/members/#{user3.id}", user)
-      expect(response).to have_http_status(200)
+      expect(response).to have_http_status(404)
     end
 
-    it "returns 200 OK when the user was not member" do
+    it "returns 404 when the user was not member" do
       expect do
         delete api("/projects/#{project.id}/members/1000000", user)
       end.to change { ProjectMember.count }.by(0)
-      expect(response).to have_http_status(200)
-      expect(json_response['id']).to eq(1000000)
-      expect(json_response['message']).to eq('Access revoked')
+      expect(response).to have_http_status(404)
     end
 
     context 'when the user is not an admin or owner' do
@@ -158,8 +154,7 @@ describe API::API, api: true  do
           delete api("/projects/#{project.id}/members/#{user3.id}", user3)
         end.to change { ProjectMember.count }.by(-1)
 
-        expect(response).to have_http_status(200)
-        expect(json_response['id']).to eq(project_member2.id)
+        expect(response).to have_http_status(204)
       end
     end
   end
diff --git a/spec/support/api/members_shared_examples.rb b/spec/support/api/members_shared_examples.rb
new file mode 100644
index 00000000000..dab71a35a55
--- /dev/null
+++ b/spec/support/api/members_shared_examples.rb
@@ -0,0 +1,11 @@
+shared_examples 'a 404 response when source is private' do
+  before do
+    source.update_column(:visibility_level, Gitlab::VisibilityLevel::PRIVATE)
+  end
+
+  it 'returns 404' do
+    route
+
+    expect(response).to have_http_status(404)
+  end
+end