diff options
-rw-r--r-- | app/controllers/concerns/membership_actions.rb | 7 | ||||
-rw-r--r-- | app/controllers/groups/group_members_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/project_members_controller.rb | 3 | ||||
-rw-r--r-- | app/services/members/authorized_destroy_service.rb | 2 | ||||
-rw-r--r-- | app/services/members/destroy_service.rb | 11 | ||||
-rw-r--r-- | lib/api/access_requests.rb | 3 | ||||
-rw-r--r-- | spec/controllers/groups/group_members_controller_spec.rb | 4 | ||||
-rw-r--r-- | spec/controllers/projects/project_members_controller_spec.rb | 4 | ||||
-rw-r--r-- | spec/services/members/destroy_service_spec.rb | 9 |
9 files changed, 30 insertions, 15 deletions
diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb index e40c8cab4a9..c13333641d3 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 f9368303d54..18cd800c619 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 1c07dd2e18b..f56b256984b 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 ca9db59cac7..b7a244c2029 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 b3d79d577bd..ee072065523 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 75be24efd59..d3db7740830 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 92b97bf3d0c..a0870891cf4 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 5e2a8cf3849..074f85157de 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 06a6b0083c9..9995f3488af 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 |