Merge branch 'fix-transfer-group-possibilities' into 'master'

Fix group transfer selection possibilities

Closes #52295

See merge request gitlab-org/gitlab-ce!26123

4 files changed, 49 insertions, 2 deletions

diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb
index 0080123407d..7d419103b1c 100644
--- a/app/finders/groups_finder.rb
+++ b/app/finders/groups_finder.rb
@@ -11,6 +11,7 @@
 #     parent: Group
 #     all_available: boolean (defaults to true)
 #     min_access_level: integer
+#     exclude_group_ids: array of integers
 #
 # Users with full private access can see all groups. The `owned` and `parent`
 # params can be used to restrict the groups that are returned.
@@ -29,6 +30,7 @@ class GroupsFinder < UnionFinder
     items = all_groups.map do |item|
       item = by_parent(item)
       item = by_custom_attributes(item)
+      item = exclude_group_ids(item)
 
       item
     end
@@ -72,6 +74,12 @@ class GroupsFinder < UnionFinder
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
+  def exclude_group_ids(groups)
+    return groups unless params[:exclude_group_ids]
+
+    groups.id_not_in(params[:exclude_group_ids])
+  end
+
   # rubocop: disable CodeReuse/ActiveRecord
   def by_parent(groups)
     return groups unless params[:parent]
diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb
index 9d028dccad7..7af766c8544 100644
--- a/app/helpers/groups_helper.rb
+++ b/app/helpers/groups_helper.rb
@@ -118,11 +118,12 @@ module GroupsHelper
   end
 
   def parent_group_options(current_group)
-    groups = current_user.owned_groups.sort_by(&:human_name).map do |group|
+    exclude_groups = current_group.self_and_descendants.pluck_primary_key
+    exclude_groups << current_group.parent_id if current_group.parent_id
+    groups = GroupsFinder.new(current_user, min_access_level: Gitlab::Access::OWNER, exclude_group_ids: exclude_groups).execute.sort_by(&:human_name).map do |group|
       { id: group.id, text: group.human_name }
     end
 
-    groups.delete_if { |group| group[:id] == current_group.id }
     groups.to_json
   end
 
diff --git a/changelogs/unreleased/fix-transfer-group-possibilities.yml b/changelogs/unreleased/fix-transfer-group-possibilities.yml
new file mode 100644
index 00000000000..ebefb47b3da
--- /dev/null
+++ b/changelogs/unreleased/fix-transfer-group-possibilities.yml
@@ -0,0 +1,5 @@
+---
+title: Fix group transfer selection possibilities
+merge_request: 26123
+author: Peter Marko
+type: fixed
diff --git a/spec/helpers/groups_helper_spec.rb b/spec/helpers/groups_helper_spec.rb
index 91541a16c13..1763c46389a 100644
--- a/spec/helpers/groups_helper_spec.rb
+++ b/spec/helpers/groups_helper_spec.rb
@@ -229,4 +229,37 @@ describe GroupsHelper do
       expect(helper.group_sidebar_links).not_to include(*cross_project_features)
     end
   end
+
+  describe 'parent_group_options', :nested_groups do
+    let(:current_user) { create(:user) }
+    let(:group) { create(:group, name: 'group') }
+    let(:group2) { create(:group, name: 'group2') }
+
+    before do
+      group.add_owner(current_user)
+      group2.add_owner(current_user)
+    end
+
+    it 'includes explicitly owned groups except self' do
+      expect(parent_group_options(group2)).to eq([{ id: group.id, text: group.human_name }].to_json)
+    end
+
+    it 'excludes parent group' do
+      subgroup = create(:group, parent: group2)
+
+      expect(parent_group_options(subgroup)).to eq([{ id: group.id, text: group.human_name }].to_json)
+    end
+
+    it 'includes subgroups with inherited ownership' do
+      subgroup = create(:group, parent: group)
+
+      expect(parent_group_options(group2)).to eq([{ id: group.id, text: group.human_name }, { id: subgroup.id, text: subgroup.human_name }].to_json)
+    end
+
+    it 'excludes own subgroups' do
+      create(:group, parent: group2)
+
+      expect(parent_group_options(group2)).to eq([{ id: group.id, text: group.human_name }].to_json)
+    end
+  end
 end