Commit 9ed00f86 authored by Jan Provaznik's avatar Jan Provaznik Committed by Sean McGivern

Ignore permission checking when counting epics

When showing epics count on group show page, we just
count all epics in the group and its subgroups no matter if
user can see them or not. This is consistent with showing
epic's child counts (where accessibility check is skipped too).
parent befe8808
...@@ -50,7 +50,9 @@ class EpicsFinder < IssuableFinder ...@@ -50,7 +50,9 @@ class EpicsFinder < IssuableFinder
Epic Epic
end end
def execute def execute(skip_visibility_check: false)
@skip_visibility_check = skip_visibility_check
raise ArgumentError, 'group_id argument is missing' unless params[:group_id] raise ArgumentError, 'group_id argument is missing' unless params[:group_id]
return Epic.none unless Ability.allowed?(current_user, :read_epic, group) return Epic.none unless Ability.allowed?(current_user, :read_epic, group)
...@@ -194,6 +196,7 @@ class EpicsFinder < IssuableFinder ...@@ -194,6 +196,7 @@ class EpicsFinder < IssuableFinder
end end
def can_read_all_epics_in_related_groups?(groups) def can_read_all_epics_in_related_groups?(groups)
return true if skip_visibility_check?
return false unless current_user return false unless current_user
# If a user is a member of a group, he also inherits access to all subgroups, # If a user is a member of a group, he also inherits access to all subgroups,
...@@ -209,4 +212,8 @@ class EpicsFinder < IssuableFinder ...@@ -209,4 +212,8 @@ class EpicsFinder < IssuableFinder
parent = params.fetch(:include_ancestor_groups, false) ? groups.first.root_ancestor : group parent = params.fetch(:include_ancestor_groups, false) ? groups.first.root_ancestor : group
Ability.allowed?(current_user, :read_confidential_epic, parent) Ability.allowed?(current_user, :read_confidential_epic, parent)
end end
def skip_visibility_check?
@skip_visibility_check && Feature.enabled?(:skip_epic_count_visibility_check, group, default_enabled: true)
end
end end
...@@ -7,7 +7,7 @@ module EE ...@@ -7,7 +7,7 @@ module EE
def group_epics_count(state:) def group_epics_count(state:)
EpicsFinder EpicsFinder
.new(current_user, group_id: @group.id, state: state) .new(current_user, group_id: @group.id, state: state)
.execute .execute(skip_visibility_check: true)
.count .count
end end
......
---
title: Show count of all epics on group page no matter if user can see them or not.
merge_request: 35129
author:
type: performance
...@@ -308,13 +308,32 @@ RSpec.describe EpicsFinder do ...@@ -308,13 +308,32 @@ RSpec.describe EpicsFinder do
let_it_be(:public_group1) { create(:group, :public, parent: base_group) } let_it_be(:public_group1) { create(:group, :public, parent: base_group) }
let_it_be(:public_epic1) { create(:epic, group: public_group1) } let_it_be(:public_epic1) { create(:epic, group: public_group1) }
let_it_be(:public_epic2) { create(:epic, :confidential, group: public_group1) } let_it_be(:public_epic2) { create(:epic, :confidential, group: public_group1) }
let(:execute_params) { {} }
subject { described_class.new(search_user, group_id: base_group.id).execute } subject { described_class.new(search_user, group_id: base_group.id).execute(execute_params) }
it 'returns only public epics' do it 'returns only public epics' do
expect(subject).to match_array([base_epic2, public_epic1]) expect(subject).to match_array([base_epic2, public_epic1])
end end
context 'when skip_visibility_check is true' do
let(:execute_params) { { skip_visibility_check: true } }
it 'returns all epics' do
expect(subject).to match_array([base_epic1, base_epic2, private_epic1, private_epic2, public_epic1, public_epic2])
end
context 'when skip_epic_count_visibility_check is disabled' do
before do
stub_feature_flags(skip_epic_count_visibility_check: false)
end
it 'returns only public epics' do
expect(subject).to match_array([base_epic2, public_epic1])
end
end
end
context 'when user is member of ancestor group' do context 'when user is member of ancestor group' do
before do before do
ancestor_group.add_developer(search_user) ancestor_group.add_developer(search_user)
......
...@@ -19,17 +19,33 @@ RSpec.describe GroupsHelper do ...@@ -19,17 +19,33 @@ RSpec.describe GroupsHelper do
describe '#group_epics_count' do describe '#group_epics_count' do
before do before do
stub_licensed_features(epics: true) stub_licensed_features(epics: true)
create_list(:epic, 3, :opened, group: group)
create_list(:epic, 2, :closed, group: group)
end end
it 'returns open epics count' do describe 'filtering by state' do
expect(helper.group_epics_count(state: 'opened')).to eq(3) before do
create_list(:epic, 3, :opened, group: group)
create_list(:epic, 2, :closed, group: group)
end
it 'returns open epics count' do
expect(helper.group_epics_count(state: 'opened')).to eq(3)
end
it 'returns closed epics count' do
expect(helper.group_epics_count(state: 'closed')).to eq(2)
end
end end
it 'returns closed epics count' do it 'counts also epics from subgroups not visible to user' do
expect(helper.group_epics_count(state: 'closed')).to eq(2) parent_group = create(:group, :public)
subgroup = create(:group, :private, parent: parent_group)
create(:epic, :opened, group: parent_group)
create(:epic, :opened, group: subgroup)
helper.instance_variable_set(:@group, parent_group)
expect(Ability.allowed?(owner, :read_epic, parent_group)).to be_truthy
expect(Ability.allowed?(owner, :read_epic, subgroup)).to be_falsey
expect(helper.group_epics_count(state: 'opened')).to eq(2)
end 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