Commit c4c0f6a0 authored by Allen Cook's avatar Allen Cook Committed by Shinya Maeda

Resolve "Resolve minor technical debts of `GroupReleasesFinder`"

parent 17e84184
......@@ -6,9 +6,8 @@ module Releases
#
# order_by - only ordering by released_at is supported
# filter by tag - currently not supported
# include_subgroups - always true for group releases finder
class GroupReleasesFinder
include Gitlab::Utils::StrongMemoize
attr_reader :parent, :current_user, :params
def initialize(parent, current_user = nil, params = {})
......@@ -25,45 +24,26 @@ module Releases
def execute(preload: true)
return Release.none unless Ability.allowed?(current_user, :read_release, parent)
releases = get_releases(preload: preload)
releases = get_releases
releases.preloaded if preload
paginate_releases(releases)
end
private
def include_subgroups?
params.fetch(:include_subgroups, false)
end
def accessible_projects_scope
if include_subgroups?
Project.for_group_and_its_subgroups(parent)
else
parent.projects
end
end
# rubocop: disable CodeReuse/ActiveRecord
def get_releases(preload: true)
def get_releases
Gitlab::Pagination::Keyset::InOperatorOptimization::QueryBuilder.new(
scope: releases_scope(preload: preload),
array_scope: accessible_projects_scope.select(:id),
scope: releases_scope,
array_scope: Project.for_group_and_its_subgroups(parent).select(:id),
array_mapping_scope: -> (project_id_expression) { Release.where(Release.arel_table[:project_id].eq(project_id_expression)) },
finder_query: -> (order_by, id_expression) { Release.where(Release.arel_table[:id].eq(id_expression)) }
)
.execute
end
def releases_scope(preload: true)
scope = Release.all
scope = order_releases(scope)
scope = scope.preloaded if preload
scope
end
def order_releases(scope)
scope.sort_by_attribute("released_at_#{params[:sort]}").order(id: params[:sort])
def releases_scope
Release.sort_by_attribute("released_at_#{params[:sort]}").order(id: params[:sort])
end
def paginate_releases(releases)
......
......@@ -95,8 +95,6 @@ RSpec.describe Releases::GroupReleasesFinder do
end
describe 'with subgroups' do
let(:params) { { include_subgroups: true } }
subject(:releases) { described_class.new(group, user, params).execute(**args) }
context 'with a single-level subgroup' do
......@@ -164,22 +162,12 @@ RSpec.describe Releases::GroupReleasesFinder do
end
end
context 'when the user a guest on the group' do
before do
group.add_guest(user)
end
it 'returns all releases' do
expect(releases).to match_array([v1_1_1, v1_1_0, v6, v1_0_0, p3])
end
end
context 'performance testing' do
shared_examples 'avoids N+1 queries' do |query_params = {}|
context 'with subgroups' do
let(:params) { query_params }
it 'include_subgroups avoids N+1 queries' do
it 'subgroups avoids N+1 queries' do
control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
releases
end.count
......@@ -196,7 +184,6 @@ RSpec.describe Releases::GroupReleasesFinder do
end
it_behaves_like 'avoids N+1 queries'
it_behaves_like 'avoids N+1 queries', { simple: true }
end
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment