Commit ef24c625 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'rc-improve-members-destroy-service' into 'master'

Improve `Members::DestroyService`

Part of #21979.

See merge request !6267
parents 02dd690a 1592d57c
...@@ -15,18 +15,17 @@ module MembershipActions ...@@ -15,18 +15,17 @@ module MembershipActions
end end
def leave def leave
@member = membershipable.members.find_by(user_id: current_user) || member = Members::DestroyService.new(membershipable, current_user, user_id: current_user.id).
membershipable.requesters.find_by(user_id: current_user) execute(:all)
Members::DestroyService.new(@member, current_user).execute
source_type = @member.real_source_type.humanize(capitalize: false) source_type = membershipable.class.to_s.humanize(capitalize: false)
notice = notice =
if @member.request? if member.request?
"Your access request to the #{source_type} has been withdrawn." "Your access request to the #{source_type} has been withdrawn."
else else
"You left the \"#{@member.source.human_name}\" #{source_type}." "You left the \"#{membershipable.human_name}\" #{source_type}."
end end
redirect_path = @member.request? ? @member.source : [:dashboard, @member.real_source_type.tableize] redirect_path = member.request? ? member.source : [:dashboard, membershipable.class.to_s.tableize]
redirect_to redirect_path, notice: notice redirect_to redirect_path, notice: notice
end end
......
...@@ -40,10 +40,7 @@ class Groups::GroupMembersController < Groups::ApplicationController ...@@ -40,10 +40,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
end end
def destroy def destroy
@group_member = @group.members.find_by(id: params[:id]) || Members::DestroyService.new(@group, current_user, id: params[:id]).execute(:all)
@group.requesters.find_by(id: params[:id])
Members::DestroyService.new(@group_member, current_user).execute
respond_to do |format| respond_to do |format|
format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' } format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' }
......
...@@ -55,10 +55,8 @@ class Projects::ProjectMembersController < Projects::ApplicationController ...@@ -55,10 +55,8 @@ class Projects::ProjectMembersController < Projects::ApplicationController
end end
def destroy def destroy
@project_member = @project.members.find_by(id: params[:id]) || Members::DestroyService.new(@project, current_user, params).
@project.requesters.find_by(id: params[:id]) execute(:all)
Members::DestroyService.new(@project_member, current_user).execute
respond_to do |format| respond_to do |format|
format.html do format.html do
......
...@@ -14,6 +14,8 @@ module Members ...@@ -14,6 +14,8 @@ module Members
if member.request? && member.user != user if member.request? && member.user != user
notification_service.decline_access_request(member) notification_service.decline_access_request(member)
end end
member
end end
end end
end end
module Members module Members
class DestroyService < BaseService class DestroyService < BaseService
attr_accessor :member, :current_user include MembersHelper
def initialize(member, current_user) attr_accessor :source
@member = member
ALLOWED_SCOPES = %i[members requesters all]
def initialize(source, current_user, params = {})
@source = source
@current_user = current_user @current_user = current_user
@params = params
end end
def execute def execute(scope = :members)
unless member && can?(current_user, "destroy_#{member.type.underscore}".to_sym, member) raise "scope :#{scope} is not allowed!" unless ALLOWED_SCOPES.include?(scope)
raise Gitlab::Access::AccessDeniedError
end member = find_member!(scope)
raise Gitlab::Access::AccessDeniedError unless can_destroy_member?(member)
AuthorizedDestroyService.new(member, current_user).execute AuthorizedDestroyService.new(member, current_user).execute
end end
private
def find_member!(scope)
condition = params[:user_id] ? { user_id: params[:user_id] } : { id: params[:id] }
case scope
when :all
source.members.find_by(condition) ||
source.requesters.find_by!(condition)
else
source.public_send(scope).find_by!(condition)
end
end
def can_destroy_member?(member)
member && can?(current_user, action_member_permission(:destroy, member), member)
end
end end
end end
...@@ -75,9 +75,8 @@ module API ...@@ -75,9 +75,8 @@ module API
required_attributes! [:user_id] required_attributes! [:user_id]
source = find_source(source_type, params[:id]) source = find_source(source_type, params[:id])
access_requester = source.requesters.find_by!(user_id: params[:user_id]) ::Members::DestroyService.new(source, current_user, params).
execute(:requesters)
::Members::DestroyService.new(access_requester, current_user).execute
end end
end end
end end
......
...@@ -134,7 +134,7 @@ module API ...@@ -134,7 +134,7 @@ module API
if member.nil? if member.nil?
{ message: "Access revoked", id: params[:user_id].to_i } { message: "Access revoked", id: params[:user_id].to_i }
else else
::Members::DestroyService.new(member, current_user).execute ::Members::DestroyService.new(source, current_user, params).execute
present member.user, with: Entities::Member, member: member present member.user, with: Entities::Member, member: member
end end
......
...@@ -87,10 +87,10 @@ describe Groups::GroupMembersController do ...@@ -87,10 +87,10 @@ describe Groups::GroupMembersController do
context 'when member is not found' do context 'when member is not found' do
before { sign_in(user) } before { sign_in(user) }
it 'returns 403' do it 'returns 404' do
delete :leave, group_id: group delete :leave, group_id: group
expect(response).to have_http_status(403) expect(response).to have_http_status(404)
end end
end end
......
...@@ -135,11 +135,11 @@ describe Projects::ProjectMembersController do ...@@ -135,11 +135,11 @@ describe Projects::ProjectMembersController do
context 'when member is not found' do context 'when member is not found' do
before { sign_in(user) } before { sign_in(user) }
it 'returns 403' do it 'returns 404' do
delete :leave, namespace_id: project.namespace, delete :leave, namespace_id: project.namespace,
project_id: project project_id: project
expect(response).to have_http_status(403) expect(response).to have_http_status(404)
end end
end end
......
...@@ -195,7 +195,7 @@ describe API::AccessRequests, api: true do ...@@ -195,7 +195,7 @@ describe API::AccessRequests, api: true do
end end
context 'when authenticated as the access requester' do context 'when authenticated as the access requester' do
it 'returns 200' do it 'deletes the access requester' 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)
...@@ -205,7 +205,7 @@ describe API::AccessRequests, api: true do ...@@ -205,7 +205,7 @@ describe API::AccessRequests, api: true do
end end
context 'when authenticated as a master/owner' do context 'when authenticated as a master/owner' do
it 'returns 200' do it 'deletes the access requester' 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)
...@@ -213,6 +213,16 @@ describe API::AccessRequests, api: true do ...@@ -213,6 +213,16 @@ describe API::AccessRequests, api: true do
end.to change { source.requesters.count }.by(-1) end.to change { source.requesters.count }.by(-1)
end end
context 'user_id matches a member, not an access requester' do
it 'returns 404' do
expect do
delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{developer.id}", master)
expect(response).to have_http_status(404)
end.not_to change { source.requesters.count }
end
end
context 'user_id does not match an existing access requester' do context 'user_id does not match an existing access requester' do
it 'returns 404' do it 'returns 404' do
expect do expect do
......
...@@ -2,70 +2,111 @@ require 'spec_helper' ...@@ -2,70 +2,111 @@ require 'spec_helper'
describe Members::DestroyService, services: true do describe Members::DestroyService, services: true do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project) } let(:member_user) { create(:user) }
let!(:member) { create(:project_member, source: project) } let(:project) { create(:project, :public) }
let(:group) { create(:group, :public) }
context 'when member is nil' do shared_examples 'a service raising ActiveRecord::RecordNotFound' do
before do it 'raises ActiveRecord::RecordNotFound' do
project.team << [user, :developer] expect { described_class.new(source, user, params).execute }.to raise_error(ActiveRecord::RecordNotFound)
end
end end
it 'does not destroy the member' do shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
expect { destroy_member(nil, user) }.to raise_error(Gitlab::Access::AccessDeniedError) it 'raises Gitlab::Access::AccessDeniedError' do
expect { described_class.new(source, user, params).execute }.to raise_error(Gitlab::Access::AccessDeniedError)
end end
end end
context 'when current user cannot destroy the given member' do shared_examples 'a service destroying a member' do
it 'destroys the member' do
expect { described_class.new(source, user, params).execute }.to change { source.members.count }.by(-1)
end
context 'when the given member is an access requester' do
before do before do
project.team << [user, :developer] source.members.find_by(user_id: member_user).destroy
source.request_access(member_user)
end end
let(:access_requester) { source.requesters.find_by(user_id: member_user) }
it_behaves_like 'a service raising ActiveRecord::RecordNotFound'
it 'does not destroy the member' do %i[requesters all].each do |scope|
expect { destroy_member(member, user) }.to raise_error(Gitlab::Access::AccessDeniedError) context "and #{scope} scope is passed" do
it 'destroys the access requester' do
expect { described_class.new(source, user, params).execute(scope) }.to change { source.requesters.count }.by(-1)
end end
it 'calls Member#after_decline_request' do
expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(access_requester)
described_class.new(source, user, params).execute(scope)
end end
context 'when current user can destroy the given member' do context 'when current user is the member' do
before do it 'does not call Member#after_decline_request' do
project.team << [user, :master] expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(access_requester)
described_class.new(source, member_user, params).execute(scope)
end
end
end
end
end
end end
it 'destroys the member' do context 'when no member are found' do
destroy_member(member, user) let(:params) { { user_id: 42 } }
expect(member).to be_destroyed it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
let(:source) { project }
end
it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
let(:source) { group }
end
end end
context 'when the given member is a requester' do context 'when a member is found' do
before do before do
member.update_column(:requested_at, Time.now) project.team << [member_user, :developer]
group.add_developer(member_user)
end end
let(:params) { { user_id: member_user.id } }
it 'calls Member#after_decline_request' do context 'when current user cannot destroy the given member' do
expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member) it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
let(:source) { project }
end
destroy_member(member, user) it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
let(:source) { group }
end
end end
context 'when current user is the member' do context 'when current user can destroy the given member' do
it 'does not call Member#after_decline_request' do before do
expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) project.team << [user, :master]
group.add_owner(user)
end
destroy_member(member, member.user) it_behaves_like 'a service destroying a member' do
let(:source) { project }
end end
it_behaves_like 'a service destroying a member' do
let(:source) { group }
end end
context 'when current user is the member and ' do context 'when given a :id' do
it 'does not call Member#after_decline_request' do let(:params) { { id: project.members.find_by!(user_id: user.id).id } }
expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member)
destroy_member(member, member.user) it 'destroys the member' do
end expect { described_class.new(project, user, params).execute }.
to change { project.members.count }.by(-1)
end end
end end
end end
def destroy_member(member, user)
Members::DestroyService.new(member, user).execute
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