Merge branch 'ee-30106-group-issues' into 'master'

Backport: Include subgroup issuables on the group page

See merge request gitlab-org/gitlab-ee!4341

app/controllers/concerns/issuable_collections.rb

@@ -94,6 +94,7 @@ module IssuableCollections
       @filter_params[:project_id] = @project.id
     elsif @group
       @filter_params[:group_id] = @group.id
+      @filter_params[:include_subgroups] = true
     else
       # TODO: this filter ignore issues/mr created in public or
       # internal repos where you are not a member. Enable this filter

app/finders/group_projects_finder.rb

@@ -87,8 +87,17 @@ class GroupProjectsFinder < ProjectsFinder
     options.fetch(:only_shared, false)
   end
 
+  # subgroups are supported only for owned projects not for shared
+  def include_subgroups?
+    options.fetch(:include_subgroups, false)
+  end
+
   def owned_projects
-    group.projects
+    if include_subgroups?
+      Project.where(namespace_id: group.self_and_descendants.select(:id))
+    else
+      group.projects
+    end
   end
 
   def shared_projects

app/finders/issuable_finder.rb

@@ -43,6 +43,7 @@ class IssuableFinder
     search
     sort
     state
+    include_subgroups
   ].freeze
   ARRAY_PARAMS = { label_name: [], iids: [], assignee_username: [] }.freeze
 
@@ -153,7 +154,8 @@ class IssuableFinder
       if current_user && params[:authorized_only].presence && !current_user_related?
         current_user.authorized_projects
       elsif group
-        GroupProjectsFinder.new(group: group, current_user: current_user).execute
+        finder_options = { include_subgroups: params[:include_subgroups], only_owned: true }
+        GroupProjectsFinder.new(group: group, current_user: current_user, options: finder_options).execute
       else
         ProjectsFinder.new(current_user: current_user, project_ids_relation: item_project_ids(items)).execute
       end

changelogs/unreleased/30106-group-issues.yml

+---
+title: Include subgroup issues and merge requests on the group page
+merge_request:
+author:
+type: changed

spec/features/groups/issues_spec.rb

@@ -3,40 +3,61 @@ require 'spec_helper'
 feature 'Group issues page' do
   include FilteredSearchHelpers
 
-  let(:path) { issues_group_path(group) }
-  let(:issuable) { create(:issue, project: project, title: "this is my created issuable")}
+  context 'with shared examples' do
+    let(:path) { issues_group_path(group) }
+    let(:issuable) { create(:issue, project: project, title: "this is my created issuable")}
 
-  include_examples 'project features apply to issuables', Issue
+    include_examples 'project features apply to issuables', Issue
 
-  context 'rss feed' do
-    let(:access_level) { ProjectFeature::ENABLED }
+    context 'rss feed' do
+      let(:access_level) { ProjectFeature::ENABLED }
 
-    context 'when signed in' do
-      let(:user) { user_in_group }
+      context 'when signed in' do
+        let(:user) { user_in_group }
 
-      it_behaves_like "it has an RSS button with current_user's RSS token"
-      it_behaves_like "an autodiscoverable RSS feed with current_user's RSS token"
-    end
+        it_behaves_like "it has an RSS button with current_user's RSS token"
+        it_behaves_like "an autodiscoverable RSS feed with current_user's RSS token"
+      end
 
-    context 'when signed out' do
-      let(:user) { nil }
+      context 'when signed out' do
+        let(:user) { nil }
 
-      it_behaves_like "it has an RSS button without an RSS token"
-      it_behaves_like "an autodiscoverable RSS feed without an RSS token"
+        it_behaves_like "it has an RSS button without an RSS token"
+        it_behaves_like "an autodiscoverable RSS feed without an RSS token"
+      end
     end
-  end
 
-  context 'assignee', :js do
-    let(:access_level) { ProjectFeature::ENABLED }
-    let(:user) { user_in_group }
-    let(:user2) { user_outside_group }
-    let(:path) { issues_group_path(group) }
+    context 'assignee', :js do
+      let(:access_level) { ProjectFeature::ENABLED }
+      let(:user) { user_in_group }
+      let(:user2) { user_outside_group }
+      let(:path) { issues_group_path(group) }
+
+      it 'filters by only group users' do
+        filtered_search.set('assignee:')
 
-    it 'filters by only group users' do
-      filtered_search.set('assignee:')
+        expect(find('#js-dropdown-assignee .filter-dropdown')).to have_content(user.name)
+        expect(find('#js-dropdown-assignee .filter-dropdown')).not_to have_content(user2.name)
+      end
+    end
+  end
 
-      expect(find('#js-dropdown-assignee .filter-dropdown')).to have_content(user.name)
-      expect(find('#js-dropdown-assignee .filter-dropdown')).not_to have_content(user2.name)
+  context 'issues list', :nested_groups do
+    let(:group) { create(:group)}
+    let(:subgroup) { create(:group, parent: group) }
+    let(:project) { create(:project, :public, group: group)}
+    let(:subgroup_project) { create(:project, :public, group: subgroup)}
+    let!(:issue) { create(:issue, project: project, title: 'root group issue') }
+    let!(:subgroup_issue) { create(:issue, project: subgroup_project, title: 'subgroup issue') }
+
+    it 'returns all group and subgroup issues' do
+      visit issues_group_path(group)
+
+      page.within('.issuable-list') do
+        expect(page).to have_selector('li.issue', count: 2)
+        expect(page).to have_content('root group issue')
+        expect(page).to have_content('subgroup issue')
+      end
     end
   end
 end

spec/finders/group_projects_finder_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe GroupProjectsFinder do
   let(:group) { create(:group) }
+  let(:subgroup) { create(:group, parent: group) }
   let(:current_user) { create(:user) }
   let(:options) { {} }
 
@@ -12,6 +13,8 @@ describe GroupProjectsFinder do
   let!(:shared_project_1) { create(:project, :public, path: '3') }
   let!(:shared_project_2) { create(:project, :private, path: '4') }
   let!(:shared_project_3) { create(:project, :internal, path: '5') }
+  let!(:subgroup_project) { create(:project, :public, path: '6', group: subgroup) }
+  let!(:subgroup_private_project) { create(:project, :private, path: '7', group: subgroup) }
 
   before do
     shared_project_1.project_group_links.create(group_access: Gitlab::Access::MASTER, group: group)
@@ -35,11 +38,31 @@ describe GroupProjectsFinder do
     context "only owned" do
       let(:options) { { only_owned: true } }
 
-      it { is_expected.to match_array([private_project, public_project]) }
+      context 'with subgroups projects', :nested_groups do
+        before do
+          options[:include_subgroups] = true
+        end
+
+        it { is_expected.to match_array([private_project, public_project, subgroup_project, subgroup_private_project]) }
+      end
+
+      context 'without subgroups projects' do
+        it { is_expected.to match_array([private_project, public_project]) }
+      end
     end
 
     context "all" do
-      it { is_expected.to match_array([shared_project_3, shared_project_2, shared_project_1, private_project, public_project]) }
+      context 'with subgroups projects', :nested_groups do
+        before do
+          options[:include_subgroups] = true
+        end
+
+        it { is_expected.to match_array([shared_project_3, shared_project_2, shared_project_1, private_project, public_project, subgroup_project, subgroup_private_project]) }
+      end
+
+      context 'without subgroups projects' do
+        it { is_expected.to match_array([shared_project_3, shared_project_2, shared_project_1, private_project, public_project]) }
+      end
     end
   end
 
@@ -71,9 +94,20 @@ describe GroupProjectsFinder do
       context "without external user" do
         before do
           private_project.add_master(current_user)
+          subgroup_private_project.add_master(current_user)
         end
 
-        it { is_expected.to match_array([private_project, public_project]) }
+        context 'with subgroups projects', :nested_groups do
+          before do
+            options[:include_subgroups] = true
+          end
+
+          it { is_expected.to match_array([private_project, public_project, subgroup_project, subgroup_private_project]) }
+        end
+
+        context 'without subgroups projects' do
+          it { is_expected.to match_array([private_project, public_project]) }
+        end
       end
 
       context "with external user" do
@@ -81,12 +115,32 @@ describe GroupProjectsFinder do
           current_user.update_attributes(external: true)
         end
 
-        it { is_expected.to eq([public_project]) }
+        context 'with subgroups projects', :nested_groups do
+          before do
+            options[:include_subgroups] = true
+          end
+
+          it { is_expected.to match_array([public_project, subgroup_project]) }
+        end
+
+        context 'without subgroups projects' do
+          it { is_expected.to eq([public_project]) }
+        end
       end
     end
 
     context "all" do
-      it { is_expected.to match_array([shared_project_3, shared_project_2, shared_project_1, public_project]) }
+      context 'with subgroups projects', :nested_groups do
+        before do
+          options[:include_subgroups] = true
+        end
+
+        it { is_expected.to match_array([shared_project_3, shared_project_2, shared_project_1, public_project, subgroup_project]) }
+      end
+
+      context 'without subgroups projects' do
+        it { is_expected.to match_array([shared_project_3, shared_project_2, shared_project_1, public_project]) }
+      end
     end
   end
 
@@ -137,7 +191,17 @@ describe GroupProjectsFinder do
     context "only owned" do
       let(:options) { { only_owned: true } }
 
-      it { is_expected.to eq([public_project]) }
+      context 'with subgroups projects', :nested_groups do
+        before do
+          options[:include_subgroups] = true
+        end
+
+        it { is_expected.to match_array([public_project, subgroup_project]) }
+      end
+
+      context 'without subgroups projects' do
+        it { is_expected.to eq([public_project]) }
+      end
     end
   end
 end

spec/finders/issues_finder_spec.rb

@@ -3,13 +3,17 @@ require 'spec_helper'
 describe IssuesFinder do
   set(:user) { create(:user) }
   set(:user2) { create(:user) }
-  set(:project1) { create(:project) }
+  set(:group) { create(:group) }
+  set(:subgroup) { create(:group, parent: group) }
+  set(:project1) { create(:project, group: group) }
   set(:project2) { create(:project) }
+  set(:project3) { create(:project, group: subgroup) }
   set(:milestone) { create(:milestone, project: project1) }
   set(:label) { create(:label, project: project2) }
   set(:issue1) { create(:issue, author: user, assignees: [user], project: project1, milestone: milestone, title: 'gitlab', created_at: 1.week.ago) }
   set(:issue2) { create(:issue, author: user, assignees: [user], project: project2, description: 'gitlab') }
   set(:issue3) { create(:issue, author: user2, assignees: [user2], project: project2, title: 'tanuki', description: 'tanuki', created_at: 1.week.from_now) }
+  set(:issue4) { create(:issue, project: project3) }
   set(:award_emoji1) { create(:award_emoji, name: 'thumbsup', user: user, awardable: issue1) }
   set(:award_emoji2) { create(:award_emoji, name: 'thumbsup', user: user2, awardable: issue2) }
   set(:award_emoji3) { create(:award_emoji, name: 'thumbsdown', user: user, awardable: issue3) }
@@ -25,10 +29,12 @@ describe IssuesFinder do
       project1.add_master(user)
       project2.add_developer(user)
       project2.add_developer(user2)
+      project3.add_developer(user)
 
       issue1
       issue2
       issue3
+      issue4
 
       award_emoji1
       award_emoji2
@@ -39,14 +45,14 @@ describe IssuesFinder do
       let(:scope) { 'all' }
 
       it 'returns all issues' do
-        expect(issues).to contain_exactly(issue1, issue2, issue3)
+        expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
       end
 
       context 'sort by issues with no weight' do
         let(:params) { { weight: Issue::WEIGHT_NONE } }
 
         it 'returns all issues' do
-          expect(issues).to contain_exactly(issue1, issue2, issue3)
+          expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
         end
       end
 
@@ -96,6 +102,26 @@ describe IssuesFinder do
         end
       end
 
+      context 'filtering by group_id' do
+        let(:params) { { group_id: group.id } }
+
+        context 'when include_subgroup param not set' do
+          it 'returns all group issues' do
+            expect(issues).to contain_exactly(issue1)
+          end
+        end
+
+        context 'when include_subgroup param is true', :nested_groups do
+          before do
+            params[:include_subgroups] = true
+          end
+
+          it 'returns all group and subgroup issues' do
+            expect(issues).to contain_exactly(issue1, issue4)
+          end
+        end
+      end
+
       context 'filtering by author ID' do
         let(:params) { { author_id: user2.id } }
 
@@ -133,7 +159,7 @@ describe IssuesFinder do
         let(:params) { { milestone_title: Milestone::None.title } }
 
         it 'returns issues with no milestone' do
-          expect(issues).to contain_exactly(issue2, issue3)
+          expect(issues).to contain_exactly(issue2, issue3, issue4)
         end
       end
 
@@ -231,7 +257,7 @@ describe IssuesFinder do
         let(:params) { { label_name: Label::None.title } }
 
         it 'returns issues with no labels' do
-          expect(issues).to contain_exactly(issue1, issue3)
+          expect(issues).to contain_exactly(issue1, issue3, issue4)
         end
       end
 
@@ -256,7 +282,7 @@ describe IssuesFinder do
           let(:params) { { state: 'opened' } }
 
           it 'returns only opened issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
           end
         end
 
@@ -272,7 +298,7 @@ describe IssuesFinder do
           let(:params) { { state: 'all' } }
 
           it 'returns all issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue, issue4)
           end
         end
 
@@ -280,7 +306,7 @@ describe IssuesFinder do
           let(:params) { { state: 'invalid_state' } }
 
           it 'returns all issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue, issue4)
           end
         end
       end
@@ -384,7 +410,7 @@ describe IssuesFinder do
       end
 
       it "doesn't return issues if feature disabled" do
-        [project1, project2].each do |project|
+        [project1, project2, project3].each do |project|
           project.project_feature.update!(issues_access_level: ProjectFeature::DISABLED)
         end
 
@@ -397,7 +423,7 @@ describe IssuesFinder do
     it 'returns the number of rows for the default state' do
       finder = described_class.new(user)
 
-      expect(finder.row_count).to eq(3)
+      expect(finder.row_count).to eq(4)
     end
 
     it 'returns the number of rows for a given state' do

spec/finders/merge_requests_finder_spec.rb

@@ -6,31 +6,36 @@ describe MergeRequestsFinder do
   let(:user)  { create :user }
   let(:user2) { create :user }
 
-  let(:project1) { create(:project, :public) }
+  let(:group) { create(:group) }
+  let(:subgroup) { create(:group, parent: group) }
+  let(:project1) { create(:project, :public, group: group) }
   let(:project2) { fork_project(project1, user) }
   let(:project3) do
     p = fork_project(project1, user)
     p.update!(archived: true)
     p
   end
+  let(:project4) { create(:project, :public, group: subgroup) }
 
   let!(:merge_request1) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project1) }
   let!(:merge_request2) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project1, state: 'closed') }
   let!(:merge_request3) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project2) }
   let!(:merge_request4) { create(:merge_request, :simple, author: user, source_project: project3, target_project: project3) }
+  let!(:merge_request5) { create(:merge_request, :simple, author: user, source_project: project4, target_project: project4) }
 
   before do
     project1.add_master(user)
     project2.add_developer(user)
     project3.add_developer(user)
     project2.add_developer(user2)
+    project4.add_developer(user)
   end
 
   describe "#execute" do
     it 'filters by scope' do
       params = { scope: 'authored', state: 'opened' }
       merge_requests = described_class.new(user, params).execute
-      expect(merge_requests.size).to eq(3)
+      expect(merge_requests.size).to eq(4)
     end
 
     it 'filters by project' do
@@ -45,10 +50,26 @@ describe MergeRequestsFinder do
       expect(merge_requests.size).to eq(1)
     end
 
+    it 'filters by group' do
+      params = { group_id: group.id }
+
+      merge_requests = described_class.new(user, params).execute
+
+      expect(merge_requests.size).to eq(2)
+    end
+
+    it 'filters by group including subgroups', :nested_groups do
+      params = { group_id: group.id, include_subgroups: true }
+
+      merge_requests = described_class.new(user, params).execute
+
+      expect(merge_requests.size).to eq(3)
+    end
+
     it 'filters by non_archived' do
       params = { non_archived: true }
       merge_requests = described_class.new(user, params).execute
-      expect(merge_requests.size).to eq(3)
+      expect(merge_requests.size).to eq(4)
     end
 
     it 'filters by iid' do
@@ -79,14 +100,14 @@ describe MergeRequestsFinder do
     end
 
     context 'with created_after and created_before params' do
-      let(:project4) { create(:project, forked_from_project: project1) }
+      let(:new_project) { create(:project, forked_from_project: project1) }
 
       let!(:new_merge_request) do
         create(:merge_request,
                :simple,
                author: user,
                created_at: 1.week.from_now,
-               source_project: project4,
+               source_project: new_project,
                target_project: project1)
       end
 
@@ -95,12 +116,12 @@ describe MergeRequestsFinder do
                :simple,
                author: user,
                created_at: 1.week.ago,
-               source_project: project4,
-               target_project: project4)
+               source_project: new_project,
+               target_project: new_project)
       end
 
       before do
-        project4.add_master(user)
+        new_project.add_master(user)
       end
 
       it 'filters by created_after' do
@@ -112,7 +133,7 @@ describe MergeRequestsFinder do
       end
 
       it 'filters by created_before' do
-        params = { project_id: project4.id, created_before: old_merge_request.created_at + 1.second }
+        params = { project_id: new_project.id, created_before: old_merge_request.created_at + 1.second }
 
         merge_requests = described_class.new(user, params).execute
 
@@ -125,7 +146,7 @@ describe MergeRequestsFinder do
     it 'returns the number of rows for the default state' do
       finder = described_class.new(user)
 
-      expect(finder.row_count).to eq(3)
+      expect(finder.row_count).to eq(4)
     end
 
     it 'returns the number of rows for a given state' do