Merge branch 'bvl-subgroup-in-dropdowns' into 'master'

Include child projects a user can manage in namespace dropdowns

Closes #39987

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

app/helpers/namespaces_helper.rb

@@ -4,8 +4,11 @@ def namespace_id_from(params)
   end
 
   def namespaces_options(selected = :current_user, display_path: false, extra_group: nil)
-    groups  = current_user.owned_groups + current_user.masters_groups
-    users   = [current_user.namespace]
+    groups = current_user.manageable_groups
+               .joins(:route)
+               .includes(:route)
+               .order('routes.path')
+    users = [current_user.namespace]
 
     unless extra_group.nil? || extra_group.is_a?(Group)
       extra_group = Group.find(extra_group) if Namespace.find(extra_group).kind == 'group'

app/models/user.rb

@@ -921,7 +921,16 @@ def toggle_star(project)
   end
 
   def manageable_namespaces
-    @manageable_namespaces ||= [namespace] + owned_groups + masters_groups
+    @manageable_namespaces ||= [namespace] + manageable_groups
+  end
+
+  def manageable_groups
+    union = Gitlab::SQL::Union.new([owned_groups.select(:id),
+                                    masters_groups.select(:id)])
+    arel_union = Arel::Nodes::SqlLiteral.new(union.to_sql)
+    owned_and_master_groups = Group.where(Group.arel_table[:id].in(arel_union))
+
+    Gitlab::GroupHierarchy.new(owned_and_master_groups).base_and_descendants
   end
 
   def namespaces

changelogs/unreleased/bvl-subgroup-in-dropdowns.yml

@@ -0,0 +1,5 @@
+---
+title: Make sure a user can add projects to subgroups they have access to
+merge_request: 15294
+author:
+type: fixed

spec/features/projects/project_settings_spec.rb

@@ -144,7 +144,10 @@
     specify 'the project is accessible via the new path' do
       transfer_project(project, group)
       new_path = namespace_project_path(group, project)
+
       visit new_path
+      wait_for_requests
+
       expect(current_path).to eq(new_path)
       expect(find('.breadcrumbs')).to have_content(project.name)
     end
@@ -153,7 +156,10 @@
       old_path = project_path(project)
       transfer_project(project, group)
       new_path = namespace_project_path(group, project)
+
       visit old_path
+      wait_for_requests
+
       expect(current_path).to eq(new_path)
       expect(find('.breadcrumbs')).to have_content(project.name)
     end

spec/features/projects/user_creates_project_spec.rb

@@ -6,10 +6,11 @@
   before do
     sign_in(user)
     create(:personal_key, user: user)
-    visit(new_project_path)
   end
 
   it 'creates a new project' do
+    visit(new_project_path)
+
     fill_in(:project_path, with: 'Empty')
 
     page.within('#content-body') do
@@ -24,4 +25,32 @@
     expect(page).to have_content('git remote')
     expect(page).to have_content(project.url_to_repo)
   end
+
+  context 'in a subgroup they do not own', :nested_groups do
+    let(:parent) { create(:group) }
+    let!(:subgroup) { create(:group, parent: parent) }
+
+    before do
+      parent.add_owner(user)
+    end
+
+    it 'creates a new project' do
+      visit(new_project_path)
+
+      fill_in :project_path, with: 'a-subgroup-project'
+
+      page.find('.js-select-namespace').click
+      page.find("div[role='option']", text: subgroup.full_path).click
+
+      page.within('#content-body') do
+        click_button('Create project')
+      end
+
+      expect(page).to have_content("Project 'a-subgroup-project' was successfully created")
+
+      project = Project.last
+
+      expect(project.namespace).to eq(subgroup)
+    end
+  end
 end

spec/features/projects/user_transfers_a_project_spec.rb

@@ -0,0 +1,49 @@
+require 'spec_helper'
+
+feature 'User transfers a project', :js do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :repository, namespace: user.namespace) }
+
+  before do
+    sign_in user
+  end
+
+  def transfer_project(project, group)
+    visit edit_project_path(project)
+
+    page.within('.js-project-transfer-form') do
+      page.find('.select2-container').click
+    end
+
+    page.find("div[role='option']", text: group.full_name).click
+
+    click_button('Transfer project')
+
+    fill_in 'confirm_name_input', with: project.name
+
+    click_button 'Confirm'
+
+    wait_for_requests
+  end
+
+  it 'allows transferring a project to a subgroup of a namespace' do
+    group = create(:group)
+    group.add_owner(user)
+
+    transfer_project(project, group)
+
+    expect(project.reload.namespace).to eq(group)
+  end
+
+  context 'when nested groups are available', :nested_groups do
+    it 'allows transferring a project to a subgroup' do
+      parent = create(:group)
+      parent.add_owner(user)
+      subgroup = create(:group, parent: parent)
+
+      transfer_project(project, subgroup)
+
+      expect(project.reload.namespace).to eq(subgroup)
+    end
+  end
+end

spec/helpers/namespaces_helper_spec.rb

@@ -29,5 +29,30 @@
       expect(options).not_to include(admin_group.name)
       expect(options).to include(user_group.name)
     end
+
+    context 'when nested groups are available', :nested_groups do
+      it 'includes groups nested in groups the user can administer' do
+        allow(helper).to receive(:current_user).and_return(user)
+        child_group = create(:group, :private, parent: user_group)
+
+        options = helper.namespaces_options
+
+        expect(options).to include(child_group.name)
+      end
+
+      it 'orders the groups correctly' do
+        allow(helper).to receive(:current_user).and_return(user)
+        child_group = create(:group, :private, parent: user_group)
+        other_child = create(:group, :private, parent: user_group)
+        sub_child = create(:group, :private, parent: child_group)
+
+        expect(helper).to receive(:options_for_group)
+                            .with([user_group, child_group, sub_child, other_child], anything)
+                            .and_call_original
+        allow(helper).to receive(:options_for_group).and_call_original
+
+        helper.namespaces_options
+      end
+    end
   end
 end

spec/models/user_spec.rb

@@ -642,16 +642,40 @@
   end
 
   describe 'groups' do
+    let(:user) { create(:user) }
+    let(:group) { create(:group) }
+
     before do
-      @user = create :user
-      @group = create :group
-      @group.add_owner(@user)
+      group.add_owner(user)
     end
 
-    it { expect(@user.several_namespaces?).to be_truthy }
-    it { expect(@user.authorized_groups).to eq([@group]) }
-    it { expect(@user.owned_groups).to eq([@group]) }
-    it { expect(@user.namespaces).to match_array([@user.namespace, @group]) }
+    it { expect(user.several_namespaces?).to be_truthy }
+    it { expect(user.authorized_groups).to eq([group]) }
+    it { expect(user.owned_groups).to eq([group]) }
+    it { expect(user.namespaces).to contain_exactly(user.namespace, group) }
+    it { expect(user.manageable_namespaces).to contain_exactly(user.namespace, group) }
+
+    context 'with child groups', :nested_groups do
+      let!(:subgroup) { create(:group, parent: group) }
+
+      describe '#manageable_namespaces' do
+        it 'includes all the namespaces the user can manage' do
+          expect(user.manageable_namespaces).to contain_exactly(user.namespace, group, subgroup)
+        end
+      end
+
+      describe '#manageable_groups' do
+        it 'includes all the namespaces the user can manage' do
+          expect(user.manageable_groups).to contain_exactly(group, subgroup)
+        end
+
+        it 'does not include duplicates if a membership was added for the subgroup' do
+          subgroup.add_owner(user)
+
+          expect(user.manageable_groups).to contain_exactly(group, subgroup)
+        end
+      end
+    end
   end
 
   describe 'group multiple owners' do