From d8e65143b0e464cfc0aa1a1d7c51250f331e48bc Mon Sep 17 00:00:00 2001
From: Andrei Zubov <azubov@gitlab.com>
Date: Tue, 27 Feb 2024 21:15:00 +0000
Subject: [PATCH] Add option to keep shared groups when filtered by parent
Now it is possible to include invited groups when searching for groups
by parent. This will allow users to select invited groups when
protecting an environment
Changelog: changed
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141462
EE: true
---
.../settings/api/access_dropdown_api.js | 5 +--
.../settings/components/access_dropdown.vue | 2 +-
app/finders/groups_finder.rb | 31 +++++++++++++++---
app/models/group.rb | 21 ++++++++++++
.../autocomplete/group_subgroups_finder.rb | 7 +++-
.../group_subgroups_finder_spec.rb | 25 ++++++++++++++-
spec/finders/groups_finder_spec.rb | 32 +++++++++++++++++--
spec/models/group_spec.rb | 31 ++++++++++++++++++
8 files changed, 142 insertions(+), 12 deletions(-)
diff --git a/app/assets/javascripts/groups/settings/api/access_dropdown_api.js b/app/assets/javascripts/groups/settings/api/access_dropdown_api.js
index 37f95a7ab30ac..e232b1977f0a2 100644
--- a/app/assets/javascripts/groups/settings/api/access_dropdown_api.js
+++ b/app/assets/javascripts/groups/settings/api/access_dropdown_api.js
@@ -7,15 +7,16 @@ const buildUrl = (urlRoot, url) => {
return joinPaths(urlRoot, url);
};
-const defaultOptions = { includeParentDescendants: false };
+const defaultOptions = { includeParentDescendants: false, includeParentSharedGroups: false };
export const getSubGroups = (options = defaultOptions) => {
- const { includeParentDescendants } = options;
+ const { includeParentDescendants, includeParentSharedGroups } = options;
return axios.get(buildUrl(gon.relative_url_root || '', GROUP_SUBGROUPS_PATH), {
params: {
group_id: gon.current_group_id,
include_parent_descendants: includeParentDescendants,
+ include_parent_shared_groups: includeParentSharedGroups,
},
});
};
diff --git a/app/assets/javascripts/groups/settings/components/access_dropdown.vue b/app/assets/javascripts/groups/settings/components/access_dropdown.vue
index fee1383c8e251..327f73b88c20b 100644
--- a/app/assets/javascripts/groups/settings/components/access_dropdown.vue
+++ b/app/assets/javascripts/groups/settings/components/access_dropdown.vue
@@ -96,7 +96,7 @@ export default {
Promise.all([
this.groups.length
? Promise.resolve({ data: this.groups })
- : getSubGroups({ includeParentDescendants: true }),
+ : getSubGroups({ includeParentDescendants: true, includeParentSharedGroups: true }),
])
.then(([groupsResponse]) => {
this.consolidateData(groupsResponse.data);
diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb
index ba691bcdf0f29..c89722327ecf0 100644
--- a/app/finders/groups_finder.rb
+++ b/app/finders/groups_finder.rb
@@ -16,6 +16,9 @@
# filter_group_ids: array of integers - only include groups from the specified list of ids
# include_parent_descendants: boolean (defaults to false) - includes descendant groups when
# filtering by parent. The parent param must be present.
+# include_parent_shared_groups: boolean (defaults to false) - includes shared groups of a parent group
+# when filtering by parent.
+# Both parent and include_parent_descendants params must be present.
# include_ancestors: boolean (defaults to true)
# organization: Scope the groups to the Organizations::Organization
#
@@ -118,17 +121,27 @@ def by_organization(groups)
groups.in_organization(organization)
end
- # rubocop: disable CodeReuse/ActiveRecord
def by_parent(groups)
return groups unless params[:parent]
- if params.fetch(:include_parent_descendants, false)
- groups.id_in(params[:parent].descendants)
+ if include_parent_descendants?
+ by_parent_descendants(groups, params[:parent])
else
- groups.where(parent: params[:parent])
+ by_parent_children(groups, params[:parent])
+ end
+ end
+
+ def by_parent_descendants(groups, parent)
+ if include_parent_shared_groups?
+ groups.descendants_with_shared_with_groups(parent)
+ else
+ groups.id_in(parent.descendants)
end
end
- # rubocop: enable CodeReuse/ActiveRecord
+
+ def by_parent_children(groups, parent)
+ groups.by_parent(parent)
+ end
def filter_group_ids(groups)
return groups unless params[:filter_group_ids]
@@ -154,6 +167,14 @@ def sort(groups)
groups.sort_by_attribute(params[:sort])
end
+ def include_parent_shared_groups?
+ params.fetch(:include_parent_shared_groups, false)
+ end
+
+ def include_parent_descendants?
+ params.fetch(:include_parent_descendants, false)
+ end
+
def min_access_level?
current_user && params[:min_access_level].present?
end
diff --git a/app/models/group.rb b/app/models/group.rb
index d8fc468e7344f..80fcd4470a044 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -239,6 +239,27 @@ def of_ancestors_and_self
.where(group_group_links: { shared_group_id: group.self_and_ancestors })
end
+ # Returns all groups that are shared with the given group (see :shared_with_group)
+ # and all descendents of the given group
+ # returns none if the given group is nil
+ scope :descendants_with_shared_with_groups, -> (group) do
+ return none if group.nil?
+
+ descendants_query = group.descendants.select(:id)
+ # since we're only interested in ids, we query GroupGroupLink directly instead of using :shared_with_group
+ # to avoid an extra JOIN in the resulting query
+ shared_groups_query = GroupGroupLink
+ .where(shared_group_id: group.id)
+ .select('shared_with_group_id AS id')
+
+ combined_query = Group
+ .from_union(descendants_query, shared_groups_query, alias_as: :combined)
+ .unscope(where: :type)
+ .select(:id)
+
+ id_in(combined_query)
+ end
+
# WARNING: This method should never be used on its own
# please do make sure the number of rows you are filtering is small
# enough for this query
diff --git a/ee/app/finders/autocomplete/group_subgroups_finder.rb b/ee/app/finders/autocomplete/group_subgroups_finder.rb
index d150de44d0528..36bbd05248001 100644
--- a/ee/app/finders/autocomplete/group_subgroups_finder.rb
+++ b/ee/app/finders/autocomplete/group_subgroups_finder.rb
@@ -15,6 +15,10 @@ def include_parent_descendants?
Gitlab::Utils.to_boolean(params[:include_parent_descendants])
end
+ def include_parent_shared_groups?
+ Gitlab::Utils.to_boolean(params[:include_parent_shared_groups])
+ end
+
def group_id
params[:group_id]
end
@@ -23,7 +27,8 @@ def group_id
def execute
group = ::Autocomplete::GroupFinder.new(current_user, nil, group_id: group_id).execute
GroupsFinder.new(current_user, parent: group,
- include_parent_descendants: include_parent_descendants?).execute.limit(LIMIT)
+ include_parent_descendants: include_parent_descendants?,
+ include_parent_shared_groups: include_parent_shared_groups?).execute.limit(LIMIT)
end
# rubocop: enable CodeReuse/Finder
end
diff --git a/ee/spec/finders/autocomplete/group_subgroups_finder_spec.rb b/ee/spec/finders/autocomplete/group_subgroups_finder_spec.rb
index bf2a3974c4c8b..ac416291199a8 100644
--- a/ee/spec/finders/autocomplete/group_subgroups_finder_spec.rb
+++ b/ee/spec/finders/autocomplete/group_subgroups_finder_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe Autocomplete::GroupSubgroupsFinder do
+RSpec.describe Autocomplete::GroupSubgroupsFinder, feature_category: :groups_and_projects do
describe '#execute' do
let_it_be(:group) { create(:group, :private) }
let_it_be(:subgroup_1) { create(:group, :private, parent: group) }
@@ -10,10 +10,17 @@
let_it_be(:grandchild_1) { create(:group, :private, parent: subgroup_1) }
let_it_be(:member_in_group) { create(:user).tap { |u| group.add_reporter(u) } }
let_it_be(:member_in_subgroup) { create(:user).tap { |u| subgroup_1.add_reporter(u) } }
+ let_it_be(:invited_to_group) { create(:group, :public) }
+ let_it_be(:invited_to_subgroup) { create(:group, :public) }
let(:params) { { group_id: group.id } }
let(:current_user) { member_in_group }
+ before do
+ group.shared_with_groups << invited_to_group
+ grandchild_1.shared_with_groups << invited_to_subgroup
+ end
+
subject { described_class.new(current_user, params).execute }
it 'returns subgroups', :aggregate_failures do
@@ -21,6 +28,22 @@
expect(subject).to contain_exactly(subgroup_1, subgroup_2)
end
+ context 'when include_parent_shared_groups parameter is true' do
+ before do
+ params[:include_parent_shared_groups] = true
+ params[:include_parent_descendants] = true
+ end
+
+ it 'returns subgroups and shared groups' do
+ expect(subject.count).to eq(4)
+ expect(subject).to contain_exactly(
+ subgroup_1,
+ subgroup_2,
+ grandchild_1,
+ invited_to_group)
+ end
+ end
+
context 'when include_parent_descendants parameter is true' do
before do
params[:include_parent_descendants] = true
diff --git a/spec/finders/groups_finder_spec.rb b/spec/finders/groups_finder_spec.rb
index b9add5b7db2de..c1ca19e29ec44 100644
--- a/spec/finders/groups_finder_spec.rb
+++ b/spec/finders/groups_finder_spec.rb
@@ -180,17 +180,29 @@
let_it_be(:internal_sub_subgroup) { create(:group, :internal, parent: public_subgroup) }
let_it_be(:private_sub_subgroup) { create(:group, :private, parent: public_subgroup) }
let_it_be(:public_sub_subgroup) { create(:group, :public, parent: public_subgroup) }
+ let_it_be(:invited_to_group) { create(:group, :public) }
+ let_it_be(:invited_to_subgroup) { create(:group, :public) }
let(:params) { { include_parent_descendants: true, parent: parent_group } }
+ before do
+ parent_group.shared_with_groups << invited_to_group
+ public_subgroup.shared_with_groups << invited_to_subgroup
+ end
+
context 'with nil parent' do
- it 'returns all accessible groups' do
+ before do
params[:parent] = nil
+ end
+
+ it 'returns all accessible groups' do
expect(described_class.new(user, params).execute).to contain_exactly(
parent_group,
public_subgroup,
internal_sub_subgroup,
- public_sub_subgroup
+ public_sub_subgroup,
+ invited_to_group,
+ invited_to_subgroup
)
end
end
@@ -227,6 +239,22 @@
private_sub_subgroup
)
end
+
+ context 'when include shared groups is set' do
+ before do
+ params[:include_parent_shared_groups] = true
+ end
+
+ it 'returns all group descendants with shared groups' do
+ expect(described_class.new(user, params).execute).to contain_exactly(
+ public_subgroup,
+ public_sub_subgroup,
+ internal_sub_subgroup,
+ private_sub_subgroup,
+ invited_to_group
+ )
+ end
+ end
end
end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index f9f18a8b23e8a..95c4af9c41c4b 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -1306,6 +1306,37 @@
it { is_expected.to match_array(groups) }
end
+
+ describe 'descendants_with_shared_with_groups' do
+ subject { described_class.descendants_with_shared_with_groups(parent_group) }
+
+ let_it_be(:grand_parent_group) { create(:group, :public) }
+ let_it_be(:parent_group) { create(:group, :public, parent: grand_parent_group) }
+ let_it_be(:subgroup) { create(:group, :public, parent: parent_group) }
+ let_it_be(:subsubgroup) { create(:group, :public, parent: subgroup) }
+
+ let_it_be(:shared_to_group) { create(:group, :public) }
+ let_it_be(:shared_to_sub_group) { create(:group, :public) }
+
+ context 'when parent group is nil' do
+ let(:parent_group) { nil }
+
+ it { is_expected.to match_array([]) }
+ end
+
+ context 'when parent group is present and there are shared groups' do
+ before do
+ parent_group.shared_with_groups << shared_to_group
+ subgroup.shared_with_groups << shared_to_sub_group
+ end
+
+ it { is_expected.to match_array([subgroup, subsubgroup, shared_to_group]) }
+ end
+
+ context 'when parent group is present and there are no shared groups' do
+ it { is_expected.to match_array([subgroup, subsubgroup]) }
+ end
+ end
end
describe '#to_reference' do
--
GitLab