From 383dafdf31adaded392664cba9ba8b7262505dc6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Mon, 26 Sep 2016 19:12:16 +0200
Subject: [PATCH] Cache the issuable counters for 2 minutes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
app/helpers/application_helper.rb | 23 ------
app/helpers/issuables_helper.rb | 36 +++++++++
app/views/shared/issuable/_nav.html.haml | 10 +--
spec/helpers/issuables_helper_spec.rb | 97 +++++++++++++++++++++++-
4 files changed, 136 insertions(+), 30 deletions(-)
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index bbc037288db..ebd78bf9888 100644
--- a/app/helpers/application_helper.rb
+++ b/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
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 5c04bba323f..8aa1ece017c 100644
--- a/app/helpers/issuables_helper.rb
+++ b/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
diff --git a/app/views/shared/issuable/_nav.html.haml b/app/views/shared/issuable/_nav.html.haml
index 475b99a2a07..5527a2f889a 100644
--- a/app/views/shared/issuable/_nav.html.haml
+++ b/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)}
diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb
index 2dd2eab0524..5effb7ea36c 100644
--- a/spec/helpers/issuables_helper_spec.rb
+++ b/spec/helpers/issuables_helper_spec.rb
@@ -1,10 +1,10 @@
require 'spec_helper'
-describe IssuablesHelper do
+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
--
GitLab