Small improvements thanks to Robert's feedback15356-filters-should-change-issue-counts

Signed-off-by: Rémy Coutable <remy@rymai.me>

10 files changed, 79 insertions, 115 deletions

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 eaa38fa6c98..fee9a0c1ac1 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..0e082ed537a 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..5cbfe6f17ea
--- /dev/null
+++ b/spec/support/matchers/have_issuable_counts.rb
@@ -0,0 +1,21 @@
+RSpec::Matchers.define :have_issuable_counts do |opts|
+  match do
+    expected_counts = opts.map do |state, count|
+      "#{state.to_s.humanize} #{count}"
+    end
+
+    page.within '.issues-state-filters' do
+      expected_counts.each do |expected_count|
+        expect(page).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