Merge branch 'refactor-projects-finder-init-collection' into 'master'
Refactor ProjectsFinder#init_collection and GroupProjectsFinder#init_collection Closes #33632 See merge request !12135
This commit is contained in:
commit
639133fc44
|
@ -33,7 +33,8 @@ class EventsFinder
|
|||
private
|
||||
|
||||
def by_current_user_access(events)
|
||||
events.merge(ProjectsFinder.new(current_user: current_user).execute).references(:project)
|
||||
events.merge(ProjectsFinder.new(current_user: current_user).execute).
|
||||
joins(:project)
|
||||
end
|
||||
|
||||
def by_action(events)
|
||||
|
|
|
@ -29,35 +29,69 @@ class GroupProjectsFinder < ProjectsFinder
|
|||
private
|
||||
|
||||
def init_collection
|
||||
only_owned = options.fetch(:only_owned, false)
|
||||
only_shared = options.fetch(:only_shared, false)
|
||||
projects = if current_user
|
||||
collection_with_user
|
||||
else
|
||||
collection_without_user
|
||||
end
|
||||
|
||||
projects = []
|
||||
union(projects)
|
||||
end
|
||||
|
||||
if current_user
|
||||
if group.users.include?(current_user)
|
||||
projects << group.projects unless only_shared
|
||||
projects << group.shared_projects unless only_owned
|
||||
def collection_with_user
|
||||
if group.users.include?(current_user)
|
||||
if only_shared?
|
||||
[shared_projects]
|
||||
elsif only_owned?
|
||||
[owned_projects]
|
||||
else
|
||||
unless only_shared
|
||||
projects << group.projects.visible_to_user(current_user)
|
||||
projects << group.projects.public_to_user(current_user)
|
||||
end
|
||||
|
||||
unless only_owned
|
||||
projects << group.shared_projects.visible_to_user(current_user)
|
||||
projects << group.shared_projects.public_to_user(current_user)
|
||||
end
|
||||
[shared_projects, owned_projects]
|
||||
end
|
||||
else
|
||||
projects << group.projects.public_only unless only_shared
|
||||
projects << group.shared_projects.public_only unless only_owned
|
||||
if only_shared?
|
||||
[shared_projects.public_or_visible_to_user(current_user)]
|
||||
elsif only_owned?
|
||||
[owned_projects.public_or_visible_to_user(current_user)]
|
||||
else
|
||||
[
|
||||
owned_projects.public_or_visible_to_user(current_user),
|
||||
shared_projects.public_or_visible_to_user(current_user)
|
||||
]
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
projects
|
||||
def collection_without_user
|
||||
if only_shared?
|
||||
[shared_projects.public_only]
|
||||
elsif only_owned?
|
||||
[owned_projects.public_only]
|
||||
else
|
||||
[shared_projects.public_only, owned_projects.public_only]
|
||||
end
|
||||
end
|
||||
|
||||
def union(items)
|
||||
find_union(items, Project)
|
||||
if items.one?
|
||||
items.first
|
||||
else
|
||||
find_union(items, Project)
|
||||
end
|
||||
end
|
||||
|
||||
def only_owned?
|
||||
options.fetch(:only_owned, false)
|
||||
end
|
||||
|
||||
def only_shared?
|
||||
options.fetch(:only_shared, false)
|
||||
end
|
||||
|
||||
def owned_projects
|
||||
group.projects
|
||||
end
|
||||
|
||||
def shared_projects
|
||||
group.shared_projects
|
||||
end
|
||||
end
|
||||
|
|
|
@ -28,34 +28,56 @@ class ProjectsFinder < UnionFinder
|
|||
end
|
||||
|
||||
def execute
|
||||
items = init_collection
|
||||
items = items.map do |item|
|
||||
item = by_ids(item)
|
||||
item = by_personal(item)
|
||||
item = by_starred(item)
|
||||
item = by_trending(item)
|
||||
item = by_visibilty_level(item)
|
||||
item = by_tags(item)
|
||||
item = by_search(item)
|
||||
by_archived(item)
|
||||
end
|
||||
items = union(items)
|
||||
sort(items)
|
||||
collection = init_collection
|
||||
collection = by_ids(collection)
|
||||
collection = by_personal(collection)
|
||||
collection = by_starred(collection)
|
||||
collection = by_trending(collection)
|
||||
collection = by_visibilty_level(collection)
|
||||
collection = by_tags(collection)
|
||||
collection = by_search(collection)
|
||||
collection = by_archived(collection)
|
||||
|
||||
sort(collection)
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def init_collection
|
||||
projects = []
|
||||
|
||||
if params[:owned].present?
|
||||
projects << current_user.owned_projects if current_user
|
||||
if current_user
|
||||
collection_with_user
|
||||
else
|
||||
projects << current_user.authorized_projects if current_user
|
||||
projects << Project.unscoped.public_to_user(current_user) unless params[:non_public].present?
|
||||
collection_without_user
|
||||
end
|
||||
end
|
||||
|
||||
projects
|
||||
def collection_with_user
|
||||
if owned_projects?
|
||||
current_user.owned_projects
|
||||
else
|
||||
if private_only?
|
||||
current_user.authorized_projects
|
||||
else
|
||||
Project.public_or_visible_to_user(current_user)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
# Builds a collection for an anonymous user.
|
||||
def collection_without_user
|
||||
if private_only? || owned_projects?
|
||||
Project.none
|
||||
else
|
||||
Project.public_to_user
|
||||
end
|
||||
end
|
||||
|
||||
def owned_projects?
|
||||
params[:owned].present?
|
||||
end
|
||||
|
||||
def private_only?
|
||||
params[:non_public].present?
|
||||
end
|
||||
|
||||
def by_ids(items)
|
||||
|
|
|
@ -266,20 +266,49 @@ class Project < ActiveRecord::Base
|
|||
|
||||
enum auto_cancel_pending_pipelines: { disabled: 0, enabled: 1 }
|
||||
|
||||
# Returns a collection of projects that is either public or visible to the
|
||||
# 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)
|
||||
else
|
||||
public_to_user
|
||||
end
|
||||
end
|
||||
|
||||
# project features may be "disabled", "internal" or "enabled". If "internal",
|
||||
# they are only available to team members. This scope returns projects where
|
||||
# the feature is either enabled, or internal with permission for the user.
|
||||
#
|
||||
# This method uses an optimised version of `with_feature_access_level` for
|
||||
# logged in users to more efficiently get private projects with the given
|
||||
# feature.
|
||||
def self.with_feature_available_for_user(feature, user)
|
||||
return with_feature_enabled(feature) if user.try(:admin?)
|
||||
visible = [nil, ProjectFeature::ENABLED]
|
||||
|
||||
unconditional = with_feature_access_level(feature, [nil, ProjectFeature::ENABLED])
|
||||
return unconditional if user.nil?
|
||||
if user&.admin?
|
||||
with_feature_enabled(feature)
|
||||
elsif user
|
||||
column = ProjectFeature.quoted_access_level_column(feature)
|
||||
|
||||
conditional = with_feature_access_level(feature, ProjectFeature::PRIVATE)
|
||||
authorized = user.authorized_projects.merge(conditional.reorder(nil))
|
||||
authorized = user.project_authorizations.select(1).
|
||||
where('project_authorizations.project_id = projects.id')
|
||||
|
||||
union = Gitlab::SQL::Union.new([unconditional.select(:id), authorized.select(:id)])
|
||||
where(arel_table[:id].in(Arel::Nodes::SqlLiteral.new(union.to_sql)))
|
||||
with_project_feature.
|
||||
where("#{column} IN (?) OR (#{column} = ? AND EXISTS (?))",
|
||||
visible,
|
||||
ProjectFeature::PRIVATE,
|
||||
authorized)
|
||||
else
|
||||
with_feature_access_level(feature, visible)
|
||||
end
|
||||
end
|
||||
|
||||
scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') }
|
||||
|
|
|
@ -27,6 +27,13 @@ class ProjectFeature < ActiveRecord::Base
|
|||
|
||||
"#{feature}_access_level".to_sym
|
||||
end
|
||||
|
||||
def quoted_access_level_column(feature)
|
||||
attribute = connection.quote_column_name(access_level_attribute(feature))
|
||||
table = connection.quote_table_name(table_name)
|
||||
|
||||
"#{table}.#{attribute}"
|
||||
end
|
||||
end
|
||||
|
||||
# Default scopes force us to unscope here since a service may need to check
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Refactor ProjectsFinder#init_collection to produce more efficient queries for
|
||||
retrieving projects
|
||||
merge_request:
|
||||
author:
|
|
@ -13,18 +13,8 @@ module Gitlab
|
|||
scope :public_and_internal_only, -> { where(visibility_level: [PUBLIC, INTERNAL] ) }
|
||||
scope :non_public_only, -> { where.not(visibility_level: PUBLIC) }
|
||||
|
||||
scope :public_to_user, -> (user) do
|
||||
if user
|
||||
if user.admin?
|
||||
all
|
||||
elsif !user.external?
|
||||
public_and_internal_only
|
||||
else
|
||||
public_only
|
||||
end
|
||||
else
|
||||
public_only
|
||||
end
|
||||
scope :public_to_user, -> (user = nil) do
|
||||
where(visibility_level: VisibilityLevel.levels_for_user(user))
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -35,6 +25,18 @@ module Gitlab
|
|||
class << self
|
||||
delegate :values, to: :options
|
||||
|
||||
def levels_for_user(user = nil)
|
||||
return [PUBLIC] unless user
|
||||
|
||||
if user.admin?
|
||||
[PRIVATE, INTERNAL, PUBLIC]
|
||||
elsif user.external?
|
||||
[PUBLIC]
|
||||
else
|
||||
[INTERNAL, PUBLIC]
|
||||
end
|
||||
end
|
||||
|
||||
def string_values
|
||||
string_options.keys
|
||||
end
|
||||
|
|
|
@ -18,4 +18,35 @@ describe Gitlab::VisibilityLevel, lib: true do
|
|||
expect(described_class.level_value(100)).to eq(Gitlab::VisibilityLevel::PRIVATE)
|
||||
end
|
||||
end
|
||||
|
||||
describe '.levels_for_user' do
|
||||
it 'returns all levels for an admin' do
|
||||
user = double(:user, admin?: true)
|
||||
|
||||
expect(described_class.levels_for_user(user)).
|
||||
to eq([Gitlab::VisibilityLevel::PRIVATE,
|
||||
Gitlab::VisibilityLevel::INTERNAL,
|
||||
Gitlab::VisibilityLevel::PUBLIC])
|
||||
end
|
||||
|
||||
it 'returns INTERNAL and PUBLIC for internal users' do
|
||||
user = double(:user, admin?: false, external?: false)
|
||||
|
||||
expect(described_class.levels_for_user(user)).
|
||||
to eq([Gitlab::VisibilityLevel::INTERNAL,
|
||||
Gitlab::VisibilityLevel::PUBLIC])
|
||||
end
|
||||
|
||||
it 'returns PUBLIC for external users' do
|
||||
user = double(:user, admin?: false, external?: true)
|
||||
|
||||
expect(described_class.levels_for_user(user)).
|
||||
to eq([Gitlab::VisibilityLevel::PUBLIC])
|
||||
end
|
||||
|
||||
it 'returns PUBLIC when no user is given' do
|
||||
expect(described_class.levels_for_user).
|
||||
to eq([Gitlab::VisibilityLevel::PUBLIC])
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -4,6 +4,18 @@ describe ProjectFeature do
|
|||
let(:project) { create(:empty_project) }
|
||||
let(:user) { create(:user) }
|
||||
|
||||
describe '.quoted_access_level_column' do
|
||||
it 'returns the table name and quoted column name for a feature' do
|
||||
expected = if Gitlab::Database.postgresql?
|
||||
'"project_features"."issues_access_level"'
|
||||
else
|
||||
'`project_features`.`issues_access_level`'
|
||||
end
|
||||
|
||||
expect(described_class.quoted_access_level_column(:issues)).to eq(expected)
|
||||
end
|
||||
end
|
||||
|
||||
describe '#feature_available?' do
|
||||
let(:features) { %w(issues wiki builds merge_requests snippets repository) }
|
||||
|
||||
|
|
|
@ -2060,4 +2060,36 @@ describe Project, models: true do
|
|||
expect(project.last_repository_updated_at.to_i).to eq(project.created_at.to_i)
|
||||
end
|
||||
end
|
||||
|
||||
describe '.public_or_visible_to_user' do
|
||||
let!(:user) { create(:user) }
|
||||
|
||||
let!(:private_project) do
|
||||
create(:empty_project, :private, creator: user, namespace: user.namespace)
|
||||
end
|
||||
|
||||
let!(:public_project) { create(:empty_project, :public) }
|
||||
|
||||
context 'with a user' do
|
||||
let(:projects) do
|
||||
Project.all.public_or_visible_to_user(user)
|
||||
end
|
||||
|
||||
it 'includes projects the user has access to' do
|
||||
expect(projects).to include(private_project)
|
||||
end
|
||||
|
||||
it 'includes projects the user can see' do
|
||||
expect(projects).to include(public_project)
|
||||
end
|
||||
end
|
||||
|
||||
context 'without a user' do
|
||||
it 'only includes public projects' do
|
||||
projects = Project.all.public_or_visible_to_user
|
||||
|
||||
expect(projects).to eq([public_project])
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
Loading…
Reference in New Issue