Refactor ProjectsFinder to not pluck IDs

app/finders/contributed_projects_finder.rb

+class ContributedProjectsFinder
+  def initialize(user)
+    @user = user
+  end
+
+  # Finds the projects "@user" contributed to, limited to either public projects
+  # or projects visible to the given user.
+  #
+  # current_user - When given the list of the projects is limited to those only
+  #                visible by this user.
+  #
+  # Returns an ActiveRecord::Relation.
+  def execute(current_user = nil)
+    if current_user
+      relation = projects_visible_to_user(current_user)
+    else
+      relation = public_projects
+    end
+
+    relation.includes(:namespace).order_id_desc
+  end
+
+  private
+
+  def projects_visible_to_user(current_user)
+    authorized = @user.contributed_projects.visible_to_user(current_user)
+
+    union = Gitlab::SQL::Union.
+      new([authorized.select(:id), public_projects.select(:id)])
+
+    Project.where("projects.id IN (#{union.to_sql})")
+  end
+
+  def public_projects
+    @user.contributed_projects.public_only
+  end
+end

app/finders/personal_projects_finder.rb

+class PersonalProjectsFinder
+  def initialize(user)
+    @user = user
+  end
+
+  # Finds the projects belonging to the user in "@user", limited to either
+  # public projects or projects visible to the given user.
+  #
+  # current_user - When given the list of projects is limited to those only
+  #                visible by this user.
+  #
+  # Returns an ActiveRecord::Relation.
+  def execute(current_user = nil)
+    if current_user
+      relation = projects_visible_to_user(current_user)
+    else
+      relation = public_projects
+    end
+
+    relation.includes(:namespace).order_id_desc
+  end
+
+  private
+
+  def projects_visible_to_user(current_user)
+    authorized = @user.personal_projects.visible_to_user(current_user)
+
+    union = Gitlab::SQL::Union.
+      new([authorized.select(:id), public_projects.select(:id)])
+
+    Project.where("projects.id IN (#{union.to_sql})")
+  end
+
+  def public_projects
+    @user.personal_projects.public_only
+  end
+end

app/finders/projects_finder.rb

 class ProjectsFinder
-  def execute(current_user, options = {})
+  # Returns all projects, optionally including group projects a user has access
+  # to.
+  #
+  # ## Examples
+  #
+  # Retrieving all public projects:
+  #
+  #     ProjectsFinder.new.execute
+  #
+  # Retrieving all public/internal projects and those the given user has access
+  # to:
+  #
+  #     ProjectsFinder.new.execute(some_user)
+  #
+  # Retrieving all public/internal projects as well as the group's projects the
+  # user has access to:
+  #
+  #     ProjectsFinder.new.execute(some_user, group: some_group)
+  #
+  # Returns an ActiveRecord::Relation.
+  def execute(current_user = nil, options = {})
     group = options[:group]
 
     if group
-      group_projects(current_user, group)
+      base, extra = group_projects(current_user, group)
     else
-      all_projects(current_user)
+      base, extra = all_projects(current_user)
+    end
+
+    if base and extra
+      union = Gitlab::SQL::Union.new([base.select(:id), extra.select(:id)])
+
+      Project.where("projects.id IN (#{union.to_sql})")
+    else
+      base
     end
   end
 
@@ -13,77 +41,36 @@ class ProjectsFinder
 
   def group_projects(current_user, group)
     if current_user
-      if group.users.include?(current_user)
-        # User is group member
-        #
-        # Return ALL group projects
-        group.projects
-      else
-        projects_members = ProjectMember.in_projects(group.projects).
-          with_user(current_user)
-
-        if projects_members.any?
-          # User is a project member
-          #
-          # Return only:
-          #   public projects
-          #   internal projects
-          #   joined projects
-          #
-          group.projects.where(
-            "projects.id IN (?) OR projects.visibility_level IN (?)",
-            projects_members.pluck(:source_id),
-            Project.public_and_internal_levels
-          )
-        else
-          # User has no access to group or group projects
-          #
-          # Return only:
-          #   public projects
-          #   internal projects
-          #
+      [
+        group_projects_for_user(current_user, group),
         group.projects.public_and_internal_only
-        end
-      end
+      ]
     else
-      # Not authenticated
-      #
-      # Return only:
-      #   public projects
-      group.projects.public_only
+      [group.projects.public_only]
     end
   end
 
   def all_projects(current_user)
     if current_user
-      if current_user.authorized_projects.any?
-        # User has access to private projects
-        #
-        # Return only:
-        #   public projects
-        #   internal projects
-        #   joined projects
-        #
-        Project.where(
-          "projects.id IN (?) OR projects.visibility_level IN (?)",
-          current_user.authorized_projects.pluck(:id),
-          Project.public_and_internal_levels
-        )
+      [current_user.authorized_projects, public_and_internal_projects]
     else
-        # User has no access to private projects
-        #
-        # Return only:
-        #   public projects
-        #   internal projects
-        #
-        Project.public_and_internal_only
+      [Project.public_only]
     end
+  end
+
+  def group_projects_for_user(current_user, group)
+    if group.users.include?(current_user)
+      group.projects
     else
-      # Not authenticated
-      #
-      # Return only:
-      #   public projects
-      Project.public_only
+      group.projects.visible_to_user(current_user)
     end
   end
+
+  def public_projects
+    Project.unscoped.public_only
+  end
+
+  def public_and_internal_projects
+    Project.unscoped.public_and_internal_only
+  end
 end

spec/finders/contributed_projects_finder_spec.rb

+require 'spec_helper'
+
+describe ContributedProjectsFinder do
+  let(:source_user) { create(:user) }
+  let(:current_user) { create(:user) }
+
+  let(:finder) { described_class.new(source_user) }
+
+  let!(:public_project) { create(:project, :public) }
+  let!(:private_project) { create(:project, :private) }
+
+  before do
+    private_project.team << [source_user, Gitlab::Access::MASTER]
+    private_project.team << [current_user, Gitlab::Access::DEVELOPER]
+    public_project.team << [source_user, Gitlab::Access::MASTER]
+
+    create(:event, action: Event::PUSHED, project: public_project,
+           target: public_project, author: source_user)
+
+    create(:event, action: Event::PUSHED, project: private_project,
+           target: private_project, author: source_user)
+  end
+
+  describe 'without a current user' do
+    subject { finder.execute }
+
+    it { is_expected.to eq([public_project]) }
+  end
+
+  describe 'with a current user' do
+    subject { finder.execute(current_user) }
+
+    it { is_expected.to eq([private_project, public_project]) }
+  end
+end

spec/finders/personal_projects_finder_spec.rb

+require 'spec_helper'
+
+describe PersonalProjectsFinder do
+  let(:source_user) { create(:user) }
+  let(:current_user) { create(:user) }
+
+  let(:finder) { described_class.new(source_user) }
+
+  let!(:public_project) do
+    create(:project, :public, namespace: source_user.namespace, name: 'A',
+           path: 'A')
+  end
+
+  let!(:private_project) do
+    create(:project, :private, namespace: source_user.namespace, name: 'B',
+           path: 'B')
+  end
+
+  before do
+    private_project.team << [current_user, Gitlab::Access::DEVELOPER]
+  end
+
+  describe 'without a current user' do
+    subject { finder.execute }
+
+    it { is_expected.to eq([public_project]) }
+  end
+
+  describe 'with a current user' do
+    subject { finder.execute(current_user) }
+
+    it { is_expected.to eq([private_project, public_project]) }
+  end
+end

spec/finders/projects_finder_spec.rb

 require 'spec_helper'
 
 describe ProjectsFinder do
-  let(:user) { create :user }
-  let(:group) { create :group }
+  describe '#execute' do
+    let(:user) { create(:user) }
 
-  let(:project1) { create(:empty_project, :public,   group: group) }
-  let(:project2) { create(:empty_project, :internal, group: group) }
-  let(:project3) { create(:empty_project, :private,  group: group) }
-  let(:project4) { create(:empty_project, :private,  group: group) }
+    let!(:private_project)  { create(:project, :private) }
+    let!(:internal_project) { create(:project, :internal) }
+    let!(:public_project)   { create(:project, :public) }
 
-  context 'non authenticated' do
-    subject { ProjectsFinder.new.execute(nil, group: group) }
+    let(:finder) { described_class.new }
 
-    it { is_expected.to include(project1) }
-    it { is_expected.not_to include(project2) }
-    it { is_expected.not_to include(project3) }
-    it { is_expected.not_to include(project4) }
+    describe 'without a group' do
+      describe 'without a user' do
+        subject { finder.execute }
+
+        it { is_expected.to eq([public_project]) }
       end
 
-  context 'authenticated' do
-    subject { ProjectsFinder.new.execute(user, group: group) }
+      describe 'with a user' do
+        subject { finder.execute(user) }
 
-    it { is_expected.to include(project1) }
-    it { is_expected.to include(project2) }
-    it { is_expected.not_to include(project3) }
-    it { is_expected.not_to include(project4) }
+        describe 'without private projects' do
+          it { is_expected.to eq([public_project, internal_project]) }
         end
 
-  context 'authenticated, project member' do
-    before { project3.team << [user, :developer] }
-
-    subject { ProjectsFinder.new.execute(user, group: group) }
+        describe 'with private projects' do
+          before do
+            private_project.team.add_user(user, Gitlab::Access::MASTER)
+          end
 
-    it { is_expected.to include(project1) }
-    it { is_expected.to include(project2) }
-    it { is_expected.to include(project3) }
-    it { is_expected.not_to include(project4) }
+          it do
+            is_expected.to eq([public_project, internal_project,
+                               private_project])
+          end
+        end
+      end
     end
 
-  context 'authenticated, group member' do
-    before { group.add_developer(user) }
+    describe 'with a group' do
+      let(:group) { public_project.group }
 
-    subject { ProjectsFinder.new.execute(user, group: group) }
+      describe 'without a user' do
+        subject { finder.execute(nil, group: group) }
 
-    it { is_expected.to include(project1) }
-    it { is_expected.to include(project2) }
-    it { is_expected.to include(project3) }
-    it { is_expected.to include(project4) }
+        it { is_expected.to eq([public_project]) }
+      end
+
+      describe 'with a user' do
+        subject { finder.execute(user, group: group) }
+
+        it { is_expected.to eq([public_project, internal_project]) }
+      end
+    end
   end
 end