From 56259155d5a0ecfbbafb7319651495582504cfb8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Thu, 29 Sep 2016 16:55:55 +0200
Subject: [PATCH] Small improvements thanks to Robert's feedback
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
.../projects/boards/issues_controller.rb | 2 +-
app/controllers/projects_controller.rb | 4 +-
app/helpers/issuables_helper.rb | 8 +--
lib/api/milestones.rb | 3 +-
spec/features/dashboard_issues_spec.rb | 18 +-----
spec/features/issues/filter_by_labels_spec.rb | 6 +-
spec/features/issues/filter_issues_spec.rb | 54 +++--------------
.../filter_by_milestone_spec.rb | 18 +-----
spec/helpers/issuables_helper_spec.rb | 60 +++++++++++--------
spec/support/matchers/have_issuable_counts.rb | 21 +++++++
10 files changed, 79 insertions(+), 115 deletions(-)
create mode 100644 spec/support/matchers/have_issuable_counts.rb
diff --git a/app/controllers/projects/boards/issues_controller.rb b/app/controllers/projects/boards/issues_controller.rb
index 9404612a993..4aa7982eab4 100644
--- a/app/controllers/projects/boards/issues_controller.rb
+++ b/app/controllers/projects/boards/issues_controller.rb
@@ -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!
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index c99aeadb5e4..62916270172 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -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])
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 8aa1ece017c..8c04200fab9 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -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
diff --git a/lib/api/milestones.rb b/lib/api/milestones.rb
index 7a0cb7c99f3..9b73f6826cf 100644
--- a/lib/api/milestones.rb
+++ b/lib/api/milestones.rb
@@ -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
diff --git a/spec/features/dashboard_issues_spec.rb b/spec/features/dashboard_issues_spec.rb
index 4dccf645454..9b54b5301e5 100644
--- a/spec/features/dashboard_issues_spec.rb
+++ b/spec/features/dashboard_issues_spec.rb
@@ -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
diff --git a/spec/features/issues/filter_by_labels_spec.rb b/spec/features/issues/filter_by_labels_spec.rb
index a615174c00e..0253629f753 100644
--- a/spec/features/issues/filter_by_labels_spec.rb
+++ b/spec/features/issues/filter_by_labels_spec.rb
@@ -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"
diff --git a/spec/features/issues/filter_issues_spec.rb b/spec/features/issues/filter_issues_spec.rb
index 8fd67acff69..8d19198efd3 100644
--- a/spec/features/issues/filter_issues_spec.rb
+++ b/spec/features/issues/filter_issues_spec.rb
@@ -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
diff --git a/spec/features/merge_requests/filter_by_milestone_spec.rb b/spec/features/merge_requests/filter_by_milestone_spec.rb
index 3d5d9614fb6..d917d5950ec 100644
--- a/spec/features/merge_requests/filter_by_milestone_spec.rb
+++ b/spec/features/merge_requests/filter_by_milestone_spec.rb
@@ -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
diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb
index 5effb7ea36c..62cc10f579a 100644
--- a/spec/helpers/issuables_helper_spec.rb
+++ b/spec/helpers/issuables_helper_spec.rb
@@ -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)
diff --git a/spec/support/matchers/have_issuable_counts.rb b/spec/support/matchers/have_issuable_counts.rb
new file mode 100644
index 00000000000..02605d6b70e
--- /dev/null
+++ b/spec/support/matchers/have_issuable_counts.rb
@@ -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
--
GitLab