Merge branch 'hide-inoperable-group-search-releases-filter-backend' into 'master'

Resolve "Hide inoperable group search Releases filter - backend"

See merge request gitlab-org/gitlab!50457

app/finders/issuable_finder.rb

@@ -4,6 +4,9 @@
 #
 # Used to filter Issues and MergeRequests collections by set of params
 #
+# Note: This class is NOT meant to be instantiated. Instead you should
+#       look at IssuesFinder or EpicsFinder, which inherit from this.
+#
 # Arguments:
 #   klass - actual class like Issue or MergeRequest
 #   current_user - which user use
@@ -92,6 +95,10 @@ class IssuableFinder
     IssuableFinder::Params
   end
 
+  def klass
+    raise NotImplementedError
+  end
+
   def initialize(current_user, params = {})
     @current_user = current_user
     @params = params_class.new(params, current_user, klass)
@@ -451,6 +458,7 @@ class IssuableFinder
 
   def by_release(items)
     return items unless params.releases?
+    return items if params.group? # don't allow release filtering at group level
 
     if params.filter_by_no_release?
       items.without_release

app/finders/issuable_finder/params.rb

@@ -108,16 +108,8 @@ class IssuableFinder
       project_id.present?
     end
 
-    def group
-      strong_memoize(:group) do
-        if params[:group_id].is_a?(Group)
-          params[:group_id]
-        elsif params[:group_id].present?
-          Group.find(params[:group_id])
-        else
-          nil
-        end
-      end
+    def group?
+      group_id.present?
     end
 
     def related_groups
@@ -143,10 +135,25 @@ class IssuableFinder
       end
     end
 
+    def group
+      strong_memoize(:group) do
+        next nil unless group?
+
+        group = group_id.is_a?(Group) ? group_id : Group.find(group_id)
+        group = nil unless Ability.allowed?(current_user, :read_group, group)
+
+        group
+      end
+    end
+
     def project_id
       params[:project_id]
     end
 
+    def group_id
+      params[:group_id]
+    end
+
     def projects
       strong_memoize(:projects) do
         next [project] if project?
@@ -216,14 +223,14 @@ class IssuableFinder
       strong_memoize(:milestones) do
         if milestones?
           if project?
-            group_id = project.group&.id
+            project_group_id = project.group&.id
             project_id = project.id
           end
 
-          group_id = group.id if group
+          project_group_id = group.id if group
 
           search_params =
-            { title: params[:milestone_title], project_ids: project_id, group_ids: group_id }
+            { title: params[:milestone_title], project_ids: project_id, group_ids: project_group_id }
 
           MilestonesFinder.new(search_params).execute # rubocop: disable CodeReuse/Finder
         else

changelogs/unreleased/hide-inoperable-group-search-releases-filter-backend.yml

+---
+title: Don't allow filtering by release tag on groups.
+merge_request: 50457
+author:
+type: fixed

ee/spec/finders/issues_finder_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { weight: Issue::WEIGHT_NONE } }
 
           it 'returns all issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5)
           end
         end
 
@@ -88,7 +88,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { epic_id: ::IssuableFinder::Params::FILTER_NONE } }
 
           it 'returns filtered issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5)
           end
         end
 
@@ -124,7 +124,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { not: { epic_id: epic_1.id } } }
 
           it 'returns issues not assigned to the epic' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue_2, issue_subepic)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, issue_2, issue_subepic)
           end
         end
       end
@@ -140,7 +140,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { iteration_id: ::IssuableFinder::Params::FILTER_NONE } }
 
           it 'returns all issues without iterations' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5)
           end
         end
 
@@ -174,7 +174,7 @@ RSpec.describe IssuesFinder do
               let(:params) { { group_id: group, not: { iteration_id: ::Iteration::Predefined::Current.title } } }
 
               it 'returns filtered issues' do
-                expect(issues).to contain_exactly(issue1, iteration_1_issue, iteration_2_issue)
+                expect(issues).to contain_exactly(issue1, issue5, iteration_1_issue, iteration_2_issue)
               end
             end
           end
@@ -208,7 +208,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { not: { iteration_title: iteration_1.title } } }
 
           it 'returns all issues that do not match the iteration title' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, iteration_2_issue)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, iteration_2_issue)
           end
         end
 
@@ -216,7 +216,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { iteration_id: nil } }
 
           it 'returns unfiltered issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, iteration_1_issue, iteration_2_issue)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, iteration_1_issue, iteration_2_issue)
           end
         end
       end

spec/finders/issues_finder_spec.rb

@@ -13,7 +13,27 @@ RSpec.describe IssuesFinder do
       let(:scope) { 'all' }
 
       it 'returns all issues' do
-        expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
+        expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5)
+      end
+
+      context 'user does not have read permissions' do
+        let(:search_user) { user2 }
+
+        context 'when filtering by project id' do
+          let(:params) { { project_id: project1.id } }
+
+          it 'returns no issues' do
+            expect(issues).to be_empty
+          end
+        end
+
+        context 'when filtering by group id' do
+          let(:params) { { group_id: group.id } }
+
+          it 'returns no issues' do
+            expect(issues).to be_empty
+          end
+        end
       end
 
       context 'assignee filtering' do
@@ -21,7 +41,7 @@ RSpec.describe IssuesFinder do
 
         it_behaves_like 'assignee ID filter' do
           let(:params) { { assignee_id: user.id } }
-          let(:expected_issuables) { [issue1, issue2] }
+          let(:expected_issuables) { [issue1, issue2, issue5] }
         end
 
         it_behaves_like 'assignee NOT ID filter' do
@@ -59,7 +79,25 @@ RSpec.describe IssuesFinder do
         end
 
         it_behaves_like 'any assignee filter' do
-          let(:expected_issuables) { [issue1, issue2, issue3] }
+          let(:expected_issuables) { [issue1, issue2, issue3, issue5] }
+        end
+      end
+
+      context 'filtering by release' do
+        context 'when the release tag is none' do
+          let(:params) { { release_tag: 'none' } }
+
+          it 'returns issues without releases' do
+            expect(issues).to contain_exactly(issue2, issue3, issue4, issue5)
+          end
+        end
+
+        context 'when the release tag exists' do
+          let(:params) { { project_id: project1.id, release_tag: release.tag } }
+
+          it 'returns the issues associated with that release' do
+            expect(issues).to contain_exactly(issue1)
+          end
         end
       end
 
@@ -68,7 +106,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { projects: [project1.id] } }
 
           it 'returns the issue belonging to the projects' do
-            expect(issues).to contain_exactly(issue1)
+            expect(issues).to contain_exactly(issue1, issue5)
           end
         end
 
@@ -76,7 +114,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { projects: Project.id_in(project1.id) } }
 
           it 'returns the issue belonging to the projects' do
-            expect(issues).to contain_exactly(issue1)
+            expect(issues).to contain_exactly(issue1, issue5)
           end
         end
       end
@@ -86,7 +124,7 @@ RSpec.describe IssuesFinder do
 
         context 'when include_subgroup param not set' do
           it 'returns all group issues' do
-            expect(issues).to contain_exactly(issue1)
+            expect(issues).to contain_exactly(issue1, issue5)
           end
 
           context 'when projects outside the group are passed' do
@@ -101,7 +139,7 @@ RSpec.describe IssuesFinder do
             let(:params) { { group_id: group.id, projects: [project1.id] } }
 
             it 'returns the issue within the group and projects' do
-              expect(issues).to contain_exactly(issue1)
+              expect(issues).to contain_exactly(issue1, issue5)
             end
           end
 
@@ -109,7 +147,15 @@ RSpec.describe IssuesFinder do
             let(:params) { { group_id: group.id, projects: Project.id_in(project1.id) } }
 
             it 'returns the issue within the group and projects' do
-              expect(issues).to contain_exactly(issue1)
+              expect(issues).to contain_exactly(issue1, issue5)
+            end
+          end
+
+          context 'when release_tag is passed as a parameter' do
+            let(:params) { { group_id: group.id, release_tag: 'dne-release-tag' } }
+
+            it 'ignores the release_tag parameter' do
+              expect(issues).to contain_exactly(issue1, issue5)
             end
           end
         end
@@ -120,7 +166,7 @@ RSpec.describe IssuesFinder do
           end
 
           it 'returns all group and subgroup issues' do
-            expect(issues).to contain_exactly(issue1, issue4)
+            expect(issues).to contain_exactly(issue1, issue4, issue5)
           end
 
           context 'when mixed projects are passed' do
@@ -145,7 +191,7 @@ RSpec.describe IssuesFinder do
         let(:params) { { not: { author_id: user2.id } } }
 
         it 'returns issues not created by that user' do
-          expect(issues).to contain_exactly(issue1, issue2, issue4)
+          expect(issues).to contain_exactly(issue1, issue2, issue4, issue5)
         end
       end
 
@@ -175,7 +221,7 @@ RSpec.describe IssuesFinder do
         let(:params) { { not: { milestone_title: milestone.title } } }
 
         it 'returns issues not assigned to that milestone' do
-          expect(issues).to contain_exactly(issue2, issue3, issue4)
+          expect(issues).to contain_exactly(issue2, issue3, issue4, issue5)
         end
       end
 
@@ -199,7 +245,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { not: { milestone_title: group_milestone.title } } }
 
           it 'returns issues not assigned to that group milestone' do
-            expect(issues).to contain_exactly(issue1, issue4)
+            expect(issues).to contain_exactly(issue1, issue4, issue5)
           end
         end
       end
@@ -208,13 +254,13 @@ RSpec.describe IssuesFinder do
         let(:params) { { milestone_title: 'None' } }
 
         it 'returns issues with no milestone' do
-          expect(issues).to contain_exactly(issue2, issue3, issue4)
+          expect(issues).to contain_exactly(issue2, issue3, issue4, issue5)
         end
 
         it 'returns issues with no milestone (deprecated)' do
           params[:milestone_title] = Milestone::None.title
 
-          expect(issues).to contain_exactly(issue2, issue3, issue4)
+          expect(issues).to contain_exactly(issue2, issue3, issue4, issue5)
         end
       end
 
@@ -343,7 +389,7 @@ RSpec.describe IssuesFinder do
             let(:params) { { not: { label_name: label.title } } }
 
             it 'returns issues that do not have that label' do
-              expect(issues).to contain_exactly(issue1, issue3, issue4)
+              expect(issues).to contain_exactly(issue1, issue3, issue4, issue5)
             end
 
             # IssuableFinder first filters using the outer params (the ones not inside the `not` key.)
@@ -383,7 +429,7 @@ RSpec.describe IssuesFinder do
             let(:params) { { not: { label_name: [label.title, label2.title].join(',') } } }
 
             it 'returns issues that do not have any of the labels provided' do
-              expect(issues).to contain_exactly(issue1, issue4)
+              expect(issues).to contain_exactly(issue1, issue4, issue5)
             end
           end
         end
@@ -405,7 +451,7 @@ RSpec.describe IssuesFinder do
             let(:params) { { not: { label_name: [label.title, label2.title].join(',') } } }
 
             it 'returns issues that do not have ANY ONE of the labels provided' do
-              expect(issues).to contain_exactly(issue1, issue4)
+              expect(issues).to contain_exactly(issue1, issue4, issue5)
             end
           end
         end
@@ -414,7 +460,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { label_name: described_class::Params::FILTER_NONE } }
 
           it 'returns issues with no labels' do
-            expect(issues).to contain_exactly(issue1, issue4)
+            expect(issues).to contain_exactly(issue1, issue4, issue5)
           end
         end
 
@@ -483,14 +529,14 @@ RSpec.describe IssuesFinder do
       end
 
       context 'filtering by issues iids' do
-        let(:params) { { iids: issue3.iid } }
+        let(:params) { { iids: [issue3.iid] } }
 
-        it 'returns issues with iids match' do
-          expect(issues).to contain_exactly(issue3)
+        it 'returns issues where iids match' do
+          expect(issues).to contain_exactly(issue3, issue5)
         end
 
         context 'using NOT' do
-          let(:params) { { not: { iids: issue3.iid } } }
+          let(:params) { { not: { iids: [issue3.iid] } } }
 
           it 'returns issues with no iids match' do
             expect(issues).to contain_exactly(issue1, issue2, issue4)
@@ -503,7 +549,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { state: 'opened' } }
 
           it 'returns only opened issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5)
           end
         end
 
@@ -519,7 +565,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { state: 'all' } }
 
           it 'returns all issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue, issue4)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue, issue4, issue5)
           end
         end
 
@@ -527,7 +573,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { state: 'invalid_state' } }
 
           it 'returns all issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue, issue4)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue, issue4, issue5)
           end
         end
       end
@@ -619,7 +665,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { my_reaction_emoji: 'None' } }
 
           it 'returns issues that the user did not react to' do
-            expect(issues).to contain_exactly(issue2, issue4)
+            expect(issues).to contain_exactly(issue2, issue4, issue5)
           end
         end
 
@@ -642,7 +688,7 @@ RSpec.describe IssuesFinder do
             let(:params) { { not: { my_reaction_emoji: 'thumbsup' } } }
 
             it 'returns issues that the user did not thumbsup to' do
-              expect(issues).to contain_exactly(issue2, issue3, issue4)
+              expect(issues).to contain_exactly(issue2, issue3, issue4, issue5)
             end
           end
         end
@@ -676,7 +722,7 @@ RSpec.describe IssuesFinder do
             let(:params) { { not: { my_reaction_emoji: 'thumbsdown' } } }
 
             it 'returns issues that the user thumbsdown to' do
-              expect(issues).to contain_exactly(issue1, issue2, issue4)
+              expect(issues).to contain_exactly(issue1, issue2, issue4, issue5)
             end
           end
         end
@@ -687,7 +733,7 @@ RSpec.describe IssuesFinder do
 
         context 'no filtering' do
           it 'returns all issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, confidential_issue)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue)
           end
         end
 
@@ -703,7 +749,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { confidential: false } }
 
           it 'returns only confdential issues' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5)
           end
         end
       end
@@ -715,7 +761,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { issue_types: [] } }
 
           it 'returns all issues' do
-            expect(issues).to contain_exactly(incident_issue, issue1, issue2, issue3, issue4)
+            expect(issues).to contain_exactly(incident_issue, issue1, issue2, issue3, issue4, issue5)
           end
         end
 
@@ -731,7 +777,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { issue_types: ['issue'] } }
 
           it 'returns all issues with type issue' do
-            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4)
+            expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5)
           end
         end
 
@@ -739,7 +785,7 @@ RSpec.describe IssuesFinder do
           let(:params) { { issue_types: %w(issue incident) } }
 
           it 'returns all issues' do
-            expect(issues).to contain_exactly(incident_issue, issue1, issue2, issue3, issue4)
+            expect(issues).to contain_exactly(incident_issue, issue1, issue2, issue3, issue4, issue5)
           end
         end
 
@@ -790,14 +836,14 @@ RSpec.describe IssuesFinder do
       let(:scope) { 'assigned_to_me' }
 
       it 'returns issue assigned to the user' do
-        expect(issues).to contain_exactly(issue1, issue2)
+        expect(issues).to contain_exactly(issue1, issue2, issue5)
       end
 
       context 'filtering by project' do
         let(:params) { { project_id: project1.id } }
 
         it 'returns issues assigned to the user in that project' do
-          expect(issues).to contain_exactly(issue1)
+          expect(issues).to contain_exactly(issue1, issue5)
         end
       end
     end
@@ -839,7 +885,7 @@ RSpec.describe IssuesFinder do
         let(:params) { base_params.merge(due_date: Issue::NoDueDate.name) }
 
         it 'returns issues with no due date' do
-          expect(issues).to contain_exactly(issue1)
+          expect(issues).to contain_exactly(issue1, issue5)
         end
       end
 
@@ -875,7 +921,7 @@ RSpec.describe IssuesFinder do
     it 'returns the number of rows for the default state' do
       finder = described_class.new(admin)
 
-      expect(finder.row_count).to eq(4)
+      expect(finder.row_count).to eq(5)
     end
 
     it 'returns the number of rows for a given state' do

spec/support/shared_contexts/finders/issues_finder_shared_contexts.rb

@@ -8,13 +8,53 @@ RSpec.shared_context 'IssuesFinder context' do
   let_it_be(:project1, reload: true) { create(:project, group: group) }
   let_it_be(:project2, reload: true) { create(:project) }
   let_it_be(:project3, reload: true) { create(:project, group: subgroup) }
-  let_it_be(:milestone) { create(:milestone, project: project1) }
+  let_it_be(:release) { create(:release, project: project1, tag: 'v1.0.0') }
+  let_it_be(:milestone) { create(:milestone, project: project1, releases: [release]) }
   let_it_be(:label) { create(:label, project: project2) }
   let_it_be(:label2) { create(:label, project: project2) }
-  let_it_be(:issue1, reload: true) { create(:issue, author: user, assignees: [user], project: project1, milestone: milestone, title: 'gitlab', created_at: 1.week.ago, updated_at: 1.week.ago) }
-  let_it_be(:issue2, reload: true) { create(:issue, author: user, assignees: [user], project: project2, description: 'gitlab', created_at: 1.week.from_now, updated_at: 1.week.from_now) }
-  let_it_be(:issue3, reload: true) { create(:issue, author: user2, assignees: [user2], project: project2, title: 'tanuki', description: 'tanuki', created_at: 2.weeks.from_now, updated_at: 2.weeks.from_now) }
+  let_it_be(:issue1, reload: true) do
+    create(:issue,
+           author: user,
+           assignees: [user],
+           project: project1,
+           milestone: milestone,
+           title: 'gitlab',
+           created_at: 1.week.ago,
+           updated_at: 1.week.ago)
+  end
+
+  let_it_be(:issue2, reload: true) do
+    create(:issue,
+           author: user,
+           assignees: [user],
+           project: project2,
+           description: 'gitlab',
+           created_at: 1.week.from_now,
+           updated_at: 1.week.from_now)
+  end
+
+  let_it_be(:issue3, reload: true) do
+    create(:issue,
+           author: user2,
+           assignees: [user2],
+           project: project2,
+           title: 'tanuki',
+           description: 'tanuki',
+           created_at: 2.weeks.from_now,
+           updated_at: 2.weeks.from_now)
+  end
+
   let_it_be(:issue4, reload: true) { create(:issue, project: project3) }
+  let_it_be(:issue5, reload: true) do
+    create(:issue,
+           author: user,
+           assignees: [user],
+           project: project1,
+           title: 'wotnot',
+           created_at: 3.days.ago,
+           updated_at: 3.days.ago)
+  end
+
   let_it_be(:award_emoji1) { create(:award_emoji, name: 'thumbsup', user: user, awardable: issue1) }
   let_it_be(:award_emoji2) { create(:award_emoji, name: 'thumbsup', user: user2, awardable: issue2) }
   let_it_be(:award_emoji3) { create(:award_emoji, name: 'thumbsdown', user: user, awardable: issue3) }