Merge branch 'issue_217569_refactoring' into 'master'

Refactor issuable metadata

See merge request gitlab-org/gitlab!32472

app/controllers/concerns/issuable_collections.rb

@@ -5,7 +5,6 @@ module IssuableCollections
   include PaginatedCollection
   include SortingHelper
   include SortingPreference
-  include Gitlab::IssuableMetadata
   include Gitlab::Utils::StrongMemoize
 
   included do
@@ -44,7 +43,7 @@ module IssuableCollections
   def set_pagination
     @issuables          = @issuables.page(params[:page])
     @issuables          = per_page_for_relative_position if params[:sort] == 'relative_position'
-    @issuable_meta_data = issuable_meta_data(@issuables, collection_type, current_user)
+    @issuable_meta_data = Gitlab::IssuableMetadata.new(current_user, @issuables).data
     @total_pages        = issuable_page_count(@issuables)
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables

app/controllers/concerns/issuable_collections_action.rb

@@ -11,7 +11,7 @@ module IssuableCollectionsAction
               .non_archived
               .page(params[:page])
 
-    @issuable_meta_data = issuable_meta_data(@issues, collection_type, current_user)
+    @issuable_meta_data = Gitlab::IssuableMetadata.new(current_user, @issues).data
 
     respond_to do |format|
       format.html
@@ -22,7 +22,7 @@ module IssuableCollectionsAction
   def merge_requests
     @merge_requests = issuables_collection.page(params[:page])
 
-    @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type, current_user)
+    @issuable_meta_data = Gitlab::IssuableMetadata.new(current_user, @merge_requests).data
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
 

app/controllers/projects_controller.rb

@@ -314,8 +314,7 @@ class ProjectsController < Projects::ApplicationController
         @wiki_home = @project_wiki.find_page('home', params[:version_id])
       elsif @project.feature_available?(:issues, current_user)
         @issues = issuables_collection.page(params[:page])
-        @collection_type = 'Issue'
-        @issuable_meta_data = issuable_meta_data(@issues, @collection_type, current_user)
+        @issuable_meta_data = Gitlab::IssuableMetadata.new(current_user, @issues).data
       end
 
       render :show

app/models/concerns/issuable.rb

@@ -39,15 +39,6 @@ module Issuable
     locked: 4
   }.with_indifferent_access.freeze
 
-  # This object is used to gather issuable meta data for displaying
-  # upvotes, downvotes, notes and closing merge requests count for issues and merge requests
-  # lists avoiding n+1 queries and improving performance.
-  IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :mrs_count) do
-    def merge_requests_count(user = nil)
-      mrs_count
-    end
-  end
-
   included do
     cache_markdown_field :title, pipeline: :single_line
     cache_markdown_field :description, issuable_state_filter_enabled: true

ee/lib/api/analytics/code_review_analytics.rb

@@ -5,8 +5,6 @@ module API
     class CodeReviewAnalytics < Grape::API
       include PaginationParams
 
-      helpers ::Gitlab::IssuableMetadata
-
       helpers do
         def project
           @project ||= find_project!(params[:project_id])
@@ -49,7 +47,7 @@ module API
           present merge_requests,
                   with: EE::API::Entities::Analytics::CodeReview::MergeRequest,
                   current_user: current_user,
-                  issuable_metadata: issuable_meta_data(merge_requests, 'MergeRequest', current_user)
+                  issuable_metadata: Gitlab::IssuableMetadata.new(current_user, merge_requests).data
         end
       end
     end

ee/lib/api/epics.rb

@@ -10,7 +10,6 @@ module API
     end
 
     helpers ::API::Helpers::EpicsHelpers
-    helpers ::Gitlab::IssuableMetadata
 
     params do
       requires :id, type: String, desc: 'The ID of a group'
@@ -44,7 +43,7 @@ module API
         epics = paginate(find_epics(finder_params: { group_id: user_group.id })).with_api_entity_associations
 
         # issuable_metadata is the standard used by the Todo API
-        extra_options = { issuable_metadata: issuable_meta_data(epics, 'Epic'), with_labels_details: declared_params[:with_labels_details] }
+        extra_options = { issuable_metadata: Gitlab::IssuableMetadata.new(current_user, epics).data, with_labels_details: declared_params[:with_labels_details] }
         present epics, epic_options.merge(extra_options)
       end
 

lib/api/issues.rb

@@ -5,7 +5,6 @@ module API
     include PaginationParams
     helpers Helpers::IssuesHelpers
     helpers Helpers::RateLimiter
-    helpers ::Gitlab::IssuableMetadata
 
     before { authenticate_non_get! }
 
@@ -108,7 +107,7 @@ module API
           with: Entities::Issue,
           with_labels_details: declared_params[:with_labels_details],
           current_user: current_user,
-          issuable_metadata: issuable_meta_data(issues, 'Issue', current_user),
+          issuable_metadata: Gitlab::IssuableMetadata.new(current_user, issues).data,
           include_subscribed: false
         }
 
@@ -134,7 +133,7 @@ module API
           with: Entities::Issue,
           with_labels_details: declared_params[:with_labels_details],
           current_user: current_user,
-          issuable_metadata: issuable_meta_data(issues, 'Issue', current_user),
+          issuable_metadata: Gitlab::IssuableMetadata.new(current_user, issues).data,
           include_subscribed: false,
           group: user_group
         }
@@ -171,7 +170,7 @@ module API
           with_labels_details: declared_params[:with_labels_details],
           current_user: current_user,
           project: user_project,
-          issuable_metadata: issuable_meta_data(issues, 'Issue', current_user),
+          issuable_metadata: Gitlab::IssuableMetadata.new(current_user, issues).data,
           include_subscribed: false
         }
 

lib/api/merge_requests.rb

@@ -8,7 +8,6 @@ module API
 
     before { authenticate_non_get! }
 
-    helpers ::Gitlab::IssuableMetadata
     helpers Helpers::MergeRequestsHelpers
 
     # EE::API::MergeRequests would override the following helpers
@@ -92,7 +91,7 @@ module API
         if params[:view] == 'simple'
           options[:with] = Entities::MergeRequestSimple
         else
-          options[:issuable_metadata] = issuable_meta_data(merge_requests, 'MergeRequest', current_user)
+          options[:issuable_metadata] = Gitlab::IssuableMetadata.new(current_user, merge_requests).data
           if Feature.enabled?(:mr_list_api_skip_merge_status_recheck, default_enabled: true)
             options[:skip_merge_status_recheck] = !declared_params[:with_merge_status_recheck]
           end

lib/api/todos.rb

@@ -6,8 +6,6 @@ module API
 
     before { authenticate! }
 
-    helpers ::Gitlab::IssuableMetadata
-
     ISSUABLE_TYPES = {
       'merge_requests' => ->(iid) { find_merge_request_with_access(iid) },
       'issues' => ->(iid) { find_project_issue(iid) }
@@ -65,7 +63,7 @@ module API
             next unless collection
 
             targets = collection.map(&:target)
-            options[type] = { issuable_metadata: issuable_meta_data(targets, type, current_user) }
+            options[type] = { issuable_metadata: Gitlab::IssuableMetadata.new(current_user, targets).data }
           end
         end
       end

lib/gitlab/issuable_metadata.rb

 # frozen_string_literal: true
 
 module Gitlab
-  module IssuableMetadata
-    def issuable_meta_data(issuable_collection, collection_type, user = nil)
+  class IssuableMetadata
+    include Gitlab::Utils::StrongMemoize
+
+    # data structure to store issuable meta data like
+    # upvotes, downvotes, notes and closing merge requests counts for issues and merge requests
+    # this avoiding n+1 queries when loading issuable collections on frontend
+    IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :mrs_count) do
+      def merge_requests_count(user = nil)
+        mrs_count
+      end
+    end
+
+    attr_reader :current_user, :issuable_collection
+
+    def initialize(current_user, issuable_collection)
+      @current_user = current_user
+      @issuable_collection = issuable_collection
+
+      validate_collection!
+    end
+
+    def data
+      return {} if issuable_ids.empty?
+
+      issuable_ids.each_with_object({}) do |id, issuable_meta|
+        issuable_meta[id] = metadata_for_issuable(id)
+      end
+    end
+
+    private
+
+    def metadata_for_issuable(id)
+      downvotes = group_issuable_votes_count.find { |votes| votes.awardable_id == id && votes.downvote? }
+      upvotes = group_issuable_votes_count.find { |votes| votes.awardable_id == id && votes.upvote? }
+      notes = grouped_issuable_notes_count.find { |notes| notes.noteable_id == id }
+      merge_requests = grouped_issuable_merge_requests_count.find { |mr| mr.first == id }
+
+      IssuableMeta.new(
+        upvotes.try(:count).to_i,
+        downvotes.try(:count).to_i,
+        notes.try(:count).to_i,
+        merge_requests.try(:last).to_i
+      )
+    end
+
+    def validate_collection!
       # ActiveRecord uses Object#extend for null relations.
       if !(issuable_collection.singleton_class < ActiveRecord::NullRelation) &&
           issuable_collection.respond_to?(:limit_value) &&
@@ -10,36 +54,43 @@ module Gitlab
 
         raise 'Collection must have a limit applied for preloading meta-data'
       end
+    end
 
-      # map has to be used here since using pluck or select will
-      # throw an error when ordering issuables by priority which inserts
-      # a new order into the collection.
-      # We cannot use reorder to not mess up the paginated collection.
-      issuable_ids = issuable_collection.map(&:id)
+    def issuable_ids
+      strong_memoize(:issuable_ids) do
+        # map has to be used here since using pluck or select will
+        # throw an error when ordering issuables by priority which inserts
+        # a new order into the collection.
+        # We cannot use reorder to not mess up the paginated collection.
+        issuable_collection.map(&:id)
+      end
+    end
 
-      return {} if issuable_ids.empty?
+    def collection_type
+      # Supports relations or paginated arrays
+      issuable_collection.try(:model)&.name ||
+        issuable_collection.first&.model_name.to_s
+    end
 
-      issuable_notes_count = ::Note.count_for_collection(issuable_ids, collection_type)
-      issuable_votes_count = ::AwardEmoji.votes_for_collection(issuable_ids, collection_type)
-      issuable_merge_requests_count =
+    def group_issuable_votes_count
+      strong_memoize(:group_issuable_votes_count) do
+        AwardEmoji.votes_for_collection(issuable_ids, collection_type)
+      end
+    end
+
+    def grouped_issuable_notes_count
+      strong_memoize(:grouped_issuable_notes_count) do
+        ::Note.count_for_collection(issuable_ids, collection_type)
+      end
+    end
+
+    def grouped_issuable_merge_requests_count
+      strong_memoize(:grouped_issuable_merge_requests_count) do
         if collection_type == 'Issue'
-          ::MergeRequestsClosingIssues.count_for_collection(issuable_ids, user)
+          ::MergeRequestsClosingIssues.count_for_collection(issuable_ids, current_user)
         else
           []
         end
-
-      issuable_ids.each_with_object({}) do |id, issuable_meta|
-        downvotes = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.downvote? }
-        upvotes = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.upvote? }
-        notes = issuable_notes_count.find { |notes| notes.noteable_id == id }
-        merge_requests = issuable_merge_requests_count.find { |mr| mr.first == id }
-
-        issuable_meta[id] = ::Issuable::IssuableMeta.new(
-          upvotes.try(:count).to_i,
-          downvotes.try(:count).to_i,
-          notes.try(:count).to_i,
-          merge_requests.try(:last).to_i
-        )
       end
     end
   end

spec/lib/gitlab/issuable_metadata_spec.rb

@@ -6,14 +6,12 @@ describe Gitlab::IssuableMetadata do
   let(:user)     { create(:user) }
   let!(:project) { create(:project, :public, :repository, creator: user, namespace: user.namespace) }
 
-  subject { Class.new { include Gitlab::IssuableMetadata }.new }
-
   it 'returns an empty Hash if an empty collection is provided' do
-    expect(subject.issuable_meta_data(Issue.none, 'Issue', user)).to eq({})
+    expect(described_class.new(user, Issue.none).data).to eq({})
   end
 
   it 'raises an error when given a collection with no limit' do
-    expect { subject.issuable_meta_data(Issue.all, 'Issue', user) }.to raise_error(/must have a limit/)
+    expect { described_class.new(user, Issue.all) }.to raise_error(/must have a limit/)
   end
 
   context 'issues' do
@@ -25,7 +23,7 @@ describe Gitlab::IssuableMetadata do
     let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) }
 
     it 'aggregates stats on issues' do
-      data = subject.issuable_meta_data(Issue.all.limit(10), 'Issue', user)
+      data = described_class.new(user, Issue.all.limit(10)).data
 
       expect(data.count).to eq(2)
       expect(data[issue.id].upvotes).to eq(1)
@@ -48,7 +46,7 @@ describe Gitlab::IssuableMetadata do
     let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") }
 
     it 'aggregates stats on merge requests' do
-      data = subject.issuable_meta_data(MergeRequest.all.limit(10), 'MergeRequest', user)
+      data = described_class.new(user, MergeRequest.all.limit(10)).data
 
       expect(data.count).to eq(2)
       expect(data[merge_request.id].upvotes).to eq(1)

spec/support/shared_examples/controllers/issuables_list_metadata_shared_examples.rb

@@ -34,7 +34,7 @@ RSpec.shared_examples 'issuables list meta-data' do |issuable_type, action = nil
 
     aggregate_failures do
       expect(meta_data.keys).to match_array(issuables.map(&:id))
-      expect(meta_data.values).to all(be_kind_of(Issuable::IssuableMeta))
+      expect(meta_data.values).to all(be_kind_of(Gitlab::IssuableMetadata::IssuableMeta))
     end
   end