Extract method User#authorizations_for_projects.42877-fix-visibility-change-performance

4 files changed, 40 insertions, 17 deletions

diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb
index 565955ffc05..d498a2d6d11 100644
--- a/app/finders/snippets_finder.rb
+++ b/app/finders/snippets_finder.rb
@@ -72,11 +72,7 @@ class SnippetsFinder < UnionFinder
     # we can shortcut and just return.
     return yield(Project.all) if current_user.full_private_access?
 
-    authorized = current_user
-      .project_authorizations
-      .select(1)
-      .where('project_authorizations.project_id = projects.id')
-    authorized_projects = yield(Project.where('EXISTS (?)', authorized))
+    authorized_projects = yield(Project.where('EXISTS (?)', current_user.authorizations_for_projects))
 
     levels = Gitlab::VisibilityLevel.levels_for_user(current_user)
     visible_projects = yield(Project.where(visibility_level: levels))
diff --git a/app/models/project.rb b/app/models/project.rb
index ad4315e1601..5b1f8b2658b 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -319,14 +319,9 @@ class Project < ActiveRecord::Base
   # logged in user.
   def self.public_or_visible_to_user(user = nil)
     if user
-      authorized = user
-        .project_authorizations
-        .select(1)
-        .where('project_authorizations.project_id = projects.id')
-
-      levels = Gitlab::VisibilityLevel.levels_for_user(user)
-
-      where('EXISTS (?) OR projects.visibility_level IN (?)', authorized, levels)
+      where('EXISTS (?) OR projects.visibility_level IN (?)',
+            user.authorizations_for_projects,
+            Gitlab::VisibilityLevel.levels_for_user(user))
     else
       public_to_user
     end
@@ -347,14 +342,11 @@ class Project < ActiveRecord::Base
     elsif user
       column = ProjectFeature.quoted_access_level_column(feature)
 
-      authorized = user.project_authorizations.select(1)
-        .where('project_authorizations.project_id = projects.id')
-
       with_project_feature
         .where("#{column} IN (?) OR (#{column} = ? AND EXISTS (?))",
               visible,
               ProjectFeature::PRIVATE,
-              authorized)
+              user.authorizations_for_projects)
     else
       with_feature_access_level(feature, visible)
     end
diff --git a/app/models/user.rb b/app/models/user.rb
index 9547506d33d..9c60adf0c90 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -601,6 +601,15 @@ class User < ActiveRecord::Base
     authorized_projects(min_access_level).exists?({ id: project.id })
   end
 
+  # Typically used in conjunction with projects table to get projects
+  # a user has been given access to.
+  #
+  # Example use:
+  # `Project.where('EXISTS(?)', user.authorizations_for_projects)`
+  def authorizations_for_projects
+    project_authorizations.select(1).where('project_authorizations.project_id = projects.id')
+  end
+
   # Returns the projects this user has reporter (or greater) access to, limited
   # to at most the given projects.
   #
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 3531de244bd..00b5226d874 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1635,6 +1635,32 @@ describe User do
     end
   end
 
+  describe '#authorizations_for_projects' do
+    let!(:user) { create(:user) }
+    subject { Project.where("EXISTS (?)", user.authorizations_for_projects) }
+
+    it 'includes projects that belong to a user, but no other projects' do
+      owned = create(:project, :private, namespace: user.namespace)
+      member = create(:project, :private).tap { |p| p.add_master(user) }
+      other = create(:project)
+
+      expect(subject).to include(owned)
+      expect(subject).to include(member)
+      expect(subject).not_to include(other)
+    end
+
+    it 'includes projects a user has access to, but no other projects' do
+      other_user = create(:user)
+      accessible = create(:project, :private, namespace: other_user.namespace) do |project|
+        project.add_developer(user)
+      end
+      other = create(:project)
+
+      expect(subject).to include(accessible)
+      expect(subject).not_to include(other)
+    end
+  end
+
   describe '#authorized_projects', :delete do
     context 'with a minimum access level' do
       it 'includes projects for which the user is an owner' do