From 9ed00f8690fb9b3c8f4dbe38f67a95cc2063810a Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Thu, 25 Jun 2020 13:59:10 +0000
Subject: [PATCH] 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).
---
ee/app/finders/epics_finder.rb | 9 +++++-
ee/app/helpers/ee/groups_helper.rb | 2 +-
ee/changelogs/unreleased/epics_count_all.yml | 5 ++++
ee/spec/finders/epics_finder_spec.rb | 21 +++++++++++++-
ee/spec/helpers/ee/groups_helper_spec.rb | 30 +++++++++++++++-----
5 files changed, 57 insertions(+), 10 deletions(-)
create mode 100644 ee/changelogs/unreleased/epics_count_all.yml
diff --git a/ee/app/finders/epics_finder.rb b/ee/app/finders/epics_finder.rb
index 5ba31aba93846..e5b437c364311 100644
--- a/ee/app/finders/epics_finder.rb
+++ b/ee/app/finders/epics_finder.rb
@@ -50,7 +50,9 @@ def klass
Epic
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]
return Epic.none unless Ability.allowed?(current_user, :read_epic, group)
@@ -194,6 +196,7 @@ def groups_with_confidential_access(groups)
end
def can_read_all_epics_in_related_groups?(groups)
+ return true if skip_visibility_check?
return false unless current_user
# If a user is a member of a group, he also inherits access to all subgroups,
@@ -209,4 +212,8 @@ def can_read_all_epics_in_related_groups?(groups)
parent = params.fetch(:include_ancestor_groups, false) ? groups.first.root_ancestor : group
Ability.allowed?(current_user, :read_confidential_epic, parent)
end
+
+ def skip_visibility_check?
+ @skip_visibility_check && Feature.enabled?(:skip_epic_count_visibility_check, group, default_enabled: true)
+ end
end
diff --git a/ee/app/helpers/ee/groups_helper.rb b/ee/app/helpers/ee/groups_helper.rb
index 8cf314e0c6d46..d319cfa6eb600 100644
--- a/ee/app/helpers/ee/groups_helper.rb
+++ b/ee/app/helpers/ee/groups_helper.rb
@@ -7,7 +7,7 @@ module GroupsHelper
def group_epics_count(state:)
EpicsFinder
.new(current_user, group_id: @group.id, state: state)
- .execute
+ .execute(skip_visibility_check: true)
.count
end
diff --git a/ee/changelogs/unreleased/epics_count_all.yml b/ee/changelogs/unreleased/epics_count_all.yml
new file mode 100644
index 0000000000000..121c2abe99aca
--- /dev/null
+++ b/ee/changelogs/unreleased/epics_count_all.yml
@@ -0,0 +1,5 @@
+---
+title: Show count of all epics on group page no matter if user can see them or not.
+merge_request: 35129
+author:
+type: performance
diff --git a/ee/spec/finders/epics_finder_spec.rb b/ee/spec/finders/epics_finder_spec.rb
index a8a608c386814..20bfd7a035279 100644
--- a/ee/spec/finders/epics_finder_spec.rb
+++ b/ee/spec/finders/epics_finder_spec.rb
@@ -308,13 +308,32 @@ def epics(params = {})
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_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
expect(subject).to match_array([base_epic2, public_epic1])
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
before do
ancestor_group.add_developer(search_user)
diff --git a/ee/spec/helpers/ee/groups_helper_spec.rb b/ee/spec/helpers/ee/groups_helper_spec.rb
index d0ab20f7fcc14..85468acdb04d9 100644
--- a/ee/spec/helpers/ee/groups_helper_spec.rb
+++ b/ee/spec/helpers/ee/groups_helper_spec.rb
@@ -19,17 +19,33 @@
describe '#group_epics_count' do
before do
stub_licensed_features(epics: true)
-
- 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)
+ describe 'filtering by state' do
+ 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
- it 'returns closed epics count' do
- expect(helper.group_epics_count(state: 'closed')).to eq(2)
+ it 'counts also epics from subgroups not visible to user' do
+ 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
--
GitLab