Merge branch '43246-checkfilter' into 'master'

Resolve "Show a message when loading the issues / merge requests dashboard without filters"

Closes #43246

See merge request gitlab-org/gitlab-ce!17961

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}`,
       },
     ];
 

app/assets/stylesheets/framework/images.scss

@@ -20,7 +20,7 @@
     width: 100%;
   }
 
-  $image-widths: 80 250 306 394 430;
+  $image-widths: 80 130 250 306 394 430;
   @each $width in $image-widths {
     &.svg-#{$width} {
       img,

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,15 @@ class DashboardController < Dashboard::ApplicationController
   def set_show_full_reference
     @show_full_reference = true
   end
+
+  def check_filters_presence!
+    @no_filters_set = FILTER_PARAMS.none? { |k| params.key?(k) }
+
+    return unless @no_filters_set
+
+    respond_to do |format|
+      format.html
+      format.atom { head :bad_request }
+    end
+  end
 end

app/helpers/application_helper.rb

@@ -228,9 +228,7 @@ module ApplicationHelper
       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]
     }

app/helpers/issuables_helper.rb

@@ -159,16 +159,18 @@ module IssuablesHelper
     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 @@ module IssuablesHelper
     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),

app/views/dashboard/issues.html.haml

 - @hide_top_links = true
-- page_title "Issues"
-- header_title  "Issues", issues_dashboard_path(assignee_id: current_user.id)
+- page_title _("Issues")
+- @breadcrumb_link = issues_dashboard_path(assignee_id: current_user.id)
 = content_for :meta_tags do
   = 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')
     = render 'shared/new_project_item_select', path: 'issues/new', label: "New issue", with_feature_enabled: 'issues', type: :issues
 
 = render 'shared/issuable/filter', type: :issues
-= render 'shared/issues'
+
+- if current_user && @no_filters_set
+  = render 'shared/dashboard/no_filter_selected'
+- else
+  = render 'shared/issues'

app/views/dashboard/merge_requests.html.haml

 - @hide_top_links = true
-- page_title "Merge Requests"
-- header_title  "Merge Requests", merge_requests_dashboard_path(assignee_id: current_user.id)
+- page_title _("Merge Requests")
+- @breadcrumb_link = 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
 
 = render 'shared/issuable/filter', type: :merge_requests
-= render 'shared/merge_requests'
+
+- if current_user && @no_filters_set
+  = render 'shared/dashboard/no_filter_selected'
+- else
+  = render 'shared/merge_requests'

app/views/shared/dashboard/_no_filter_selected.html.haml

+.row.empty-state.text-center
+  .col-xs-12
+    .svg-130.prepend-top-default
+      = image_tag 'illustrations/issue-dashboard_results-without-filter.svg'
+  .col-xs-12
+    .text-content
+      %h4
+        = _("Please select at least one filter to see results")

app/views/shared/issuable/_filter.html.haml

@@ -24,12 +24,9 @@
         .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'
+        - unless @no_filters_set
+          .pull-right
+            = render 'shared/sort_dropdown'
 
   - has_labels = @labels && @labels.any?
   .row-content-block.second-block.filtered-labels{ class: ("hidden" unless has_labels) }

app/views/shared/issuable/_nav.html.haml

 - 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)

changelogs/unreleased/43246-checkfilter.yml

+---
+title: Require at least one filter when listing issues or merge requests on dashboard
+  page
+merge_request:
+author:
+type: performance

spec/controllers/dashboard_controller_spec.rb

@@ -11,9 +11,11 @@ describe DashboardController do
 
   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

spec/features/atom/dashboard_issues_spec.rb

@@ -13,17 +13,26 @@ describe "Dashboard Issues Feed"  do
     end
 
     describe "atom feed" do
-      it "renders atom feed via personal access token" do
+      it "returns 400 if no filter is used" do
         personal_access_token = create(:personal_access_token, user: user)
 
         visit issues_dashboard_path(:atom, private_token: personal_access_token.token)
 
+        expect(response_headers['Content-Type']).to have_content('application/atom+xml')
+        expect(page.status_code).to eq(400)
+      end
+
+      it "renders atom feed via personal access token" do
+        personal_access_token = create(:personal_access_token, user: user)
+
+        visit issues_dashboard_path(:atom, private_token: personal_access_token.token, assignee_id: user.id)
+
         expect(response_headers['Content-Type']).to have_content('application/atom+xml')
         expect(body).to have_selector('title', text: "#{user.name} issues")
       end
 
       it "renders atom feed via RSS token" do
-        visit issues_dashboard_path(:atom, rss_token: user.rss_token)
+        visit issues_dashboard_path(:atom, rss_token: user.rss_token, assignee_id: user.id)
 
         expect(response_headers['Content-Type']).to have_content('application/atom+xml')
         expect(body).to have_selector('title', text: "#{user.name} issues")
@@ -44,7 +53,7 @@ describe "Dashboard Issues Feed"  do
         let!(:issue2) { create(:issue, author: user, assignees: [assignee], project: project2, description: 'test desc') }
 
         it "renders issue fields" do
-          visit issues_dashboard_path(:atom, rss_token: user.rss_token)
+          visit issues_dashboard_path(:atom, rss_token: user.rss_token, assignee_id: assignee.id)
 
           entry = find(:xpath, "//feed/entry[contains(summary/text(),'#{issue2.title}')]")
 
@@ -67,7 +76,7 @@ describe "Dashboard Issues Feed"  do
         end
 
         it "renders issue label and milestone info" do
-          visit issues_dashboard_path(:atom, rss_token: user.rss_token)
+          visit issues_dashboard_path(:atom, rss_token: user.rss_token, assignee_id: assignee.id)
 
           entry = find(:xpath, "//feed/entry[contains(summary/text(),'#{issue1.title}')]")
 

spec/features/dashboard/issues_filter_spec.rb

@@ -17,6 +17,12 @@ feature 'Dashboard Issues filtering', :js do
     visit_issues
   end
 
+  context 'without any filter' do
+    it 'shows error message' do
+      expect(page).to have_content 'Please select at least one filter to see results'
+    end
+  end
+
   context 'filtering by milestone' do
     it 'shows all issues with no milestone' do
       show_milestone_dropdown
@@ -27,15 +33,6 @@ feature 'Dashboard Issues filtering', :js do
       expect(page).to have_selector('.issue', count: 1)
     end
 
-    it 'shows all issues with any milestone' do
-      show_milestone_dropdown
-
-      click_link 'Any Milestone'
-
-      expect(page).to have_issuable_counts(open: 2, closed: 0, all: 2)
-      expect(page).to have_selector('.issue', count: 2)
-    end
-
     it 'shows all issues with the selected milestone' do
       show_milestone_dropdown
 
@@ -68,13 +65,6 @@ feature 'Dashboard Issues filtering', :js do
     let(:label) { create(:label, project: project) }
     let!(:label_link) { create(:label_link, label: label, target: issue) }
 
-    it 'shows all issues without filter' do
-      page.within 'ul.content-list' do
-        expect(page).to have_content issue.title
-        expect(page).to have_content issue2.title
-      end
-    end
-
     it 'shows all issues with the selected label' do
       page.within '.labels-filter' do
         find('.dropdown').click
@@ -89,9 +79,13 @@ feature 'Dashboard Issues filtering', :js do
   end
 
   context 'sorting' do
-    it 'shows sorted issues' do
+    before do
+      visit_issues(assignee_id: user.id)
+    end
+
+    it 'remembers last sorting value' do
       sort_by('Created date')
-      visit_issues
+      visit_issues(assignee_id: user.id)
 
       expect(find('.issues-filters')).to have_content('Created date')
     end

spec/features/dashboard/issues_spec.rb

@@ -51,15 +51,6 @@ RSpec.describe 'Dashboard Issues' do
       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)

spec/features/dashboard/merge_requests_spec.rb

@@ -103,15 +103,11 @@ feature 'Dashboard Merge Requests' do
       expect(page).not_to have_content(other_merge_request.title)
     end
 
-    it 'shows all merge requests', :js do
+    it 'shows error message without filter', :js do
       filter_item_select('Any Assignee', '.js-assignee-search')
       filter_item_select('Any Author', '.js-author-search')
 
-      expect(page).to have_content(authored_merge_request.title)
-      expect(page).to have_content(authored_merge_request_from_fork.title)
-      expect(page).to have_content(assigned_merge_request.title)
-      expect(page).to have_content(assigned_merge_request_from_fork.title)
-      expect(page).to have_content(other_merge_request.title)
+      expect(page).to have_content('Please select at least one filter to see results')
     end
 
     it 'shows sorted merge requests' do

spec/features/search/user_uses_header_search_field_spec.rb

@@ -9,49 +9,25 @@ describe 'User uses header search field' do
   before do
     project.add_reporter(user)
     sign_in(user)
-
-    visit(project_path(project))
-  end
-
-  it 'starts searching by pressing the enter key', :js do
-    fill_in('search', with: 'gitlab')
-    find('#search').native.send_keys(:enter)
-
-    page.within('.breadcrumbs-sub-title') do
-      expect(page).to have_content('Search')
-    end
   end
 
-  it 'contains location badge' do
-    expect(page).to have_selector('.has-location-badge')
-  end
-
-  context 'when clicking the search field', :js do
+  context 'when user is in a global scope', :js do
     before do
+      visit(root_path)
       page.find('#search').click
     end
 
-    it 'shows category search dropdown' do
-      expect(page).to have_selector('.dropdown-header', text: /#{project.name}/i)
-    end
-
     context 'when clicking issues' do
-      let!(:issue) { create(:issue, project: project, author: user, assignees: [user]) }
-
       it 'shows assigned issues' do
-        find('.dropdown-menu').click_link('Issues assigned to me')
+        find('.search-input-container .dropdown-menu').click_link('Issues assigned to me')
 
-        expect(page).to have_selector('.filtered-search')
-        expect_tokens([assignee_token(user.name)])
-        expect_filtered_search_input_empty
+        expect(find('.js-assignee-search')).to have_content(user.name)
       end
 
       it 'shows created issues' do
-        find('.dropdown-menu').click_link("Issues I've created")
+        find('.search-input-container .dropdown-menu').click_link("Issues I've created")
 
-        expect(page).to have_selector('.filtered-search')
-        expect_tokens([author_token(user.name)])
-        expect_filtered_search_input_empty
+        expect(find('.js-author-search')).to have_content(user.name)
       end
     end
 
@@ -59,32 +35,97 @@ describe 'User uses header search field' do
       let!(:merge_request) { create(:merge_request, source_project: project, author: user, assignee: user) }
 
       it 'shows assigned merge requests' do
-        find('.dropdown-menu').click_link('Merge requests assigned to me')
+        find('.search-input-container .dropdown-menu').click_link('Merge requests assigned to me')
 
-        expect(page).to have_selector('.merge-requests-holder')
-        expect_tokens([assignee_token(user.name)])
-        expect_filtered_search_input_empty
+        expect(find('.js-assignee-search')).to have_content(user.name)
       end
 
       it 'shows created merge requests' do
-        find('.dropdown-menu').click_link("Merge requests I've created")
+        find('.search-input-container .dropdown-menu').click_link("Merge requests I've created")
 
-        expect(page).to have_selector('.merge-requests-holder')
-        expect_tokens([author_token(user.name)])
-        expect_filtered_search_input_empty
+        expect(find('.js-author-search')).to have_content(user.name)
       end
     end
   end
 
-  context 'when entering text into the search field', :js do
+  context 'when user is in a project scope' do
     before do
-      page.within('.search-input-wrap') do
-        fill_in('search', with: project.name[0..3])
+      visit(project_path(project))
+    end
+
+    it 'starts searching by pressing the enter key', :js do
+      fill_in('search', with: 'gitlab')
+      find('#search').native.send_keys(:enter)
+
+      page.within('.breadcrumbs-sub-title') do
+        expect(page).to have_content('Search')
       end
     end
 
-    it 'does not display the category search dropdown' do
-      expect(page).not_to have_selector('.dropdown-header', text: /#{project.name}/i)
+    it 'contains location badge' do
+      expect(page).to have_selector('.has-location-badge')
+    end
+
+    context 'when clicking the search field', :js do
+      before do
+        page.find('#search').click
+      end
+
+      it 'shows category search dropdown' do
+        expect(page).to have_selector('.dropdown-header', text: /#{project.name}/i)
+      end
+
+      context 'when clicking issues' do
+        let!(:issue) { create(:issue, project: project, author: user, assignees: [user]) }
+
+        it 'shows assigned issues' do
+          find('.dropdown-menu').click_link('Issues assigned to me')
+
+          expect(page).to have_selector('.filtered-search')
+          expect_tokens([assignee_token(user.name)])
+          expect_filtered_search_input_empty
+        end
+
+        it 'shows created issues' do
+          find('.dropdown-menu').click_link("Issues I've created")
+
+          expect(page).to have_selector('.filtered-search')
+          expect_tokens([author_token(user.name)])
+          expect_filtered_search_input_empty
+        end
+      end
+
+      context 'when clicking merge requests' do
+        let!(:merge_request) { create(:merge_request, source_project: project, author: user, assignee: user) }
+
+        it 'shows assigned merge requests' do
+          find('.dropdown-menu').click_link('Merge requests assigned to me')
+
+          expect(page).to have_selector('.merge-requests-holder')
+          expect_tokens([assignee_token(user.name)])
+          expect_filtered_search_input_empty
+        end
+
+        it 'shows created merge requests' do
+          find('.dropdown-menu').click_link("Merge requests I've created")
+
+          expect(page).to have_selector('.merge-requests-holder')
+          expect_tokens([author_token(user.name)])
+          expect_filtered_search_input_empty
+        end
+      end
+    end
+
+    context 'when entering text into the search field', :js do
+      before do
+        page.within('.search-input-wrap') do
+          fill_in('search', with: project.name[0..3])
+        end
+      end
+
+      it 'does not display the category search dropdown' do
+        expect(page).not_to have_selector('.dropdown-header', text: /#{project.name}/i)
+      end
     end
   end
 end

spec/helpers/issuables_helper_spec.rb

@@ -40,22 +40,22 @@ describe IssuablesHelper do
       end
 
       it 'returns "Open" when state is :opened' do
-        expect(helper.issuables_state_counter_text(:issues, :opened))
+        expect(helper.issuables_state_counter_text(:issues, :opened, true))
           .to eq('<span>Open</span> <span class="badge">42</span>')
       end
 
       it 'returns "Closed" when state is :closed' do
-        expect(helper.issuables_state_counter_text(:issues, :closed))
+        expect(helper.issuables_state_counter_text(:issues, :closed, true))
           .to eq('<span>Closed</span> <span class="badge">42</span>')
       end
 
       it 'returns "Merged" when state is :merged' do
-        expect(helper.issuables_state_counter_text(:merge_requests, :merged))
+        expect(helper.issuables_state_counter_text(:merge_requests, :merged, true))
           .to eq('<span>Merged</span> <span class="badge">42</span>')
       end
 
       it 'returns "All" when state is :all' do
-        expect(helper.issuables_state_counter_text(:merge_requests, :all))
+        expect(helper.issuables_state_counter_text(:merge_requests, :all, true))
           .to eq('<span>All</span> <span class="badge">42</span>')
       end
     end
@@ -101,27 +101,6 @@ describe IssuablesHelper do
     end
   end
 
-  describe '#issuable_filter_present?' do
-    it 'returns true when any key is present' do
-      allow(helper).to receive(:params).and_return(
-        ActionController::Parameters.new(milestone_title: 'Velit consectetur asperiores natus delectus.',
-                                         project_id: 'gitlabhq',
-                                         scope: 'all')
-      )
-
-      expect(helper.issuable_filter_present?).to be_truthy
-    end
-
-    it 'returns false when no key is present' do
-      allow(helper).to receive(:params).and_return(
-        ActionController::Parameters.new(project_id: 'gitlabhq',
-                                         scope: 'all')
-      )
-
-      expect(helper.issuable_filter_present?).to be_falsey
-    end
-  end
-
   describe '#updated_at_by' do
     let(:user) { create(:user) }
     let(:unedited_issuable) { create(:issue) }

spec/support/issuables_list_metadata_shared_examples.rb

@@ -5,9 +5,9 @@ shared_examples 'issuables list meta-data' do |issuable_type, action = nil|
     %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 @@ shared_examples 'issuables list meta-data' do |issuable_type, action = nil|
 
   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 @@ shared_examples 'issuables list meta-data' do |issuable_type, action = nil|
     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

spec/support/issuables_requiring_filter_shared_examples.rb

+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