diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb index e40c8cab4a9a1e35882b9ca510b7afeaeac472f8..c13333641d33e4da0a82de791828d40cd01b0b8d 100644 --- a/app/controllers/concerns/membership_actions.rb +++ b/app/controllers/concerns/membership_actions.rb @@ -15,16 +15,17 @@ module MembershipActions end def leave - Members::DestroyService.new(membershipable, current_user, user_id: current_user.id).execute(:all) + member = Members::DestroyService.new(membershipable, current_user, user_id: current_user.id). + execute(:all) source_type = membershipable.class.to_s.humanize(capitalize: false) notice = - if @member.request? + if member.request? "Your access request to the #{source_type} has been withdrawn." else "You left the \"#{membershipable.human_name}\" #{source_type}." end - redirect_path = @member.request? ? @member.source : [:dashboard, membershipable.class.to_s.tableize] + redirect_path = member.request? ? member.source : [:dashboard, membershipable.class.to_s.tableize] redirect_to redirect_path, notice: notice end diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index f9368303d54bcc0e1bb73746c406bd7e6afdbe0e..18cd800c6196df6cab275b90b61ec09ab6ca9b0b 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -40,7 +40,7 @@ class Groups::GroupMembersController < Groups::ApplicationController end def destroy - Members::DestroyService.new(@group, current_user, user_id: params[:id]).execute(:all) + Members::DestroyService.new(@group, current_user, id: params[:id]).execute(:all) respond_to do |format| format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' } diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index 1c07dd2e18b95f8af21c18f03f7afcf013f11ec7..f56b256984ba27347a49ab099c28edf5cd3080e5 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -55,7 +55,8 @@ class Projects::ProjectMembersController < Projects::ApplicationController end def destroy - Members::DestroyService.new(@project, current_user, user_id: params[:id]).execute(:all) + Members::DestroyService.new(@project, current_user, params). + execute(:all) respond_to do |format| format.html do diff --git a/app/services/members/authorized_destroy_service.rb b/app/services/members/authorized_destroy_service.rb index ca9db59cac706762c7586ffb5f6fb16308ab6d4c..b7a244c2029282cc8ad8cdd720e35414a8bd1926 100644 --- a/app/services/members/authorized_destroy_service.rb +++ b/app/services/members/authorized_destroy_service.rb @@ -14,6 +14,8 @@ module Members if member.request? && member.user != user notification_service.decline_access_request(member) end + + member end end end diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index b3d79d577bda01669e8901c7d1a9bfd1814da4df..ee0720655239f654a293f33aadd56efea5c53ead 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -15,7 +15,7 @@ module Members def execute(scope = :members) raise "scope :#{scope} is not allowed!" unless ALLOWED_SCOPES.include?(scope) - member = find_member(scope) + member = find_member!(scope) raise Gitlab::Access::AccessDeniedError if cannot_destroy_member?(member) @@ -24,13 +24,14 @@ module Members private - def find_member(scope) + def find_member!(scope) + condition = params[:user_id] ? { user_id: params[:user_id] } : { id: params[:id] } case scope when :all - source.members.find_by(user_id: params[:user_id]) || - source.requesters.find_by!(user_id: params[:user_id]) + source.members.find_by(condition) || + source.requesters.find_by!(condition) else - source.public_send(scope).find_by!(user_id: params[:user_id]) + source.public_send(scope).find_by!(condition) end end diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index 75be24efd5947448c5b7198f5f2192f2cc7f5c43..d3db77408302c92ec80204ffaafa2978df3355e5 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -75,7 +75,8 @@ module API required_attributes! [:user_id] source = find_source(source_type, params[:id]) - ::Members::DestroyService.new(source, current_user, declared(params)).execute(:requesters) + ::Members::DestroyService.new(source, current_user, params). + execute(:requesters) end end end diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb index 92b97bf3d0cc1962d8ac582f5e5ae37864d20b30..a0870891cf4a4ca7d8386bc69ab4903bdf0b28ac 100644 --- a/spec/controllers/groups/group_members_controller_spec.rb +++ b/spec/controllers/groups/group_members_controller_spec.rb @@ -87,10 +87,10 @@ describe Groups::GroupMembersController do context 'when member is not found' do before { sign_in(user) } - it 'returns 403' do + it 'returns 404' do delete :leave, group_id: group - expect(response).to have_http_status(403) + expect(response).to have_http_status(404) end end diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb index 5e2a8cf38490ba81d46d6e900ecac8ca0da5be31..074f85157dec5d807e08d7beb53ea7c76d1d362d 100644 --- a/spec/controllers/projects/project_members_controller_spec.rb +++ b/spec/controllers/projects/project_members_controller_spec.rb @@ -135,11 +135,11 @@ describe Projects::ProjectMembersController do context 'when member is not found' do before { sign_in(user) } - it 'returns 403' do + it 'returns 404' do delete :leave, namespace_id: project.namespace, project_id: project - expect(response).to have_http_status(403) + expect(response).to have_http_status(404) end end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 06a6b0083c9371441dadc1361ee69496faf70176..9995f3488af78d6e4b7ea0639836d95bbe814868 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -98,6 +98,15 @@ describe Members::DestroyService, services: true do it_behaves_like 'a service destroying a member' do let(:source) { group } end + + context 'when given a :id' do + let(:params) { { id: project.members.find_by!(user_id: user.id).id } } + + it 'destroys the member' do + expect { described_class.new(project, user, params).execute }. + to change { project.members.count }.by(-1) + end + end end end end