From c1b71e2fa1e49da82b15ee7f12148a372face09c Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Fri, 23 Mar 2018 15:27:15 +0100
Subject: [PATCH] Check if at least one filter is set on dashboard

When listing issues and merge requests on dasboard page,
make sure that at least one filter is enabled.

User's id is used in search autocomplete widget instead
of username, which allows presetting user in filter dropdowns.

Related to #43246
---
 app/assets/javascripts/search_autocomplete.js |  8 +++----
 app/controllers/dashboard_controller.rb       | 14 +++++++++++
 app/helpers/application_helper.rb             |  2 --
 app/helpers/issuables_helper.rb               | 24 +++++--------------
 app/views/dashboard/issues.html.haml          |  2 +-
 app/views/dashboard/merge_requests.html.haml  |  2 +-
 app/views/shared/issuable/_filter.html.haml   |  4 ----
 app/views/shared/issuable/_nav.html.haml      | 11 +++++----
 spec/controllers/dashboard_controller_spec.rb |  2 ++
 spec/features/dashboard/issues_spec.rb        |  9 -------
 ...issuables_list_metadata_shared_examples.rb |  8 +++----
 ...uables_requiring_filter_shared_examples.rb | 15 ++++++++++++
 12 files changed, 53 insertions(+), 48 deletions(-)
 create mode 100644 spec/support/issuables_requiring_filter_shared_examples.rb

diff --git a/app/assets/javascripts/search_autocomplete.js b/app/assets/javascripts/search_autocomplete.js
index 7dd3e9858c608..2da022fde63b7 100644
--- a/app/assets/javascripts/search_autocomplete.js
+++ b/app/assets/javascripts/search_autocomplete.js
@@ -233,21 +233,21 @@ export default class SearchAutocomplete {
     const issueItems = [
       {
         text: 'Issues assigned to me',
-        url: `${issuesPath}/?assignee_username=${userName}`,
+        url: `${issuesPath}/?assignee_id=${userId}`,
       },
       {
         text: "Issues I've created",
-        url: `${issuesPath}/?author_username=${userName}`,
+        url: `${issuesPath}/?author_id=${userId}`,
       },
     ];
     const mergeRequestItems = [
       {
         text: 'Merge requests assigned to me',
-        url: `${mrPath}/?assignee_username=${userName}`,
+        url: `${mrPath}/?assignee_id=${userId}`,
       },
       {
         text: "Merge requests I've created",
-        url: `${mrPath}/?author_username=${userName}`,
+        url: `${mrPath}/?author_id=${userId}`,
       },
     ];
 
diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb
index 280ed93faf805..d228956d8e359 100644
--- a/app/controllers/dashboard_controller.rb
+++ b/app/controllers/dashboard_controller.rb
@@ -2,9 +2,17 @@ class DashboardController < Dashboard::ApplicationController
   include IssuesAction
   include MergeRequestsAction
 
+  FILTER_PARAMS = [
+    :author_id,
+    :assignee_id,
+    :milestone_title,
+    :label_name
+  ].freeze
+
   before_action :event_filter, only: :activity
   before_action :projects, only: [:issues, :merge_requests]
   before_action :set_show_full_reference, only: [:issues, :merge_requests]
+  before_action :check_filters_presence, only: [:issues, :merge_requests]
 
   respond_to :html
 
@@ -39,4 +47,10 @@ def load_events
   def set_show_full_reference
     @show_full_reference = true
   end
+
+  def check_filters_presence
+    @no_filters_set = FILTER_PARAMS.none? { |k| params.key?(k) }
+
+    render action: action_name if @no_filters_set
+  end
 end
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 86ec500ceb3c0..228c8d2e8f980 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -228,9 +228,7 @@ def page_filter_path(options = {})
       scope: params[:scope],
       milestone_title: params[:milestone_title],
       assignee_id: params[:assignee_id],
-      assignee_username: params[:assignee_username],
       author_id: params[:author_id],
-      author_username: params[:author_username],
       search: params[:search],
       label_name: params[:label_name]
     }
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 6d6b840f485c6..06c3e569c8483 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -159,16 +159,18 @@ def issuable_labels_tooltip(labels, limit: 5)
     label_names.join(', ')
   end
 
-  def issuables_state_counter_text(issuable_type, state)
+  def issuables_state_counter_text(issuable_type, state, display_count)
     titles = {
       opened: "Open"
     }
 
     state_title = titles[state] || state.to_s.humanize
-    count = issuables_count_for_state(issuable_type, state)
-
     html = content_tag(:span, state_title)
-    html << " " << content_tag(:span, number_with_delimiter(count), class: 'badge')
+
+    if display_count
+      count = issuables_count_for_state(issuable_type, state)
+      html << " " << content_tag(:span, number_with_delimiter(count), class: 'badge')
+    end
 
     html.html_safe
   end
@@ -191,24 +193,10 @@ def assigned_issuables_count(issuable_type)
     end
   end
 
-  def issuable_filter_params
-    [
-      :search,
-      :author_id,
-      :assignee_id,
-      :milestone_title,
-      :label_name
-    ]
-  end
-
   def issuable_reference(issuable)
     @show_full_reference ? issuable.to_reference(full: true) : issuable.to_reference(@group || @project)
   end
 
-  def issuable_filter_present?
-    issuable_filter_params.any? { |k| params.key?(k) }
-  end
-
   def issuable_initial_data(issuable)
     data = {
       endpoint: issuable_path(issuable),
diff --git a/app/views/dashboard/issues.html.haml b/app/views/dashboard/issues.html.haml
index 3e85535dae041..24d69b56c2082 100644
--- a/app/views/dashboard/issues.html.haml
+++ b/app/views/dashboard/issues.html.haml
@@ -5,7 +5,7 @@
   = auto_discovery_link_tag(:atom, params.merge(rss_url_options), title: "#{current_user.name} issues")
 
 .top-area
-  = render 'shared/issuable/nav', type: :issues
+  = render 'shared/issuable/nav', type: :issues, display_count: !@no_filters_set
   .nav-controls
     = link_to params.merge(rss_url_options), class: 'btn has-tooltip', data: { container: 'body' }, title: 'Subscribe' do
       = icon('rss')
diff --git a/app/views/dashboard/merge_requests.html.haml b/app/views/dashboard/merge_requests.html.haml
index 53cd1130299b7..fb57ec7835c73 100644
--- a/app/views/dashboard/merge_requests.html.haml
+++ b/app/views/dashboard/merge_requests.html.haml
@@ -3,7 +3,7 @@
 - header_title  "Merge Requests", merge_requests_dashboard_path(assignee_id: current_user.id)
 
 .top-area
-  = render 'shared/issuable/nav', type: :merge_requests
+  = render 'shared/issuable/nav', type: :merge_requests, display_count: !@no_filters_set
   .nav-controls
     = render 'shared/new_project_item_select', path: 'merge_requests/new', label: "New merge request", with_feature_enabled: 'merge_requests', type: :merge_requests
 
diff --git a/app/views/shared/issuable/_filter.html.haml b/app/views/shared/issuable/_filter.html.haml
index 7704c88905b47..58b37290cd564 100644
--- a/app/views/shared/issuable/_filter.html.haml
+++ b/app/views/shared/issuable/_filter.html.haml
@@ -24,10 +24,6 @@
         .filter-item.inline.labels-filter
           = render "shared/issuable/label_dropdown", selected: selected_labels, use_id: false, selected_toggle: params[:label_name], data_options: { field_name: "label_name[]" }
 
-        - if issuable_filter_present?
-          .filter-item.inline.reset-filters
-            %a{ href: page_filter_path(without: issuable_filter_params) } Reset filters
-
         .pull-right
           = render 'shared/sort_dropdown'
 
diff --git a/app/views/shared/issuable/_nav.html.haml b/app/views/shared/issuable/_nav.html.haml
index 4d8109eb90ca8..a5f40ea934b68 100644
--- a/app/views/shared/issuable/_nav.html.haml
+++ b/app/views/shared/issuable/_nav.html.haml
@@ -1,22 +1,23 @@
 - type = local_assigns.fetch(:type, :issues)
 - page_context_word = type.to_s.humanize(capitalize: false)
+- display_count = local_assigns.fetch(:display_count, :true)
 
 %ul.nav-links.issues-state-filters.mobile-separator
   %li{ class: active_when(params[:state] == 'opened') }>
     = link_to page_filter_path(state: 'opened', label: true), id: 'state-opened', title: "Filter by #{page_context_word} that are currently opened.", data: { state: 'opened' } do
-      #{issuables_state_counter_text(type, :opened)}
+      #{issuables_state_counter_text(type, :opened, display_count)}
 
   - if type == :merge_requests
     %li{ class: active_when(params[:state] == 'merged') }>
       = link_to page_filter_path(state: 'merged', label: true), id: 'state-merged', title: 'Filter by merge requests that are currently merged.', data: { state: 'merged' } do
-        #{issuables_state_counter_text(type, :merged)}
+        #{issuables_state_counter_text(type, :merged, display_count)}
 
     %li{ class: active_when(params[:state] == 'closed') }>
       = link_to page_filter_path(state: 'closed', label: true), id: 'state-closed', title: 'Filter by merge requests that are currently closed and unmerged.', data: { state: 'closed' } do
-        #{issuables_state_counter_text(type, :closed)}
+        #{issuables_state_counter_text(type, :closed, display_count)}
   - else
     %li{ class: active_when(params[:state] == 'closed') }>
       = link_to page_filter_path(state: 'closed', label: true), id: 'state-closed', title: 'Filter by issues that are currently closed.', data: { state: 'closed' } do
-        #{issuables_state_counter_text(type, :closed)}
+        #{issuables_state_counter_text(type, :closed, display_count)}
 
-  = render 'shared/issuable/nav_links/all', page_context_word: page_context_word, counter: issuables_state_counter_text(type, :all)
+  = render 'shared/issuable/nav_links/all', page_context_word: page_context_word, counter: issuables_state_counter_text(type, :all, display_count)
diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb
index 97c2c3fb940d0..3458d67910798 100644
--- a/spec/controllers/dashboard_controller_spec.rb
+++ b/spec/controllers/dashboard_controller_spec.rb
@@ -11,9 +11,11 @@
 
   describe 'GET issues' do
     it_behaves_like 'issuables list meta-data', :issue, :issues
+    it_behaves_like 'issuables requiring filter', :issues
   end
 
   describe 'GET merge requests' do
     it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests
+    it_behaves_like 'issuables requiring filter', :merge_requests
   end
 end
diff --git a/spec/features/dashboard/issues_spec.rb b/spec/features/dashboard/issues_spec.rb
index 8d1d5a5175079..e41a2e4ce091c 100644
--- a/spec/features/dashboard/issues_spec.rb
+++ b/spec/features/dashboard/issues_spec.rb
@@ -51,15 +51,6 @@
       expect(page).not_to have_content(other_issue.title)
     end
 
-    it 'shows all issues' do
-      click_link('Reset filters')
-
-      expect(page).to have_content(authored_issue.title)
-      expect(page).to have_content(authored_issue_on_public_project.title)
-      expect(page).to have_content(assigned_issue.title)
-      expect(page).to have_content(other_issue.title)
-    end
-
     it 'state filter tabs work' do
       find('#state-closed').click
       expect(page).to have_current_path(issues_dashboard_url(assignee_id: current_user.id, state: 'closed'), url: true)
diff --git a/spec/support/issuables_list_metadata_shared_examples.rb b/spec/support/issuables_list_metadata_shared_examples.rb
index 75982432ab42e..e61983c60b41e 100644
--- a/spec/support/issuables_list_metadata_shared_examples.rb
+++ b/spec/support/issuables_list_metadata_shared_examples.rb
@@ -5,9 +5,9 @@
     %w[fix improve/awesome].each do |source_branch|
       issuable =
         if issuable_type == :issue
-          create(issuable_type, project: project)
+          create(issuable_type, project: project, author: project.creator)
         else
-          create(issuable_type, source_project: project, source_branch: source_branch)
+          create(issuable_type, source_project: project, source_branch: source_branch, author: project.creator)
         end
 
       @issuable_ids << issuable.id
@@ -16,7 +16,7 @@
 
   it "creates indexed meta-data object for issuable notes and votes count" do
     if action
-      get action
+      get action, author_id: project.creator.id
     else
       get :index, namespace_id: project.namespace, project_id: project
     end
@@ -35,7 +35,7 @@
     it "doesn't execute any queries with false conditions" do
       get_action =
         if action
-          proc { get action }
+          proc { get action, author_id: project.creator.id }
         else
           proc { get :index, namespace_id: project2.namespace, project_id: project2 }
         end
diff --git a/spec/support/issuables_requiring_filter_shared_examples.rb b/spec/support/issuables_requiring_filter_shared_examples.rb
new file mode 100644
index 0000000000000..439ef5ed92e33
--- /dev/null
+++ b/spec/support/issuables_requiring_filter_shared_examples.rb
@@ -0,0 +1,15 @@
+shared_examples 'issuables requiring filter' do |action|
+  it "doesn't load any issuables if no filter is set" do
+    expect_any_instance_of(described_class).not_to receive(:issuables_collection)
+
+    get action
+
+    expect(response).to render_template(action)
+  end
+
+  it "loads issuables if at least one filter is set" do
+    expect_any_instance_of(described_class).to receive(:issuables_collection).and_call_original
+
+    get action, author_id: user.id
+  end
+end
-- 
GitLab