Cache total issue / MR counts for project by user type

This runs a slightly slower query to get the issue and MR counts in the
navigation, but caches by user type (can see all / none confidential issues) for
two minutes.

4 files changed, 77 insertions, 31 deletions

diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index 3455a75b8bc..328198c026a 100644
--- a/app/finders/issues_finder.rb
+++ b/app/finders/issues_finder.rb
@@ -36,6 +36,19 @@ class IssuesFinder < IssuableFinder
       project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id))
   end
 
+  def user_can_see_all_confidential_issues?
+    return false unless current_user
+    return true if current_user.full_private_access?
+
+    project? &&
+      project &&
+      project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
+  end
+
+  def user_cannot_see_confidential_issues?
+    current_user.blank?
+  end
+
   private
 
   def init_collection
@@ -57,17 +70,4 @@ class IssuesFinder < IssuableFinder
   def item_project_ids(items)
     items&.reorder(nil)&.select(:project_id)
   end
-
-  def user_can_see_all_confidential_issues?
-    return false unless current_user
-    return true if current_user.full_private_access?
-
-    project? &&
-      project &&
-      project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
-  end
-
-  def user_cannot_see_confidential_issues?
-    current_user.blank?
-  end
 end
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 3259a9c1933..d99a9bab12f 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -165,11 +165,7 @@ module IssuablesHelper
     }
 
     state_title = titles[state] || state.to_s.humanize
-
-    count =
-      Rails.cache.fetch(issuables_state_counter_cache_key(issuable_type, state), expires_in: 2.minutes) do
-        issuables_count_for_state(issuable_type, state)
-      end
+    count = issuables_count_for_state(issuable_type, state)
 
     html = content_tag(:span, state_title)
     html << " " << content_tag(:span, number_with_delimiter(count), class: 'badge')
@@ -255,22 +251,35 @@ module IssuablesHelper
     end
   end
 
-  def issuables_count_for_state(issuable_type, state)
+  def issuables_count_for_state(issuable_type, state, finder: nil)
+    finder ||= public_send("#{issuable_type}_finder")
+    cache_key = issuables_state_counter_cache_key(issuable_type, finder, state)
+
     @counts ||= {}
-    @counts[issuable_type] ||= public_send("#{issuable_type}_finder").count_by_state
-    @counts[issuable_type][state]
+    @counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do
+      finder.count_by_state
+    end
+
+    @counts[cache_key][state]
   end
 
   IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page].freeze
   private_constant :IRRELEVANT_PARAMS_FOR_CACHE_KEY
 
-  def issuables_state_counter_cache_key(issuable_type, state)
+  def issuables_state_counter_cache_key(issuable_type, finder, state)
     opts = params.with_indifferent_access
     opts[:state] = state
     opts.except!(*IRRELEVANT_PARAMS_FOR_CACHE_KEY)
     opts.delete_if { |_, value| value.blank? }
 
-    hexdigest(['issuables_count', issuable_type, opts.sort].flatten.join('-'))
+    key_components = ['issuables_count', issuable_type, opts.sort]
+
+    if issuable_type == :issues
+      key_components << finder.user_can_see_all_confidential_issues?
+      key_components << finder.user_cannot_see_confidential_issues?
+    end
+
+    hexdigest(key_components.flatten.join('-'))
   end
 
   def issuable_templates(issuable)
diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml
index a2c6e44425a..b095adcfe7e 100644
--- a/app/views/layouts/nav/_project.html.haml
+++ b/app/views/layouts/nav/_project.html.haml
@@ -28,7 +28,7 @@
           %span
             Issues
             - if @project.default_issues_tracker?
-              %span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id, state: :opened).execute.count)
+              %span.badge.count.issue_counter= number_with_delimiter(issuables_count_for_state(:issues, :opened, finder: IssuesFinder.new(current_user, project_id: @project.id)))
 
     - if project_nav_tab? :merge_requests
       - controllers = [:merge_requests, 'projects/merge_requests/conflicts']
@@ -37,7 +37,7 @@
         = link_to namespace_project_merge_requests_path(@project.namespace, @project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do
           %span
             Merge Requests
-            %span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id, state: :opened).execute.count)
+            %span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(issuables_count_for_state(:merge_requests, :opened, finder: MergeRequestsFinder.new(current_user, project_id: @project.id)))
 
     - if project_nav_tab? :pipelines
       = nav_link(controller: [:pipelines, :builds, :environments, :artifacts]) do
diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb
index 15cb620199d..7dfda388de4 100644
--- a/spec/helpers/issuables_helper_spec.rb
+++ b/spec/helpers/issuables_helper_spec.rb
@@ -77,20 +77,58 @@ describe IssuablesHelper do
         }.with_indifferent_access
       end
 
+      let(:finder) { double(:finder, user_cannot_see_confidential_issues?: true, user_can_see_all_confidential_issues?: false) }
+
+      before do
+        allow(helper).to receive(:issues_finder).and_return(finder)
+        allow(helper).to receive(:merge_requests_finder).and_return(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(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(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
+        allow(helper).to receive(:params).and_return(params)
+        expect(finder).to receive(:count_by_state).and_return(opened: 42)
+
+        expect(helper.issuables_state_counter_text(:issues, :opened))
+          .to include('42')
+
+        expect(finder).to receive(:user_cannot_see_confidential_issues?).and_return(false)
+        expect(finder).to receive(:count_by_state).and_return(opened: 40)
+
+        expect(helper.issuables_state_counter_text(:issues, :opened))
+          .to include('40')
+
+        expect(finder).to receive(:user_can_see_all_confidential_issues?).and_return(true)
+        expect(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
+        allow(helper).to receive(:params).and_return(params)
+        expect(finder).to receive(:count_by_state).and_return(opened: 42)
+        expect(finder).not_to receive(:user_cannot_see_confidential_issues?)
+        expect(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(finder).to receive(:count_by_state).and_return(opened: 42)
         expect(helper).to receive(:params).and_return({
           author_id: '11',
           state: 'foo',
@@ -98,11 +136,11 @@ describe IssuablesHelper do
           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(finder).not_to receive(:count_by_state)
         expect(helper).to receive(:params).and_return({
           author_id: '11',
           state: 'bar',
@@ -110,7 +148,6 @@ describe IssuablesHelper do
           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>')
@@ -118,13 +155,13 @@ describe IssuablesHelper do
 
       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(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(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>')