Commit 73f3d786 authored by Mayra Cabrera's avatar Mayra Cabrera

Merge branch 'fix-open-issues-count' into 'master'

Sidebar open issues count should not include hidden issues for non-admins

See merge request gitlab-org/gitlab!67379
parents b67689b5 3bad2a3f
......@@ -20,6 +20,7 @@
# sort: string
# my_reaction_emoji: string
# public_only: boolean
# include_hidden: boolean
# due_date: date or '0', '', 'overdue', 'week', or 'month'
# created_after: datetime
# created_before: datetime
......@@ -47,8 +48,6 @@ class IssuesFinder < IssuableFinder
# rubocop: disable CodeReuse/ActiveRecord
def with_confidentiality_access_check
return Issue.all if params.user_can_see_all_issues?
# Only admins can see hidden issues, so for non-admins, we filter out any hidden issues
issues = Issue.without_hidden
......@@ -76,7 +75,9 @@ class IssuesFinder < IssuableFinder
private
def init_collection
if params.public_only?
if params.include_hidden?
Issue.all
elsif params.public_only?
Issue.public_only
else
with_confidentiality_access_check
......
......@@ -6,6 +6,10 @@ class IssuesFinder
params.fetch(:public_only, false)
end
def include_hidden?
user_can_see_all_issues?
end
def filter_by_no_due_date?
due_date? && params[:due_date] == Issue::NoDueDate.name
end
......
......@@ -128,13 +128,15 @@ class Issue < ApplicationRecord
}
scope :with_issue_type, ->(types) { where(issue_type: types) }
scope :public_only, -> { where(confidential: false) }
scope :public_only, -> {
without_hidden.where(confidential: false)
}
scope :confidential_only, -> { where(confidential: true) }
scope :without_hidden, -> {
if Feature.enabled?(:ban_user_feature_flag)
where(id: joins('LEFT JOIN banned_users ON banned_users.user_id = issues.author_id WHERE banned_users.user_id IS NULL')
.select('issues.id'))
where('NOT EXISTS (?)', Users::BannedUser.select(1).where('issues.author_id = banned_users.user_id'))
else
all
end
......
......@@ -3,11 +3,15 @@
module Groups
# Service class for counting and caching the number of open issues of a group.
class OpenIssuesCountService < Groups::CountService
PUBLIC_COUNT_KEY = 'group_public_open_issues_count'
TOTAL_COUNT_KEY = 'group_total_open_issues_count'
# TOTAL_COUNT_KEY includes confidential and hidden issues (admin)
# TOTAL_COUNT_WITHOUT_HIDDEN_KEY includes confidential issues but not hidden issues (reporter and above)
# PUBLIC_COUNT_WITHOUT_HIDDEN_KEY does not include confidential or hidden issues (guest)
TOTAL_COUNT_KEY = 'group_open_issues_including_hidden_count'
TOTAL_COUNT_WITHOUT_HIDDEN_KEY = 'group_open_issues_without_hidden_count'
PUBLIC_COUNT_WITHOUT_HIDDEN_KEY = 'group_open_public_issues_without_hidden_count'
def clear_all_cache_keys
[cache_key(PUBLIC_COUNT_KEY), cache_key(TOTAL_COUNT_KEY)].each do |key|
[cache_key(TOTAL_COUNT_KEY), cache_key(TOTAL_COUNT_WITHOUT_HIDDEN_KEY), cache_key(PUBLIC_COUNT_WITHOUT_HIDDEN_KEY)].each do |key|
Rails.cache.delete(key)
end
end
......@@ -15,7 +19,19 @@ module Groups
private
def cache_key_name
public_only? ? PUBLIC_COUNT_KEY : TOTAL_COUNT_KEY
if include_hidden?
TOTAL_COUNT_KEY
elsif public_only?
PUBLIC_COUNT_WITHOUT_HIDDEN_KEY
else
TOTAL_COUNT_WITHOUT_HIDDEN_KEY
end
end
def include_hidden?
strong_memoize(:user_is_admin) do
user&.can_admin_all_resources?
end
end
def public_only?
......@@ -35,7 +51,8 @@ module Groups
state: 'opened',
non_archived: true,
include_subgroups: true,
public_only: public_only?
public_only: public_only?,
include_hidden: include_hidden?
).execute
end
......
......@@ -7,8 +7,12 @@ module Projects
include Gitlab::Utils::StrongMemoize
# Cache keys used to store issues count
PUBLIC_COUNT_KEY = 'public_open_issues_count'
TOTAL_COUNT_KEY = 'total_open_issues_count'
# TOTAL_COUNT_KEY includes confidential and hidden issues (admin)
# TOTAL_COUNT_WITHOUT_HIDDEN_KEY includes confidential issues but not hidden issues (reporter and above)
# PUBLIC_COUNT_WITHOUT_HIDDEN_KEY does not include confidential or hidden issues (guest)
TOTAL_COUNT_KEY = 'project_open_issues_including_hidden_count'
TOTAL_COUNT_WITHOUT_HIDDEN_KEY = 'project_open_issues_without_hidden_count'
PUBLIC_COUNT_WITHOUT_HIDDEN_KEY = 'project_open_public_issues_without_hidden_count'
def initialize(project, user = nil)
@user = user
......@@ -16,16 +20,53 @@ module Projects
super(project)
end
# rubocop: disable CodeReuse/ActiveRecord
def refresh_cache(&block)
if block_given?
super(&block)
else
update_cache_for_key(total_count_cache_key) do
issues_with_hidden
end
update_cache_for_key(public_count_without_hidden_cache_key) do
issues_without_hidden_without_confidential
end
update_cache_for_key(total_count_without_hidden_cache_key) do
issues_without_hidden_with_confidential
end
end
end
private
def relation_for_count
self.class.query(@project, public_only: public_only?, include_hidden: include_hidden?)
end
def cache_key_name
public_only? ? PUBLIC_COUNT_KEY : TOTAL_COUNT_KEY
if include_hidden?
TOTAL_COUNT_KEY
elsif public_only?
PUBLIC_COUNT_WITHOUT_HIDDEN_KEY
else
TOTAL_COUNT_WITHOUT_HIDDEN_KEY
end
end
def include_hidden?
user_is_admin?
end
def public_only?
!user_is_at_least_reporter?
end
def relation_for_count
self.class.query(@project, public_only: public_only?)
def user_is_admin?
strong_memoize(:user_is_admin) do
@user&.can_admin_all_resources?
end
end
def user_is_at_least_reporter?
......@@ -34,46 +75,43 @@ module Projects
end
end
def public_count_cache_key
cache_key(PUBLIC_COUNT_KEY)
def total_count_without_hidden_cache_key
cache_key(TOTAL_COUNT_WITHOUT_HIDDEN_KEY)
end
def public_count_without_hidden_cache_key
cache_key(PUBLIC_COUNT_WITHOUT_HIDDEN_KEY)
end
def total_count_cache_key
cache_key(TOTAL_COUNT_KEY)
end
# rubocop: disable CodeReuse/ActiveRecord
def refresh_cache(&block)
if block_given?
super(&block)
else
count_grouped_by_confidential = self.class.query(@project, public_only: false).group(:confidential).count
public_count = count_grouped_by_confidential[false] || 0
total_count = public_count + (count_grouped_by_confidential[true] || 0)
update_cache_for_key(public_count_cache_key) do
public_count
def issues_with_hidden
self.class.query(@project, public_only: false, include_hidden: true).count
end
update_cache_for_key(total_count_cache_key) do
total_count
end
def issues_without_hidden_without_confidential
self.class.query(@project, public_only: true, include_hidden: false).count
end
def issues_without_hidden_with_confidential
self.class.query(@project, public_only: false, include_hidden: false).count
end
# rubocop: enable CodeReuse/ActiveRecord
# We only show total issues count for reporters
# which are allowed to view confidential issues
# We only show total issues count for admins, who are allowed to view hidden issues.
# We also only show issues count including confidential for reporters, who are allowed to view confidential issues.
# This will still show a discrepancy on issues number but should be less than before.
# Check https://gitlab.com/gitlab-org/gitlab-foss/issues/38418 description.
# rubocop: disable CodeReuse/ActiveRecord
def self.query(projects, public_only: true)
issues_filtered_by_type = Issue.opened.with_issue_type(Issue::TYPES_FOR_LIST)
if public_only
issues_filtered_by_type.public_only.where(project: projects)
def self.query(projects, public_only: true, include_hidden: false)
if include_hidden
Issue.opened.with_issue_type(Issue::TYPES_FOR_LIST).where(project: projects)
elsif public_only
Issue.public_only.opened.with_issue_type(Issue::TYPES_FOR_LIST).where(project: projects)
else
issues_filtered_by_type.where(project: projects)
Issue.without_hidden.opened.with_issue_type(Issue::TYPES_FOR_LIST).where(project: projects)
end
end
# rubocop: enable CodeReuse/ActiveRecord
......
......@@ -230,43 +230,29 @@ RSpec.describe IssuesFinder do
end
end
end
end
end
describe '#with_confidentiality_access_check' do
let_it_be(:guest) { create(:user) }
let_it_be(:authorized_user) { create(:user) }
context 'with hidden issues' do
let_it_be(:banned_user) { create(:user, :banned) }
let_it_be(:project) { create(:project, namespace: authorized_user.namespace) }
let_it_be(:public_issue) { create(:issue, project: project) }
let_it_be(:confidential_issue) { create(:issue, project: project, confidential: true) }
let_it_be(:hidden_issue) { create(:issue, project: project, author: banned_user) }
context 'when no project filter is given' do
let(:params) { {} }
let_it_be(:hidden_issue) { create(:issue, project: project1, author: banned_user) }
context 'for an auditor' do
let(:auditor_user) { create(:user, :auditor) }
let(:user) { create(:user, :auditor) }
subject { described_class.new(auditor_user, params).with_confidentiality_access_check }
context 'when no project filter is given' do
let(:params) { {} }
it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue, hidden_issue)
end
expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, hidden_issue)
end
end
context 'when searching within a specific project' do
let(:params) { { project_id: project.id } }
context 'for an auditor' do
let(:auditor_user) { create(:user, :auditor) }
subject { described_class.new(auditor_user, params).with_confidentiality_access_check }
let(:params) { { project_id: project1.id } }
it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue, hidden_issue)
expect(issues).to contain_exactly(issue1, issue5, hidden_issue)
end
end
end
end
end
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe IssuesFinder::Params do
describe '#include_hidden' do
subject { described_class.new(params, user, IssuesFinder) }
context 'when param is not set' do
let(:params) { {} }
context 'with an admin', :enable_admin_mode do
let(:user) { create(:user, :admin) }
it 'returns true' do
expect(subject.include_hidden?).to be_truthy
end
end
context 'with a regular user' do
let(:user) { create(:user) }
it 'returns false' do
expect(subject.include_hidden?).to be_falsey
end
end
end
context 'when param is set' do
let(:params) { { include_hidden: true } }
context 'with an admin', :enable_admin_mode do
let(:user) { create(:user, :admin) }
it 'returns true' do
expect(subject.include_hidden?).to be_truthy
end
end
context 'with a regular user' do
let(:user) { create(:user) }
it 'returns false' do
expect(subject.include_hidden?).to be_falsey
end
end
end
end
end
This diff is collapsed.
......@@ -1204,12 +1204,24 @@ RSpec.describe Issue do
end
describe '.public_only' do
it 'only returns public issues' do
public_issue = create(:issue, project: reusable_project)
create(:issue, project: reusable_project, confidential: true)
let_it_be(:banned_user) { create(:user, :banned) }
let_it_be(:public_issue) { create(:issue, project: reusable_project) }
let_it_be(:confidential_issue) { create(:issue, project: reusable_project, confidential: true) }
let_it_be(:hidden_issue) { create(:issue, project: reusable_project, author: banned_user) }
it 'only returns public issues' do
expect(described_class.public_only).to eq([public_issue])
end
context 'when feature flag is disabled' do
before do
stub_feature_flags(ban_user_feature_flag: false)
end
it 'returns public and hidden issues' do
expect(described_class.public_only).to eq([public_issue, hidden_issue])
end
end
end
describe '.confidential_only' do
......
......@@ -3,12 +3,18 @@
require 'spec_helper'
RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_caching do
let_it_be(:group) { create(:group, :public)}
let_it_be(:group) { create(:group, :public) }
let_it_be(:project) { create(:project, :public, namespace: group) }
let_it_be(:admin) { create(:user, :admin) }
let_it_be(:user) { create(:user) }
let_it_be(:issue) { create(:issue, :opened, project: project) }
let_it_be(:confidential) { create(:issue, :opened, confidential: true, project: project) }
let_it_be(:closed) { create(:issue, :closed, project: project) }
let_it_be(:banned_user) { create(:user, :banned) }
before do
create(:issue, :opened, project: project)
create(:issue, :opened, confidential: true, project: project)
create(:issue, :opened, author: banned_user, project: project)
create(:issue, :closed, project: project)
end
subject { described_class.new(group, user) }
......@@ -20,17 +26,27 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac
it 'uses the IssuesFinder to scope issues' do
expect(IssuesFinder)
.to receive(:new)
.with(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: true)
.with(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: true, include_hidden: false)
subject.count
end
end
describe '#count' do
context 'when user is nil' do
it 'does not include confidential issues in the issue count' do
expect(described_class.new(group).count).to eq(1)
shared_examples 'counts public issues, does not count hidden or confidential' do
it 'counts only public issues' do
expect(subject.count).to eq(1)
end
it 'uses PUBLIC_COUNT_WITHOUT_HIDDEN_KEY cache key' do
expect(subject.cache_key).to include('group_open_public_issues_without_hidden_count')
end
end
context 'when user is nil' do
let(:user) { nil }
it_behaves_like 'counts public issues, does not count hidden or confidential'
end
context 'when user is provided' do
......@@ -39,9 +55,13 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac
group.add_reporter(user)
end
it 'returns the right count with confidential issues' do
it 'includes confidential issues and does not include hidden issues in count' do
expect(subject.count).to eq(2)
end
it 'uses TOTAL_COUNT_WITHOUT_HIDDEN_KEY cache key' do
expect(subject.cache_key).to include('group_open_issues_without_hidden_count')
end
end
context 'when user cannot read confidential issues' do
......@@ -49,8 +69,24 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac
group.add_guest(user)
end
it 'does not include confidential issues' do
expect(subject.count).to eq(1)
it_behaves_like 'counts public issues, does not count hidden or confidential'
end
context 'when user is an admin' do
let(:user) { admin }
context 'when admin mode is enabled', :enable_admin_mode do
it 'includes confidential and hidden issues in count' do
expect(subject.count).to eq(3)
end
it 'uses TOTAL_COUNT_KEY cache key' do
expect(subject.cache_key).to include('group_open_issues_including_hidden_count')
end
end
context 'when admin mode is disabled' do
it_behaves_like 'counts public issues, does not count hidden or confidential'
end
end
......@@ -61,11 +97,13 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac
describe '#clear_all_cache_keys' do
it 'calls `Rails.cache.delete` with the correct keys' do
expect(Rails.cache).to receive(:delete)
.with(['groups', 'open_issues_count_service', 1, group.id, described_class::PUBLIC_COUNT_KEY])
.with(['groups', 'open_issues_count_service', 1, group.id, described_class::PUBLIC_COUNT_WITHOUT_HIDDEN_KEY])
expect(Rails.cache).to receive(:delete)
.with(['groups', 'open_issues_count_service', 1, group.id, described_class::TOTAL_COUNT_KEY])
expect(Rails.cache).to receive(:delete)
.with(['groups', 'open_issues_count_service', 1, group.id, described_class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY])
subject.clear_all_cache_keys
described_class.new(group).clear_all_cache_keys
end
end
end
......@@ -225,7 +225,7 @@ RSpec.describe Issues::CloseService do
it 'verifies the number of queries' do
recorded = ActiveRecord::QueryRecorder.new { close_issue }
expected_queries = 25
expected_queries = 27
expect(recorded.count).to be <= expected_queries
expect(recorded.cached_count).to eq(0)
......
......@@ -5,6 +5,7 @@ require 'spec_helper'
RSpec.describe Projects::BatchOpenIssuesCountService do
let!(:project_1) { create(:project) }
let!(:project_2) { create(:project) }
let!(:banned_user) { create(:user, :banned) }
let(:subject) { described_class.new([project_1, project_2]) }
......@@ -12,32 +13,41 @@ RSpec.describe Projects::BatchOpenIssuesCountService do
before do
create(:issue, project: project_1)
create(:issue, project: project_1, confidential: true)
create(:issue, project: project_1, author: banned_user)
create(:issue, project: project_2)
create(:issue, project: project_2, confidential: true)
create(:issue, project: project_2, author: banned_user)
end
context 'when cache is clean' do
context 'when cache is clean', :aggregate_failures do
it 'refreshes cache keys correctly' do
subject.refresh_cache_and_retrieve_data
expect(get_cache_key(project_1)).to eq(nil)
expect(get_cache_key(project_2)).to eq(nil)
subject.count_service.new(project_1).refresh_cache
subject.count_service.new(project_2).refresh_cache
expect(get_cache_key(project_1)).to eq(1)
expect(get_cache_key(project_2)).to eq(1)
# It does not update total issues cache
expect(Rails.cache.read(get_cache_key(subject, project_1))).to eq(nil)
expect(Rails.cache.read(get_cache_key(subject, project_2))).to eq(nil)
expect(get_cache_key(project_1, true)).to eq(2)
expect(get_cache_key(project_2, true)).to eq(2)
expect(Rails.cache.read(get_cache_key(subject, project_1, true))).to eq(1)
expect(Rails.cache.read(get_cache_key(subject, project_1, true))).to eq(1)
expect(get_cache_key(project_1, true, true)).to eq(3)
expect(get_cache_key(project_2, true, true)).to eq(3)
end
end
end
def get_cache_key(subject, project, public_key = false)
def get_cache_key(project, with_confidential = false, with_hidden = false)
service = subject.count_service.new(project)
if public_key
service.cache_key(service.class::PUBLIC_COUNT_KEY)
if with_confidential && with_hidden
Rails.cache.read(service.cache_key(service.class::TOTAL_COUNT_KEY))
elsif with_confidential
Rails.cache.read(service.cache_key(service.class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY))
else
service.cache_key(service.class::TOTAL_COUNT_KEY)
Rails.cache.read(service.cache_key(service.class::PUBLIC_COUNT_WITHOUT_HIDDEN_KEY))
end
end
end
......@@ -4,89 +4,102 @@ require 'spec_helper'
RSpec.describe Projects::OpenIssuesCountService, :use_clean_rails_memory_store_caching do
let(:project) { create(:project) }
let(:user) { create(:user) }
let(:banned_user) { create(:user, :banned) }
subject { described_class.new(project) }
subject { described_class.new(project, user) }
it_behaves_like 'a counter caching service'
describe '#count' do
context 'when user is nil' do
it 'does not include confidential issues in the issue count' do
before do
create(:issue, :opened, project: project)
create(:issue, :opened, confidential: true, project: project)
create(:issue, :opened, author: banned_user, project: project)
create(:issue, :closed, project: project)
expect(described_class.new(project).count).to eq(1)
described_class.new(project).refresh_cache
end
describe '#count' do
shared_examples 'counts public issues, does not count hidden or confidential' do
it 'counts only public issues' do
expect(subject.count).to eq(1)
end
context 'when user is provided' do
let(:user) { create(:user) }
it 'uses PUBLIC_COUNT_WITHOUT_HIDDEN_KEY cache key' do
expect(subject.cache_key).to include('project_open_public_issues_without_hidden_count')
end
end
context 'when user is nil' do
let(:user) { nil }
it_behaves_like 'counts public issues, does not count hidden or confidential'
end
context 'when user is provided' do
context 'when user can read confidential issues' do
before do
project.add_reporter(user)
end
it 'returns the right count with confidential issues' do
create(:issue, :opened, project: project)
create(:issue, :opened, confidential: true, project: project)
expect(described_class.new(project, user).count).to eq(2)
it 'includes confidential issues and does not include hidden issues in count' do
expect(subject.count).to eq(2)
end
it 'uses total_open_issues_count cache key' do
expect(described_class.new(project, user).cache_key_name).to eq('total_open_issues_count')
it 'uses TOTAL_COUNT_WITHOUT_HIDDEN_KEY cache key' do
expect(subject.cache_key).to include('project_open_issues_without_hidden_count')
end
end
context 'when user cannot read confidential issues' do
context 'when user cannot read confidential or hidden issues' do
before do
project.add_guest(user)
end
it 'does not include confidential issues' do
create(:issue, :opened, project: project)
create(:issue, :opened, confidential: true, project: project)
expect(described_class.new(project, user).count).to eq(1)
it_behaves_like 'counts public issues, does not count hidden or confidential'
end
it 'uses public_open_issues_count cache key' do
expect(described_class.new(project, user).cache_key_name).to eq('public_open_issues_count')
context 'when user is an admin' do
let_it_be(:user) { create(:user, :admin) }
context 'when admin mode is enabled', :enable_admin_mode do
it 'includes confidential and hidden issues in count' do
expect(subject.count).to eq(3)
end
it 'uses TOTAL_COUNT_KEY cache key' do
expect(subject.cache_key).to include('project_open_issues_including_hidden_count')
end
end
describe '#refresh_cache' do
before do
create(:issue, :opened, project: project)
create(:issue, :opened, project: project)
create(:issue, :opened, confidential: true, project: project)
context 'when admin mode is disabled' do
it_behaves_like 'counts public issues, does not count hidden or confidential'
end
end
end
end
describe '#refresh_cache', :aggregate_failures do
context 'when cache is empty' do
it 'refreshes cache keys correctly' do
subject.refresh_cache
expect(Rails.cache.read(subject.cache_key(described_class::PUBLIC_COUNT_KEY))).to eq(2)
expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_KEY))).to eq(3)
expect(Rails.cache.read(described_class.new(project).cache_key(described_class::PUBLIC_COUNT_WITHOUT_HIDDEN_KEY))).to eq(1)
expect(Rails.cache.read(described_class.new(project).cache_key(described_class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY))).to eq(2)
expect(Rails.cache.read(described_class.new(project).cache_key(described_class::TOTAL_COUNT_KEY))).to eq(3)
end
end
context 'when cache is outdated' do
before do
subject.refresh_cache
end
it 'refreshes cache keys correctly' do
create(:issue, :opened, project: project)
create(:issue, :opened, confidential: true, project: project)
create(:issue, :opened, author: banned_user, project: project)
subject.refresh_cache
described_class.new(project).refresh_cache
expect(Rails.cache.read(subject.cache_key(described_class::PUBLIC_COUNT_KEY))).to eq(3)
expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_KEY))).to eq(5)
end
expect(Rails.cache.read(described_class.new(project).cache_key(described_class::PUBLIC_COUNT_WITHOUT_HIDDEN_KEY))).to eq(2)
expect(Rails.cache.read(described_class.new(project).cache_key(described_class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY))).to eq(4)
expect(Rails.cache.read(described_class.new(project).cache_key(described_class::TOTAL_COUNT_KEY))).to eq(6)
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