Commit c64ce000 authored by Mark Chao's avatar Mark Chao

Merge branch 'group-members-include-inherited' into 'master'

Group members: include inherited group links on group members page

See merge request gitlab-org/gitlab!71465
parents ad80d746 4ae7ac3b
......@@ -32,7 +32,7 @@ export const isGroup = (member) => {
};
export const isDirectMember = (member) => {
return isGroup(member) || member.isDirectMember;
return member.isDirectMember;
};
export const isCurrentUser = (member, currentUserId) => {
......
......@@ -32,12 +32,25 @@ initMembersApp(document.querySelector('.js-group-members-list-app'), {
},
},
[MEMBER_TYPES.group]: {
tableFields: SHARED_FIELDS.concat('granted'),
tableFields: gon?.features?.groupMemberInheritedGroup
? SHARED_FIELDS.concat(['source', 'granted'])
: SHARED_FIELDS.concat(['granted']),
tableAttrs: {
table: { 'data-qa-selector': 'groups_list' },
tr: { 'data-qa-selector': 'group_row' },
},
requestFormatter: groupLinkRequestFormatter,
...(gon?.features?.groupMemberInheritedGroup
? {
filteredSearchBar: {
show: true,
tokens: ['with_inherited_permissions'],
searchParam: 'search_groups',
placeholder: s__('Members|Filter groups'),
recentSearchesStorageKey: 'group_links_members',
},
}
: {}),
},
[MEMBER_TYPES.invite]: {
tableFields: SHARED_FIELDS.concat('invited'),
......
......@@ -23,7 +23,10 @@ class Groups::GroupMembersController < Groups::ApplicationController
feature_category :authentication_and_authorization
def index
push_frontend_feature_flag(:group_member_inherited_group, @group)
@sort = params[:sort].presence || sort_value_name
@include_relations ||= requested_relations
if can?(current_user, :admin_group_member, @group)
@invited_members = invited_members
......
......@@ -9,10 +9,10 @@ module Groups::GroupMembersHelper
{ multiple: true, class: 'input-clamp qa-member-select-field ', scope: :all, email_user: true }
end
def group_members_app_data(group, members:, invited:, access_requests:)
def group_members_app_data(group, members:, invited:, access_requests:, include_relations:, search:)
{
user: group_members_list_data(group, members, { param_name: :page, params: { invited_members_page: nil, search_invited: nil } }),
group: group_group_links_list_data(group),
group: group_group_links_list_data(group, include_relations, search),
invite: group_members_list_data(group, invited.nil? ? [] : invited, { param_name: :invited_members_page, params: { page: nil } }),
access_request: group_members_list_data(group, access_requests.nil? ? [] : access_requests),
source_id: group.id,
......@@ -26,8 +26,8 @@ module Groups::GroupMembersHelper
MemberSerializer.new.represent(members, { current_user: current_user, group: group, source: group })
end
def group_group_links_serialized(group_links)
GroupLink::GroupGroupLinkSerializer.new.represent(group_links, { current_user: current_user })
def group_group_links_serialized(group, group_links)
GroupLink::GroupGroupLinkSerializer.new.represent(group_links, { current_user: current_user, source: group })
end
# Overridden in `ee/app/helpers/ee/groups/group_members_helper.rb`
......@@ -39,11 +39,29 @@ module Groups::GroupMembersHelper
}
end
def group_group_links_list_data(group)
group_links = group.shared_with_group_links
def group_group_links(group, include_relations)
group_links = case include_relations
when [:direct]
group.shared_with_group_links
when [:inherited]
group.shared_with_group_links.of_ancestors
else
group.shared_with_group_links.of_ancestors_and_self
end
group_links.distinct_on_shared_with_group_id_with_group_access
end
def group_group_links_list_data(group, include_relations, search)
if ::Feature.enabled?(:group_member_inherited_group, group, default_enabled: :yaml)
group_links = group_group_links(group, include_relations)
group_links = group_links.search(search) if search
else
group_links = group.shared_with_group_links
end
{
members: group_group_links_serialized(group_links),
members: group_group_links_serialized(group, group_links),
pagination: members_pagination_data(group_links),
member_path: group_group_link_path(group, ':id')
}
......
......@@ -18,8 +18,8 @@ module Projects::ProjectMembersHelper
MemberSerializer.new.represent(members, { current_user: current_user, group: project.group, source: project })
end
def project_group_links_serialized(group_links)
GroupLink::ProjectGroupLinkSerializer.new.represent(group_links, { current_user: current_user })
def project_group_links_serialized(project, group_links)
GroupLink::ProjectGroupLinkSerializer.new.represent(group_links, { current_user: current_user, source: project })
end
def project_members_list_data(project, members, pagination = {})
......@@ -32,7 +32,7 @@ module Projects::ProjectMembersHelper
def project_group_links_list_data(project, group_links)
{
members: project_group_links_serialized(group_links),
members: project_group_links_serialized(project, group_links),
pagination: members_pagination_data(group_links),
member_path: project_group_link_path(project, ':id')
}
......
......@@ -43,7 +43,28 @@ class Group < Namespace
has_many :milestones
has_many :integrations
has_many :shared_group_links, foreign_key: :shared_with_group_id, class_name: 'GroupGroupLink'
has_many :shared_with_group_links, foreign_key: :shared_group_id, class_name: 'GroupGroupLink'
has_many :shared_with_group_links, foreign_key: :shared_group_id, class_name: 'GroupGroupLink' do
def of_ancestors
group = proxy_association.owner
return GroupGroupLink.none unless group.has_parent?
GroupGroupLink.where(shared_group_id: group.ancestors.reorder(nil).select(:id))
end
def of_ancestors_and_self
group = proxy_association.owner
source_ids =
if group.has_parent?
group.self_and_ancestors.reorder(nil).select(:id)
else
group.id
end
GroupGroupLink.where(shared_group_id: source_ids)
end
end
has_many :shared_groups, through: :shared_group_links, source: :shared_group
has_many :shared_with_groups, through: :shared_with_group_links, source: :shared_with_group
has_many :project_group_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
......
......@@ -16,6 +16,19 @@ class GroupGroupLink < ApplicationRecord
scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) }
scope :preload_shared_with_groups, -> { preload(:shared_with_group) }
scope :distinct_on_shared_with_group_id_with_group_access, -> do
distinct_group_links = select('DISTINCT ON (shared_with_group_id) *')
.order('shared_with_group_id, group_access DESC, expires_at DESC, created_at ASC')
unscoped.from(distinct_group_links, :group_group_links)
end
alias_method :shared_from, :shared_group
def self.search(query)
joins(:shared_with_group).merge(Group.search(query))
end
def self.access_options
Gitlab::Access.options_with_owner
end
......
......@@ -18,6 +18,7 @@ class ProjectGroupLink < ApplicationRecord
scope :in_group, -> (group_ids) { where(group_id: group_ids) }
alias_method :shared_with_group, :group
alias_method :shared_from, :project
def self.access_options
Gitlab::Access.options
......
......@@ -4,22 +4,14 @@ module GroupLink
class GroupGroupLinkEntity < GroupLink::GroupLinkEntity
include RequestAwareEntity
expose :can_update do |group_link|
can_manage?(group_link)
end
expose :can_remove do |group_link|
can_manage?(group_link)
expose :source do |group_link|
GroupEntity.represent(group_link.shared_from, only: [:id, :full_name, :web_url])
end
private
def current_user
options[:current_user]
end
def can_manage?(group_link)
can?(current_user, :admin_group_member, group_link.shared_group)
def admin_permission_name
:admin_group_member
end
end
end
......@@ -30,5 +30,32 @@ module GroupLink
expose :shared_with_group, merge: true, using: GroupBasicEntity
end
expose :can_update do |group_link, options|
can_admin_shared_from?(group_link, options)
end
expose :can_remove do |group_link, options|
can_admin_shared_from?(group_link, options)
end
expose :is_direct_member do |group_link, options|
direct_member?(group_link, options)
end
private
def current_user
options[:current_user]
end
def direct_member?(group_link, options)
group_link.shared_from == options[:source]
end
def can_admin_shared_from?(group_link, options)
direct_member?(group_link, options) &&
can?(current_user, admin_permission_name, group_link.shared_from)
end
end
end
......@@ -4,18 +4,14 @@ module GroupLink
class ProjectGroupLinkEntity < GroupLink::GroupLinkEntity
include RequestAwareEntity
expose :can_update do |group_link|
can?(current_user, :admin_project_member, group_link.project)
end
expose :can_remove do |group_link|
can?(current_user, :admin_project_member, group_link.project)
expose :source do |group_link|
ProjectEntity.represent(group_link.shared_from, only: [:id, :full_name])
end
private
def current_user
options[:current_user]
def admin_permission_name
:admin_project_member
end
end
end
......@@ -27,5 +27,7 @@
.js-group-members-list-app{ data: { members_data: group_members_app_data(@group,
members: @members,
invited: @invited_members,
access_requests: @requesters).to_json } }
access_requests: @requesters,
include_relations: @include_relations,
search: params[:search_groups]).to_json } }
= gl_loading_icon(css_class: 'gl-my-5', size: 'md')
---
name: group_member_inherited_group
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71465
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/357244
milestone: '14.10'
type: development
group: group::workspace
default_enabled: false
......@@ -288,10 +288,14 @@ To share a given group, for example, `Frontend` with another group, for example,
1. On the left sidebar, select **Group information > Members**.
1. Select **Invite a group**.
1. In the **Select a group to invite** list, select `Engineering`.
1. Select a [role](../permissions.md).
1. Select a [role](../permissions.md) as maximum access level.
1. Select **Invite**.
All the members of the `Engineering` group are added to the `Frontend` group.
After sharing the `Frontend` group with the `Engineering` group:
- The **Groups** tab lists the `Engineering` group.
- The **Groups** tab lists a group regardless of whether it is a public or private group.
- All members of the `Engineering` group have access to the `Frontend` group. The same access levels of the members apply up to the maximum access level selected when sharing the group.
## Manage group memberships via LDAP **(PREMIUM SELF)**
......
......@@ -16,7 +16,7 @@ module EE::Groups::GroupMembersHelper
end
override :group_members_app_data
def group_members_app_data(group, members:, invited:, access_requests:)
def group_members_app_data(group, members:, invited:, access_requests:, include_relations:, search:)
super.merge!({
can_export_members: can?(current_user, :export_group_memberships, group),
export_csv_path: export_csv_group_group_members_path(group),
......
......@@ -28,7 +28,9 @@ RSpec.describe Groups::GroupMembersHelper do
group,
members: [],
invited: [],
access_requests: []
access_requests: [],
include_relations: [:inherited, :direct],
search: nil
)
end
......
......@@ -23508,6 +23508,9 @@ msgstr ""
msgid "Members|Expiration date updated successfully."
msgstr ""
msgid "Members|Filter groups"
msgstr ""
msgid "Members|Filter members"
msgstr ""
......
......@@ -4,12 +4,19 @@
{ "$ref": "group_link.json" },
{
"required": [
"can_update",
"can_remove"
"source"
],
"properties": {
"can_update": { "type": "boolean" },
"can_remove": { "type": "boolean" }
"source": {
"type": "object",
"required": ["id", "full_name", "web_url"],
"properties": {
"id": { "type": "integer" },
"full_name": { "type": "string" },
"web_url": { "type": "string" }
},
"additionalProperties": false
}
}
}
]
......
......@@ -5,7 +5,10 @@
"created_at",
"expires_at",
"access_level",
"valid_roles"
"valid_roles",
"can_update",
"can_remove",
"is_direct_member"
],
"properties": {
"id": { "type": "integer" },
......@@ -33,6 +36,9 @@
"web_url": { "type": "string" }
},
"additionalProperties": false
}
},
"can_update": { "type": "boolean" },
"can_remove": { "type": "boolean" },
"is_direct_member": { "type": "boolean" }
}
}
......@@ -4,12 +4,18 @@
{ "$ref": "group_link.json" },
{
"required": [
"can_update",
"can_remove"
"source"
],
"properties": {
"can_update": { "type": "boolean" },
"can_remove": { "type": "boolean" }
"source": {
"type": "object",
"required": ["id", "full_name"],
"properties": {
"id": { "type": "integer" },
"full_name": { "type": "string" }
},
"additionalProperties": false
}
}
}
]
......
......@@ -58,6 +58,7 @@ export const group = {
webUrl: 'https://gitlab.com/groups/parent-group/commit451',
},
id: 3,
isDirectMember: true,
createdAt: '2020-08-06T15:31:07.662Z',
expiresAt: null,
validRoles: { Guest: 10, Reporter: 20, Developer: 30, Maintainer: 40, Owner: 50 },
......
......@@ -38,7 +38,9 @@ RSpec.describe Groups::GroupMembersHelper do
shared_group,
members: present_members(members_collection),
invited: present_members(invited),
access_requests: present_members(access_requests)
access_requests: present_members(access_requests),
include_relations: [:inherited, :direct],
search: nil
)
end
......@@ -96,6 +98,64 @@ RSpec.describe Groups::GroupMembersHelper do
it 'sets `member_path` property' do
expect(subject[:group][:member_path]).to eq('/groups/foo-bar/-/group_links/:id')
end
context 'inherited' do
let_it_be(:sub_shared_group) { create(:group, parent: shared_group) }
let_it_be(:sub_shared_with_group) { create(:group) }
let_it_be(:sub_group_group_link) { create(:group_group_link, shared_group: sub_shared_group, shared_with_group: sub_shared_with_group) }
let_it_be(:subject_group) { sub_shared_group }
before do
allow(helper).to receive(:group_group_member_path).with(sub_shared_group, ':id').and_return('/groups/foo-bar/-/group_members/:id')
allow(helper).to receive(:group_group_link_path).with(sub_shared_group, ':id').and_return('/groups/foo-bar/-/group_links/:id')
allow(helper).to receive(:can?).with(current_user, :admin_group_member, sub_shared_group).and_return(true)
allow(helper).to receive(:can?).with(current_user, :export_group_memberships, sub_shared_group).and_return(true)
end
subject do
helper.group_members_app_data(
sub_shared_group,
members: present_members(members_collection),
invited: present_members(invited),
access_requests: present_members(access_requests),
include_relations: include_relations,
search: nil
)
end
using RSpec::Parameterized::TableSyntax
where(:include_relations, :result) do
[:inherited, :direct] | lazy { [group_group_link, sub_group_group_link].map(&:id) }
[:inherited] | lazy { [group_group_link].map(&:id) }
[:direct] | lazy { [sub_group_group_link].map(&:id) }
end
with_them do
it 'returns correct group links' do
expect(subject[:group][:members].map { |link| link[:id] }).to match_array(result)
end
end
context 'when group_member_inherited_group disabled' do
before do
stub_feature_flags(group_member_inherited_group: false)
end
where(:include_relations, :result) do
[:inherited, :direct] | lazy { [sub_group_group_link.id] }
[:inherited] | lazy { [sub_group_group_link.id] }
[:direct] | lazy { [sub_group_group_link.id] }
end
with_them do
it 'always returns direct member links' do
expect(subject[:group][:members].map { |link| link[:id] }).to match_array(result)
end
end
end
end
end
context 'when pagination is not available' do
......
......@@ -29,6 +29,49 @@ RSpec.describe GroupGroupLink do
])
end
end
describe '.distinct_on_shared_with_group_id_with_group_access' do
let_it_be(:sub_shared_group) { create(:group, parent: shared_group) }
let_it_be(:other_group) { create(:group) }
let_it_be(:group_group_link_2) do
create(
:group_group_link,
shared_group: shared_group,
shared_with_group: other_group,
group_access: Gitlab::Access::GUEST
)
end
let_it_be(:group_group_link_3) do
create(
:group_group_link,
shared_group: sub_shared_group,
shared_with_group: group,
group_access: Gitlab::Access::GUEST
)
end
let_it_be(:group_group_link_4) do
create(
:group_group_link,
shared_group: sub_shared_group,
shared_with_group: other_group,
group_access: Gitlab::Access::DEVELOPER
)
end
it 'returns only one group link per group (with max group access)' do
distinct_group_group_links = described_class.distinct_on_shared_with_group_id_with_group_access
expect(described_class.all.count).to eq(4)
expect(distinct_group_group_links.count).to eq(2)
expect(distinct_group_group_links).to include(group_group_link)
expect(distinct_group_group_links).not_to include(group_group_link_2)
expect(distinct_group_group_links).not_to include(group_group_link_3)
expect(distinct_group_group_links).to include(group_group_link_4)
end
end
end
describe 'validation' do
......@@ -57,4 +100,9 @@ RSpec.describe GroupGroupLink do
group_group_link.human_access
end
end
describe 'search by group name' do
it { expect(described_class.search(group.name)).to eq([group_group_link]) }
it { expect(described_class.search('not-a-group-name')).to be_empty }
end
end
......@@ -3371,4 +3371,50 @@ RSpec.describe Group do
let(:feature_flag_method) { :work_items_feature_flag_enabled? }
end
end
describe 'group shares' do
let!(:sub_group) { create(:group, parent: group) }
let!(:sub_sub_group) { create(:group, parent: sub_group) }
let!(:shared_group_1) { create(:group) }
let!(:shared_group_2) { create(:group) }
let!(:shared_group_3) { create(:group) }
before do
group.shared_with_groups << shared_group_1
sub_group.shared_with_groups << shared_group_2
sub_sub_group.shared_with_groups << shared_group_3
end
describe '#shared_with_group_links.of_ancestors' do
using RSpec::Parameterized::TableSyntax
where(:subject_group, :result) do
ref(:group) | []
ref(:sub_group) | lazy { [shared_group_1].map(&:id) }
ref(:sub_sub_group) | lazy { [shared_group_1, shared_group_2].map(&:id) }
end
with_them do
it 'returns correct group shares' do
expect(subject_group.shared_with_group_links.of_ancestors.map(&:shared_with_group_id)).to match_array(result)
end
end
end
describe '#shared_with_group_links.of_ancestors_and_self' do
using RSpec::Parameterized::TableSyntax
where(:subject_group, :result) do
ref(:group) | lazy { [shared_group_1].map(&:id) }
ref(:sub_group) | lazy { [shared_group_1, shared_group_2].map(&:id) }
ref(:sub_sub_group) | lazy { [shared_group_1, shared_group_2, shared_group_3].map(&:id) }
end
with_them do
it 'returns correct group shares' do
expect(subject_group.shared_with_group_links.of_ancestors_and_self.map(&:shared_with_group_id)).to match_array(result)
end
end
end
end
end
......@@ -7,7 +7,7 @@ RSpec.describe GroupLink::GroupGroupLinkEntity do
let_it_be(:current_user) { create(:user) }
let(:entity) { described_class.new(group_group_link) }
let(:entity) { described_class.new(group_group_link, { current_user: current_user, source: shared_group }) }
before do
allow(entity).to receive(:current_user).and_return(current_user)
......@@ -17,16 +17,56 @@ RSpec.describe GroupLink::GroupGroupLinkEntity do
expect(entity.to_json).to match_schema('group_link/group_group_link')
end
context 'source' do
it 'exposes `source`' do
expect(entity.as_json[:source]).to include(
id: shared_group.id,
full_name: shared_group.full_name,
web_url: shared_group.web_url
)
end
end
context 'is_direct_member' do
it 'exposes `is_direct_member` as true for corresponding group' do
expect(entity.as_json[:is_direct_member]).to be true
end
it 'exposes `is_direct_member` as false for other source' do
entity = described_class.new(group_group_link, { current_user: current_user, source: shared_with_group })
expect(entity.as_json[:is_direct_member]).to be false
end
end
context 'when current user has `:admin_group_member` permissions' do
before do
allow(entity).to receive(:can?).with(current_user, :admin_group_member, shared_group).and_return(true)
end
it 'exposes `can_update` and `can_remove` as `true`' do
json = entity.as_json
context 'when direct_member? is true' do
before do
allow(entity).to receive(:direct_member?).and_return(true)
end
it 'exposes `can_update` and `can_remove` as `true`' do
json = entity.as_json
expect(json[:can_update]).to be true
expect(json[:can_remove]).to be true
end
end
context 'when direct_member? is false' do
before do
allow(entity).to receive(:direct_member?).and_return(false)
end
it 'exposes `can_update` and `can_remove` as `true`' do
json = entity.as_json
expect(json[:can_update]).to be true
expect(json[:can_remove]).to be true
expect(json[:can_update]).to be false
expect(json[:can_remove]).to be false
end
end
end
end
......@@ -6,7 +6,7 @@ RSpec.describe GroupLink::ProjectGroupLinkEntity do
let_it_be(:current_user) { create(:user) }
let_it_be(:project_group_link) { create(:project_group_link) }
let(:entity) { described_class.new(project_group_link) }
let(:entity) { described_class.new(project_group_link, { current_user: current_user, source: project_group_link.project }) }
before do
allow(entity).to receive(:current_user).and_return(current_user)
......
# frozen_string_literal: true
RSpec.shared_context 'group_group_link' do
let(:shared_with_group) { create(:group) }
let(:shared_group) { create(:group) }
let_it_be(:shared_with_group) { create(:group) }
let_it_be(:shared_group) { create(:group) }
let!(:group_group_link) do
let_it_be(:group_group_link) do
create(
:group_group_link,
{
......
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