From 20bb678d91715817f3da04c7a1b73db84295accd Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 22 Jun 2017 20:58:20 +0100 Subject: 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. --- app/finders/issues_finder.rb | 26 ++++++++--------- app/helpers/issuables_helper.rb | 29 ++++++++++++------- app/views/layouts/nav/_project.html.haml | 4 +-- spec/helpers/issuables_helper_spec.rb | 49 ++++++++++++++++++++++++++++---- 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('Open 42') - 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('Open 42') 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('Open 42') + 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('Open 42') @@ -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('Open 42') 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('Open 42') -- cgit v1.2.3