Small improvements thanks to Robert's feedback
Signed-off-by: Rémy Coutable <remy@rymai.me>
This commit is contained in:
parent
383dafdf31
commit
56259155d5
|
@ -33,7 +33,7 @@ module Projects
|
|||
|
||||
def issue
|
||||
@issue ||=
|
||||
IssuesFinder.new(current_user, project_id: project.id, state: 'all')
|
||||
IssuesFinder.new(current_user, project_id: project.id)
|
||||
.execute
|
||||
.where(iid: params[:id])
|
||||
.first!
|
||||
|
|
|
@ -137,10 +137,10 @@ class ProjectsController < Projects::ApplicationController
|
|||
noteable =
|
||||
case params[:type]
|
||||
when 'Issue'
|
||||
IssuesFinder.new(current_user, project_id: @project.id, state: 'all').
|
||||
IssuesFinder.new(current_user, project_id: @project.id).
|
||||
execute.find_by(iid: params[:type_id])
|
||||
when 'MergeRequest'
|
||||
MergeRequestsFinder.new(current_user, project_id: @project.id, state: 'all').
|
||||
MergeRequestsFinder.new(current_user, project_id: @project.id).
|
||||
execute.find_by(iid: params[:type_id])
|
||||
when 'Commit'
|
||||
@project.commit(params[:type_id])
|
||||
|
|
|
@ -137,13 +137,13 @@ module IssuablesHelper
|
|||
issuables_finder.execute.page(1).total_count
|
||||
end
|
||||
|
||||
IRRELEVANT_PARAMS_FOR_CACHE_KEY = %w[utf8 sort page]
|
||||
IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[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) }
|
||||
opts = params.with_indifferent_access
|
||||
opts[:state] = state
|
||||
opts.except!(*IRRELEVANT_PARAMS_FOR_CACHE_KEY)
|
||||
|
||||
hexdigest(['issuables_count', issuable_type, opts.sort].flatten.join('-'))
|
||||
end
|
||||
|
|
|
@ -108,8 +108,7 @@ module API
|
|||
|
||||
finder_params = {
|
||||
project_id: user_project.id,
|
||||
milestone_title: @milestone.title,
|
||||
state: 'all'
|
||||
milestone_title: @milestone.title
|
||||
}
|
||||
|
||||
issues = IssuesFinder.new(current_user, finder_params).execute
|
||||
|
|
|
@ -21,11 +21,7 @@ describe "Dashboard Issues filtering", feature: true, js: true do
|
|||
|
||||
click_link 'No Milestone'
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 1')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 1')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1)
|
||||
expect(page).to have_selector('.issue', count: 1)
|
||||
end
|
||||
|
||||
|
@ -34,11 +30,7 @@ describe "Dashboard Issues filtering", feature: true, js: true do
|
|||
|
||||
click_link 'Any Milestone'
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 2')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 2')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 2, closed: 0, all: 2)
|
||||
expect(page).to have_selector('.issue', count: 2)
|
||||
end
|
||||
|
||||
|
@ -49,11 +41,7 @@ describe "Dashboard Issues filtering", feature: true, js: true do
|
|||
click_link milestone.title
|
||||
end
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 1')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 1')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1)
|
||||
expect(page).to have_selector('.issue', count: 1)
|
||||
end
|
||||
end
|
||||
|
|
|
@ -83,11 +83,7 @@ feature 'Issue filtering by Labels', feature: true, js: true do
|
|||
end
|
||||
|
||||
it 'applies the filters' do
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 1')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 1')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1)
|
||||
expect(page).to have_content "Bugfix2"
|
||||
expect(page).not_to have_content "Feature1"
|
||||
expect(find('.filtered-labels')).to have_content "bug"
|
||||
|
|
|
@ -227,11 +227,7 @@ describe 'Filter issues', feature: true do
|
|||
it 'filters by text and label' do
|
||||
fill_in 'issuable_search', with: 'Bug'
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 2')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 2')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 2, closed: 0, all: 2)
|
||||
page.within '.issues-list' do
|
||||
expect(page).to have_selector('.issue', count: 2)
|
||||
end
|
||||
|
@ -242,11 +238,7 @@ describe 'Filter issues', feature: true do
|
|||
end
|
||||
find('.dropdown-menu-close-icon').click
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 1')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 1')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1)
|
||||
page.within '.issues-list' do
|
||||
expect(page).to have_selector('.issue', count: 1)
|
||||
end
|
||||
|
@ -255,11 +247,7 @@ describe 'Filter issues', feature: true do
|
|||
it 'filters by text and milestone' do
|
||||
fill_in 'issuable_search', with: 'Bug'
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 2')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 2')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 2, closed: 0, all: 2)
|
||||
page.within '.issues-list' do
|
||||
expect(page).to have_selector('.issue', count: 2)
|
||||
end
|
||||
|
@ -269,11 +257,7 @@ describe 'Filter issues', feature: true do
|
|||
click_link '8'
|
||||
end
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 1')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 1')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1)
|
||||
page.within '.issues-list' do
|
||||
expect(page).to have_selector('.issue', count: 1)
|
||||
end
|
||||
|
@ -282,11 +266,7 @@ describe 'Filter issues', feature: true do
|
|||
it 'filters by text and assignee' do
|
||||
fill_in 'issuable_search', with: 'Bug'
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 2')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 2')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 2, closed: 0, all: 2)
|
||||
page.within '.issues-list' do
|
||||
expect(page).to have_selector('.issue', count: 2)
|
||||
end
|
||||
|
@ -296,11 +276,7 @@ describe 'Filter issues', feature: true do
|
|||
click_link user.name
|
||||
end
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 1')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 1')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1)
|
||||
page.within '.issues-list' do
|
||||
expect(page).to have_selector('.issue', count: 1)
|
||||
end
|
||||
|
@ -309,11 +285,7 @@ describe 'Filter issues', feature: true do
|
|||
it 'filters by text and author' do
|
||||
fill_in 'issuable_search', with: 'Bug'
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 2')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 2')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 2, closed: 0, all: 2)
|
||||
page.within '.issues-list' do
|
||||
expect(page).to have_selector('.issue', count: 2)
|
||||
end
|
||||
|
@ -323,11 +295,7 @@ describe 'Filter issues', feature: true do
|
|||
click_link user.name
|
||||
end
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 1')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 1')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1)
|
||||
page.within '.issues-list' do
|
||||
expect(page).to have_selector('.issue', count: 1)
|
||||
end
|
||||
|
@ -356,11 +324,7 @@ describe 'Filter issues', feature: true do
|
|||
find('.dropdown-menu-close-icon').click
|
||||
wait_for_ajax
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 2')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 2')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 2, closed: 0, all: 2)
|
||||
page.within '.issues-list' do
|
||||
expect(page).to have_selector('.issue', count: 2)
|
||||
end
|
||||
|
|
|
@ -17,11 +17,7 @@ feature 'Merge Request filtering by Milestone', feature: true do
|
|||
visit_merge_requests(project)
|
||||
filter_by_milestone(Milestone::None.title)
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 1')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 1')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1)
|
||||
expect(page).to have_css('.merge-request', count: 1)
|
||||
end
|
||||
|
||||
|
@ -44,11 +40,7 @@ feature 'Merge Request filtering by Milestone', feature: true do
|
|||
visit_merge_requests(project)
|
||||
filter_by_milestone(Milestone::Upcoming.title)
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 1')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 1')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1)
|
||||
expect(page).to have_css('.merge-request', count: 1)
|
||||
end
|
||||
|
||||
|
@ -71,11 +63,7 @@ feature 'Merge Request filtering by Milestone', feature: true do
|
|||
visit_merge_requests(project)
|
||||
filter_by_milestone(milestone.title)
|
||||
|
||||
page.within '.issues-state-filters' do
|
||||
expect(page).to have_content('Open 1')
|
||||
expect(page).to have_content('Closed 0')
|
||||
expect(page).to have_content('All 1')
|
||||
end
|
||||
expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1)
|
||||
expect(page).to have_css('.merge-request', count: 1)
|
||||
end
|
||||
|
||||
|
|
|
@ -46,18 +46,18 @@ describe IssuablesHelper do
|
|||
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
|
||||
}
|
||||
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
|
||||
}.with_indifferent_access
|
||||
end
|
||||
|
||||
it 'returns the cached value when called for the same issuable type & with the same params' do
|
||||
|
@ -73,25 +73,33 @@ describe IssuablesHelper do
|
|||
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)
|
||||
it 'does not take some keys into account in the cache key' do
|
||||
expect(helper).to receive(:params).and_return({
|
||||
author_id: '11',
|
||||
state: 'foo',
|
||||
sort: 'foo',
|
||||
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(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).to receive(:params).and_return({
|
||||
author_id: '11',
|
||||
state: 'bar',
|
||||
sort: 'bar',
|
||||
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>')
|
||||
end
|
||||
end
|
||||
expect(helper.issuables_state_counter_text(:issues, :opened)).
|
||||
to eq('<span>Open</span> <span class="badge">42</span>')
|
||||
end
|
||||
|
||||
it 'does not take params order in acount in the cache key' 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)
|
||||
|
||||
|
|
|
@ -0,0 +1,21 @@
|
|||
RSpec::Matchers.define :have_issuable_counts do |opts|
|
||||
match do |actual|
|
||||
expected_counts = opts.map do |state, count|
|
||||
"#{state.to_s.humanize} #{count}"
|
||||
end
|
||||
|
||||
actual.within '.issues-state-filters' do
|
||||
expected_counts.each do |expected_count|
|
||||
expect(actual).to have_content(expected_count)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
description do
|
||||
"displays the following issuable counts: #{expected_counts.inspect}"
|
||||
end
|
||||
|
||||
failure_message do
|
||||
"expected the following issuable counts: #{expected_counts.inspect} to be displayed"
|
||||
end
|
||||
end
|
Loading…
Reference in New Issue