From 5a74d5e40b90775accc0f793e23ca800fee2b151 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 13 Jul 2021 16:16:23 -0500 Subject: [PATCH 01/10] Reset feature branch Reset hard master and create banned user table Add ban service spec Add specs for banned user and ban service Add unban service file BannedUser object gets created and destroyed Add unban service specs Add specs for unban user Run rake translate Changelog: added --- locale/gitlab.pot | 6 ++++++ spec/services/users/unban_service_spec.rb | 26 +++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index c1ff7dc75c47b1..beec473b73c0eb 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -38177,6 +38177,9 @@ msgstr "" msgid "You are not allowed to approve a user" msgstr "" +msgid "You are not allowed to ban a user" +msgstr "" + msgid "You are not allowed to log in using password" msgstr "" @@ -38186,6 +38189,9 @@ msgstr "" msgid "You are not allowed to reject a user" msgstr "" +msgid "You are not allowed to unban a user" +msgstr "" + msgid "You are not allowed to unlink your primary login account" msgstr "" diff --git a/spec/services/users/unban_service_spec.rb b/spec/services/users/unban_service_spec.rb index d536baafdcc9bb..c1f368db45be7a 100644 --- a/spec/services/users/unban_service_spec.rb +++ b/spec/services/users/unban_service_spec.rb @@ -45,6 +45,7 @@ end context 'when failed' do +<<<<<<< HEAD context 'when user is already active', :enable_admin_mode do it 'returns state error message' do response = unban_user @@ -54,10 +55,22 @@ end it_behaves_like 'does not modify the BannedUser record or user state' +======= + context 'when user is not already banned', :enable_admin_mode do + it 'error result' do + expect(operation[:status]).to eq(:error) + expect(operation[:message]).to match(/State cannot transition/) + end + + it 'does not change the user state', :aggregate_failures do + expect { operation }.not_to change { user.state } + end +>>>>>>> 048ac186b4e (Reset feature branch) end context 'when user is not an admin' do before do +<<<<<<< HEAD user.ban! end @@ -69,6 +82,19 @@ end it_behaves_like 'does not modify the BannedUser record or user state' +======= + Users::BanService.new(current_user).execute(user) + end + + it 'error result' do + expect(operation[:status]).to eq(:error) + expect(operation[:message]).to match(/You are not allowed to unban a user/) + end + + it 'does not change the user state', :aggregate_failures do + expect { operation }.not_to change { user.state } + end +>>>>>>> 048ac186b4e (Reset feature branch) end end end -- GitLab From 901e3c0cee762d5c0ab9fe95e4bcaea336ec2419 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 13 Jul 2021 20:18:50 -0500 Subject: [PATCH 02/10] Fix open issues count Update state machine transition Update ban and unban specs Remove enable admin mode Remove unneeded method Remove unneeded method Add specs for ban and unban DRY ban and unban service Consolidate ban and unban user service into one file DRY spec shared example Create Users BaseService Need to fix specs Fix Base Service Update specs Use error instead of service error Change service error to permission error Rename Users BaseService DRY ban and unban service Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Remove bad rebase changes Some files got left over, remove them Remove bad rebase changes Some files got left over, remove them Run rails migration Remove issue hidden attribute Also update ban copy Add issue specs Add specs for issue model Use single quotes Add hidden issue to user moderation doc Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Remove bad rebase changes Some files got left over, remove them Remove bad rebase changes Some files got left over, remove them Run rails migration Remove issue hidden attribute Also update ban copy Add issue specs Add specs for issue model Use single quotes Add hidden issue to user moderation doc Use safe operator Fix failing specs Reset feature branch Reset hard master and create banned user table Add ban service spec Add specs for banned user and ban service Add unban service file BannedUser object gets created and destroyed Add unban service specs Add specs for unban user Run rake translate Changelog: added Add aggregate failures Update state machine transition Update ban and unban specs Remove enable admin mode Remove unneeded method Remove unneeded method Add specs for ban and unban DRY ban and unban service Consolidate ban and unban user service into one file DRY spec shared example Create Users BaseService Need to fix specs Fix Base Service Update specs Use error instead of service error Change service error to permission error Rename Users BaseService DRY ban and unban service Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Remove bad rebase changes Some files got left over, remove them Remove bad rebase changes Some files got left over, remove them Run rails migration Remove issue hidden attribute Also update ban copy Add issue specs Add specs for issue model Use single quotes Add hidden issue to user moderation doc Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Remove bad rebase changes Some files got left over, remove them Remove bad rebase changes Some files got left over, remove them Run rails migration Remove issue hidden attribute Also update ban copy Add issue specs Add specs for issue model Use single quotes Add hidden issue to user moderation doc Fix failing specs New way of fixing pipeline failures Address MR review comments Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Remove bad rebase changes Some files got left over, remove them Remove bad rebase changes Some files got left over, remove them Run rails migration Remove issue hidden attribute Also update ban copy Add issue specs Add specs for issue model Use single quotes Add hidden issue to user moderation doc Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Remove bad rebase changes Some files got left over, remove them Remove bad rebase changes Some files got left over, remove them Run rails migration Remove issue hidden attribute Also update ban copy Add issue specs Add specs for issue model Use single quotes Add hidden issue to user moderation doc Run rubocop linter Run bin rake translate Ban and unban user service Move execute to base service Run rubocop linter Rename user ban spec Run translation script Use action method instead of override initialize Move some conceptual material to main page and other minor improvements Add hidden column to issues table Does not allow non admin user to read the issue Rebase db structure with master Add hidden only issues scope Exclude hidden issues from issues finder If user is non admin, do not show hidden issues Add issues finder specs Update issue scopes Also fix specs Add ampersand to issue policy Revert unwanted rebase changes Add migration files Migration files got overwritten by rebase Remove bad rebase changes Some files got left over, remove them Remove bad rebase changes Some files got left over, remove them Run rails migration Remove issue hidden attribute Also update ban copy Add issue specs Add specs for issue model Use single quotes New keys for OpenIssuesCountService In order to correctly show sidebar issue counts, need new keys for project and group issue count service Specs for group issue count Add specs for group issue count service Need to fix Fix public only issue scope Fix public only scope Fix issue public only scope Change query limit Remove issue policy admin Remove repeated scope --- app/finders/issues_finder.rb | 2 +- .../groups/open_issues_count_service.rb | 24 +++- .../projects/open_issues_count_service.rb | 64 ++++++--- locale/gitlab.pot | 6 - .../groups/open_issues_count_service_spec.rb | 60 +++++++-- spec/services/issues/close_service_spec.rb | 2 +- .../batch_open_issues_count_service_spec.rb | 28 ++-- .../open_issues_count_service_spec.rb | 123 ++++++++++-------- spec/services/users/unban_service_spec.rb | 26 ---- 9 files changed, 201 insertions(+), 134 deletions(-) diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 7595b1c7a15933..25619ddc26409e 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -77,7 +77,7 @@ def with_confidentiality_access_check def init_collection if params.public_only? - Issue.public_only + Issue.without_hidden.public_only else with_confidentiality_access_check end diff --git a/app/services/groups/open_issues_count_service.rb b/app/services/groups/open_issues_count_service.rb index 17cf3d38987ebd..ee21bdcc41e764 100644 --- a/app/services/groups/open_issues_count_service.rb +++ b/app/services/groups/open_issues_count_service.rb @@ -3,25 +3,37 @@ 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) + # PUBLIC_COUNT_WITHOUT_HIDDEN_KEY does not include confidential or hidden issues (guest) + TOTAL_COUNT_KEY = 'group_total_open_issues_including_hidden_count' + TOTAL_COUNT_WITHOUT_HIDDEN_KEY = 'group_total_open_issues_without_hidden_count' + PUBLIC_COUNT_WITHOUT_HIDDEN_KEY = 'group_public_open_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 - private - def cache_key_name - public_only? ? PUBLIC_COUNT_KEY : TOTAL_COUNT_KEY + if user_is_admin? + TOTAL_COUNT_KEY + else + public_only? ? PUBLIC_COUNT_WITHOUT_HIDDEN_KEY : TOTAL_COUNT_WITHOUT_HIDDEN_KEY + end end def public_only? !user_is_at_least_reporter? end + def user_is_admin? + strong_memoize(:user_is_admin) do + user&.can_admin_all_resources? + end + end + def user_is_at_least_reporter? strong_memoize(:user_is_at_least_reporter) do group.member?(user, Gitlab::Access::REPORTER) diff --git a/app/services/projects/open_issues_count_service.rb b/app/services/projects/open_issues_count_service.rb index dc450311db281f..211c8895a832b6 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) + # PUBLIC_COUNT_WITHOUT_HIDDEN_KEY does not include confidential or hidden issues (guest) + TOTAL_COUNT_KEY = 'open_issues_including_hidden_count' + TOTAL_COUNT_WITHOUT_HIDDEN_KEY = 'open_issues_without_hidden_count' + PUBLIC_COUNT_WITHOUT_HIDDEN_KEY = 'open_public_issues_without_hidden_count' def initialize(project, user = nil) @user = user @@ -17,7 +21,15 @@ def initialize(project, user = nil) end def cache_key_name - public_only? ? PUBLIC_COUNT_KEY : TOTAL_COUNT_KEY + if user_is_admin? + TOTAL_COUNT_KEY + else + public_only? ? PUBLIC_COUNT_WITHOUT_HIDDEN_KEY : TOTAL_COUNT_WITHOUT_HIDDEN_KEY + end + end + + def user_is_admin? + @user&.can_admin_all_resources? end def public_only? @@ -25,7 +37,7 @@ def public_only? end def relation_for_count - self.class.query(@project, public_only: public_only?) + self.class.query(@project) end def user_is_at_least_reporter? @@ -34,8 +46,12 @@ 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 @@ -47,33 +63,39 @@ 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) + with_hidden_issues = self.class.query(@project, include_hidden: true) + without_hidden_issues = self.class.query(@project, include_hidden: false) - update_cache_for_key(public_count_cache_key) do - public_count - end + without_hidden_count_grouped_by_confidential = without_hidden_issues.group(:confidential).count + + total_public_without_hidden_count = without_hidden_count_grouped_by_confidential[false] || 0 + total_confidential_without_hidden_count = without_hidden_count_grouped_by_confidential[true] || 0 update_cache_for_key(total_count_cache_key) do - total_count + with_hidden_issues.count + end + + update_cache_for_key(public_count_without_hidden_cache_key) do + total_public_without_hidden_count + end + + update_cache_for_key(total_count_without_hidden_cache_key) do + total_public_without_hidden_count + total_confidential_without_hidden_count end end 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, include_hidden: true) + if include_hidden + Issue.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/locale/gitlab.pot b/locale/gitlab.pot index beec473b73c0eb..c1ff7dc75c47b1 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -38177,9 +38177,6 @@ msgstr "" msgid "You are not allowed to approve a user" msgstr "" -msgid "You are not allowed to ban a user" -msgstr "" - msgid "You are not allowed to log in using password" msgstr "" @@ -38189,9 +38186,6 @@ msgstr "" msgid "You are not allowed to reject a user" msgstr "" -msgid "You are not allowed to unban a user" -msgstr "" - msgid "You are not allowed to unlink your primary login account" msgstr "" diff --git a/spec/services/groups/open_issues_count_service_spec.rb b/spec/services/groups/open_issues_count_service_spec.rb index fca09bfdebe21e..802d05302bee8e 100644 --- a/spec/services/groups/open_issues_count_service_spec.rb +++ b/spec/services/groups/open_issues_count_service_spec.rb @@ -3,14 +3,11 @@ 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(: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) } - subject { described_class.new(group, user) } + subject { described_class.new(group) } describe '#relation_for_count' do before do @@ -22,14 +19,29 @@ .to receive(:new) .with(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: true) - subject.count + described_class.new(group, user).count end end describe '#count' do + let_it_be(:banned_user) { build(: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) + + subject.refresh_cache + end + 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) + it 'does not include confidential or hidden issues in the issue count' do + expect(subject.count).to eq(1) + end + + it 'uses group_public_open_issues_without_hidden_count cache key' do + expect(subject.cache_key_name).to eq('group_public_open_issues_without_hidden_count') end end @@ -39,8 +51,12 @@ group.add_reporter(user) end - it 'returns the right count with confidential issues' do - expect(subject.count).to eq(2) + it 'includes confidential issues and does not include hidden issues in count' do + expect(described_class.new(group, user).count).to eq(2) + end + + it 'uses group_total_open_issues_without_hidden_count cache key' do + expect(described_class.new(group, user).cache_key_name).to eq('group_total_open_issues_without_hidden_count') end end @@ -49,8 +65,24 @@ group.add_guest(user) end - it 'does not include confidential issues' do - expect(subject.count).to eq(1) + it 'does not include confidential or hidden issues in count' do + expect(described_class.new(group, user).count).to eq(1) + end + + it 'uses group_public_open_issues_without_hidden_count cache key' do + expect(described_class.new(group, user).cache_key_name).to eq('group_public_open_issues_without_hidden_count') + end + end + + context 'when user is an admin', :enable_admin_mode do + let_it_be(:admin) { create(:user, :admin) } + + it 'includes confidential and hidden issues in count' do + expect(described_class.new(group, admin).count).to eq(3) + end + + it 'uses group_total_open_issues_including_hidden_count cache key' do + expect(described_class.new(group, admin).cache_key_name).to eq('group_total_open_issues_including_hidden_count') end end @@ -61,9 +93,11 @@ 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 end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 15a082ddf0d15e..26ae96eee175aa 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 = 26 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 89a4abbf9c9780..28e05dc5ad4d9d 100644 --- a/spec/services/projects/batch_open_issues_count_service_spec.rb +++ b/spec/services/projects/batch_open_issues_count_service_spec.rb @@ -5,39 +5,49 @@ 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]) } + let(:count_service) { subject.count_service } describe '#refresh_cache_and_retrieve_data', :use_clean_rails_memory_store_caching 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 it 'refreshes cache keys correctly' do subject.refresh_cache_and_retrieve_data - # 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(Rails.cache.read(get_cache_key(subject, project_1))).to eq(1) + expect(Rails.cache.read(get_cache_key(subject, project_2))).to eq(1) - 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(Rails.cache.read(get_cache_key(subject, project_1, true))).to eq(2) + expect(Rails.cache.read(get_cache_key(subject, project_2, true))).to eq(2) + + expect(Rails.cache.read(get_cache_key(subject, project_1, true, true))).to eq(3) + expect(Rails.cache.read(get_cache_key(subject, project_2, true, true))).to eq(3) end end end - def get_cache_key(subject, project, public_key = false) + def get_cache_key(subject, project, include_confidential = false, include_hidden = false) service = subject.count_service.new(project) - if public_key - service.cache_key(service.class::PUBLIC_COUNT_KEY) + if include_confidential + if include_hidden + service.cache_key(service.class::TOTAL_COUNT_KEY) + else + service.cache_key(service.class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY) + end else - service.cache_key(service.class::TOTAL_COUNT_KEY) + 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 c739fea5ecfc91..87340c53db600b 100644 --- a/spec/services/projects/open_issues_count_service_spec.rb +++ b/spec/services/projects/open_issues_count_service_spec.rb @@ -4,89 +4,110 @@ RSpec.describe Projects::OpenIssuesCountService, :use_clean_rails_memory_store_caching do let(:project) { create(:project) } + let(:banned_user) { create(:user, :banned) } subject { described_class.new(project) } 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 + context 'returns different issue counts depending on user' do + before do create(:issue, :opened, project: project) create(:issue, :opened, confidential: true, project: project) - - expect(described_class.new(project).count).to eq(1) + create(:issue, :opened, author: banned_user, project: project) + subject.refresh_cache end - end - context 'when user is provided' do - let(:user) { create(:user) } + context 'when user is nil' do + it 'does not include confidential or hidden issues in the issue count' do + expect(subject.count).to eq(1) + end - context 'when user can read confidential issues' do - before do - project.add_reporter(user) + it 'uses open_public_issues_without_hidden_count cache key' do + expect(subject.cache_key_name).to eq('open_public_issues_without_hidden_count') end + end - it 'returns the right count with confidential issues' do - create(:issue, :opened, project: project) - create(:issue, :opened, confidential: true, project: project) + context 'when user is provided' do + let(:user) { create(:user) } - expect(described_class.new(project, user).count).to eq(2) - end + context 'when user can read confidential issues' do + before do + project.add_reporter(user) + 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') - end - end + it 'includes confidential issues and does not include hidden issues in count' do + expect(described_class.new(project, user).count).to eq(2) + end - context 'when user cannot read confidential issues' do - before do - project.add_guest(user) + it 'uses open_issues_without_hidden_count cache key' do + expect(described_class.new(project, user).cache_key_name).to eq('open_issues_without_hidden_count') + end end - it 'does not include confidential issues' do - create(:issue, :opened, project: project) - create(:issue, :opened, confidential: true, project: project) + context 'when user cannot read confidential or hidden issues' do + before do + project.add_guest(user) + end + + it 'does not include confidential or hidden issues' do + expect(described_class.new(project, user).count).to eq(1) + end - expect(described_class.new(project, user).count).to eq(1) + it 'uses open_public_issues_without_hidden_count cache key' do + expect(described_class.new(project, user).cache_key_name).to eq('open_public_issues_without_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 user is an admin', :enable_admin_mode do + let(:admin) { create(:admin) } + + it 'includes confidential and hidden issues in count' do + expect(described_class.new(project, admin).count).to eq(3) + end + + it 'uses open_issues_including_hidden_count cache key' do + expect(described_class.new(project, admin).cache_key_name).to eq('open_issues_including_hidden_count') + end 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 + describe '#refresh_cache', :aggregate_failures do + before do + create(:issue, :opened, project: project) + create(:issue, :opened, project: project) + create(:issue, :opened, confidential: true, project: project) + create(:issue, :opened, author: banned_user, project: project) + end - context 'when cache is empty' do - it 'refreshes cache keys correctly' do - subject.refresh_cache + 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 + expect(Rails.cache.read(subject.cache_key(described_class::PUBLIC_COUNT_WITHOUT_HIDDEN_KEY))).to eq(2) + expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY))).to eq(3) + expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_KEY))).to eq(4) end + end - context 'when cache is outdated' do - before do - subject.refresh_cache - 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) + 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 + subject.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(subject.cache_key(described_class::PUBLIC_COUNT_WITHOUT_HIDDEN_KEY))).to eq(3) + expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY))).to eq(5) + expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_KEY))).to eq(7) end end end diff --git a/spec/services/users/unban_service_spec.rb b/spec/services/users/unban_service_spec.rb index c1f368db45be7a..d536baafdcc9bb 100644 --- a/spec/services/users/unban_service_spec.rb +++ b/spec/services/users/unban_service_spec.rb @@ -45,7 +45,6 @@ end context 'when failed' do -<<<<<<< HEAD context 'when user is already active', :enable_admin_mode do it 'returns state error message' do response = unban_user @@ -55,22 +54,10 @@ end it_behaves_like 'does not modify the BannedUser record or user state' -======= - context 'when user is not already banned', :enable_admin_mode do - it 'error result' do - expect(operation[:status]).to eq(:error) - expect(operation[:message]).to match(/State cannot transition/) - end - - it 'does not change the user state', :aggregate_failures do - expect { operation }.not_to change { user.state } - end ->>>>>>> 048ac186b4e (Reset feature branch) end context 'when user is not an admin' do before do -<<<<<<< HEAD user.ban! end @@ -82,19 +69,6 @@ end it_behaves_like 'does not modify the BannedUser record or user state' -======= - Users::BanService.new(current_user).execute(user) - end - - it 'error result' do - expect(operation[:status]).to eq(:error) - expect(operation[:message]).to match(/You are not allowed to unban a user/) - end - - it 'does not change the user state', :aggregate_failures do - expect { operation }.not_to change { user.state } - end ->>>>>>> 048ac186b4e (Reset feature branch) end end end -- GitLab From 6a5995de67a63a2aa8f80fe1811738d84d898601 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Thu, 12 Aug 2021 18:34:38 -0500 Subject: [PATCH 03/10] Update batch issue count spec Dry open issue count specs DRY group and project issue count specs Add back in private Update issue count specs Apply MR review suggestions Refactor projects open issues count service Revert "Update issue count specs" This reverts commit 2e84df3b1309e4269bcf115f41b68f483676cccb. Add back in public only Modify project group issue count specs Also add private method Raise query limit Exclude hidden issues from public only Also group non hidden issues by confidential Add include hidden param Also update public only scope Fix issues finder spec Fix without hidden scope Fix specs include hidden Fix public only scope query Fix issue finder specs Move init collection to public methods Use public only in query Move specs around Add public only scope specs Raise query limit Test issue collection Test init collection with class specs, move init_collection back to private Fix EE issue finder specs Reuse without hidden scope Use updated scope --- app/finders/issues_finder.rb | 13 +- app/finders/issues_finder/params.rb | 4 + app/models/issue.rb | 5 +- .../groups/open_issues_count_service.rb | 23 +- .../projects/open_issues_count_service.rb | 88 +++--- ee/spec/finders/issues_finder_spec.rb | 48 ++-- spec/finders/issues_finder_spec.rb | 270 +++++++----------- spec/models/issue_spec.rb | 18 +- .../groups/open_issues_count_service_spec.rb | 72 ++--- spec/services/issues/close_service_spec.rb | 2 +- .../batch_open_issues_count_service_spec.rb | 30 +- .../open_issues_count_service_spec.rb | 118 ++++---- 12 files changed, 333 insertions(+), 358 deletions(-) diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 25619ddc26409e..babcc4d28adda2 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,8 +75,10 @@ def with_confidentiality_access_check private def init_collection - if params.public_only? - Issue.without_hidden.public_only + if params.include_hidden? + Issue.all + elsif params.public_only? + Issue.public_only else with_confidentiality_access_check end @@ -90,11 +91,13 @@ def filter_items(items) by_issue_types(issues) end + # rubocop: disable CodeReuse/ActiveRecord def by_confidential(items) return items if params[:confidential].nil? - params[:confidential] ? items.confidential_only : items.public_only + params[:confidential] ? items.confidential_only : items.where(confidential: false) end + # rubocop: enable CodeReuse/ActiveRecord def by_due_date(items) return items unless params.due_date? diff --git a/app/finders/issues_finder/params.rb b/app/finders/issues_finder/params.rb index 2edd8a6f099b75..c9d50b3e87bcec 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? + params.fetch(: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 724b9d08fdf657..45ecab16bb5786 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -128,7 +128,10 @@ 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, -> { diff --git a/app/services/groups/open_issues_count_service.rb b/app/services/groups/open_issues_count_service.rb index ee21bdcc41e764..2d79272ae2e291 100644 --- a/app/services/groups/open_issues_count_service.rb +++ b/app/services/groups/open_issues_count_service.rb @@ -4,11 +4,11 @@ module Groups # Service class for counting and caching the number of open issues of a group. class OpenIssuesCountService < Groups::CountService # TOTAL_COUNT_KEY includes confidential and hidden issues (admin) - # TOTAL_COUNT_WITHOUT_HIDDEN_KEY includes confidential issues but not hidden issues (reporter) + # 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_total_open_issues_including_hidden_count' - TOTAL_COUNT_WITHOUT_HIDDEN_KEY = 'group_total_open_issues_without_hidden_count' - PUBLIC_COUNT_WITHOUT_HIDDEN_KEY = 'group_public_open_issues_without_hidden_count' + 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(TOTAL_COUNT_KEY), cache_key(TOTAL_COUNT_WITHOUT_HIDDEN_KEY), cache_key(PUBLIC_COUNT_WITHOUT_HIDDEN_KEY)].each do |key| @@ -16,14 +16,22 @@ def clear_all_cache_keys end end + private + def cache_key_name - if user_is_admin? + if include_hidden? TOTAL_COUNT_KEY + elsif public_only? + PUBLIC_COUNT_WITHOUT_HIDDEN_KEY else - public_only? ? PUBLIC_COUNT_WITHOUT_HIDDEN_KEY : TOTAL_COUNT_WITHOUT_HIDDEN_KEY + TOTAL_COUNT_WITHOUT_HIDDEN_KEY end end + def include_hidden? + user_is_admin? + end + def public_only? !user_is_at_least_reporter? end @@ -47,7 +55,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 211c8895a832b6..8b7a418edf5426 100644 --- a/app/services/projects/open_issues_count_service.rb +++ b/app/services/projects/open_issues_count_service.rb @@ -8,11 +8,11 @@ class OpenIssuesCountService < Projects::CountService # Cache keys used to store issues count # TOTAL_COUNT_KEY includes confidential and hidden issues (admin) - # TOTAL_COUNT_WITHOUT_HIDDEN_KEY includes confidential issues but not hidden issues (reporter) + # 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 = 'open_issues_including_hidden_count' - TOTAL_COUNT_WITHOUT_HIDDEN_KEY = 'open_issues_without_hidden_count' - PUBLIC_COUNT_WITHOUT_HIDDEN_KEY = 'open_public_issues_without_hidden_count' + 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 @@ -20,24 +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 - if user_is_admin? + if include_hidden? TOTAL_COUNT_KEY + elsif public_only? + PUBLIC_COUNT_WITHOUT_HIDDEN_KEY else - public_only? ? PUBLIC_COUNT_WITHOUT_HIDDEN_KEY : TOTAL_COUNT_WITHOUT_HIDDEN_KEY + TOTAL_COUNT_WITHOUT_HIDDEN_KEY end end - def user_is_admin? - @user&.can_admin_all_resources? + def include_hidden? + user_is_admin? end def public_only? !user_is_at_least_reporter? end - def relation_for_count - self.class.query(@project) + def user_is_admin? + strong_memoize(:user_is_admin) do + @user&.can_admin_all_resources? + end end def user_is_at_least_reporter? @@ -58,42 +87,29 @@ 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 - with_hidden_issues = self.class.query(@project, include_hidden: true) - without_hidden_issues = self.class.query(@project, include_hidden: false) - - without_hidden_count_grouped_by_confidential = without_hidden_issues.group(:confidential).count - - total_public_without_hidden_count = without_hidden_count_grouped_by_confidential[false] || 0 - total_confidential_without_hidden_count = without_hidden_count_grouped_by_confidential[true] || 0 - - update_cache_for_key(total_count_cache_key) do - with_hidden_issues.count - end + def issues_with_hidden + self.class.query(@project, public_only: false, include_hidden: true).count + end - update_cache_for_key(public_count_without_hidden_cache_key) do - total_public_without_hidden_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_without_hidden_cache_key) do - total_public_without_hidden_count + total_confidential_without_hidden_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 admins who are allowed to view hidden 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, include_hidden: true) + + 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 Issue.without_hidden.opened.with_issue_type(Issue::TYPES_FOR_LIST).where(project: projects) end diff --git a/ee/spec/finders/issues_finder_spec.rb b/ee/spec/finders/issues_finder_spec.rb index eb350dd00be97d..94cd38388177d6 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_spec.rb b/spec/finders/issues_finder_spec.rb index 0cb73f3da6df02..4f09f43cad1e00 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -12,8 +12,88 @@ context 'scope: all' do let(:scope) { 'all' } - it 'returns all issues' do - expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5) + context 'init_collection' 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 'include_hidden param' do + context 'when include_hidden is true' do + let(:params) { { include_hidden: true } } + + it 'returns all issues' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, hidden_issue, confidential_issue) + end + end + + context 'when include_hidden is false' do + let(:params) { { include_hidden: false } } + + it 'does not return hidden issue' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) + end + end + + context 'when include_hidden is not defined' do + 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 + it 'does not return hidden issue' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) + end + 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 + + context 'public_only param' 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 confidential issue' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) + end + end + + context 'when public_only is not defined' do + it 'returns 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 +1081,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 +1147,9 @@ let(:params) { { project_id: project.id } } 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' + let(:user) { nil } - 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 +1158,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 +1166,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 +1178,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 +1199,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 09528b6304ef6c..10a42996e076a7 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 802d05302bee8e..7dd8c2a59a0f78 100644 --- a/spec/services/groups/open_issues_count_service_spec.rb +++ b/spec/services/groups/open_issues_count_service_spec.rb @@ -5,9 +5,18 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_caching do 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(:banned_user) { create(:user, :banned) } - subject { described_class.new(group) } + 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) } describe '#relation_for_count' do before do @@ -17,32 +26,27 @@ 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) - described_class.new(group, user).count + subject.count end end describe '#count' do - let_it_be(:banned_user) { build(: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) + 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 - subject.refresh_cache + 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 - it 'does not include confidential or hidden issues in the issue count' do - expect(subject.count).to eq(1) - end + let(:user) { nil } - it 'uses group_public_open_issues_without_hidden_count cache key' do - expect(subject.cache_key_name).to eq('group_public_open_issues_without_hidden_count') - end + it_behaves_like 'counts public issues, does not count hidden or confidential' end context 'when user is provided' do @@ -52,11 +56,11 @@ end it 'includes confidential issues and does not include hidden issues in count' do - expect(described_class.new(group, user).count).to eq(2) + expect(subject.count).to eq(2) end - it 'uses group_total_open_issues_without_hidden_count cache key' do - expect(described_class.new(group, user).cache_key_name).to eq('group_total_open_issues_without_hidden_count') + it 'uses TOTAL_COUNT_WITHOUT_HIDDEN_KEY cache key' do + expect(subject.cache_key).to include('group_open_issues_without_hidden_count') end end @@ -65,24 +69,24 @@ group.add_guest(user) end - it 'does not include confidential or hidden issues in count' do - expect(described_class.new(group, user).count).to eq(1) - end - - it 'uses group_public_open_issues_without_hidden_count cache key' do - expect(described_class.new(group, user).cache_key_name).to eq('group_public_open_issues_without_hidden_count') - end + it_behaves_like 'counts public issues, does not count hidden or confidential' end - context 'when user is an admin', :enable_admin_mode do - let_it_be(:admin) { create(:user, :admin) } + 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 'includes confidential and hidden issues in count' do - expect(described_class.new(group, admin).count).to eq(3) + it 'uses TOTAL_COUNT_KEY cache key' do + expect(subject.cache_key).to include('group_open_issues_including_hidden_count') + end end - it 'uses group_total_open_issues_including_hidden_count cache key' do - expect(described_class.new(group, admin).cache_key_name).to eq('group_total_open_issues_including_hidden_count') + context 'when admin mode is disabled' do + it_behaves_like 'counts public issues, does not count hidden or confidential' end end @@ -99,7 +103,7 @@ 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 26ae96eee175aa..14e6b44f7b0750 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 = 26 + 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 28e05dc5ad4d9d..9c5d014efc998f 100644 --- a/spec/services/projects/batch_open_issues_count_service_spec.rb +++ b/spec/services/projects/batch_open_issues_count_service_spec.rb @@ -5,17 +5,15 @@ RSpec.describe Projects::BatchOpenIssuesCountService do let!(:project_1) { create(:project) } let!(:project_2) { create(:project) } - let(:banned_user) { create(:user, :banned) } + let!(:banned_user) { create(:user, :banned) } let(:subject) { described_class.new([project_1, project_2]) } - let(:count_service) { subject.count_service } describe '#refresh_cache_and_retrieve_data', :use_clean_rails_memory_store_caching 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) @@ -25,29 +23,27 @@ it 'refreshes cache keys correctly' do subject.refresh_cache_and_retrieve_data - expect(Rails.cache.read(get_cache_key(subject, project_1))).to eq(1) - expect(Rails.cache.read(get_cache_key(subject, project_2))).to eq(1) + expect(get_cache_key(project_1)).to eq(1) + expect(get_cache_key(project_2)).to eq(1) - expect(Rails.cache.read(get_cache_key(subject, project_1, true))).to eq(2) - expect(Rails.cache.read(get_cache_key(subject, project_2, true))).to eq(2) + 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, true))).to eq(3) - expect(Rails.cache.read(get_cache_key(subject, project_2, true, true))).to eq(3) + 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, include_confidential = false, include_hidden = false) + def get_cache_key(project, with_confidential = false, with_hidden = false) service = subject.count_service.new(project) - if include_confidential - if include_hidden - service.cache_key(service.class::TOTAL_COUNT_KEY) - else - service.cache_key(service.class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY) - end + 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::PUBLIC_COUNT_WITHOUT_HIDDEN_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 87340c53db600b..8710d0c02671bf 100644 --- a/spec/services/projects/open_issues_count_service_spec.rb +++ b/spec/services/projects/open_issues_count_service_spec.rb @@ -4,110 +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' - describe '#count' do - context 'returns different issue counts depending on user' do - before do - create(:issue, :opened, project: project) - create(:issue, :opened, confidential: true, project: project) - create(:issue, :opened, author: banned_user, project: project) - subject.refresh_cache - end + 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) - context 'when user is nil' do - it 'does not include confidential or hidden issues in the issue count' do - expect(subject.count).to eq(1) - end + described_class.new(project).refresh_cache + end - it 'uses open_public_issues_without_hidden_count cache key' do - expect(subject.cache_key_name).to eq('open_public_issues_without_hidden_count') - 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 can read confidential issues' do - before do - project.add_reporter(user) - end + context 'when user is nil' do + let(:user) { nil } - it 'includes confidential issues and does not include hidden issues in count' do - expect(described_class.new(project, user).count).to eq(2) - end + it_behaves_like 'counts public issues, does not count hidden or confidential' + end - it 'uses open_issues_without_hidden_count cache key' do - expect(described_class.new(project, user).cache_key_name).to eq('open_issues_without_hidden_count') - end + context 'when user is provided' do + context 'when user can read confidential issues' do + before do + project.add_reporter(user) end - context 'when user cannot read confidential or hidden issues' do - before do - project.add_guest(user) - end + it 'includes confidential issues and does not include hidden issues in count' do + expect(subject.count).to eq(2) + end - it 'does not include confidential or hidden issues' do - expect(described_class.new(project, user).count).to eq(1) - end + it 'uses TOTAL_COUNT_WITHOUT_HIDDEN_KEY cache key' do + expect(subject.cache_key).to include('project_open_issues_without_hidden_count') + end + end - it 'uses open_public_issues_without_hidden_count cache key' do - expect(described_class.new(project, user).cache_key_name).to eq('open_public_issues_without_hidden_count') - end + context 'when user cannot read confidential or hidden issues' do + before do + project.add_guest(user) end - context 'when user is an admin', :enable_admin_mode do - let(:admin) { create(:admin) } + 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(described_class.new(project, admin).count).to eq(3) + expect(subject.count).to eq(3) end - it 'uses open_issues_including_hidden_count cache key' do - expect(described_class.new(project, admin).cache_key_name).to eq('open_issues_including_hidden_count') + it 'uses TOTAL_COUNT_KEY cache key' do + expect(subject.cache_key).to include('project_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 end end describe '#refresh_cache', :aggregate_failures do - before do - create(:issue, :opened, project: project) - create(:issue, :opened, project: project) - create(:issue, :opened, confidential: true, project: project) - create(:issue, :opened, author: banned_user, 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_WITHOUT_HIDDEN_KEY))).to eq(2) - expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY))).to eq(3) - expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_KEY))).to eq(4) + 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_WITHOUT_HIDDEN_KEY))).to eq(3) - expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_WITHOUT_HIDDEN_KEY))).to eq(5) - expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_KEY))).to eq(7) + 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 -- GitLab From 172fc52e8bdd567958dbf5d9e26c83ec1b5b411b Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 24 Aug 2021 01:18:23 -0500 Subject: [PATCH 04/10] Do not reuse without hidden scope --- app/finders/issues_finder.rb | 2 +- app/models/issue.rb | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index babcc4d28adda2..1912cf6d230c6b 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -95,7 +95,7 @@ def filter_items(items) def by_confidential(items) return items if params[:confidential].nil? - params[:confidential] ? items.confidential_only : items.where(confidential: false) + params[:confidential] ? items.confidential_only : items.public_only end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/models/issue.rb b/app/models/issue.rb index 45ecab16bb5786..ae58d33f8881a5 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -129,7 +129,13 @@ def most_recent scope :with_issue_type, ->(types) { where(issue_type: types) } scope :public_only, -> { - without_hidden.where(confidential: false) + 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: nil }) + .select('issues.id').where('issues.confidential IS FALSE')) + else + where(confidential: false) + end } scope :confidential_only, -> { where(confidential: true) } -- GitLab From 1cb84ab1a86000cc8e71371c57124a11dde8574f Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 24 Aug 2021 20:05:51 -0500 Subject: [PATCH 05/10] Remove rubocop exception --- app/finders/issues_finder.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 1912cf6d230c6b..abf0c180d6b755 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -91,13 +91,11 @@ def filter_items(items) by_issue_types(issues) end - # rubocop: disable CodeReuse/ActiveRecord def by_confidential(items) return items if params[:confidential].nil? params[:confidential] ? items.confidential_only : items.public_only end - # rubocop: enable CodeReuse/ActiveRecord def by_due_date(items) return items unless params.due_date? -- GitLab From cebd60fb020b027e9c1aba1fade12622b2a63655 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 31 Aug 2021 18:37:01 -0500 Subject: [PATCH 06/10] Optimize without hidden query Query does not have to be assigned to variable Change spec context name Fix batch open issue count specs Apply MR review suggestions Update query count Apply MR review suggestions Explicitly test param spec Remove extra space Add back in deleted method Use param method --- app/finders/issues_finder/params.rb | 2 +- app/models/issue.rb | 11 +---- .../groups/open_issues_count_service.rb | 10 ++-- spec/finders/issues_finder/params_spec.rb | 49 +++++++++++++++++++ spec/finders/issues_finder_spec.rb | 46 +---------------- .../batch_open_issues_count_service_spec.rb | 8 ++- 6 files changed, 62 insertions(+), 64 deletions(-) create mode 100644 spec/finders/issues_finder/params_spec.rb diff --git a/app/finders/issues_finder/params.rb b/app/finders/issues_finder/params.rb index c9d50b3e87bcec..02b89f08f9e05f 100644 --- a/app/finders/issues_finder/params.rb +++ b/app/finders/issues_finder/params.rb @@ -7,7 +7,7 @@ def public_only? end def include_hidden? - params.fetch(:include_hidden, user_can_see_all_issues?) + user_can_see_all_issues? end def filter_by_no_due_date? diff --git a/app/models/issue.rb b/app/models/issue.rb index ae58d33f8881a5..847521ccb5b720 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -129,21 +129,14 @@ def most_recent scope :with_issue_type, ->(types) { where(issue_type: types) } scope :public_only, -> { - 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: nil }) - .select('issues.id').where('issues.confidential IS FALSE')) - else - where(confidential: false) - end + 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 2d79272ae2e291..c18d239998b1dc 100644 --- a/app/services/groups/open_issues_count_service.rb +++ b/app/services/groups/open_issues_count_service.rb @@ -29,19 +29,15 @@ def cache_key_name end def include_hidden? - user_is_admin? + strong_memoize(:user_is_admin) do + user&.can_admin_all_resources? + end end def public_only? !user_is_at_least_reporter? end - def user_is_admin? - strong_memoize(:user_is_admin) do - user&.can_admin_all_resources? - end - end - def user_is_at_least_reporter? strong_memoize(:user_is_at_least_reporter) do group.member?(user, Gitlab::Access::REPORTER) diff --git a/spec/finders/issues_finder/params_spec.rb b/spec/finders/issues_finder/params_spec.rb new file mode 100644 index 00000000000000..566e2387cb941f --- /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 + context 'include_hidden param' 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 true' 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 true' 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 4f09f43cad1e00..b36e1ae163e68c 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -12,55 +12,11 @@ context 'scope: all' do let(:scope) { 'all' } - context 'init_collection' do + context 'public_only param' 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 'include_hidden param' do - context 'when include_hidden is true' do - let(:params) { { include_hidden: true } } - - it 'returns all issues' do - expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, hidden_issue, confidential_issue) - end - end - - context 'when include_hidden is false' do - let(:params) { { include_hidden: false } } - - it 'does not return hidden issue' do - expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) - end - end - - context 'when include_hidden is not defined' do - 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 - it 'does not return hidden issue' do - expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) - end - 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 - context 'public_only param' do context 'when public_only is true' do let(:params) { { public_only: true } } 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 9c5d014efc998f..17bd5f7a37b425 100644 --- a/spec/services/projects/batch_open_issues_count_service_spec.rb +++ b/spec/services/projects/batch_open_issues_count_service_spec.rb @@ -19,9 +19,13 @@ 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) -- GitLab From 07e087c9abb0294c1718ce594d54ffc426f64218 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 14 Sep 2021 17:26:52 -0500 Subject: [PATCH 07/10] Apply maintainer suggestions Fix specs --- spec/finders/issues_finder/params_spec.rb | 6 ++-- spec/finders/issues_finder_spec.rb | 42 +++++++++++------------ 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/spec/finders/issues_finder/params_spec.rb b/spec/finders/issues_finder/params_spec.rb index 566e2387cb941f..dcbb21704acddf 100644 --- a/spec/finders/issues_finder/params_spec.rb +++ b/spec/finders/issues_finder/params_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe IssuesFinder::Params do - context 'include_hidden param' do + describe 'include_hidden param' do subject { described_class.new(params, user, IssuesFinder) } context 'when param is not set' do @@ -20,7 +20,7 @@ context 'with a regular user' do let(:user) { create(:user) } - it 'returns true' do + it 'returns false' do expect(subject.include_hidden?).to be_falsey end end @@ -40,7 +40,7 @@ context 'with a regular user' do let(:user) { create(:user) } - it 'returns true' do + it 'returns false' do expect(subject.include_hidden?).to be_falsey end end diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index b36e1ae163e68c..db1818af73f8df 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -17,37 +17,35 @@ let_it_be(:hidden_issue) { create(:issue, project: project1, author: banned_user) } let_it_be(:confidential_issue) { create(:issue, project: project1, confidential: true) } - context 'public_only param' do - context 'when public_only is true' do - let(:params) { { public_only: true } } + 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 + 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 } } + context 'when public_only is false' do + let(:params) { { public_only: false } } - it 'returns confidential issue' do - expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) - end + it 'returns confidential issue' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) end + end - context 'when public_only is not defined' do - it 'returns confidential issue' do - expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) - end + context 'when public_only is not defined' do + it 'returns 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 + 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 + it 'returns all issues' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, hidden_issue, confidential_issue) end end end -- GitLab From 2574099fbe8ccd45cc552b9bf30bc25707cb01b5 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Wed, 15 Sep 2021 15:30:34 -0500 Subject: [PATCH 08/10] Add issues finder specs Add specs for admin --- spec/finders/issues_finder_spec.rb | 48 ++++++++++++++++++------------ 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index db1818af73f8df..ac099a6e811ddf 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -12,40 +12,50 @@ context 'scope: all' do let(:scope) { 'all' } - context 'public_only param' do + 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 public_only is true' do - let(:params) { { public_only: true } } + context 'when user is an admin', :enable_admin_mode do + let(:user) { create(:user, :admin) } - it 'returns public issues' do - expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5) + it 'returns all issues' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, hidden_issue, confidential_issue) end end - context 'when public_only is false' do - let(:params) { { public_only: false } } + context 'when user is not an admin' do + context 'when public_only is true' do + let(:params) { { public_only: true } } - it 'returns confidential issue' do - expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) + it 'returns public issues' do + expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5) + end end - end - context 'when public_only is not defined' do - it 'returns confidential issue' do - expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, confidential_issue) + 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 - end - context 'when ban_user_feature_flag is false' do - before do - stub_feature_flags(ban_user_feature_flag: false) + 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 - it 'returns all issues' do - expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, hidden_issue, confidential_issue) + 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 -- GitLab From 37e3b8572eb8a2a476e92779cb410fc65244da33 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Wed, 15 Sep 2021 20:39:21 -0500 Subject: [PATCH 09/10] Apply maintainer review --- spec/finders/issues_finder/params_spec.rb | 2 +- spec/finders/issues_finder_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/finders/issues_finder/params_spec.rb b/spec/finders/issues_finder/params_spec.rb index dcbb21704acddf..879ecc364a2c62 100644 --- a/spec/finders/issues_finder/params_spec.rb +++ b/spec/finders/issues_finder/params_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe IssuesFinder::Params do - describe 'include_hidden param' do + describe '#include_hidden' do subject { described_class.new(params, user, IssuesFinder) } context 'when param is not set' do diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index ac099a6e811ddf..80a832c68e9ced 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -12,7 +12,7 @@ context 'scope: all' do let(:scope) { 'all' } - context 'include_hidden and public_only params' do + context '#include_hidden and #public_only' 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) } -- GitLab From d09a63f4cd65fd77f20316faf05d03a429daa26f Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Wed, 15 Sep 2021 20:40:02 -0500 Subject: [PATCH 10/10] Apply maintainer suggestions --- spec/finders/issues_finder_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 80a832c68e9ced..ac099a6e811ddf 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -12,7 +12,7 @@ context 'scope: all' do let(:scope) { 'all' } - context '#include_hidden and #public_only' do + 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) } -- GitLab