Commit 7c2879b1 authored by Paco Guzman's avatar Paco Guzman
Browse files

Move to project dropdown with infinite scroll for better performance

Use just SQL to check is a user can admin_issue on a project

Tradeoff

- we duplicate how we check admin_issue in a SQL relation in the Ability class
parent a7c62ece3747
......@@ -97,6 +97,7 @@
- Add commit stats in commit api. !5517 (dixpac)
- Add CI configuration button on project page
- Make error pages responsive (Takuya Noguchi)
- The performance of the project dropdown used for moving issues has been improved
- Fix skip_repo parameter being ignored when destroying a namespace
- Change requests_profiles resource constraint to catch virtually any file
- Bump gitlab_git to lazy load compare commits
......
......@@ -35,14 +35,9 @@
def projects
project = Project.find_by_id(params[:project_id])
projects = current_user.authorized_projects
projects = projects.search(params[:search]) if params[:search].present?
projects = projects.select do |project|
current_user.can?(:admin_issue, project)
end
projects = projects_finder.execute(project, search: params[:search], offset_id: params[:offset_id])
no_project = {
id: 0,
name_with_namespace: 'No project',
}
......@@ -44,10 +39,9 @@
no_project = {
id: 0,
name_with_namespace: 'No project',
}
projects.unshift(no_project)
projects.delete(project)
projects.unshift(no_project) unless params[:offset_id].present?
render json: projects.to_json(only: [:id, :name_with_namespace], methods: :name_with_namespace)
end
......@@ -79,4 +73,8 @@
end
end
end
def projects_finder
MoveToProjectFinder.new(current_user)
end
end
class MoveToProjectFinder
def initialize(user)
@user = user
end
def execute(from_project, search: nil, offset_id: nil)
projects = @user.projects_where_can_admin_issues
projects = projects.search(search) if search.present?
projects = projects.excluding_project(from_project)
# to ask for Project#name_with_namespace
projects.includes(namespace: :owner)
end
end
......@@ -197,6 +197,8 @@
scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') }
scope :abandoned, -> { where('projects.last_activity_at < ?', 6.months.ago) }
scope :excluding_project, ->(project) { where.not(id: project) }
state_machine :import_status, initial: :none do
event :import_start do
transition [:none, :finished] => :started
......
......@@ -429,6 +429,13 @@
owned_groups.select(:id), namespace.id).joins(:namespace)
end
# Returns projects which user can admin issues on (for example to move an issue to that project).
#
# This logic is duplicated 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
......
......@@ -2,7 +2,15 @@
describe AutocompleteController do
let!(:project) { create(:project) }
let!(:user) { create(:user) }
let!(:user2) { create(:user) }
let!(:non_member) { create(:user) }
let!(:user) { create(:user) }
context 'users and members' do
let!(:user2) { create(:user) }
let!(:non_member) { create(:user) }
context 'project members' do
before do
sign_in(user)
project.team << [user, :master]
end
......@@ -8,7 +16,22 @@
context 'project members' do
before do
sign_in(user)
project.team << [user, :master]
describe 'GET #users with project ID' do
before do
get(:users, project_id: project.id)
end
let(:body) { JSON.parse(response.body) }
it { expect(body).to be_kind_of(Array) }
it { expect(body.size).to eq 1 }
it { expect(body.map { |u| u["username"] }).to include(user.username) }
end
describe 'GET #users with unknown project' do
before do
get(:users, project_id: 'unknown')
end
it { expect(response).to have_http_status(404) }
end
end
......@@ -13,4 +36,6 @@
end
describe 'GET #users with project ID' do
context 'group members' do
let(:group) { create(:group) }
before do
......@@ -16,7 +41,56 @@
before do
get(:users, project_id: project.id)
sign_in(user)
group.add_owner(user)
end
let(:body) { JSON.parse(response.body) }
describe 'GET #users with group ID' do
before do
get(:users, group_id: group.id)
end
it { expect(body).to be_kind_of(Array) }
it { expect(body.size).to eq 1 }
it { expect(body.first["username"]).to eq user.username }
end
describe 'GET #users with unknown group ID' do
before do
get(:users, group_id: 'unknown')
end
it { expect(response).to have_http_status(404) }
end
end
context 'non-member login for public project' do
let!(:project) { create(:project, :public) }
before do
sign_in(non_member)
project.team << [user, :master]
end
let(:body) { JSON.parse(response.body) }
describe 'GET #users with project ID' do
before do
get(:users, project_id: project.id, current_user: true)
end
it { expect(body).to be_kind_of(Array) }
it { expect(body.size).to eq 2 }
it { expect(body.map { |u| u['username'] }).to match_array([user.username, non_member.username]) }
end
end
context 'all users' do
before do
sign_in(user)
get(:users)
end
let(:body) { JSON.parse(response.body) }
it { expect(body).to be_kind_of(Array) }
......@@ -18,9 +92,8 @@
end
let(:body) { JSON.parse(response.body) }
it { expect(body).to be_kind_of(Array) }
it { expect(body.size).to eq 1 }
it { expect(body.map { |u| u["username"] }).to include(user.username) }
it { expect(body.size).to eq User.count }
end
......@@ -25,7 +98,16 @@
end
describe 'GET #users with unknown project' do
before do
get(:users, project_id: 'unknown')
context 'unauthenticated user' do
let(:public_project) { create(:project, :public) }
let(:body) { JSON.parse(response.body) }
describe 'GET #users with public project' do
before do
public_project.team << [user, :guest]
get(:users, project_id: public_project.id)
end
it { expect(body).to be_kind_of(Array) }
it { expect(body.size).to eq 1 }
end
......@@ -30,6 +112,7 @@
end
it { expect(response).to have_http_status(404) }
end
end
describe 'GET #users with project' do
before do
get(:users, project_id: project.id)
end
......@@ -35,4 +118,4 @@
context 'group members' do
let(:group) { create(:group) }
it { expect(response).to have_http_status(404) }
end
......@@ -38,6 +121,9 @@
before do
sign_in(user)
group.add_owner(user)
end
describe 'GET #users with unknown project' do
before do
get(:users, project_id: 'unknown')
end
it { expect(response).to have_http_status(404) }
end
......@@ -43,3 +129,7 @@
let(:body) { JSON.parse(response.body) }
describe 'GET #users with inaccessible group' do
before do
project.team << [user, :guest]
get(:users, group_id: user.namespace.id)
end
......@@ -45,6 +135,4 @@
describe 'GET #users with group ID' do
before do
get(:users, group_id: group.id)
it { expect(response).to have_http_status(404) }
end
......@@ -49,7 +137,12 @@
end
it { expect(body).to be_kind_of(Array) }
it { expect(body.size).to eq 1 }
it { expect(body.first["username"]).to eq user.username }
describe 'GET #users with no project' do
before do
get(:users)
end
it { expect(body).to be_kind_of(Array) }
it { expect(body.size).to eq 0 }
end
end
......@@ -54,4 +147,4 @@
end
describe 'GET #users with unknown group ID' do
context 'author of issuable included' do
before do
......@@ -57,4 +150,4 @@
before do
get(:users, group_id: 'unknown')
sign_in(user)
end
......@@ -59,6 +152,7 @@
end
it { expect(response).to have_http_status(404) }
end
end
let(:body) { JSON.parse(response.body) }
it 'includes the author' do
get(:users, author_id: non_member.id)
......@@ -64,4 +158,4 @@
context 'non-member login for public project' do
let!(:project) { create(:project, :public) }
expect(body.first["username"]).to eq non_member.username
end
......@@ -67,6 +161,8 @@
before do
sign_in(non_member)
project.team << [user, :master]
it 'rejects non existent user ids' do
get(:users, author_id: 99999)
expect(body.collect { |u| u['id'] }).not_to include(99999)
end
end
......@@ -71,4 +167,8 @@
end
let(:body) { JSON.parse(response.body) }
context 'skip_users parameter included' do
before { sign_in(user) }
it 'skips the user IDs passed' do
get(:users, skip_users: [user, user2].map(&:id))
......@@ -74,5 +174,6 @@
describe 'GET #users with project ID' do
before do
get(:users, project_id: project.id, current_user: true)
other_user_ids = [non_member, project.owner, project.creator].map(&:id)
response_user_ids = JSON.parse(response.body).map { |user| user['id'] }
expect(response_user_ids).to contain_exactly(*other_user_ids)
end
......@@ -78,8 +179,4 @@
end
it { expect(body).to be_kind_of(Array) }
it { expect(body.size).to eq 2 }
it { expect(body.map { |u| u['username'] }).to match_array([user.username, non_member.username]) }
end
end
......@@ -83,6 +180,9 @@
end
end
context 'all users' do
context 'projects' do
let(:authorized_project) { create(:project) }
let(:authorized_search_project) { create(:project, name: 'rugged') }
before do
sign_in(user)
......@@ -87,5 +187,5 @@
before do
sign_in(user)
get(:users)
project.team << [user, :master]
end
......@@ -90,14 +190,4 @@
end
let(:body) { JSON.parse(response.body) }
it { expect(body).to be_kind_of(Array) }
it { expect(body.size).to eq User.count }
end
context 'unauthenticated user' do
let(:public_project) { create(:project, :public) }
let(:body) { JSON.parse(response.body) }
describe 'GET #users with public project' do
context 'authorized projects' do
before do
......@@ -103,5 +193,4 @@
before do
public_project.team << [user, :guest]
get(:users, project_id: public_project.id)
authorized_project.team << [user, :master]
end
......@@ -106,6 +195,7 @@
end
it { expect(body).to be_kind_of(Array) }
it { expect(body.size).to eq 1 }
end
describe 'GET #projects with project ID' do
before do
get(:projects, project_id: project.id)
end
......@@ -111,6 +201,3 @@
describe 'GET #users with project' do
before do
get(:users, project_id: project.id)
end
let(:body) { JSON.parse(response.body) }
......@@ -116,4 +203,5 @@
it { expect(response).to have_http_status(404) }
end
it do
expect(body).to be_kind_of(Array)
expect(body.size).to eq 2
......@@ -119,5 +207,8 @@
describe 'GET #users with unknown project' do
before do
get(:users, project_id: 'unknown')
expect(body.first['id']).to eq 0
expect(body.first['name_with_namespace']).to eq 'No project'
expect(body.last['id']).to eq authorized_project.id
expect(body.last['name_with_namespace']).to eq authorized_project.name_with_namespace
end
end
......@@ -123,5 +214,3 @@
end
it { expect(response).to have_http_status(404) }
end
......@@ -126,4 +215,4 @@
end
describe 'GET #users with inaccessible group' do
context 'authorized projects and search' do
before do
......@@ -129,5 +218,5 @@
before do
project.team << [user, :guest]
get(:users, group_id: user.namespace.id)
authorized_project.team << [user, :master]
authorized_search_project.team << [user, :master]
end
......@@ -132,5 +221,7 @@
end
it { expect(response).to have_http_status(404) }
end
describe 'GET #projects with project ID and search' do
before do
get(:projects, project_id: project.id, search: 'rugged')
end
......@@ -136,6 +227,3 @@
describe 'GET #users with no project' do
before do
get(:users)
end
let(:body) { JSON.parse(response.body) }
......@@ -141,6 +229,5 @@
it { expect(body).to be_kind_of(Array) }
it { expect(body.size).to eq 0 }
end
end
it do
expect(body).to be_kind_of(Array)
expect(body.size).to eq 2
......@@ -146,6 +233,7 @@
context 'author of issuable included' do
before do
sign_in(user)
expect(body.last['id']).to eq authorized_search_project.id
expect(body.last['name_with_namespace']).to eq authorized_search_project.name_with_namespace
end
end
end
......@@ -150,7 +238,6 @@
end
let(:body) { JSON.parse(response.body) }
it 'includes the author' do
get(:users, author_id: non_member.id)
context 'authorized projects without admin_issue ability' do
before(:each) do
authorized_project.team << [user, :guest]
......@@ -156,7 +243,4 @@
expect(body.first["username"]).to eq non_member.username
end
it 'rejects non existent user ids' do
get(:users, author_id: 99999)
expect(user.can?(:admin_issue, authorized_project)).to eq(false)
end
......@@ -162,5 +246,6 @@
expect(body.collect { |u| u['id'] }).not_to include(99999)
end
end
describe 'GET #projects with project ID' do
before do
get(:projects, project_id: project.id)
end
......@@ -166,4 +251,3 @@
context 'skip_users parameter included' do
before { sign_in(user) }
let(:body) { JSON.parse(response.body) }
......@@ -169,4 +253,5 @@
it 'skips the user IDs passed' do
get(:users, skip_users: [user, user2].map(&:id))
it do
expect(body).to be_kind_of(Array)
expect(body.size).to eq 1 # 'No project'
......@@ -172,8 +257,7 @@
other_user_ids = [non_member, project.owner, project.creator].map(&:id)
response_user_ids = JSON.parse(response.body).map { |user| user['id'] }
expect(response_user_ids).to contain_exactly(*other_user_ids)
expect(body.first['id']).to eq 0
end
end
end
end
end
require 'spec_helper'
describe MoveToProjectFinder do
let(:user) { create(:user) }
let(:project) { create(:project) }
let(:no_access_project) { create(:project) }
let(:guest_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
guest_project.team << [user, :guest]
expect(subject.execute(project)).to be_empty
end
it 'returns projects equal or above Gitlab::Access::REPORTER ordered by id in descending order' 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 include the source project' 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 for which issues are 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
end
context 'search' do
it 'uses Project#search' do
expect(user).to receive_message_chain(:projects_where_can_admin_issues, :search) { Project.all }
subject.execute(project, search: 'wadus')
end
it 'returns projects matching a search query' do
foo_project = create(:project)
foo_project.team << [user, :master]
wadus_project = create(:project, name: 'wadus')
wadus_project.team << [user, :master]