Merge branch 'speed-up-issue-counting-for-a-project' into 'master'

Speed up issue counting for a project

Closes #33913

See merge request !12457

2 files changed, 158 insertions, 24 deletions

diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb
index 8ace1fb5751..4a52f0d5c58 100644
--- a/spec/finders/issues_finder_spec.rb
+++ b/spec/finders/issues_finder_spec.rb
@@ -295,22 +295,121 @@ describe IssuesFinder do
     end
   end
 
-  describe '.not_restricted_by_confidentiality' do
-    let(:authorized_user) { create(:user) }
-    let(:project) { create(:empty_project, namespace: authorized_user.namespace) }
-    let!(:public_issue) { create(:issue, project: project) }
-    let!(:confidential_issue) { create(:issue, project: project, confidential: true) }
-
-    it 'returns non confidential issues for nil user' do
-      expect(described_class.send(:not_restricted_by_confidentiality, nil)).to include(public_issue)
-    end
+  describe '#with_confidentiality_access_check' do
+    let(:guest) { create(:user) }
+    set(:authorized_user) { create(:user) }
+    set(:project) { create(:empty_project, namespace: authorized_user.namespace) }
+    set(:public_issue) { create(:issue, project: project) }
+    set(:confidential_issue) { create(:issue, project: project, confidential: true) }
+
+    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 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+      end
+
+      context 'for a user without project membership' do
+        subject { described_class.new(user, params).with_confidentiality_access_check }
+
+        it 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+      end
+
+      context 'for a guest user' do
+        subject { described_class.new(guest, params).with_confidentiality_access_check }
+
+        before do
+          project.add_guest(guest)
+        end
+
+        it 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+      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 'returns non confidential issues for user not authorized for the issues projects' do
-      expect(described_class.send(:not_restricted_by_confidentiality, user)).to include(public_issue)
+        it 'returns all issues' do
+          expect(subject).to include(public_issue, confidential_issue)
+        end
+      end
     end
 
-    it 'returns all issues for user authorized for the issues projects' do
-      expect(described_class.send(:not_restricted_by_confidentiality, authorized_user)).to include(public_issue, confidential_issue)
+    context 'when searching within a specific project' do
+      let(:params) { { project_id: project.id } }
+
+      context 'for an anonymous user' do
+        subject { described_class.new(nil, params).with_confidentiality_access_check }
+
+        it 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+
+        it 'does not filter by confidentiality' do
+          expect(Issue).not_to receive(:where).with(a_string_matching('confidential'), anything)
+
+          subject
+        end
+      end
+
+      context 'for a user without project membership' do
+        subject { described_class.new(user, params).with_confidentiality_access_check }
+
+        it 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+
+        it 'filters by confidentiality' do
+          expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything)
+
+          subject
+        end
+      end
+
+      context 'for a guest user' do
+        subject { described_class.new(guest, params).with_confidentiality_access_check }
+
+        before do
+          project.add_guest(guest)
+        end
+
+        it 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+
+        it 'filters by confidentiality' do
+          expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything)
+
+          subject
+        end
+      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 'returns all issues' do
+          expect(subject).to include(public_issue, confidential_issue)
+        end
+
+        it 'does not filter by confidentiality' do
+          expect(Issue).not_to receive(:where).with(a_string_matching('confidential'), anything)
+
+          subject
+        end
+      end
     end
   end
 end
diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb
index 15cb620199d..d2e918ef014 100644
--- a/spec/helpers/issuables_helper_spec.rb
+++ b/spec/helpers/issuables_helper_spec.rb
@@ -77,54 +77,89 @@ describe IssuablesHelper do
         }.with_indifferent_access
       end
 
+      let(:issues_finder) { IssuesFinder.new(nil, params) }
+      let(:merge_requests_finder) { MergeRequestsFinder.new(nil, params) }
+
+      before do
+        allow(helper).to receive(:issues_finder).and_return(issues_finder)
+        allow(helper).to receive(:merge_requests_finder).and_return(merge_requests_finder)
+      end
+
       it 'returns the cached value when called for the same issuable type & with the same params' do
-        expect(helper).to receive(:params).twice.and_return(params)
-        expect(helper).to receive(:issuables_count_for_state).with(:issues, :opened).and_return(42)
+        expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
 
         expect(helper.issuables_state_counter_text(:issues, :opened))
           .to eq('<span>Open</span> <span class="badge">42</span>')
 
-        expect(helper).not_to receive(:issuables_count_for_state)
+        expect(issues_finder).not_to receive(:count_by_state)
 
         expect(helper.issuables_state_counter_text(:issues, :opened))
           .to eq('<span>Open</span> <span class="badge">42</span>')
       end
 
+      it 'takes confidential status into account when searching for issues' do
+        expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
+
+        expect(helper.issuables_state_counter_text(:issues, :opened))
+          .to include('42')
+
+        expect(issues_finder).to receive(:user_cannot_see_confidential_issues?).twice.and_return(false)
+        expect(issues_finder).to receive(:count_by_state).and_return(opened: 40)
+
+        expect(helper.issuables_state_counter_text(:issues, :opened))
+          .to include('40')
+
+        expect(issues_finder).to receive(:user_can_see_all_confidential_issues?).and_return(true)
+        expect(issues_finder).to receive(:count_by_state).and_return(opened: 45)
+
+        expect(helper.issuables_state_counter_text(:issues, :opened))
+          .to include('45')
+      end
+
+      it 'does not take confidential status into account when searching for merge requests' do
+        expect(merge_requests_finder).to receive(:count_by_state).and_return(opened: 42)
+        expect(merge_requests_finder).not_to receive(:user_cannot_see_confidential_issues?)
+        expect(merge_requests_finder).not_to receive(:user_can_see_all_confidential_issues?)
+
+        expect(helper.issuables_state_counter_text(:merge_requests, :opened))
+          .to include('42')
+      end
+
       it 'does not take some keys into account in the cache key' do
-        expect(helper).to receive(:params).and_return({
+        expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
+        expect(issues_finder).to receive(:params).and_return({
           author_id: '11',
           state: 'foo',
           sort: 'foo',
           utf8: 'foo',
           page: 'foo'
         }.with_indifferent_access)
-        expect(helper).to receive(:issuables_count_for_state).with(:issues, :opened).and_return(42)
 
         expect(helper.issuables_state_counter_text(:issues, :opened))
           .to eq('<span>Open</span> <span class="badge">42</span>')
 
-        expect(helper).to receive(:params).and_return({
+        expect(issues_finder).not_to receive(:count_by_state)
+        expect(issues_finder).to receive(:params).and_return({
           author_id: '11',
           state: 'bar',
           sort: 'bar',
           utf8: 'bar',
           page: 'bar'
         }.with_indifferent_access)
-        expect(helper).not_to receive(:issuables_count_for_state)
 
         expect(helper.issuables_state_counter_text(:issues, :opened))
           .to eq('<span>Open</span> <span class="badge">42</span>')
       end
 
       it 'does not take params order into account in the cache key' do
-        expect(helper).to receive(:params).and_return('author_id' => '11', 'state' => 'opened')
-        expect(helper).to receive(:issuables_count_for_state).with(:issues, :opened).and_return(42)
+        expect(issues_finder).to receive(:params).and_return('author_id' => '11', 'state' => 'opened')
+        expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
 
         expect(helper.issuables_state_counter_text(:issues, :opened))
           .to eq('<span>Open</span> <span class="badge">42</span>')
 
-        expect(helper).to receive(:params).and_return('state' => 'opened', 'author_id' => '11')
-        expect(helper).not_to receive(:issuables_count_for_state)
+        expect(issues_finder).to receive(:params).and_return('state' => 'opened', 'author_id' => '11')
+        expect(issues_finder).not_to receive(:count_by_state)
 
         expect(helper.issuables_state_counter_text(:issues, :opened))
           .to eq('<span>Open</span> <span class="badge">42</span>')