Use just SQL to check is a user can admin_issue on a project17932-move-to-project-dropdown-sql

Using offset pagination instead pages to avoid a count query

Tradeoff

- we duplicate how we check admin_issue in a SQL relation in the Ability class

5 files changed, 111 insertions, 32 deletions

diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb
index 350106a0b29..c39a0e2c6a5 100644
--- a/app/controllers/autocomplete_controller.rb
+++ b/app/controllers/autocomplete_controller.rb
@@ -36,8 +36,6 @@ class AutocompleteController < ApplicationController
     project = Project.find_by_id(params[:project_id])
     projects = projects_finder.execute(project, search: params[:search], offset_id: params[:offset_id])
 
-    ActiveRecord::Associations::Preloader.new.preload(projects, { namespace: :owner })
-
     no_project = {
       id: 0,
       name_with_namespace: 'No project',
diff --git a/app/finders/move_to_project_finder.rb b/app/finders/move_to_project_finder.rb
index 02592796ddc..b2a0dd7d699 100644
--- a/app/finders/move_to_project_finder.rb
+++ b/app/finders/move_to_project_finder.rb
@@ -6,26 +6,15 @@ class MoveToProjectFinder
   end
 
   def execute(from_project, search: nil, offset_id: nil)
-    projects = @user.authorized_projects
+    projects = @user.projects_where_can_admin_issues
     projects = projects.search(search) if search.present?
-    projects = skip_projects_before(projects, offset_id.to_i) if offset_id.present?
-    ProjectTeam.preload_max_member_access(projects.map(&:team))
-    projects = take_projects(projects)
-    projects.delete(from_project)
-    projects
-  end
-
-  private
+    projects = projects.where.not(id: from_project.id).order(id: :desc)
 
-  def skip_projects_before(projects, offset_project_id)
-    index = projects.index { |project| project.id == offset_project_id }
-
-    index ? projects.drop(index + 1) : projects
-  end
+    # infinite scroll using offset
+    projects = projects.where("projects.id < #{offset_id}") if offset_id.present?
+    projects = projects.limit(PAGE_SIZE)
 
-  def take_projects(projects)
-    projects.lazy.select do |project|
-      @user.can?(:admin_issue, project)
-    end.take(PAGE_SIZE).to_a
+    # to ask for Project#name_with_namespace
+    projects.preload(namespace: :owner)
   end
 end
diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index 2c630c1f626..b7c1782646f 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -5,18 +5,6 @@ class ProjectTeam
     @project = project
   end
 
-  class << self
-    def preload_max_member_access(project_teams)
-      projects = project_teams.map(&:project)
-      run_preload(projects, [:namespace, :group])
-      run_preload(projects.select(&:allowed_to_share_with_group?), [project_group_links: :group])
-    end
-
-    def run_preload(projects, associations)
-      ActiveRecord::Associations::Preloader.new.preload(projects, associations)
-    end
-  end
-
   # Shortcut to add users
   #
   # Use:
diff --git a/app/models/user.rb b/app/models/user.rb
index db747434959..18de66f6f34 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -429,6 +429,13 @@ class User < ActiveRecord::Base
                     owned_groups.select(:id), namespace.id).joins(:namespace)
   end
 
+  # Returns projects user can admin_issue on (for example to move an issue to that project).
+  #
+  # This logic is extracted from `Ability#project_abilities` into a SQL form.
+  def projects_where_can_admin_issues
+    authorized_projects(Gitlab::Access::REPORTER).non_archived.where.not(issues_enabled: false)
+  end
+
   def is_admin?
     admin
   end
diff --git a/spec/finders/move_to_project_finder_spec.rb b/spec/finders/move_to_project_finder_spec.rb
new file mode 100644
index 00000000000..67ec15ff27d
--- /dev/null
+++ b/spec/finders/move_to_project_finder_spec.rb
@@ -0,0 +1,97 @@
+require 'spec_helper'
+
+describe MoveToProjectFinder do
+  let(:user) { create(:user) }
+  let(:project) { create(:project) }
+
+  let(:no_access_project) { create(:project) }
+  let(:guess_project) { create(:project) }
+  let(:reporter_project) { create(:project) }
+  let(:developer_project) { create(:project) }
+  let(:master_project) { create(:project) }
+
+  subject { described_class.new(user) }
+
+  describe '#execute' do
+    context 'filter' do
+      it 'does not return projects under Gitlab::Access::REPORTER' do
+        guess_project.team << [user, :guest]
+
+        expect(subject.execute(project)).to be_empty
+      end
+
+      it 'returns projects equal or above Gitlab::Access::REPORTER ordered by id desc' do
+        reporter_project.team << [user, :reporter]
+        developer_project.team << [user, :developer]
+        master_project.team << [user, :master]
+
+        expect(subject.execute(project).to_a).to eq([master_project, developer_project, reporter_project])
+      end
+
+      it 'does not return the project we pass (from the project we move the issue)' do
+        project.team << [user, :reporter]
+
+        expect(subject.execute(project).to_a).to be_empty
+      end
+
+      it 'does not return archived projects' do
+        reporter_project.team << [user, :reporter]
+        reporter_project.update_attributes(archived: true)
+        other_reporter_project = create(:project)
+        other_reporter_project.team << [user, :reporter]
+
+        expect(subject.execute(project).to_a).to eq([other_reporter_project])
+      end
+
+      it 'does not return projects with issues disabled' do
+        reporter_project.team << [user, :reporter]
+        reporter_project.update_attributes(issues_enabled: false)
+        other_reporter_project = create(:project)
+        other_reporter_project.team << [user, :reporter]
+
+        expect(subject.execute(project).to_a).to eq([other_reporter_project])
+      end
+
+      it 'returns a page of projects ordered by id desc' do
+        stub_const 'MoveToProjectFinder::PAGE_SIZE', 2
+
+        reporter_project.team << [user, :reporter]
+        developer_project.team << [user, :developer]
+        master_project.team << [user, :master]
+
+        expect(subject.execute(project).to_a).to eq([master_project, developer_project])
+      end
+
+      it 'returns projects after the offset_id provided' do
+        stub_const 'MoveToProjectFinder::PAGE_SIZE', 2
+
+        reporter_project.team << [user, :reporter]
+        developer_project.team << [user, :developer]
+        master_project.team << [user, :master]
+
+        expect(subject.execute(project, search: nil, offset_id: master_project.id).to_a).to eq([developer_project, reporter_project])
+        expect(subject.execute(project, search: nil, offset_id: developer_project.id).to_a).to eq([reporter_project])
+        expect(subject.execute(project, search: nil, offset_id: reporter_project.id).to_a).to be_empty
+      end
+    end
+
+    context 'search' do
+      it 'uses Project#search' do
+        expect(user).to receive_message_chain(:can_admin_issue_projects, :search) { Project.all }
+
+        subject.execute(project, search: 'wadus')
+      end
+
+      it 'returns searched projects' do
+        foo_project = create(:project)
+        foo_project.team << [user, :master]
+
+        wadus_project = create(:project, name: 'wadus')
+        wadus_project.team << [user, :master]
+
+        expect(subject.execute(project).to_a).to eq([wadus_project, foo_project])
+        expect(subject.execute(project, search: 'wadus').to_a).to eq([wadus_project])
+      end
+    end
+  end
+end