Comments from code review applied. Also switched forked_from_project and ForkedProjectLinks to ForkNetworkMember

12 files changed, 96 insertions, 73 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index 76262acf50c..27287f079a4 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -289,6 +289,14 @@ class Group < Namespace
     "#{parent.full_path}/#{path_was}"
   end
 
+  def group_member(user)
+    if group_members.loaded?
+      group_members.find { |gm| gm.user_id == user.id }
+    else
+      group_members.find_by(user_id: user)
+    end
+  end
+
   private
 
   def update_two_factor_requirement
diff --git a/app/models/project.rb b/app/models/project.rb
index eaf4f555d3b..6f57763b05b 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1114,7 +1114,11 @@ class Project < ActiveRecord::Base
   end
 
   def project_member(user)
-    project_members.find_by(user_id: user)
+    if project_members.loaded?
+      project_members.find { |member| member.user_id == user.id }
+    else
+      project_members.find_by(user_id: user)
+    end
   end
 
   def default_branch
diff --git a/app/models/user.rb b/app/models/user.rb
index 14941fd7f98..ee12f541b0f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -998,7 +998,11 @@ class User < ActiveRecord::Base
   end
 
   def notification_settings_for(source)
-    notification_settings.find_or_initialize_by(source: source)
+    if notification_settings.loaded?
+      notification_settings.find { |notification| notification.source == source }
+    else
+      notification_settings.find_or_initialize_by(source: source)
+    end
   end
 
   # Lazy load global notification setting
diff --git a/app/services/projects/batch_count_service.rb b/app/services/projects/batch_count_service.rb
index b8a030e7bc5..a79fdb62d69 100644
--- a/app/services/projects/batch_count_service.rb
+++ b/app/services/projects/batch_count_service.rb
@@ -4,28 +4,19 @@ module Projects
       @projects = projects
     end
 
-    def count
-      @projects.map do |project|
-        [project.id, current_count_service(project).count]
-      end.to_h
-    end
-
     def refresh_cache
       @projects.each do |project|
-        unless current_count_service(project).count_stored?
-          current_count_service(project).refresh_cache { global_count[project.id].to_i }
+        service = count_service.new(project)
+        unless service.count_stored?
+          service.refresh_cache { global_count[project.id].to_i }
         end
       end
     end
 
-    def current_count_service(project)
-      if defined? @service
-        @service.project = project
-      else
-        @service = count_service.new(project)
-      end
+    def project_ids
+      return @projects if @projects.is_a?(ActiveRecord::Relation)
 
-      @service
+      @projects.map(&:id)
     end
 
     def global_count(project)
diff --git a/app/services/projects/batch_forks_count_service.rb b/app/services/projects/batch_forks_count_service.rb
index 51fad1c12a4..b9ed0b3cf4f 100644
--- a/app/services/projects/batch_forks_count_service.rb
+++ b/app/services/projects/batch_forks_count_service.rb
@@ -2,9 +2,11 @@ module Projects
   # Service class for getting and caching the number of forks of several projects
   class BatchForksCountService < Projects::BatchCountService
     def global_count
-      @global_count ||= ForkedProjectLink.where(forked_from_project: @projects.map(&:id))
-                                         .group(:forked_from_project_id)
-                                         .count
+      @global_count ||= begin
+        count_service.query(project_ids)
+                     .group(:forked_from_project_id)
+                     .count
+      end
     end
 
     def count_service
diff --git a/app/services/projects/batch_open_issues_count_service.rb b/app/services/projects/batch_open_issues_count_service.rb
index a0d9120487c..5087684863b 100644
--- a/app/services/projects/batch_open_issues_count_service.rb
+++ b/app/services/projects/batch_open_issues_count_service.rb
@@ -2,10 +2,9 @@ module Projects
   # Service class for getting and caching the number of forks of several projects
   class BatchOpenIssuesCountService < Projects::BatchCountService
     def global_count
-      @global_count ||= Issue.opened.public_only
-                             .where(project: @projects.map(&:id))
-                             .group(:project_id)
-                             .count
+      @global_count ||= begin
+        count_service.query(project_ids).group(:project_id).count
+      end
     end
 
     def count_service
diff --git a/app/services/projects/count_service.rb b/app/services/projects/count_service.rb
index a3ec52232dd..7e575b2d6f3 100644
--- a/app/services/projects/count_service.rb
+++ b/app/services/projects/count_service.rb
@@ -7,8 +7,6 @@ module Projects
     # all caches.
     VERSION = 1
 
-    attr_accessor :project
-
     def initialize(project)
       @project = project
     end
diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb
index d9bdf3a8ad7..d67ae78b268 100644
--- a/app/services/projects/forks_count_service.rb
+++ b/app/services/projects/forks_count_service.rb
@@ -2,11 +2,15 @@ module Projects
   # Service class for getting and caching the number of forks of a project.
   class ForksCountService < Projects::CountService
     def relation_for_count
-      @project.forks
+      self.class.query(@project.id)
     end
 
     def cache_key_name
       'forks_count'
     end
+
+    def self.query(project_ids)
+      ForkNetworkMember.where(forked_from_project: project_ids)
+    end
   end
 end
diff --git a/app/services/projects/open_issues_count_service.rb b/app/services/projects/open_issues_count_service.rb
index 25de97325e2..ab1c477936a 100644
--- a/app/services/projects/open_issues_count_service.rb
+++ b/app/services/projects/open_issues_count_service.rb
@@ -3,13 +3,17 @@ module Projects
   # project.
   class OpenIssuesCountService < Projects::CountService
     def relation_for_count
-      # We don't include confidential issues in this number since this would
-      # expose the number of confidential issues to non project members.
-      @project.issues.opened.public_only
+      self.class.query(@project.id)
     end
 
     def cache_key_name
       'open_issues_count'
     end
+
+    def self.query(project_ids)
+      # We don't include confidential issues in this number since this would
+      # expose the number of confidential issues to non project members.
+      Issue.opened.public_only.where(project: project_ids)
+    end
   end
 end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 8f41b930c0a..dada353a314 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -96,7 +96,7 @@ module API
       expose :star_count, :forks_count
       expose :created_at, :last_activity_at
 
-      def self.preload_relation(projects_relation)
+      def self.preload_relation(projects_relation, options =  {})
         projects_relation.preload(:project_feature, :route)
                          .preload(namespace: [:route, :owner],
                                   tags: :taggings)
@@ -134,18 +134,6 @@ module API
         expose :members do |project|
           expose_url(api_v4_projects_members_path(id: project.id))
         end
-
-        def self.preload_relation(projects_relation)
-          super(projects_relation).preload(:group)
-                                  .preload(project_group_links: :group,
-                                           fork_network: :root_project,
-                                           forked_project_link: :forked_from_project,
-                                           forked_from_project: [:route, :forks, namespace: :route, tags: :taggings])
-        end
-
-        def self.forks_counting_projects(projects_relation)
-          projects_relation + projects_relation.map(&:forked_from_project).compact
-        end
       end
 
       expose :archived?, as: :archived
@@ -165,7 +153,9 @@ module API
       expose :lfs_enabled?, as: :lfs_enabled
       expose :creator_id
       expose :namespace, using: 'API::Entities::NamespaceBasic'
-      expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? }
+      expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? } do |project, options|
+        project.fork_network_member.forked_from_project
+      end
       expose :import_status
       expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] }
 
@@ -182,6 +172,20 @@ module API
       expose :printing_merge_request_link_enabled
 
       expose :statistics, using: 'API::Entities::ProjectStatistics', if: :statistics
+
+      def self.preload_relation(projects_relation, options =  {})
+        relation = super(projects_relation).preload(:group)
+                                           .preload(project_group_links: :group,
+                                                    fork_network: :root_project,
+                                                    fork_network_member: [forked_from_project: [:route, namespace: :route, tags: :taggings]])
+
+        # Remove this preload once forked_project_links and forked_from_project models have been removed
+        relation.preload(forked_project_link: :forked_from_project)
+      end
+
+      def self.forks_counting_projects(projects_relation)
+        projects_relation + projects_relation.map(&:fork_network_member).compact.map(&:forked_from_project).compact
+      end
     end
 
     class ProjectStatistics < Grape::Entity
@@ -653,16 +657,10 @@ module API
     class MemberAccess < Grape::Entity
       expose :access_level
       expose :notification_level do |member, options|
-        notification = member_notification_setting(member)
-        ::NotificationSetting.levels[notification.level] if notification
-      end
-
-      private
-
-      def member_notification_setting(member)
-        member.user.notification_settings.select do |notification|
-          notification.source_id == member.source_id && notification.source_type == member.source_type
-        end.last
+        # binding.pry if member.id == 5
+        if member.notification_setting
+          ::NotificationSetting.levels[member.notification_setting.level]
+        end
       end
     end
 
@@ -705,25 +703,36 @@ module API
       expose :permissions do
         expose :project_access, using: Entities::ProjectAccess do |project, options|
           if options.key?(:project_members)
-            (options[:project_members] || []).select { |member| member.source_id == project.id }.last
-          elsif project.project_members.any?
-            # This is not the bet option to search in a CollectionProxy, but if
-            # we use find_by we will perform another query, even if the association
-            # is loaded
-            project.project_members.select { |project_member| project_member.user_id == options[:current_user].id }.last
+            (options[:project_members] || []).find { |member| member.source_id == project.id }
+          else
+            project.project_member(options[:current_user])
           end
         end
 
         expose :group_access, using: Entities::GroupAccess do |project, options|
           if project.group
             if options.key?(:group_members)
-              (options[:group_members] || []).select { |member| member.source_id == project.namespace_id }.last
-            elsif project.group.group_members.any?
-              project.group.group_members.select { |group_member| group_member.user_id == options[:current_user].id }.last
+              (options[:group_members] || []).find { |member| member.source_id == project.namespace_id }
+            else
+              project.group.group_member(options[:current_user])
             end
           end
         end
       end
+
+      def self.preload_relation(projects_relation, options = {})
+        relation = super(projects_relation, options)
+
+        unless options.key?(:group_members)
+          relation = relation.preload(group: [group_members: [:source, user: [notification_settings: :source]]])
+        end
+
+        unless options.key?(:project_members)
+          relation = relation.preload(project_members: [:source, user: [notification_settings: :source]])
+        end
+
+        relation
+      end
     end
 
     class LabelBasic < Grape::Entity
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 12506203429..9e92ff1417e 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -82,20 +82,20 @@ module API
         projects = paginate(projects)
 
         if current_user
-          project_members = current_user.project_members
-          group_members = current_user.group_members
+          project_members = current_user.project_members.preload(:source, user: [notification_settings: :source])
+          group_members = current_user.group_members.preload(:source, user: [notification_settings: :source])
         end
 
         options = options.reverse_merge(
           with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails,
           statistics: params[:statistics],
-          project_members: project_members,
-          group_members: group_members,
+          project_members: nil,
+          group_members: nil,
           current_user: current_user
         )
         options[:with] = Entities::BasicProjectDetails if params[:simple]
 
-        present options[:with].prepare_relation(projects), options
+        present options[:with].prepare_relation(projects, options), options
       end
     end
 
diff --git a/lib/api/projects_relation_builder.rb b/lib/api/projects_relation_builder.rb
index 57df6157225..2e042d51c05 100644
--- a/lib/api/projects_relation_builder.rb
+++ b/lib/api/projects_relation_builder.rb
@@ -3,13 +3,13 @@ module API
     extend ActiveSupport::Concern
 
     module ClassMethods
-      def prepare_relation(relation)
-        relation = preload_relation(relation)
-        execute_batch_counting(relation)
-        relation
+      def prepare_relation(projects_relation, options = {})
+        projects_relation = preload_relation(projects_relation, options)
+        execute_batch_counting(projects_relation)
+        projects_relation
       end
 
-      def preload_relation(relation)
+      def preload_relation(projects_relation, options =  {})
         raise NotImplementedError, 'self.preload_relation method must be defined and return a relation'
       end