Cache the issuable counters for 2 minutes

Signed-off-by: Rémy Coutable <remy@rymai.me>

app/helpers/application_helper.rb

@@ -280,29 +280,6 @@ module ApplicationHelper
     end
   end
 
-  def issuables_state_counter_text(state, issuables)
-    titles = {
-      opened: "Open"
-    }
-
-    state_title = titles[state] || state.to_s.humanize
-
-    count =
-      if @issues || @merge_requests
-        issuables_finder = @issues ? issues_finder : merge_requests_finder
-        issuables_finder.params[:state] = state
-        issuables_finder.execute.page(1).total_count
-      end
-
-    html = content_tag(:span, state_title)
-
-    if count.present?
-      html << " " << content_tag(:span, number_with_delimiter(count), class: 'badge')
-    end
-
-    html.html_safe
-  end
-
   def truncate_first_line(message, length = 50)
     truncate(message.each_line.first.chomp, length: length) if message
   end

app/helpers/issuables_helper.rb

@@ -94,6 +94,24 @@ module IssuablesHelper
     label_names.join(', ')
   end
 
+  def issuables_state_counter_text(issuable_type, state)
+    titles = {
+      opened: "Open"
+    }
+
+    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
+
+    html = content_tag(:span, state_title)
+    html << " " << content_tag(:span, number_with_delimiter(count), class: 'badge')
+
+    html.html_safe
+  end
+
   private
 
   def sidebar_gutter_collapsed?
@@ -111,4 +129,22 @@ module IssuablesHelper
       issuable.open? ? :opened : :closed
     end
   end
+
+  def issuables_count_for_state(issuable_type, state)
+    issuables_finder = public_send("#{issuable_type}_finder")
+    issuables_finder.params[:state] = state
+
+    issuables_finder.execute.page(1).total_count
+  end
+
+  IRRELEVANT_PARAMS_FOR_CACHE_KEY = %w[utf8 sort page]
+  private_constant :IRRELEVANT_PARAMS_FOR_CACHE_KEY
+
+  def issuables_state_counter_cache_key(issuable_type, state)
+    opts = params.dup
+    opts['state'] = state
+    opts.delete_if { |k, v| IRRELEVANT_PARAMS_FOR_CACHE_KEY.include?(k) }
+
+    hexdigest(['issuables_count', issuable_type, opts.sort].flatten.join('-'))
+  end
 end

app/views/shared/issuable/_nav.html.haml

@@ -5,21 +5,21 @@
 %ul.nav-links.issues-state-filters
   %li{class: ("active" if params[:state] == 'opened')}
     = link_to page_filter_path(state: 'opened', label: true), title: "Filter by #{page_context_word} that are currently opened." do
-      #{issuables_state_counter_text(:opened, issuables)}
+      #{issuables_state_counter_text(type, :opened)}
 
   - if type == :merge_requests
     %li{class: ("active" if params[:state] == 'merged')}
       = link_to page_filter_path(state: 'merged', label: true), title: 'Filter by merge requests that are currently merged.' do
-        #{issuables_state_counter_text(:merged, issuables)}
+        #{issuables_state_counter_text(type, :merged)}
 
     %li{class: ("active" if params[:state] == 'closed')}
       = link_to page_filter_path(state: 'closed', label: true), title: 'Filter by merge requests that are currently closed and unmerged.' do
-        #{issuables_state_counter_text(:closed, issuables)}
+        #{issuables_state_counter_text(type, :closed)}
   - else
     %li{class: ("active" if params[:state] == 'closed')}
       = link_to page_filter_path(state: 'closed', label: true), title: 'Filter by issues that are currently closed.' do
-        #{issuables_state_counter_text(:closed, issuables)}
+        #{issuables_state_counter_text(type, :closed)}
 
   %li{class: ("active" if params[:state] == 'all')}
     = link_to page_filter_path(state: 'all', label: true), title: "Show all #{page_context_word}." do
-      #{issuables_state_counter_text(:all, issuables)}
+      #{issuables_state_counter_text(type, :all)}

spec/helpers/issuables_helper_spec.rb

@@ -4,7 +4,7 @@ describe IssuablesHelper do
   let(:label)  { build_stubbed(:label) }
   let(:label2) { build_stubbed(:label) }
 
-  context 'label tooltip' do
+  describe '#issuable_labels_tooltip' do
     it 'returns label text' do
       expect(issuable_labels_tooltip([label])).to eq(label.title)
     end
@@ -13,4 +13,97 @@ describe IssuablesHelper do
       expect(issuable_labels_tooltip([label, label2], limit: 1)).to eq("#{label.title}, and 1 more")
     end
   end
+
+  describe '#issuables_state_counter_text' do
+    let(:user) { create(:user) }
+
+    describe 'state text' do
+      before do
+        allow(helper).to receive(:issuables_count_for_state).and_return(42)
+      end
+
+      it 'returns "Open" when state is :opened' do
+        expect(helper.issuables_state_counter_text(:issues, :opened)).
+          to eq('<span>Open</span> <span class="badge">42</span>')
+      end
+
+      it 'returns "Closed" when state is :closed' do
+        expect(helper.issuables_state_counter_text(:issues, :closed)).
+          to eq('<span>Closed</span> <span class="badge">42</span>')
+      end
+
+      it 'returns "Merged" when state is :merged' do
+        expect(helper.issuables_state_counter_text(:merge_requests, :merged)).
+          to eq('<span>Merged</span> <span class="badge">42</span>')
+      end
+
+      it 'returns "All" when state is :all' do
+        expect(helper.issuables_state_counter_text(:merge_requests, :all)).
+          to eq('<span>All</span> <span class="badge">42</span>')
+      end
+    end
+
+    describe 'counter caching based on issuable type and params', :caching do
+      let(:params) do
+        {
+          'scope' => 'created-by-me',
+          'state' => 'opened',
+          'utf8' => '✓',
+          'author_id' => '11',
+          'assignee_id' => '18',
+          'label_name' => ['bug', 'discussion', 'documentation'],
+          'milestone_title' => 'v4.0',
+          'sort' => 'due_date_asc',
+          'namespace_id' => 'gitlab-org',
+          'project_id' => 'gitlab-ce',
+          'page' => 2
+        }
+      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(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(helper.issuables_state_counter_text(:issues, :opened)).
+          to eq('<span>Open</span> <span class="badge">42</span>')
+      end
+
+      describe 'keys not taken in account in the cache key' do
+        %w[state sort utf8 page].each do |param|
+          it "does not take in account params['#{param}'] in the cache key" do
+            expect(helper).to receive(:params).and_return('author_id' => '11', param => 'foo')
+            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('author_id' => '11', param => 'bar')
+            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
+        end
+      end
+
+      it 'does not take params order in acount 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(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(helper.issuables_state_counter_text(:issues, :opened)).
+          to eq('<span>Open</span> <span class="badge">42</span>')
+      end
+    end
+  end
 end