Merge branch 'iterations-finder-reduce-queries' into 'master'

Reduce number of queries for finding iterations

See merge request gitlab-org/gitlab!62303

ee/app/finders/ee/issues_finder.rb

@@ -126,10 +126,13 @@ module EE
     end
 
     def iterations_finder_params
-      IterationsFinder.params_for_parent(params.parent, include_ancestors: true).merge!(
+      {
+        parent: params.parent,
+        include_ancestors: true,
         state: 'opened',
         start_date: Date.today,
-        end_date: Date.today)
+        end_date: Date.today
+      }
     end
   end
 end

ee/app/finders/iterations_finder.rb

@@ -3,8 +3,8 @@
 # Search for iterations
 #
 # params - Hash
-#   project_ids: Array of project ids or single project id or ActiveRecord relation.
-#   group_ids: Array of group ids or single group id or ActiveRecord relation.
+#   parent - The group in which to look-up iterations.
+#   include_ancestors - whether to look-up iterations in group ancestors.
 #   order - Orders by field default due date asc.
 #   title - Filter by title.
 #   state - Filters by state.
@@ -15,39 +15,16 @@ class IterationsFinder
 
   attr_reader :params, :current_user
 
-  class << self
-    def params_for_parent(parent, include_ancestors: false)
-      case parent
-      when Group
-        if include_ancestors
-          { group_ids: parent.self_and_ancestors.select(:id) }
-        else
-          { group_ids: parent.id }
-        end
-      when Project
-        if include_ancestors && parent.parent_id.present?
-          { group_ids: parent.parent.self_and_ancestors.select(:id), project_ids: parent.id }
-        else
-          { project_ids: parent.id }
-        end
-      else
-        raise ArgumentError, 'Invalid parent class. Only Project and Group are supported.'
-      end
-    end
-  end
-
   def initialize(current_user, params = {})
     @params = params
     @current_user = current_user
   end
 
   def execute
-    filter_permissions
-
     items = Iteration.all
     items = by_id(items)
     items = by_iid(items)
-    items = by_groups_and_projects(items)
+    items = by_groups(items)
     items = by_title(items)
     items = by_search_title(items)
     items = by_state(items)
@@ -59,33 +36,10 @@ class IterationsFinder
 
   private
 
-  def filter_permissions
-    filter_allowed_projects
-    filter_allowed_groups
-
-    # Only allow either one project_id or one group_id when filtering by `iid`
-    if params[:iid] && params.slice(:project_ids, :group_ids).keys.count > 1
-      raise ArgumentError, 'You can specify only one scope if you use iid filter'
-    end
-  end
-
-  def filter_allowed_projects
-    return unless params[:project_ids].present?
-
-    projects = Project.id_in(params[:project_ids])
-    params[:project_ids] = Project.projects_user_can(projects, current_user, :read_iteration)
-  end
-
-  def filter_allowed_groups
-    return unless params[:group_ids].present?
-
-    groups = Group.id_in(params[:group_ids])
-
-    params[:group_ids] = Group.groups_user_can(groups, current_user, :read_iteration)
-  end
+  def by_groups(items)
+    return Iteration.none unless Ability.allowed?(current_user, :read_iteration, params[:parent])
 
-  def by_groups_and_projects(items)
-    items.for_projects_and_groups(params[:project_ids], params[:group_ids])
+    items.of_groups(groups)
   end
 
   def by_id(items)
@@ -128,4 +82,19 @@ class IterationsFinder
     items.reorder(order_statement).order(:title)
   end
   # rubocop: enable CodeReuse/ActiveRecord
+
+  def groups
+    parent = params[:parent]
+
+    group = case parent
+            when Group
+              parent
+            when Project
+              parent.parent
+            else
+              raise ArgumentError, 'Invalid parent class. Only Project and Group are supported.'
+            end
+
+    params[:include_ancestors] ? group.self_and_ancestors : group
+  end
 end

ee/app/graphql/mutations/iterations/update.rb

@@ -67,7 +67,7 @@ module Mutations
       private
 
       def find_object(parent:, id:)
-        params = IterationsFinder.params_for_parent(parent).merge!(id: id)
+        params = { parent: parent, id: id }
 
         IterationsFinder.new(context[:current_user], params).execute.first
       end

ee/app/graphql/resolvers/iterations_resolver.rb

@@ -51,7 +51,9 @@ module Resolvers
     private
 
     def iterations_finder_params(args)
-      IterationsFinder.params_for_parent(parent, include_ancestors: args[:include_ancestors]).merge!(
+      {
+        parent: parent,
+        include_ancestors: args[:include_ancestors],
         id: args[:id],
         iid: args[:iid],
         iteration_cadence_ids: args[:iteration_cadence_ids],
@@ -59,7 +61,7 @@ module Resolvers
         start_date: args.dig(:timeframe, :start) || args[:start_date],
         end_date: args.dig(:timeframe, :end) || args[:end_date],
         search_title: args[:title]
-      )
+      }
     end
 
     def parent

ee/app/services/ee/boards/base_service.rb

@@ -53,7 +53,7 @@ module EE
       end
 
       def iterations_finder_params
-        IterationsFinder.params_for_parent(parent, include_ancestors: true).merge(state: 'all')
+        { parent: parent, include_ancestors: true, state: 'all' }
       end
     end
   end

ee/app/services/ee/boards/lists/create_service.rb

@@ -83,7 +83,7 @@ module EE
         end
 
         def find_iteration(board)
-          parent_params = ::IterationsFinder.params_for_parent(board.resource_parent, include_ancestors: true)
+          parent_params = { parent: board.resource_parent, include_ancestors: true }
           ::IterationsFinder.new(current_user, parent_params).find_by(id: params['iteration_id']) # rubocop: disable CodeReuse/ActiveRecord
         end
 

ee/lib/api/iterations.rb

@@ -23,10 +23,12 @@ module API
       end
 
       def iterations_finder_params(parent)
-        IterationsFinder.params_for_parent(parent, include_ancestors: params[:include_ancestors]).merge!(
+        {
+          parent: parent,
+          include_ancestors: params[:include_ancestors],
           state: params[:state],
           search_title: params[:search]
-        )
+        }
       end
     end
 

ee/lib/ee/gitlab/quick_actions/issue_actions.rb

@@ -131,7 +131,7 @@ module EE
           end
 
           def find_iterations(project, params = {})
-            parent_params = ::IterationsFinder.params_for_parent(project, include_ancestors: true)
+            parent_params = { parent: project, include_ancestors: true }
 
             ::IterationsFinder.new(current_user, params.merge(parent_params)).execute
           end

ee/spec/graphql/resolvers/iterations_resolver_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe Resolvers::IterationsResolver do
         id: nil,
         iid: nil,
         iteration_cadence_ids: nil,
-        group_ids: nil,
+        parent: nil,
         state: nil,
         start_date: nil,
         end_date: nil,
@@ -43,7 +43,7 @@ RSpec.describe Resolvers::IterationsResolver do
 
       context 'without parameters' do
         it 'calls IterationsFinder to retrieve all iterations' do
-          params = params_list.merge(group_ids: Group.where(id: group.id).select(:id), state: 'all')
+          params = params_list.merge(parent: group, include_ancestors: true, state: 'all')
 
           expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
 
@@ -60,7 +60,7 @@ RSpec.describe Resolvers::IterationsResolver do
           iid = 2
           iteration_cadence_ids = ['5']
 
-          params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, group_ids: group.id, state: 'closed', start_date: start_date, end_date: end_date, search_title: search)
+          params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, parent: group, include_ancestors: nil, state: 'closed', start_date: start_date, end_date: end_date, search_title: search)
 
           expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
 
@@ -75,7 +75,7 @@ RSpec.describe Resolvers::IterationsResolver do
           iid = 2
           iteration_cadence_ids = ['5']
 
-          params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, group_ids: group.id, state: 'closed', start_date: start_date, end_date: end_date, search_title: search)
+          params = params_list.merge(id: id, iid: iid, iteration_cadence_ids: iteration_cadence_ids, parent: group, include_ancestors: nil, state: 'closed', start_date: start_date, end_date: end_date, search_title: search)
 
           expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
 
@@ -85,7 +85,7 @@ RSpec.describe Resolvers::IterationsResolver do
         it 'accepts a raw model id for backward compatibility' do
           id = 1
           iid = 2
-          params = params_list.merge(id: id, iid: iid, group_ids: group.id, state: 'all')
+          params = params_list.merge(id: id, iid: iid, parent: group, include_ancestors: nil, state: 'all')
 
           expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
 
@@ -97,7 +97,7 @@ RSpec.describe Resolvers::IterationsResolver do
         let_it_be(:subgroup) { create(:group, :private, parent: group) }
 
         it 'defaults to include_ancestors' do
-          params = params_list.merge(group_ids: subgroup.self_and_ancestors.select(:id), state: 'all')
+          params = params_list.merge(parent: subgroup, include_ancestors: true, state: 'all')
 
           expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
 
@@ -105,7 +105,7 @@ RSpec.describe Resolvers::IterationsResolver do
         end
 
         it 'does not default to include_ancestors if IID is supplied' do
-          params = params_list.merge(iid: 1, group_ids: subgroup.id, state: 'all')
+          params = params_list.merge(iid: 1, parent: subgroup, include_ancestors: false, state: 'all')
 
           expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
 
@@ -113,7 +113,7 @@ RSpec.describe Resolvers::IterationsResolver do
         end
 
         it 'accepts include_ancestors false' do
-          params = params_list.merge(group_ids: subgroup.id, state: 'all')
+          params = params_list.merge(parent: subgroup, include_ancestors: false, state: 'all')
 
           expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
 

ee/spec/requests/api/graphql/iteration_spec.rb

@@ -107,16 +107,6 @@ RSpec.describe 'Querying an Iteration' do
           let(:expected_web_url) { /#{expected_web_path}$/ }
         end
       end
-
-      describe 'project-owned iteration' do
-        it_behaves_like 'scoped path' do
-          let(:queried_iteration) { project_iteration }
-          let(:expected_scope_path) { project_iteration_path(project, project_iteration.id) }
-          let(:expected_scope_url) { /#{expected_scope_path}$/ }
-          let(:expected_web_path) { project_iteration_path(project, project_iteration.id) }
-          let(:expected_web_url) { /#{expected_web_path}$/ }
-        end
-      end
     end
 
     context 'inside a group context' do

ee/spec/requests/api/iterations_spec.rb

@@ -78,11 +78,11 @@ RSpec.describe API::Iterations do
 
     it_behaves_like 'iterations list'
 
-    it 'excludes ancestor iterations when include_ancestors is set to false' do
+    it 'return direct parent group iterations when include_ancestors is set to false' do
       get api(api_path, user), params: { include_ancestors: false }
 
       expect(response).to have_gitlab_http_status(:ok)
-      expect(json_response.size).to eq(0)
+      expect(json_response.map { |i| i['id'] }).to contain_exactly(iteration.id, closed_iteration.id)
     end
   end
 end

ee/spec/services/boards/create_service_spec.rb

@@ -34,12 +34,24 @@ RSpec.describe Boards::CreateService, services: true do
       end
     end
 
-    it_behaves_like 'setting a milestone scope' do
-      subject { described_class.new(parent, double, milestone_id: milestone.id).execute.payload }
-    end
+    context 'when setting a timebox' do
+      let(:user) { create(:user) }
+
+      before do
+        parent.add_reporter(user)
+      end
 
-    it_behaves_like 'setting an iteration scope' do
-      subject { described_class.new(parent, nil, iteration_id: iteration.id).execute.payload }
+      it_behaves_like 'setting a milestone scope' do
+        before do
+          parent.add_reporter(user)
+        end
+
+        subject { described_class.new(parent, user, milestone_id: milestone.id).execute.payload }
+      end
+
+      it_behaves_like 'setting an iteration scope' do
+        subject { described_class.new(parent, user, iteration_id: iteration.id).execute.payload }
+      end
     end
   end
 end

ee/spec/services/boards/update_service_spec.rb

@@ -34,6 +34,10 @@ RSpec.describe Boards::UpdateService, services: true do
         hide_backlog_list: true, hide_closed_list: true }
     end
 
+    before do
+      project.add_reporter(user)
+    end
+
     context 'with group board' do
       let!(:board) { create(:board, group: group, name: 'Backend') }
 
@@ -46,19 +50,27 @@ RSpec.describe Boards::UpdateService, services: true do
       it_behaves_like 'board update service'
     end
 
-    it_behaves_like 'setting a milestone scope' do
-      subject { board.reload }
+    context 'when setting a timebox' do
+      let(:user) { create(:user) }
 
       before do
-        described_class.new(parent, double, milestone_id: milestone.id).execute(board)
+        parent.add_reporter(user)
       end
-    end
 
-    it_behaves_like 'setting an iteration scope' do
-      subject { board.reload }
+      it_behaves_like 'setting a milestone scope' do
+        subject { board.reload }
 
-      before do
-        described_class.new(parent, nil, iteration_id: iteration.id).execute(board)
+        before do
+          described_class.new(parent, user, milestone_id: milestone.id).execute(board)
+        end
+      end
+
+      it_behaves_like 'setting an iteration scope' do
+        subject { board.reload }
+
+        before do
+          described_class.new(parent, user, iteration_id: iteration.id).execute(board)
+        end
       end
     end
   end