Commit 0c6cdd07 authored by Sean McGivern's avatar Sean McGivern
Browse files

Make finders responsible for counter cache keys

parent 8deece32
......@@ -20,6 +20,7 @@
#
class IssuableFinder
NONE = '0'.freeze
IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page].freeze
attr_accessor :current_user, :params
......@@ -86,6 +87,10 @@ def find_by!(*params)
execute.find_by!(*params)
end
def state_counter_cache_key(state)
Digest::SHA1.hexdigest(state_counter_cache_key_components(state).flatten.join('-'))
end
def group
return @group if defined?(@group)
......@@ -418,4 +423,13 @@ def by_created_at(items)
def current_user_related?
params[:scope] == 'created-by-me' || params[:scope] == 'authored' || params[:scope] == 'assigned-to-me'
end
def state_counter_cache_key_components(state)
opts = params.with_indifferent_access
opts[:state] = state
opts.except!(*IRRELEVANT_PARAMS_FOR_CACHE_KEY)
opts.delete_if { |_, value| value.blank? }
['issuables_count', klass.to_ability_name, opts.sort]
end
end
......@@ -22,7 +22,7 @@ def klass
Issue
end
def not_restricted_by_confidentiality
def with_confidentiality_access_check
return Issue.all if user_can_see_all_confidential_issues?
return Issue.where('issues.confidential IS NOT TRUE') if user_cannot_see_confidential_issues?
......@@ -36,7 +36,15 @@ def not_restricted_by_confidentiality
project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id))
end
private
def init_collection
with_confidentiality_access_check
end
def user_can_see_all_confidential_issues?
return @user_can_see_all_confidential_issues if defined?(@user_can_see_all_confidential_issues)
return @user_can_see_all_confidential_issues = false if current_user.blank?
return @user_can_see_all_confidential_issues = true if current_user.full_private_access?
......@@ -46,16 +54,19 @@ def user_can_see_all_confidential_issues?
project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
end
def user_cannot_see_confidential_issues?
def user_cannot_see_confidential_issues?(for_counting: false)
return false if user_can_see_all_confidential_issues?
current_user.blank? || params[:for_counting]
current_user.blank? || for_counting || params[:for_counting]
end
private
def state_counter_cache_key_components(state)
extra_components = [
user_can_see_all_confidential_issues?,
user_cannot_see_confidential_issues?(for_counting: true)
]
def init_collection
not_restricted_by_confidentiality
super + extra_components
end
def by_assignee(items)
......
......@@ -253,7 +253,7 @@ def issuable_state_scope(issuable)
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)
cache_key = finder.state_counter_cache_key(state)
@counts ||= {}
@counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do
......@@ -263,25 +263,6 @@ def issuables_count_for_state(issuable_type, state, finder: nil)
@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, finder, state)
opts = params.with_indifferent_access
opts[:state] = state
opts.except!(*IRRELEVANT_PARAMS_FOR_CACHE_KEY)
opts.delete_if { |_, value| value.blank? }
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)
@issuable_templates ||=
case issuable
......
......@@ -295,7 +295,7 @@
end
end
describe '#not_restricted_by_confidentiality' do
describe '#with_confidentiality_access_check' do
let(:guest) { create(:user) }
set(:authorized_user) { create(:user) }
set(:project) { create(:empty_project, namespace: authorized_user.namespace) }
......@@ -306,7 +306,7 @@
let(:params) { {} }
context 'for an anonymous user' do
subject { described_class.new(nil, params).not_restricted_by_confidentiality }
subject { described_class.new(nil, params).with_confidentiality_access_check }
it 'returns only public issues' do
expect(subject).to include(public_issue)
......@@ -315,7 +315,7 @@
end
context 'for a user without project membership' do
subject { described_class.new(user, params).not_restricted_by_confidentiality }
subject { described_class.new(user, params).with_confidentiality_access_check }
it 'returns only public issues' do
expect(subject).to include(public_issue)
......@@ -324,7 +324,7 @@
end
context 'for a guest user' do
subject { described_class.new(guest, params).not_restricted_by_confidentiality }
subject { described_class.new(guest, params).with_confidentiality_access_check }
before do
project.add_guest(guest)
......@@ -337,7 +337,7 @@
end
context 'for a project member with access to view confidential issues' do
subject { described_class.new(authorized_user, params).not_restricted_by_confidentiality }
subject { described_class.new(authorized_user, params).with_confidentiality_access_check }
it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue)
......@@ -349,7 +349,7 @@
let(:params) { { project_id: project.id } }
context 'for an anonymous user' do
subject { described_class.new(nil, params).not_restricted_by_confidentiality }
subject { described_class.new(nil, params).with_confidentiality_access_check }
it 'returns only public issues' do
expect(subject).to include(public_issue)
......@@ -364,7 +364,7 @@
end
context 'for a user without project membership' do
subject { described_class.new(user, params).not_restricted_by_confidentiality }
subject { described_class.new(user, params).with_confidentiality_access_check }
it 'returns only public issues' do
expect(subject).to include(public_issue)
......@@ -379,7 +379,7 @@
end
context 'for a guest user' do
subject { described_class.new(guest, params).not_restricted_by_confidentiality }
subject { described_class.new(guest, params).with_confidentiality_access_check }
before do
project.add_guest(guest)
......@@ -398,7 +398,7 @@
end
context 'for a project member with access to view confidential issues' do
subject { described_class.new(authorized_user, params).not_restricted_by_confidentiality }
subject { described_class.new(authorized_user, params).with_confidentiality_access_check }
it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue)
......
......@@ -77,59 +77,57 @@
}.with_indifferent_access
end
let(:finder) { double(:finder, user_cannot_see_confidential_issues?: true, user_can_see_all_confidential_issues?: false) }
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(finder)
allow(helper).to receive(:merge_requests_finder).and_return(finder)
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(finder).to receive(:count_by_state).and_return(opened: 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(finder).not_to receive(:count_by_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
allow(helper).to receive(:params).and_return(params)
expect(finder).to receive(:count_by_state).and_return(opened: 42)
expect(issues_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(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(finder).to receive(:user_can_see_all_confidential_issues?).and_return(true)
expect(finder).to receive(:count_by_state).and_return(opened: 45)
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
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(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(finder).to receive(:count_by_state).and_return(opened: 42)
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',
......@@ -140,8 +138,8 @@
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({
expect(issues_finder).not_to receive(:count_by_state)
expect(issues_finder).to receive(:params).and_return({
author_id: '11',
state: 'bar',
sort: 'bar',
......@@ -154,14 +152,14 @@
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(finder).to receive(:count_by_state).and_return(opened: 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(finder).not_to receive(:count_by_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>')
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment