From c1b71e2fa1e49da82b15ee7f12148a372face09c Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Fri, 23 Mar 2018 15:27:15 +0100 Subject: [PATCH 1/7] 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 7dd3e9858c6..2da022fde63 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 280ed93faf8..d228956d8e3 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 @@ 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) } + + 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 86ec500ceb3..228c8d2e8f9 100644 --- a/app/helpers/application_helper.rb +++ b/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] } diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 6d6b840f485..06c3e569c84 100644 --- a/app/helpers/issuables_helper.rb +++ b/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), diff --git a/app/views/dashboard/issues.html.haml b/app/views/dashboard/issues.html.haml index 3e85535dae0..24d69b56c20 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 53cd1130299..fb57ec7835c 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 7704c88905b..58b37290cd5 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 4d8109eb90c..a5f40ea934b 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 97c2c3fb940..3458d679107 100644 --- a/spec/controllers/dashboard_controller_spec.rb +++ b/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 diff --git a/spec/features/dashboard/issues_spec.rb b/spec/features/dashboard/issues_spec.rb index 8d1d5a51750..e41a2e4ce09 100644 --- a/spec/features/dashboard/issues_spec.rb +++ b/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) diff --git a/spec/support/issuables_list_metadata_shared_examples.rb b/spec/support/issuables_list_metadata_shared_examples.rb index 75982432ab4..e61983c60b4 100644 --- a/spec/support/issuables_list_metadata_shared_examples.rb +++ b/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 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 00000000000..439ef5ed92e --- /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 From a32941aee32ddfd7b114011f951741e06a53be5d Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Thu, 29 Mar 2018 13:43:53 +0200 Subject: [PATCH 2/7] Display illustration and message if no filter is selected --- app/assets/stylesheets/framework/images.scss | 2 +- app/views/dashboard/issues.html.haml | 6 +++++- app/views/dashboard/merge_requests.html.haml | 6 +++++- app/views/shared/dashboard/_no_filter_selected.html.haml | 8 ++++++++ app/views/shared/issuable/_filter.html.haml | 5 +++-- 5 files changed, 22 insertions(+), 5 deletions(-) create mode 100644 app/views/shared/dashboard/_no_filter_selected.html.haml diff --git a/app/assets/stylesheets/framework/images.scss b/app/assets/stylesheets/framework/images.scss index df1cafc9f8e..62a0fba3da3 100644 --- a/app/assets/stylesheets/framework/images.scss +++ b/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, diff --git a/app/views/dashboard/issues.html.haml b/app/views/dashboard/issues.html.haml index 24d69b56c20..35bf92a0d07 100644 --- a/app/views/dashboard/issues.html.haml +++ b/app/views/dashboard/issues.html.haml @@ -12,4 +12,8 @@ = 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' diff --git a/app/views/dashboard/merge_requests.html.haml b/app/views/dashboard/merge_requests.html.haml index fb57ec7835c..7b69a6f5d0f 100644 --- a/app/views/dashboard/merge_requests.html.haml +++ b/app/views/dashboard/merge_requests.html.haml @@ -8,4 +8,8 @@ = 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' diff --git a/app/views/shared/dashboard/_no_filter_selected.html.haml b/app/views/shared/dashboard/_no_filter_selected.html.haml new file mode 100644 index 00000000000..e5c2f5b2346 --- /dev/null +++ b/app/views/shared/dashboard/_no_filter_selected.html.haml @@ -0,0 +1,8 @@ +.row.empty-state.text-center + .col-xs-12 + .svg-130 + = image_tag 'illustrations/issue-dashboard_results-without-filter.svg' + .col-xs-12 + .text-content + %h4 + = _("Please select at least one filter to see results") diff --git a/app/views/shared/issuable/_filter.html.haml b/app/views/shared/issuable/_filter.html.haml index 58b37290cd5..994198e2f69 100644 --- a/app/views/shared/issuable/_filter.html.haml +++ b/app/views/shared/issuable/_filter.html.haml @@ -24,8 +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[]" } - .pull-right - = render 'shared/sort_dropdown' + - if !@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) } From d10416e231a27c63c322ba7338f2ea8279436336 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Tue, 27 Mar 2018 17:29:13 +0200 Subject: [PATCH 3/7] Fixed dashboard filtering tests --- app/controllers/dashboard_controller.rb | 11 +- changelogs/unreleased/43246-checkfilter.yml | 6 + spec/features/atom/dashboard_issues_spec.rb | 17 ++- spec/features/dashboard/issues_filter_spec.rb | 32 ++--- .../features/dashboard/merge_requests_spec.rb | 8 +- .../user_uses_header_search_field_spec.rb | 127 ++++++++++++------ spec/helpers/issuables_helper_spec.rb | 29 +--- spec/javascripts/search_autocomplete_spec.js | 8 +- 8 files changed, 137 insertions(+), 101 deletions(-) create mode 100644 changelogs/unreleased/43246-checkfilter.yml diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index d228956d8e3..68d328fa797 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -12,7 +12,7 @@ class DashboardController < Dashboard::ApplicationController 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] + before_action :check_filters_presence!, only: [:issues, :merge_requests] respond_to :html @@ -48,9 +48,14 @@ class DashboardController < Dashboard::ApplicationController @show_full_reference = true end - def check_filters_presence + def check_filters_presence! @no_filters_set = FILTER_PARAMS.none? { |k| params.key?(k) } - render action: action_name if @no_filters_set + return unless @no_filters_set + + respond_to do |format| + format.html + format.atom { head :bad_request } + end end end diff --git a/changelogs/unreleased/43246-checkfilter.yml b/changelogs/unreleased/43246-checkfilter.yml new file mode 100644 index 00000000000..e6c0e716213 --- /dev/null +++ b/changelogs/unreleased/43246-checkfilter.yml @@ -0,0 +1,6 @@ +--- +title: Require at least one filter when listing issues or merge requests on dashboard + page +merge_request: +author: +type: performance diff --git a/spec/features/atom/dashboard_issues_spec.rb b/spec/features/atom/dashboard_issues_spec.rb index d673bac4995..fb6c71ce997 100644 --- a/spec/features/atom/dashboard_issues_spec.rb +++ b/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}')]") diff --git a/spec/features/dashboard/issues_filter_spec.rb b/spec/features/dashboard/issues_filter_spec.rb index 029fc45c791..b15f3afca4a 100644 --- a/spec/features/dashboard/issues_filter_spec.rb +++ b/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,19 @@ feature 'Dashboard Issues filtering', :js do end context 'sorting' do +<<<<<<< HEAD it 'shows sorted issues' do sort_by('Created date') visit_issues +======= + before do + visit_issues(assignee_id: user.id) + end + + it 'remembers last sorting value' do + sorting_by('Created date') + visit_issues(assignee_id: user.id) +>>>>>>> Fixed dashboard filtering tests expect(find('.issues-filters')).to have_content('Created date') end diff --git a/spec/features/dashboard/merge_requests_spec.rb b/spec/features/dashboard/merge_requests_spec.rb index 4a9344115d2..0965b745c03 100644 --- a/spec/features/dashboard/merge_requests_spec.rb +++ b/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 diff --git a/spec/features/search/user_uses_header_search_field_spec.rb b/spec/features/search/user_uses_header_search_field_spec.rb index 5ddea36add5..a9128104b87 100644 --- a/spec/features/search/user_uses_header_search_field_spec.rb +++ b/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 diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb index 2fecd1a3d27..4224cea4652 100644 --- a/spec/helpers/issuables_helper_spec.rb +++ b/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('Open 42') 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('Closed 42') 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('Merged 42') 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('All 42') 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) } diff --git a/spec/javascripts/search_autocomplete_spec.js b/spec/javascripts/search_autocomplete_spec.js index 40115792652..ac47f14ab46 100644 --- a/spec/javascripts/search_autocomplete_spec.js +++ b/spec/javascripts/search_autocomplete_spec.js @@ -98,8 +98,8 @@ import * as urlUtils from '~/lib/utils/url_utility'; assertLinks = function(list, issuesPath, mrsPath) { var a1, a2, a3, a4, issuesAssignedToMeLink, issuesIHaveCreatedLink, mrsAssignedToMeLink, mrsIHaveCreatedLink; if (issuesPath) { - issuesAssignedToMeLink = issuesPath + "/?assignee_username=" + userName; - issuesIHaveCreatedLink = issuesPath + "/?author_username=" + userName; + issuesAssignedToMeLink = issuesPath + "/?assignee_id=" + userId; + issuesIHaveCreatedLink = issuesPath + "/?author_id=" + userId; a1 = "a[href='" + issuesAssignedToMeLink + "']"; a2 = "a[href='" + issuesIHaveCreatedLink + "']"; expect(list.find(a1).length).toBe(1); @@ -107,8 +107,8 @@ import * as urlUtils from '~/lib/utils/url_utility'; expect(list.find(a2).length).toBe(1); expect(list.find(a2).text()).toBe("Issues I've created"); } - mrsAssignedToMeLink = mrsPath + "/?assignee_username=" + userName; - mrsIHaveCreatedLink = mrsPath + "/?author_username=" + userName; + mrsAssignedToMeLink = mrsPath + "/?assignee_id=" + userId; + mrsIHaveCreatedLink = mrsPath + "/?author_id=" + userId; a3 = "a[href='" + mrsAssignedToMeLink + "']"; a4 = "a[href='" + mrsIHaveCreatedLink + "']"; expect(list.find(a3).length).toBe(1); From d343ef8bcee69b81190f52aded3f0ae217ffdc94 Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Tue, 3 Apr 2018 20:19:51 +0200 Subject: [PATCH 4/7] Resolve conflicts in issues_filter_spec.rb --- spec/features/dashboard/issues_filter_spec.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/spec/features/dashboard/issues_filter_spec.rb b/spec/features/dashboard/issues_filter_spec.rb index b15f3afca4a..bab34ac9346 100644 --- a/spec/features/dashboard/issues_filter_spec.rb +++ b/spec/features/dashboard/issues_filter_spec.rb @@ -79,19 +79,13 @@ feature 'Dashboard Issues filtering', :js do end context 'sorting' do -<<<<<<< HEAD - it 'shows sorted issues' do - sort_by('Created date') - visit_issues -======= before do visit_issues(assignee_id: user.id) end it 'remembers last sorting value' do - sorting_by('Created date') + sort_by('Created date') visit_issues(assignee_id: user.id) ->>>>>>> Fixed dashboard filtering tests expect(find('.issues-filters')).to have_content('Created date') end From d7f81b7e5def5b241c9bf7ba35a8b6c0f55a686e Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Wed, 4 Apr 2018 15:39:17 +0200 Subject: [PATCH 5/7] Increase top margin of illustration --- app/views/shared/dashboard/_no_filter_selected.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/dashboard/_no_filter_selected.html.haml b/app/views/shared/dashboard/_no_filter_selected.html.haml index e5c2f5b2346..b2e6967f6aa 100644 --- a/app/views/shared/dashboard/_no_filter_selected.html.haml +++ b/app/views/shared/dashboard/_no_filter_selected.html.haml @@ -1,6 +1,6 @@ .row.empty-state.text-center .col-xs-12 - .svg-130 + .svg-130.prepend-top-default = image_tag 'illustrations/issue-dashboard_results-without-filter.svg' .col-xs-12 .text-content From 02968711128d4bd8939467181504cbe314274cc2 Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Thu, 5 Apr 2018 10:19:55 +0200 Subject: [PATCH 6/7] Use template strings in search_autocomplete_spec.js --- spec/javascripts/search_autocomplete_spec.js | 216 ++++++++++--------- 1 file changed, 111 insertions(+), 105 deletions(-) diff --git a/spec/javascripts/search_autocomplete_spec.js b/spec/javascripts/search_autocomplete_spec.js index ac47f14ab46..1a27955983d 100644 --- a/spec/javascripts/search_autocomplete_spec.js +++ b/spec/javascripts/search_autocomplete_spec.js @@ -6,8 +6,21 @@ import SearchAutocomplete from '~/search_autocomplete'; import '~/lib/utils/common_utils'; import * as urlUtils from '~/lib/utils/url_utility'; -(function() { - var assertLinks, dashboardIssuesPath, dashboardMRsPath, groupIssuesPath, groupMRsPath, groupName, mockDashboardOptions, mockGroupOptions, mockProjectOptions, projectIssuesPath, projectMRsPath, projectName, userId, widget; +describe('Search autocomplete dropdown', () => { + var assertLinks, + dashboardIssuesPath, + dashboardMRsPath, + groupIssuesPath, + groupMRsPath, + groupName, + mockDashboardOptions, + mockGroupOptions, + mockProjectOptions, + projectIssuesPath, + projectMRsPath, + projectName, + userId, + widget; var userName = 'root'; widget = null; @@ -66,133 +79,126 @@ import * as urlUtils from '~/lib/utils/url_utility'; // Mock `gl` object in window for dashboard specific page. App code will need it. mockDashboardOptions = function() { window.gl || (window.gl = {}); - return window.gl.dashboardOptions = { + return (window.gl.dashboardOptions = { issuesPath: dashboardIssuesPath, - mrPath: dashboardMRsPath - }; + mrPath: dashboardMRsPath, + }); }; // Mock `gl` object in window for project specific page. App code will need it. mockProjectOptions = function() { window.gl || (window.gl = {}); - return window.gl.projectOptions = { + return (window.gl.projectOptions = { 'gitlab-ce': { issuesPath: projectIssuesPath, mrPath: projectMRsPath, - projectName: projectName - } - }; + projectName: projectName, + }, + }); }; mockGroupOptions = function() { window.gl || (window.gl = {}); - return window.gl.groupOptions = { + return (window.gl.groupOptions = { 'gitlab-org': { issuesPath: groupIssuesPath, mrPath: groupMRsPath, - projectName: groupName - } - }; + projectName: groupName, + }, + }); }; assertLinks = function(list, issuesPath, mrsPath) { - var a1, a2, a3, a4, issuesAssignedToMeLink, issuesIHaveCreatedLink, mrsAssignedToMeLink, mrsIHaveCreatedLink; if (issuesPath) { - issuesAssignedToMeLink = issuesPath + "/?assignee_id=" + userId; - issuesIHaveCreatedLink = issuesPath + "/?author_id=" + userId; - a1 = "a[href='" + issuesAssignedToMeLink + "']"; - a2 = "a[href='" + issuesIHaveCreatedLink + "']"; - expect(list.find(a1).length).toBe(1); - expect(list.find(a1).text()).toBe('Issues assigned to me'); - expect(list.find(a2).length).toBe(1); - expect(list.find(a2).text()).toBe("Issues I've created"); + const issuesAssignedToMeLink = `a[href="${issuesPath}/?assignee_id=${userId}"]`; + const issuesIHaveCreatedLink = `a[href="${issuesPath}/?author_id=${userId}"]`; + expect(list.find(issuesAssignedToMeLink).length).toBe(1); + expect(list.find(issuesAssignedToMeLink).text()).toBe('Issues assigned to me'); + expect(list.find(issuesIHaveCreatedLink).length).toBe(1); + expect(list.find(issuesIHaveCreatedLink).text()).toBe("Issues I've created"); } - mrsAssignedToMeLink = mrsPath + "/?assignee_id=" + userId; - mrsIHaveCreatedLink = mrsPath + "/?author_id=" + userId; - a3 = "a[href='" + mrsAssignedToMeLink + "']"; - a4 = "a[href='" + mrsIHaveCreatedLink + "']"; - expect(list.find(a3).length).toBe(1); - expect(list.find(a3).text()).toBe('Merge requests assigned to me'); - expect(list.find(a4).length).toBe(1); - return expect(list.find(a4).text()).toBe("Merge requests I've created"); + const mrsAssignedToMeLink = `a[href="${mrsPath}/?assignee_id=${userId}"]`; + const mrsIHaveCreatedLink = `a[href="${mrsPath}/?author_id=${userId}"]`; + expect(list.find(mrsAssignedToMeLink).length).toBe(1); + expect(list.find(mrsAssignedToMeLink).text()).toBe('Merge requests assigned to me'); + expect(list.find(mrsIHaveCreatedLink).length).toBe(1); + expect(list.find(mrsIHaveCreatedLink).text()).toBe("Merge requests I've created"); }; - describe('Search autocomplete dropdown', function() { - preloadFixtures('static/search_autocomplete.html.raw'); - beforeEach(function() { - loadFixtures('static/search_autocomplete.html.raw'); + preloadFixtures('static/search_autocomplete.html.raw'); + beforeEach(function() { + loadFixtures('static/search_autocomplete.html.raw'); - // Prevent turbolinks from triggering within gl_dropdown - spyOn(urlUtils, 'visitUrl').and.returnValue(true); + // Prevent turbolinks from triggering within gl_dropdown + spyOn(urlUtils, 'visitUrl').and.returnValue(true); - window.gon = {}; - window.gon.current_user_id = userId; - window.gon.current_username = userName; + window.gon = {}; + window.gon.current_user_id = userId; + window.gon.current_username = userName; - return widget = new SearchAutocomplete(); - }); - - afterEach(function() { - // Undo what we did to the shared - removeBodyAttributes(); - window.gon = {}; - }); - it('should show Dashboard specific dropdown menu', function() { - var list; - addBodyAttributes(); - mockDashboardOptions(); - widget.searchInput.triggerHandler('focus'); - list = widget.wrap.find('.dropdown-menu').find('ul'); - return assertLinks(list, dashboardIssuesPath, dashboardMRsPath); - }); - it('should show Group specific dropdown menu', function() { - var list; - addBodyAttributes('group'); - mockGroupOptions(); - widget.searchInput.triggerHandler('focus'); - list = widget.wrap.find('.dropdown-menu').find('ul'); - return assertLinks(list, groupIssuesPath, groupMRsPath); - }); - it('should show Project specific dropdown menu', function() { - var list; - addBodyAttributes('project'); - mockProjectOptions(); - widget.searchInput.triggerHandler('focus'); - list = widget.wrap.find('.dropdown-menu').find('ul'); - return assertLinks(list, projectIssuesPath, projectMRsPath); - }); - it('should show only Project mergeRequest dropdown menu items when project issues are disabled', function() { - addBodyAttributes('project'); - disableProjectIssues(); - mockProjectOptions(); - widget.searchInput.triggerHandler('focus'); - const list = widget.wrap.find('.dropdown-menu').find('ul'); - assertLinks(list, null, projectMRsPath); - }); - it('should not show category related menu if there is text in the input', function() { - var link, list; - addBodyAttributes('project'); - mockProjectOptions(); - widget.searchInput.val('help'); - widget.searchInput.triggerHandler('focus'); - list = widget.wrap.find('.dropdown-menu').find('ul'); - link = "a[href='" + projectIssuesPath + "/?assignee_id=" + userId + "']"; - return expect(list.find(link).length).toBe(0); - }); - return it('should not submit the search form when selecting an autocomplete row with the keyboard', function() { - var ENTER = 13; - var DOWN = 40; - addBodyAttributes(); - mockDashboardOptions(true); - var submitSpy = spyOnEvent('form', 'submit'); - widget.searchInput.triggerHandler('focus'); - widget.wrap.trigger($.Event('keydown', { which: DOWN })); - var enterKeyEvent = $.Event('keydown', { which: ENTER }); - widget.searchInput.trigger(enterKeyEvent); - // This does not currently catch failing behavior. For security reasons, - // browsers will not trigger default behavior (form submit, in this - // example) on JavaScript-created keypresses. - expect(submitSpy).not.toHaveBeenTriggered(); - }); + return (widget = new SearchAutocomplete()); }); -}).call(window); + + afterEach(function() { + // Undo what we did to the shared + removeBodyAttributes(); + window.gon = {}; + }); + it('should show Dashboard specific dropdown menu', function() { + var list; + addBodyAttributes(); + mockDashboardOptions(); + widget.searchInput.triggerHandler('focus'); + list = widget.wrap.find('.dropdown-menu').find('ul'); + return assertLinks(list, dashboardIssuesPath, dashboardMRsPath); + }); + it('should show Group specific dropdown menu', function() { + var list; + addBodyAttributes('group'); + mockGroupOptions(); + widget.searchInput.triggerHandler('focus'); + list = widget.wrap.find('.dropdown-menu').find('ul'); + return assertLinks(list, groupIssuesPath, groupMRsPath); + }); + it('should show Project specific dropdown menu', function() { + var list; + addBodyAttributes('project'); + mockProjectOptions(); + widget.searchInput.triggerHandler('focus'); + list = widget.wrap.find('.dropdown-menu').find('ul'); + return assertLinks(list, projectIssuesPath, projectMRsPath); + }); + it('should show only Project mergeRequest dropdown menu items when project issues are disabled', function() { + addBodyAttributes('project'); + disableProjectIssues(); + mockProjectOptions(); + widget.searchInput.triggerHandler('focus'); + const list = widget.wrap.find('.dropdown-menu').find('ul'); + assertLinks(list, null, projectMRsPath); + }); + it('should not show category related menu if there is text in the input', function() { + var link, list; + addBodyAttributes('project'); + mockProjectOptions(); + widget.searchInput.val('help'); + widget.searchInput.triggerHandler('focus'); + list = widget.wrap.find('.dropdown-menu').find('ul'); + link = "a[href='" + projectIssuesPath + '/?assignee_id=' + userId + "']"; + return expect(list.find(link).length).toBe(0); + }); + it('should not submit the search form when selecting an autocomplete row with the keyboard', function() { + var ENTER = 13; + var DOWN = 40; + addBodyAttributes(); + mockDashboardOptions(true); + var submitSpy = spyOnEvent('form', 'submit'); + widget.searchInput.triggerHandler('focus'); + widget.wrap.trigger($.Event('keydown', { which: DOWN })); + var enterKeyEvent = $.Event('keydown', { which: ENTER }); + widget.searchInput.trigger(enterKeyEvent); + // This does not currently catch failing behavior. For security reasons, + // browsers will not trigger default behavior (form submit, in this + // example) on JavaScript-created keypresses. + expect(submitSpy).not.toHaveBeenTriggered(); + }); +}); From 9296d47352df0e7afdd5c5abce6e82476049bef4 Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Fri, 6 Apr 2018 11:52:39 +0200 Subject: [PATCH 7/7] Fix breadcrumb links --- app/views/dashboard/issues.html.haml | 4 ++-- app/views/dashboard/merge_requests.html.haml | 4 ++-- app/views/shared/issuable/_filter.html.haml | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/views/dashboard/issues.html.haml b/app/views/dashboard/issues.html.haml index 35bf92a0d07..bb472b4c900 100644 --- a/app/views/dashboard/issues.html.haml +++ b/app/views/dashboard/issues.html.haml @@ -1,6 +1,6 @@ - @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") diff --git a/app/views/dashboard/merge_requests.html.haml b/app/views/dashboard/merge_requests.html.haml index 7b69a6f5d0f..61aae31be60 100644 --- a/app/views/dashboard/merge_requests.html.haml +++ b/app/views/dashboard/merge_requests.html.haml @@ -1,6 +1,6 @@ - @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, display_count: !@no_filters_set diff --git a/app/views/shared/issuable/_filter.html.haml b/app/views/shared/issuable/_filter.html.haml index 994198e2f69..1bd5b4164b1 100644 --- a/app/views/shared/issuable/_filter.html.haml +++ b/app/views/shared/issuable/_filter.html.haml @@ -24,7 +24,7 @@ .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 !@no_filters_set + - unless @no_filters_set .pull-right = render 'shared/sort_dropdown'