Commit 3557470f authored by Jan Provaznik's avatar Jan Provaznik

Merge branch 'fix/option-remove-memberships-from-subresources' into 'master'

Remove group member: option to remove membership of subgroups/projects

See merge request gitlab-org/gitlab!55980
parents 8324b2d7 be344e70
......@@ -13,6 +13,11 @@ export default {
type: Number,
required: true,
},
memberType: {
type: String,
required: false,
default: null,
},
message: {
type: String,
required: true,
......@@ -50,6 +55,7 @@ export default {
:aria-label="title"
:icon="icon"
:data-member-path="computedMemberPath"
:data-member-type="memberType"
:data-is-access-request="isAccessRequest"
:data-message="message"
data-qa-selector="delete_member_button"
......
......@@ -59,6 +59,7 @@ export default {
<remove-member-button
v-else
:member-id="member.id"
:member-type="member.type"
:message="message"
:title="s__('Member|Remove member')"
/>
......
......@@ -22,6 +22,9 @@ export default {
isAccessRequest() {
return parseBoolean(this.modalData.isAccessRequest);
},
isGroupMember() {
return this.modalData.memberType === 'GroupMember';
},
actionText() {
return this.isAccessRequest ? __('Deny access request') : __('Remove member');
},
......@@ -70,6 +73,9 @@ export default {
<input ref="method" type="hidden" name="_method" value="delete" />
<input :value="$options.csrf.token" type="hidden" name="authenticity_token" />
<gl-form-checkbox v-if="isGroupMember" name="remove_sub_memberships">
{{ __('Also remove direct user membership from subgroups and projects') }}
</gl-form-checkbox>
<gl-form-checkbox v-if="!isAccessRequest" name="unassign_issuables">
{{ __('Also unassign this user from related issues and merge requests') }}
</gl-form-checkbox>
......
......@@ -43,10 +43,11 @@ module MembershipActions
def destroy
member = membershipable.members_and_requesters.find(params[:id])
skip_subresources = !ActiveRecord::Type::Boolean.new.cast(params.delete(:remove_sub_memberships))
# !! is used in case unassign_issuables contains empty string which would result in nil
unassign_issuables = !!ActiveRecord::Type::Boolean.new.cast(params.delete(:unassign_issuables))
Members::DestroyService.new(current_user).execute(member, unassign_issuables: unassign_issuables)
Members::DestroyService.new(current_user).execute(member, skip_subresources: skip_subresources, unassign_issuables: unassign_issuables)
respond_to do |format|
format.html do
......@@ -54,7 +55,11 @@ module MembershipActions
begin
case membershipable
when Namespace
_("User was successfully removed from group and any subresources.")
if skip_subresources
_("User was successfully removed from group.")
else
_("User was successfully removed from group and any subgroups and projects.")
end
else
_("User was successfully removed from project.")
end
......
......@@ -36,6 +36,8 @@ class MemberEntity < Grape::Entity
GroupEntity.represent(member.source, only: [:id, :full_name, :web_url])
end
expose :type
expose :valid_level_roles, as: :valid_roles
expose :user, if: -> (member) { member.user.present? }, using: MemberUserEntity
......
......@@ -120,7 +120,7 @@
= sprite_icon('leave', css_class: 'gl-icon')
= _('Leave')
- else
%button{ data: { member_path: member_path(member.member), message: remove_member_message(member), is_access_request: member.request?.to_s, qa_selector: 'delete_member_button' },
%button{ data: { member_path: member_path(member.member), member_type: member.type, message: remove_member_message(member), is_access_request: member.request?.to_s, qa_selector: 'delete_member_button' },
class: "js-remove-member-button btn gl-button btn-danger align-self-center m-0 #{'ml-sm-2 btn-icon' unless force_mobile_view}",
title: remove_member_title(member) }
%span{ class: ('d-block d-sm-none' unless force_mobile_view) }
......
---
title: 'Remove group member: add option to also remove direct user membership from
subgroups and projects'
merge_request: 55980
author: Jonas Wälter @wwwjon
type: changed
......@@ -494,7 +494,10 @@ DELETE /projects/:id/members/:user_id
| --------- | ---- | -------- | ----------- |
| `id` | integer/string | yes | The ID or [URL-encoded path of the project or group](README.md#namespaced-path-encoding) owned by the authenticated user |
| `user_id` | integer | yes | The user ID of the member |
| `unassign_issuables` | boolean | false | Flag indicating if the removed member should be unassigned from any issues or merge requests inside a given group or project |
| `skip_subresources` | boolean | false | Whether the deletion of direct memberships of the removed member in subgroups and projects should be skipped. Default is `false`. |
| `unassign_issuables` | boolean | false | Whether the removed member should be unassigned from any issues or merge requests inside a given group or project. Default is `false`. |
Example request:
```shell
curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/:id/members/:user_id"
......
......@@ -155,6 +155,8 @@ module API
desc 'Removes a user from a group or project.'
params do
requires :user_id, type: Integer, desc: 'The user ID of the member'
optional :skip_subresources, type: Boolean, default: false,
desc: 'Flag indicating if the deletion of direct memberships of the removed member in subgroups and projects should be skipped'
optional :unassign_issuables, type: Boolean, default: false,
desc: 'Flag indicating if the removed member should be unassigned from any issues or merge requests within given group or project'
end
......@@ -164,7 +166,7 @@ module API
member = source_members(source).find_by!(user_id: params[:user_id])
destroy_conditionally!(member) do
::Members::DestroyService.new(current_user).execute(member, unassign_issuables: params[:unassign_issuables])
::Members::DestroyService.new(current_user).execute(member, skip_subresources: params[:skip_subresources], unassign_issuables: params[:unassign_issuables])
end
end
# rubocop: enable CodeReuse/ActiveRecord
......
......@@ -3211,6 +3211,9 @@ msgstr ""
msgid "Also called \"Relying party service URL\" or \"Reply URL\""
msgstr ""
msgid "Also remove direct user membership from subgroups and projects"
msgstr ""
msgid "Also unassign this user from related issues and merge requests"
msgstr ""
......@@ -33014,7 +33017,10 @@ msgstr ""
msgid "User was successfully created."
msgstr ""
msgid "User was successfully removed from group and any subresources."
msgid "User was successfully removed from group and any subgroups and projects."
msgstr ""
msgid "User was successfully removed from group."
msgstr ""
msgid "User was successfully removed from project."
......
......@@ -288,7 +288,9 @@ RSpec.describe Groups::GroupMembersController do
end
describe 'DELETE destroy' do
let(:member) { create(:group_member, :developer, group: group) }
let(:sub_group) { create(:group, parent: group) }
let!(:member) { create(:group_member, :developer, group: group) }
let!(:sub_member) { create(:group_member, :developer, group: sub_group, user: member.user) }
before do
sign_in(user)
......@@ -324,9 +326,19 @@ RSpec.describe Groups::GroupMembersController do
it '[HTML] removes user from members' do
delete :destroy, params: { group_id: group, id: member }
expect(response).to set_flash.to 'User was successfully removed from group and any subresources.'
expect(response).to set_flash.to 'User was successfully removed from group.'
expect(response).to redirect_to(group_group_members_path(group))
expect(group.members).not_to include member
expect(sub_group.members).to include sub_member
end
it '[HTML] removes user from members including subgroups and projects' do
delete :destroy, params: { group_id: group, id: member, remove_sub_memberships: true }
expect(response).to set_flash.to 'User was successfully removed from group and any subgroups and projects.'
expect(response).to redirect_to(group_group_members_path(group))
expect(group.members).not_to include member
expect(sub_group.members).not_to include sub_member
end
it '[JS] removes user from members' do
......
......@@ -8,6 +8,7 @@
"requested_at",
"source",
"valid_roles",
"type",
"can_update",
"can_remove",
"is_direct_member"
......@@ -40,6 +41,7 @@
"additionalProperties": false
},
"valid_roles": { "type": "object" },
"type": { "type": "string" },
"created_by": {
"type": "object",
"required": ["name", "web_url"],
......
......@@ -39,6 +39,7 @@ describe('InviteActionButtons', () => {
it('sets props correctly', () => {
expect(findRemoveMemberButton().props()).toEqual({
memberId: member.id,
memberType: null,
message: `Are you sure you want to revoke the invitation for ${member.invite.email} to join "${member.source.fullName}"`,
title: 'Revoke invite',
isAccessRequest: false,
......
......@@ -24,6 +24,7 @@ describe('RemoveMemberButton', () => {
store: createStore(state),
propsData: {
memberId: 1,
memberType: 'GroupMember',
message: 'Are you sure you want to remove John Smith?',
title: 'Remove member',
isAccessRequest: true,
......@@ -44,6 +45,7 @@ describe('RemoveMemberButton', () => {
expect(wrapper.attributes()).toMatchObject({
'data-member-path': '/groups/foo-bar/-/group_members/1',
'data-member-type': 'GroupMember',
'data-message': 'Are you sure you want to remove John Smith?',
'data-is-access-request': 'true',
'aria-label': 'Remove member',
......
......@@ -39,6 +39,7 @@ describe('UserActionButtons', () => {
it('sets props correctly', () => {
expect(findRemoveMemberButton().props()).toEqual({
memberId: member.id,
memberType: 'GroupMember',
message: `Are you sure you want to remove ${member.user.name} from "${member.source.fullName}"`,
title: 'Remove member',
isAccessRequest: false,
......@@ -86,4 +87,40 @@ describe('UserActionButtons', () => {
expect(findRemoveMemberButton().exists()).toBe(false);
});
});
describe('when group member', () => {
beforeEach(() => {
createComponent({
member: {
...member,
type: 'GroupMember',
},
permissions: {
canRemove: true,
},
});
});
it('sets member type correctly', () => {
expect(findRemoveMemberButton().props().memberType).toBe('GroupMember');
});
});
describe('when project member', () => {
beforeEach(() => {
createComponent({
member: {
...member,
type: 'ProjectMember',
},
permissions: {
canRemove: true,
},
});
});
it('sets member type correctly', () => {
expect(findRemoveMemberButton().props().memberType).toBe('ProjectMember');
});
});
});
......@@ -11,6 +11,7 @@ export const member = {
fullName: 'Foo Bar',
webUrl: 'https://gitlab.com/groups/foo-bar',
},
type: 'GroupMember',
user: {
id: 123,
name: 'Administrator',
......
import { GlFormCheckbox, GlModal } from '@gitlab/ui';
import { GlModal } from '@gitlab/ui';
import { shallowMount } from '@vue/test-utils';
import RemoveMemberModal from '~/vue_shared/components/remove_member_modal.vue';
......@@ -15,12 +15,20 @@ describe('RemoveMemberModal', () => {
});
describe.each`
state | isAccessRequest | actionText | checkboxTestDescription | checkboxExpected | message
${'removing a member'} | ${'false'} | ${'Remove member'} | ${'shows a checkbox to allow removal from related issues and MRs'} | ${true} | ${'Are you sure you want to remove Jane Doe from the Gitlab Org / Gitlab Test project?'}
${'denying an access request'} | ${'true'} | ${'Deny access request'} | ${'does not show a checkbox'} | ${false} | ${"Are you sure you want to deny Jane Doe's request to join the Gitlab Org / Gitlab Test project?"}
state | memberType | isAccessRequest | actionText | removeSubMembershipsCheckboxExpected | unassignIssuablesCheckboxExpected | message
${'removing a group member'} | ${'GroupMember'} | ${'false'} | ${'Remove member'} | ${true} | ${true} | ${'Are you sure you want to remove Jane Doe from the Gitlab Org / Gitlab Test project?'}
${'removing a project member'} | ${'ProjectMember'} | ${'false'} | ${'Remove member'} | ${false} | ${true} | ${'Are you sure you want to remove Jane Doe from the Gitlab Org / Gitlab Test project?'}
${'denying an access request'} | ${'ProjectMember'} | ${'true'} | ${'Deny access request'} | ${false} | ${false} | ${"Are you sure you want to deny Jane Doe's request to join the Gitlab Org / Gitlab Test project?"}
`(
'when $state',
({ actionText, isAccessRequest, message, checkboxTestDescription, checkboxExpected }) => {
({
actionText,
memberType,
isAccessRequest,
message,
removeSubMembershipsCheckboxExpected,
unassignIssuablesCheckboxExpected,
}) => {
beforeEach(() => {
wrapper = shallowMount(RemoveMemberModal, {
data() {
......@@ -29,6 +37,7 @@ describe('RemoveMemberModal', () => {
isAccessRequest,
message,
memberPath,
memberType,
},
};
},
......@@ -47,8 +56,20 @@ describe('RemoveMemberModal', () => {
expect(wrapper.find('[data-testid=modal-message]').text()).toBe(message);
});
it(`${checkboxTestDescription}`, () => {
expect(wrapper.find(GlFormCheckbox).exists()).toBe(checkboxExpected);
it(`shows ${
removeSubMembershipsCheckboxExpected ? 'a' : 'no'
} checkbox to remove direct memberships of subgroups/projects`, () => {
expect(wrapper.find('[name=remove_sub_memberships]').exists()).toBe(
removeSubMembershipsCheckboxExpected,
);
});
it(`shows ${
unassignIssuablesCheckboxExpected ? 'a' : 'no'
} checkbox to allow removal from related issues and MRs`, () => {
expect(wrapper.find('[name=unassign_issuables]').exists()).toBe(
unassignIssuablesCheckboxExpected,
);
});
it('submits the form when the modal is submitted', () => {
......
......@@ -555,6 +555,34 @@ RSpec.describe API::Members do
end
end
describe 'DELETE /groups/:id/members/:user_id' do
let(:other_user) { create(:user) }
let(:nested_group) { create(:group, parent: group) }
before do
nested_group.add_developer(developer)
nested_group.add_developer(other_user)
end
it 'deletes only the member with skip_subresources=true' do
expect do
delete api("/groups/#{group.id}/members/#{developer.id}", maintainer), params: { skip_subresources: true }
expect(response).to have_gitlab_http_status(:no_content)
end.to change { group.members.count }.by(-1)
.and change { nested_group.members.count }.by(0)
end
it 'deletes member and its sub memberships with skip_subresources=false' do
expect do
delete api("/groups/#{group.id}/members/#{developer.id}", maintainer), params: { skip_subresources: false }
expect(response).to have_gitlab_http_status(:no_content)
end.to change { group.members.count }.by(-1)
.and change { nested_group.members.count }.by(-1)
end
end
[false, true].each do |all|
it_behaves_like 'GET /:source_type/:id/members/(all)', 'project', all do
let(:source) { project }
......
......@@ -289,60 +289,137 @@ RSpec.describe Members::DestroyService do
let(:group_project) { create(:project, :public, group: group) }
let(:control_project) { create(:project, group: subsubgroup) }
before do
create(:group_member, :developer, group: subsubgroup, user: member_user)
create(:project_member, :invited, project: group_project, created_by: member_user)
create(:group_member, :invited, group: group, created_by: member_user)
create(:project_member, :invited, project: subsubproject, created_by: member_user)
create(:group_member, :invited, group: subgroup, created_by: member_user)
context 'with memberships' do
before do
subgroup.add_developer(member_user)
subsubgroup.add_developer(member_user)
subsubproject.add_developer(member_user)
group_project.add_developer(member_user)
control_project.add_maintainer(user)
group.add_owner(user)
@group_member = create(:group_member, :developer, group: group, user: member_user)
end
subsubproject.add_developer(member_user)
control_project.add_maintainer(user)
group.add_owner(user)
context 'with skipping of subresources' do
before do
described_class.new(user).execute(@group_member, skip_subresources: true)
end
group_member = create(:group_member, :developer, group: group, user: member_user)
it 'removes the group membership' do
expect(group.members.map(&:user)).not_to include(member_user)
end
described_class.new(user).execute(group_member)
end
it 'does not remove the project membership' do
expect(group_project.members.map(&:user)).to include(member_user)
end
it 'removes the project membership' do
expect(group_project.members.map(&:user)).not_to include(member_user)
end
it 'does not remove the subgroup membership' do
expect(subgroup.members.map(&:user)).to include(member_user)
end
it 'removes the group membership' do
expect(group.members.map(&:user)).not_to include(member_user)
end
it 'does not remove the subsubgroup membership' do
expect(subsubgroup.members.map(&:user)).to include(member_user)
end
it 'removes the subgroup membership' do
expect(subgroup.members.map(&:user)).not_to include(member_user)
end
it 'does not remove the subsubproject membership' do
expect(subsubproject.members.map(&:user)).to include(member_user)
end
it 'removes the subsubgroup membership' do
expect(subsubgroup.members.map(&:user)).not_to include(member_user)
end
it 'does not remove the user from the control project' do
expect(control_project.members.map(&:user)).to include(user)
end
end
it 'removes the subsubproject membership' do
expect(subsubproject.members.map(&:user)).not_to include(member_user)
end
context 'without skipping of subresources' do
before do
described_class.new(user).execute(@group_member, skip_subresources: false)
end
it 'does not remove the user from the control project' do
expect(control_project.members.map(&:user)).to include(user)
end
it 'removes the project membership' do
expect(group_project.members.map(&:user)).not_to include(member_user)
end
it 'removes group members invited by deleted user' do
expect(group.members.not_accepted_invitations_by_user(member_user)).to be_empty
end
it 'removes the group membership' do
expect(group.members.map(&:user)).not_to include(member_user)
end
it 'removes project members invited by deleted user' do
expect(group_project.members.not_accepted_invitations_by_user(member_user)).to be_empty
end
it 'removes the subgroup membership' do
expect(subgroup.members.map(&:user)).not_to include(member_user)
end
it 'removes the subsubgroup membership' do
expect(subsubgroup.members.map(&:user)).not_to include(member_user)
end
it 'removes the subsubproject membership' do
expect(subsubproject.members.map(&:user)).not_to include(member_user)
end
it 'removes subgroup members invited by deleted user' do
expect(subgroup.members.not_accepted_invitations_by_user(member_user)).to be_empty
it 'does not remove the user from the control project' do
expect(control_project.members.map(&:user)).to include(user)
end
end
end
it 'removes subproject members invited by deleted user' do
expect(subsubproject.members.not_accepted_invitations_by_user(member_user)).to be_empty
context 'with invites' do
before do
create(:group_member, :developer, group: subsubgroup, user: member_user)
create(:project_member, :invited, project: group_project, created_by: member_user)
create(:group_member, :invited, group: group, created_by: member_user)
create(:project_member, :invited, project: subsubproject, created_by: member_user)
create(:group_member, :invited, group: subgroup, created_by: member_user)
subsubproject.add_developer(member_user)
control_project.add_maintainer(user)
group.add_owner(user)
@group_member = create(:group_member, :developer, group: group, user: member_user)
end
context 'with skipping of subresources' do
before do
described_class.new(user).execute(@group_member, skip_subresources: true)
end
it 'does not remove group members invited by deleted user' do
expect(group.members.not_accepted_invitations_by_user(member_user)).not_to be_empty
end
it 'does not remove project members invited by deleted user' do
expect(group_project.members.not_accepted_invitations_by_user(member_user)).not_to be_empty
end
it 'does not remove subgroup members invited by deleted user' do
expect(subgroup.members.not_accepted_invitations_by_user(member_user)).not_to be_empty
end
it 'does not remove subproject members invited by deleted user' do
expect(subsubproject.members.not_accepted_invitations_by_user(member_user)).not_to be_empty
end
end
context 'without skipping of subresources' do
before do
described_class.new(user).execute(@group_member, skip_subresources: false)
end
it 'removes group members invited by deleted user' do
expect(group.members.not_accepted_invitations_by_user(member_user)).to be_empty
end
it 'removes project members invited by deleted user' do
expect(group_project.members.not_accepted_invitations_by_user(member_user)).to be_empty
end
it 'removes subgroup members invited by deleted user' do
expect(subgroup.members.not_accepted_invitations_by_user(member_user)).to be_empty
end
it 'removes subproject members invited by deleted user' do
expect(subsubproject.members.not_accepted_invitations_by_user(member_user)).to be_empty
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