diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2016-08-17 10:55:47 +0300 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2016-08-17 10:55:47 +0300 |
commit | d1da2e8180d92e5f4a8b5ebb36b0f4e4d0618bf8 (patch) | |
tree | 24913e88546ebfce79f66a84a28e0a2f536776c0 | |
parent | 1b338d59f641ce629cbecb839b64c9fd65561276 (diff) | |
parent | 03386633a42bd56b0b0b31b70eebaaaa33e1494e (diff) |
Merge branch '17932-move-to-project-dropdown-improve' into 'master'
Move to project dropdown with infinite scroll for better performance
See merge request !5828
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/controllers/autocomplete_controller.rb | 14 | ||||
-rw-r--r-- | app/finders/move_to_project_finder.rb | 14 | ||||
-rw-r--r-- | app/models/project.rb | 2 | ||||
-rw-r--r-- | app/models/user.rb | 7 | ||||
-rw-r--r-- | spec/controllers/autocomplete_controller_spec.rb | 292 | ||||
-rw-r--r-- | spec/finders/move_to_project_finder_spec.rb | 75 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 47 |
8 files changed, 340 insertions, 112 deletions
diff --git a/CHANGELOG b/CHANGELOG index e9c8a4895e1..837e9e27aba 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -97,6 +97,7 @@ v 8.11.0 (unreleased) - 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 diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index e1641ba6265..b48668eea87 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -35,19 +35,13 @@ class AutocompleteController < ApplicationController 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', } - 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 @@ class AutocompleteController < ApplicationController end end end + + def projects_finder + MoveToProjectFinder.new(current_user) + end end diff --git a/app/finders/move_to_project_finder.rb b/app/finders/move_to_project_finder.rb new file mode 100644 index 00000000000..3334b8556df --- /dev/null +++ b/app/finders/move_to_project_finder.rb @@ -0,0 +1,14 @@ +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 diff --git a/app/models/project.rb b/app/models/project.rb index e0b28160937..eefdae35615 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -197,6 +197,8 @@ class Project < ActiveRecord::Base 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 diff --git a/app/models/user.rb b/app/models/user.rb index 87a2d999843..48e83ab7e56 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 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 diff --git a/spec/controllers/autocomplete_controller_spec.rb b/spec/controllers/autocomplete_controller_spec.rb index ed0b7f9e240..44128a43362 100644 --- a/spec/controllers/autocomplete_controller_spec.rb +++ b/spec/controllers/autocomplete_controller_spec.rb @@ -2,178 +2,262 @@ require 'spec_helper' 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 'project members' do - before do - sign_in(user) - project.team << [user, :master] - end + context 'users and members' do + let!(:user2) { create(:user) } + let!(:non_member) { create(:user) } - describe 'GET #users with project ID' do + context 'project members' do before do - get(:users, project_id: project.id) + sign_in(user) + 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) + end - 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 + let(:body) { JSON.parse(response.body) } - describe 'GET #users with unknown project' do - before do - get(:users, project_id: 'unknown') + 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 - it { expect(response).to have_http_status(404) } - end - end - - context 'group members' do - let(:group) { create(:group) } + describe 'GET #users with unknown project' do + before do + get(:users, project_id: 'unknown') + end - before do - sign_in(user) - group.add_owner(user) + it { expect(response).to have_http_status(404) } + end end - let(:body) { JSON.parse(response.body) } + context 'group members' do + let(:group) { create(:group) } - describe 'GET #users with group ID' do before do - get(:users, group_id: group.id) + sign_in(user) + group.add_owner(user) 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 } + 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 - describe 'GET #users with unknown group ID' do + context 'non-member login for public project' do + let!(:project) { create(:project, :public) } + before do - get(:users, group_id: 'unknown') + sign_in(non_member) + project.team << [user, :master] end - it { expect(response).to have_http_status(404) } - end - end + let(:body) { JSON.parse(response.body) } - context 'non-member login for public project' do - let!(:project) { create(:project, :public) } + describe 'GET #users with project ID' do + before do + get(:users, project_id: project.id, current_user: true) + end - before do - sign_in(non_member) - project.team << [user, :master] + 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 - let(:body) { JSON.parse(response.body) } - - describe 'GET #users with project ID' do + context 'all users' do before do - get(:users, project_id: project.id, current_user: true) + sign_in(user) + get(:users) end + let(:body) { JSON.parse(response.body) } + 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]) } + it { expect(body.size).to eq User.count } end - end - context 'all users' do - before do - sign_in(user) - get(:users) - end + context 'unauthenticated user' do + let(:public_project) { create(:project, :public) } + let(:body) { JSON.parse(response.body) } - 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 User.count } - end + it { expect(body).to be_kind_of(Array) } + it { expect(body.size).to eq 1 } + end - context 'unauthenticated user' do - let(:public_project) { create(:project, :public) } - let(:body) { JSON.parse(response.body) } + describe 'GET #users with project' do + before do + get(:users, project_id: project.id) + end - describe 'GET #users with public project' do - before do - public_project.team << [user, :guest] - get(:users, project_id: public_project.id) + it { expect(response).to have_http_status(404) } end - it { expect(body).to be_kind_of(Array) } - it { expect(body.size).to eq 1 } - end + describe 'GET #users with unknown project' do + before do + get(:users, project_id: 'unknown') + end - describe 'GET #users with project' do - before do - get(:users, project_id: project.id) + it { expect(response).to have_http_status(404) } end - it { expect(response).to have_http_status(404) } - end + describe 'GET #users with inaccessible group' do + before do + project.team << [user, :guest] + get(:users, group_id: user.namespace.id) + end - describe 'GET #users with unknown project' do - before do - get(:users, project_id: 'unknown') + it { expect(response).to have_http_status(404) } end - it { expect(response).to have_http_status(404) } + 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 - describe 'GET #users with inaccessible group' do + context 'author of issuable included' do before do - project.team << [user, :guest] - get(:users, group_id: user.namespace.id) + sign_in(user) end - it { expect(response).to have_http_status(404) } - end + let(:body) { JSON.parse(response.body) } - describe 'GET #users with no project' do - before do - get(:users) + it 'includes the author' do + get(:users, author_id: non_member.id) + + expect(body.first["username"]).to eq non_member.username end - it { expect(body).to be_kind_of(Array) } - it { expect(body.size).to eq 0 } + it 'rejects non existent user ids' do + get(:users, author_id: 99999) + + expect(body.collect { |u| u['id'] }).not_to include(99999) + end + end + + 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)) + + 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 end end - context 'author of issuable included' do + context 'projects' do + let(:authorized_project) { create(:project) } + let(:authorized_search_project) { create(:project, name: 'rugged') } + before do sign_in(user) + project.team << [user, :master] end - let(:body) { JSON.parse(response.body) } + context 'authorized projects' do + before do + authorized_project.team << [user, :master] + end + + describe 'GET #projects with project ID' do + before do + get(:projects, project_id: project.id) + end + + let(:body) { JSON.parse(response.body) } + + it do + expect(body).to be_kind_of(Array) + expect(body.size).to eq 2 - it 'includes the author' do - get(:users, author_id: non_member.id) + expect(body.first['id']).to eq 0 + expect(body.first['name_with_namespace']).to eq 'No project' - expect(body.first["username"]).to eq non_member.username + expect(body.last['id']).to eq authorized_project.id + expect(body.last['name_with_namespace']).to eq authorized_project.name_with_namespace + end + end end - it 'rejects non existent user ids' do - get(:users, author_id: 99999) + context 'authorized projects and search' do + before do + authorized_project.team << [user, :master] + authorized_search_project.team << [user, :master] + end + + describe 'GET #projects with project ID and search' do + before do + get(:projects, project_id: project.id, search: 'rugged') + end + + let(:body) { JSON.parse(response.body) } - expect(body.collect { |u| u['id'] }).not_to include(99999) + it do + expect(body).to be_kind_of(Array) + expect(body.size).to eq 2 + + 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 - end - context 'skip_users parameter included' do - before { sign_in(user) } + context 'authorized projects without admin_issue ability' do + before(:each) do + authorized_project.team << [user, :guest] + + expect(user.can?(:admin_issue, authorized_project)).to eq(false) + end - it 'skips the user IDs passed' do - get(:users, skip_users: [user, user2].map(&:id)) + describe 'GET #projects with project ID' do + before do + get(:projects, project_id: project.id) + end - other_user_ids = [non_member, project.owner, project.creator].map(&:id) - response_user_ids = JSON.parse(response.body).map { |user| user['id'] } + let(:body) { JSON.parse(response.body) } - expect(response_user_ids).to contain_exactly(*other_user_ids) + it do + expect(body).to be_kind_of(Array) + expect(body.size).to eq 1 # 'No project' + + expect(body.first['id']).to eq 0 + end + end end end 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..4f3304f7b6d --- /dev/null +++ b/spec/finders/move_to_project_finder_spec.rb @@ -0,0 +1,75 @@ +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] + + 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 54505f6b822..51e4780e2b1 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -957,6 +957,53 @@ describe User, models: true do end end + describe '#projects_where_can_admin_issues' do + let(:user) { create(:user) } + + it 'includes projects for which the user access level is above or equal to reporter' do + create(:project) + reporter_project = create(:project) + developer_project = create(:project) + master_project = create(:project) + + reporter_project.team << [user, :reporter] + developer_project.team << [user, :developer] + master_project.team << [user, :master] + + expect(user.projects_where_can_admin_issues.to_a).to eq([master_project, developer_project, reporter_project]) + expect(user.can?(:admin_issue, master_project)).to eq(true) + expect(user.can?(:admin_issue, developer_project)).to eq(true) + expect(user.can?(:admin_issue, reporter_project)).to eq(true) + end + + it 'does not include for which the user access level is below reporter' do + project = create(:project) + guest_project = create(:project) + + guest_project.team << [user, :guest] + + expect(user.projects_where_can_admin_issues.to_a).to be_empty + expect(user.can?(:admin_issue, guest_project)).to eq(false) + expect(user.can?(:admin_issue, project)).to eq(false) + end + + it 'does not include archived projects' do + project = create(:project) + project.update_attributes(archived: true) + + expect(user.projects_where_can_admin_issues.to_a).to be_empty + expect(user.can?(:admin_issue, project)).to eq(false) + end + + it 'does not include projects for which issues are disabled' do + project = create(:project) + project.update_attributes(issues_enabled: false) + + expect(user.projects_where_can_admin_issues.to_a).to be_empty + expect(user.can?(:admin_issue, project)).to eq(false) + end + end + describe '#ci_authorized_runners' do let(:user) { create(:user) } let(:runner) { create(:ci_runner) } |