Merge branch 'sarnold-issuable-parent-param-refactor' into 'master'

Issuable parent param refactor

See merge request gitlab-org/gitlab!40220

app/finders/issuable_finder.rb

@@ -44,6 +44,7 @@ class IssuableFinder
   NEGATABLE_PARAMS_HELPER_KEYS = %i[project_id scope status include_subgroups].freeze
 
   attr_accessor :current_user, :params
+  attr_writer :parent
 
   delegate(*%i[assignee milestones], to: :params)
 
@@ -204,8 +205,26 @@ class IssuableFinder
     end
   end
 
+  def parent_param=(obj)
+    @parent = obj
+    params[parent_param] = parent if parent
+  end
+
+  def parent_param
+    case parent
+    when Project
+      :project_id
+    when Group
+      :group_id
+    else
+      raise "Unexpected parent: #{parent.class}"
+    end
+  end
+
   private
 
+  attr_reader :parent
+
   def not_params
     strong_memoize(:not_params) do
       params_class.new(params[:not].dup, current_user, klass).tap do |not_params|

app/graphql/resolvers/concerns/issue_resolver_fields.rb → app/graphql/resolvers/concerns/issue_resolver_arguments.rb

 # frozen_string_literal: true
 
-module IssueResolverFields
+module IssueResolverArguments
   extend ActiveSupport::Concern
 
   prepended do

app/graphql/resolvers/issue_status_counts_resolver.rb

@@ -2,26 +2,13 @@
 
 module Resolvers
   class IssueStatusCountsResolver < BaseResolver
-    prepend IssueResolverFields
+    prepend IssueResolverArguments
 
     type Types::IssueStatusCountsType, null: true
 
     def continue_issue_resolve(parent, finder, **args)
-      finder.params[parent_param(parent)] = parent if parent
+      finder.parent_param = parent
       apply_lookahead(Gitlab::IssuablesCountForState.new(finder, parent))
     end
-
-    private
-
-    def parent_param(parent)
-      case parent
-      when Project
-        :project_id
-      when Group
-        :group_id
-      else
-        raise "Unexpected type of parent: #{parent.class}. Must be Project or Group"
-      end
-    end
   end
 end

app/graphql/resolvers/issues_resolver.rb

@@ -2,7 +2,7 @@
 
 module Resolvers
   class IssuesResolver < BaseResolver
-    prepend IssueResolverFields
+    prepend IssueResolverArguments
 
     argument :state, Types::IssuableStateEnum,
               required: false,

lib/gitlab/graphql/loaders/issuable_loader.rb

@@ -15,6 +15,7 @@ module Gitlab
 
         def batching_find_all(&with_query)
           if issuable_finder.params.keys == ['iids']
+            issuable_finder.parent = parent
             batch_load_issuables(issuable_finder.params[:iids], with_query)
           else
             post_process(find_all, with_query)
@@ -22,24 +23,12 @@ module Gitlab
         end
 
         def find_all
-          issuable_finder.params[parent_param] = parent if parent
-
+          issuable_finder.parent_param = parent if parent
           issuable_finder.execute
         end
 
         private
 
-        def parent_param
-          case parent
-          when Project
-            :project_id
-          when Group
-            :group_id
-          else
-            raise "Unexpected parent: #{parent.class}"
-          end
-        end
-
         def post_process(query, with_query)
           if with_query
             with_query.call(query)
@@ -56,7 +45,7 @@ module Gitlab
           return if parent.nil?
 
           BatchLoader::GraphQL
-            .for([parent_param, iid.to_s])
+            .for([issuable_finder.parent_param, iid.to_s])
             .batch(key: batch_key) do |params, loader, args|
               batch_key = args[:key]
               user = batch_key.current_user

spec/finders/issues_finder_spec.rb

@@ -3,6 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe IssuesFinder do
+  using RSpec::Parameterized::TableSyntax
   include_context 'IssuesFinder context'
 
   describe '#execute' do
@@ -1022,4 +1023,33 @@ RSpec.describe IssuesFinder do
       end
     end
   end
+
+  describe '#parent_param=' do
+    let(:finder) { described_class.new(nil) }
+
+    subject { finder.parent_param = obj }
+
+    where(:klass, :param) do
+      :Project | :project_id
+      :Group   | :group_id
+    end
+
+    with_them do
+      let(:obj) { Object.const_get(klass, false).new }
+
+      it 'sets the params' do
+        subject
+
+        expect(finder.params[param]).to eq(obj)
+      end
+    end
+
+    context 'unexpected parent' do
+      let(:obj) { MergeRequest.new }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error('Unexpected parent: MergeRequest')
+      end
+    end
+  end
 end

spec/graphql/resolvers/issue_status_counts_resolver_spec.rb

@@ -41,6 +41,12 @@ RSpec.describe Resolvers::IssueStatusCountsResolver do
 
     it_behaves_like 'returns expected results'
 
+    context 'project used as parent' do
+      let(:parent) { project }
+
+      it_behaves_like 'returns expected results'
+    end
+
     context 'group used as parent' do
       let(:parent) { project.group }
 

spec/lib/gitlab/graphql/loaders/issuable_loader_spec.rb

@@ -6,9 +6,26 @@ RSpec.describe Gitlab::Graphql::Loaders::IssuableLoader do
   subject { described_class.new(parent, finder) }
 
   let(:params) { HashWithIndifferentAccess.new }
+  let(:finder_params) { finder.params.to_h.with_indifferent_access }
+
+  # Dumb finder class, that only implements what we need, and has
+  # predictable query counts.
+  let(:finder_class) do
+    Class.new(IssuesFinder) do
+      def execute
+        params[:project_id].issues.where(iid: params[:iids])
+      end
+
+      private
+
+      def params_class
+        IssuesFinder::Params
+      end
+    end
+  end
 
   describe '#find_all' do
-    let(:finder) { double(:finder, params: params, execute: %i[x y z]) }
+    let(:finder) { issuable_finder(params: params, result: [:x, :y, :z]) }
 
     where(:factory, :param_name) do
       %i[project group].map { |thing| [thing, :"#{thing}_id"] }
@@ -19,7 +36,7 @@ RSpec.describe Gitlab::Graphql::Loaders::IssuableLoader do
 
       it 'assignes the parent parameter, and batching_find_alls the finder' do
         expect(subject.find_all).to contain_exactly(:x, :y, :z)
-        expect(params).to include(param_name => parent)
+        expect(finder_params).to include(param_name => parent)
       end
     end
 
@@ -34,12 +51,12 @@ RSpec.describe Gitlab::Graphql::Loaders::IssuableLoader do
 
   describe '#batching_find_all' do
     context 'the finder params are anything other than [iids]' do
-      let(:finder) { double(:finder, params: params, execute: [:foo]) }
+      let(:finder) { issuable_finder(params: params, result: [:foo]) }
       let(:parent) { build_stubbed(:project) }
 
       it 'batching_find_alls the finder, setting the correct parent parameter' do
         expect(subject.batching_find_all).to eq([:foo])
-        expect(params[:project_id]).to eq(parent)
+        expect(finder_params[:project_id]).to eq(parent)
       end
 
       it 'allows a post-process block' do
@@ -48,23 +65,6 @@ RSpec.describe Gitlab::Graphql::Loaders::IssuableLoader do
     end
 
     context 'the finder params are exactly [iids]' do
-      # Dumb finder class, that only implements what we need, and has
-      # predictable query counts.
-      let(:finder_class) do
-        Class.new do
-          attr_reader :current_user, :params
-
-          def initialize(user, args)
-            @current_user = user
-            @params = HashWithIndifferentAccess.new(args.to_h)
-          end
-
-          def execute
-            params[:project_id].issues.where(iid: params[:iids])
-          end
-        end
-      end
-
       it 'batches requests' do
         issue_a = create(:issue)
         issue_b = create(:issue)
@@ -93,4 +93,13 @@ RSpec.describe Gitlab::Graphql::Loaders::IssuableLoader do
       end
     end
   end
+
+  private
+
+  def issuable_finder(user: double(:user), params: {}, result: nil)
+    new_finder = finder_class.new(user, params)
+    allow(new_finder).to receive(:execute).and_return(result) if result
+
+    new_finder
+  end
 end