diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 7595b1c7a15933ad55683611325f727e3d7b57b7..abf0c180d6b75524c3668930af30ed848365db6f 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -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 @@ def params_class # 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 @@ def with_confidentiality_access_check 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 diff --git a/app/finders/issues_finder/params.rb b/app/finders/issues_finder/params.rb index 2edd8a6f099b75e32b4aa57f4f47b48169647a96..02b89f08f9e05f9198ad1559f6c71ee2482db128 100644 --- a/app/finders/issues_finder/params.rb +++ b/app/finders/issues_finder/params.rb @@ -6,6 +6,10 @@ def public_only? 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 diff --git a/app/models/issue.rb b/app/models/issue.rb index 724b9d08fdf65719c0d7b7f8028e20ee891d4c95..847521ccb5b7206e1eb95631948c00d6c8d24bdf 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -128,13 +128,15 @@ def most_recent } 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 diff --git a/app/services/groups/open_issues_count_service.rb b/app/services/groups/open_issues_count_service.rb index 17cf3d38987ebda34b2202e9a276df72663b9444..c18d239998b1dc97706ea0a5cbdafb3530023b02 100644 --- a/app/services/groups/open_issues_count_service.rb +++ b/app/services/groups/open_issues_count_service.rb @@ -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 @@ def clear_all_cache_keys 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 @@ def relation_for_count state: 'opened', non_archived: true, include_subgroups: true, - public_only: public_only? + public_only: public_only?, + include_hidden: include_hidden? ).execute end diff --git a/app/services/projects/open_issues_count_service.rb b/app/services/projects/open_issues_count_service.rb index dc450311db281fe6dc377c05a2a03f2151b2f1a2..8b7a418edf542655f551c2dd691f589f30806fd8 100644 --- a/app/services/projects/open_issues_count_service.rb +++ b/app/services/projects/open_issues_count_service.rb @@ -7,8 +7,12 @@ class OpenIssuesCountService < Projects::CountService 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 @@ def initialize(project, user = nil) 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 @@ def user_is_at_least_reporter? 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) + def issues_with_hidden + self.class.query(@project, public_only: false, include_hidden: true).count + end - update_cache_for_key(public_count_cache_key) do - public_count - end + def issues_without_hidden_without_confidential + self.class.query(@project, public_only: true, include_hidden: false).count + end - update_cache_for_key(total_count_cache_key) do - total_count - end - 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 diff --git a/ee/spec/finders/issues_finder_spec.rb b/ee/spec/finders/issues_finder_spec.rb index eb350dd00be97db47898fccbcebbccf427836d4a..94cd38388177d66b088ffc5c6a06907c077ba5cf 100644 --- a/ee/spec/finders/issues_finder_spec.rb +++ b/ee/spec/finders/issues_finder_spec.rb @@ -230,43 +230,29 @@ end end end - end - end - - describe '#with_confidentiality_access_check' do - let_it_be(:guest) { create(:user) } - - let_it_be(:authorized_user) { create(:user) } - 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) { {} } + context 'with hidden issues' do + let_it_be(:banned_user) { create(:user, :banned) } + let_it_be(:hidden_issue) { create(:issue, project: project1, author: banned_user) } - context 'for an auditor' do - let(:auditor_user) { create(:user, :auditor) } + context 'for an auditor' do + 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 - 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) } + it 'returns all issues' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, hidden_issue) + end + end - subject { described_class.new(auditor_user, params).with_confidentiality_access_check } + context 'when searching within a specific project' do + let(:params) { { project_id: project1.id } } - it 'returns all issues' do - expect(subject).to include(public_issue, confidential_issue, hidden_issue) + it 'returns all issues' do + expect(issues).to contain_exactly(issue1, issue5, hidden_issue) + end + end end end end diff --git a/spec/finders/issues_finder/params_spec.rb b/spec/finders/issues_finder/params_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..879ecc364a2c62af249164b7a45f4faa53d31e05 --- /dev/null +++ b/spec/finders/issues_finder/params_spec.rb @@ -0,0 +1,49 @@ +# 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 diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 0cb73f3da6df02dbb7dada3430a9ccd8268e8183..ac099a6e811ddf4df847182d3367f1e2fdf6851b 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -12,8 +12,52 @@ context 'scope: all' do let(:scope) { 'all' } - it 'returns all issues' do - expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5) + context 'include_hidden and public_only params' do + let_it_be(:banned_user) { create(:user, :banned) } + let_it_be(:hidden_issue) { create(:issue, project: project1, author: banned_user) } + let_it_be(:confidential_issue) { create(:issue, project: project1, confidential: true) } + + context 'when user is an admin', :enable_admin_mode do + let(:user) { create(:user, :admin) } + + it 'returns all issues' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, hidden_issue, confidential_issue) + end + end + + context 'when user is not an admin' do + context 'when public_only is true' do + let(:params) { { public_only: true } } + + it 'returns public issues' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5) + end + end + + context 'when public_only is false' do + let(:params) { { public_only: false } } + + it 'returns public and confidential issues' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) + end + end + + context 'when public_only is not set' do + it 'returns public and confidential issue' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) + end + end + + context 'when ban_user_feature_flag is false' do + before do + stub_feature_flags(ban_user_feature_flag: false) + end + + it 'returns all issues' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, hidden_issue, confidential_issue) + end + end + end end context 'user does not have read permissions' do @@ -1001,132 +1045,64 @@ end describe '#with_confidentiality_access_check' do - let(:guest) { create(:user) } + let(:user) { create(:user) } let_it_be(:authorized_user) { create(:user) } - 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) } - shared_examples 'returns public, does not return hidden or confidential' do + shared_examples 'returns public, does not return confidential' do it 'returns only public issues' do expect(subject).to include(public_issue) - expect(subject).not_to include(confidential_issue, hidden_issue) + expect(subject).not_to include(confidential_issue) end end - shared_examples 'returns public and confidential, does not return hidden' do - it 'returns only public and confidential issues' do + shared_examples 'returns public and confidential' do + it 'returns public and confidential issues' do expect(subject).to include(public_issue, confidential_issue) - expect(subject).not_to include(hidden_issue) end end - shared_examples 'returns public and hidden, does not return confidential' do - it 'returns only public and hidden issues' do - expect(subject).to include(public_issue, hidden_issue) - expect(subject).not_to include(confidential_issue) - end - end - - shared_examples 'returns public, confidential, and hidden' do - it 'returns all issues' do - expect(subject).to include(public_issue, confidential_issue, hidden_issue) - end - end + subject { described_class.new(user, params).with_confidentiality_access_check } context 'when no project filter is given' do let(:params) { {} } context 'for an anonymous user' do - subject { described_class.new(nil, params).with_confidentiality_access_check } - - it_behaves_like 'returns public, does not return hidden or confidential' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public and hidden, does not return confidential' - end + it_behaves_like 'returns public, does not return confidential' end context 'for a user without project membership' do - subject { described_class.new(user, params).with_confidentiality_access_check } - - it_behaves_like 'returns public, does not return hidden or confidential' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public and hidden, does not return confidential' - end + it_behaves_like 'returns public, does not return confidential' end context 'for a guest user' do - subject { described_class.new(guest, params).with_confidentiality_access_check } - before do - project.add_guest(guest) + project.add_guest(user) end - it_behaves_like 'returns public, does not return hidden or confidential' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public and hidden, does not return confidential' - end + it_behaves_like 'returns public, does not return confidential' end context 'for a project member with access to view confidential issues' do - subject { described_class.new(authorized_user, params).with_confidentiality_access_check } - - it_behaves_like 'returns public and confidential, does not return hidden' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public, confidential, and hidden' + before do + project.add_reporter(user) end + + it_behaves_like 'returns public and confidential' end context 'for an admin' do - let(:admin_user) { create(:user, :admin) } - - subject { described_class.new(admin_user, params).with_confidentiality_access_check } + let(:user) { create(:user, :admin) } context 'when admin mode is enabled', :enable_admin_mode do - it_behaves_like 'returns public, confidential, and hidden' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public, confidential, and hidden' - end + it_behaves_like 'returns public and confidential' end context 'when admin mode is disabled' do - it_behaves_like 'returns public, does not return hidden or confidential' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public and hidden, does not return confidential' - end + it_behaves_like 'returns public, does not return confidential' end end end @@ -1135,17 +1111,9 @@ let(:params) { { project_id: project.id } } context 'for an anonymous user' do - subject { described_class.new(nil, params).with_confidentiality_access_check } + let(:user) { nil } - it_behaves_like 'returns public, does not return hidden or confidential' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public and hidden, does not return confidential' - end + it_behaves_like 'returns public, does not return confidential' it 'does not filter by confidentiality' do expect(Issue).not_to receive(:where).with(a_string_matching('confidential'), anything) @@ -1154,17 +1122,7 @@ end context 'for a user without project membership' do - subject { described_class.new(user, params).with_confidentiality_access_check } - - it_behaves_like 'returns public, does not return hidden or confidential' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public and hidden, does not return confidential' - end + it_behaves_like 'returns public, does not return confidential' it 'filters by confidentiality' do expect(subject.to_sql).to match("issues.confidential") @@ -1172,21 +1130,11 @@ end context 'for a guest user' do - subject { described_class.new(guest, params).with_confidentiality_access_check } - before do - project.add_guest(guest) + project.add_guest(user) end - it_behaves_like 'returns public, does not return hidden or confidential' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public and hidden, does not return confidential' - end + it_behaves_like 'returns public, does not return confidential' it 'filters by confidentiality' do expect(subject.to_sql).to match("issues.confidential") @@ -1194,40 +1142,18 @@ end context 'for a project member with access to view confidential issues' do - subject { described_class.new(authorized_user, params).with_confidentiality_access_check } - - it_behaves_like 'returns public and confidential, does not return hidden' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public, confidential, and hidden' + before do + project.add_reporter(user) end - it 'does not filter by confidentiality' do - expect(Issue).not_to receive(:where).with(a_string_matching('confidential'), anything) - - subject - end + it_behaves_like 'returns public and confidential' end context 'for an admin' do - let(:admin_user) { create(:user, :admin) } - - subject { described_class.new(admin_user, params).with_confidentiality_access_check } + let(:user) { create(:user, :admin) } context 'when admin mode is enabled', :enable_admin_mode do - it_behaves_like 'returns public, confidential, and hidden' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public, confidential, and hidden' - end + it_behaves_like 'returns public and confidential' it 'does not filter by confidentiality' do expect(Issue).not_to receive(:where).with(a_string_matching('confidential'), anything) @@ -1237,19 +1163,7 @@ end context 'when admin mode is disabled' do - it_behaves_like 'returns public, does not return hidden or confidential' - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ban_user_feature_flag: false) - end - - it_behaves_like 'returns public and hidden, does not return confidential' - end - - it 'filters by confidentiality' do - expect(subject.to_sql).to match("issues.confidential") - end + it_behaves_like 'returns public, does not return confidential' end end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 09528b6304ef6c83ce755f0b097faf034555913c..10a42996e076a74ebb5899ffd4c35ca2c6fc9ef5 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -1185,12 +1185,24 @@ 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 diff --git a/spec/services/groups/open_issues_count_service_spec.rb b/spec/services/groups/open_issues_count_service_spec.rb index fca09bfdebe21e0cc7f1db08070b8edbe89df7ce..7dd8c2a59a0f78833df58266092ed41ab1a6c2c6 100644 --- a/spec/services/groups/open_issues_count_service_spec.rb +++ b/spec/services/groups/open_issues_count_service_spec.rb @@ -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,28 +26,42 @@ 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 context 'when user can read confidential issues' do before do 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 @@ 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 @@ 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 diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 15a082ddf0d15e396f454ca8033baea84bcc2fa6..14e6b44f7b0750f3515d9cbcd97b370858b99275 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -225,7 +225,7 @@ 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) diff --git a/spec/services/projects/batch_open_issues_count_service_spec.rb b/spec/services/projects/batch_open_issues_count_service_spec.rb index 89a4abbf9c978063e0c00d0d78869425909dd218..17bd5f7a37b42523cd51836c1556f5779069741f 100644 --- a/spec/services/projects/batch_open_issues_count_service_spec.rb +++ b/spec/services/projects/batch_open_issues_count_service_spec.rb @@ -5,6 +5,7 @@ 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 @@ 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 diff --git a/spec/services/projects/open_issues_count_service_spec.rb b/spec/services/projects/open_issues_count_service_spec.rb index c739fea5ecfc91b5eb174b90c2cb9ec9e78282a6..8710d0c02671bffbfb47ee85f8fe2fb674cddb5a 100644 --- a/spec/services/projects/open_issues_count_service_spec.rb +++ b/spec/services/projects/open_issues_count_service_spec.rb @@ -4,89 +4,102 @@ 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' + 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) + + described_class.new(project).refresh_cache + end + describe '#count' do - context 'when user is nil' do - it 'does not include confidential issues in the issue count' do - create(:issue, :opened, project: project) - create(:issue, :opened, confidential: true, project: project) + 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 - expect(described_class.new(project).count).to eq(1) + 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 provided' do - let(:user) { create(:user) } + 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) + it_behaves_like 'counts public issues, does not count hidden or confidential' + end + + 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 - expect(described_class.new(project, user).count).to eq(1) + it 'uses TOTAL_COUNT_KEY cache key' do + expect(subject.cache_key).to include('project_open_issues_including_hidden_count') + end 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 admin mode is disabled' do + it_behaves_like 'counts public issues, does not count hidden or confidential' end end 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) - end - - 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) - end + describe '#refresh_cache', :aggregate_failures do + context 'when cache is empty' do + it 'refreshes cache keys correctly' do + 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) + context 'when cache is outdated' do + 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